Messages in this thread |  | | Date | Sat, 02 Apr 2022 14:47:41 +0200 | From | "Sven Peter" <> | Subject | Re: [PATCH 6/9] nvme-apple: Add initial Apple SoC NVMe driver |
| |
Hi,
Thanks for the review!
On Thu, Mar 24, 2022, at 07:16, Christoph Hellwig wrote: >> + >> +//#define DEBUG > > This should not leak into the driverṡ
Agreed, I'll remove it.
> >> +#include <linux/blk-integrity.h> > > As far as I can tell this driver does not support metadata or PI, > so why is this include needed?
Yup, you're right, it's not required at all. It's a left-over from when I hadn't realized that this controller doesn't support metadata and still had it implemented.
> >> +/* NVM Express NVM Command Set Specification, Revision 1.0a, Figure 18 */ >> +#define NVME_OPCODE_DATA_XFER_HOST_TO_CTRL BIT(0) >> +#define NVME_OPCODE_DATA_XFER_CTRL_TO_HOST BIT(1) > > Please just use the nvme_is_write helper where you are using these.
I didn't realize that helper exists, will use it!
> >> +static int apple_nvme_sart_dma_setup(void *cookie, struct apple_rtkit_shmem *bfr, > > Please avoid > 80 character lines.
Will fix it.
> >> +static void apple_nvme_free_ctrl(struct nvme_ctrl *ctrl) >> +{ >> +} > > So where are the apple specific resources free? ->free_ctrl is > the callback from the struct device release callback, so without > one the resource release can't be tried to the device release.
The resources are all devres managed but I only just realized that the nvme_ctrl creates its own struct device that has to be tied to the platform device. I think I'll at least need a get_device in _probe and a put_device here just like in pci.c to tie those two together. I'll also double check to make sure that all resources are indeed devres-managed.
Thanks,
Sven
|  |