Re: [PATCH 6/9] nvme-apple: Add initial Apple SoC NVMe driver

From: Sven Peter
Date: Sat Apr 02 2022 - 08:48:08 EST


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 */
> 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