Re: [PATCH v2 7/9] nvme: apple: Add Apple A11 support

[Nick Chan <towinchenmi@gmail.com>]

On 19/8/2025 16:30, Christoph Hellwig wrote:
> On Mon, Aug 18, 2025 at 04:43:00PM +0800, Nick Chan wrote:
>>  };
>>  
>> +struct apple_nvme_hw {
>> +	bool has_lsq_nvmmu;
>> +	u32 max_queue_depth;
>> +	void (*submit_cmd)(struct apple_nvme_queue *q, struct nvme_command *cmd);
> 
> Please stick to 80 character lines for the NVMe code.
> 
> Also I don't think an indirect call here is a good idea.  This is right
> in the command submission fast path.  A simple branch will be a lot
> faster.

Ack for both of these points will check the other codes that got more
indented as well.
> 
>> +
>> +	if (q->is_adminq)
>> +		memcpy(&q->sqes[tag], cmd, sizeof(*cmd));
>> +	else
>> +		memcpy((void *)q->sqes + (tag << APPLE_NVME_IOSQES), cmd, sizeof(*cmd));
> 
> This could use a helper and / or comment to make the calculation
> more obvious.

This part of the code could be further simplified and after that
it is similar to nvme_sq_copy_cmd() in pci.c:

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index f3999b8ef7ab..ff4c2f87770c 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -295,20 +295,17 @@ static void apple_nvme_submit_cmd_t8015(struct apple_nvme_queue *q,
 				  struct nvme_command *cmd)
 {
 	struct apple_nvme *anv = queue_to_apple_nvme(q);
-	u32 tag;
 
 	spin_lock_irq(&anv->lock);
 
-	tag = q->sq_tail;
-	q->sq_tail += 1;
-
-	if (q->sq_tail == anv->hw->max_queue_depth)
-		q->sq_tail = 0;
-
 	if (q->is_adminq)
-		memcpy(&q->sqes[tag], cmd, sizeof(*cmd));
+		memcpy(&q->sqes[q->sq_tail], cmd, sizeof(*cmd));
 	else
-		memcpy((void *)q->sqes + (tag << APPLE_NVME_IOSQES), cmd, sizeof(*cmd));
+		memcpy((void *)q->sqes + (q->sq_tail << APPLE_NVME_IOSQES),
+			cmd, sizeof(*cmd));
+
+	if (++q->sq_tail == anv->hw->max_queue_depth)
+		q->sq_tail = 0;
 
 	writel(q->sq_tail, q->sq_db);
 	spin_unlock_irq(&anv->lock);

And this seems obvious enough on its own, and should not need comments.

> 
>> +	anv->hw = (const struct apple_nvme_hw *)of_device_get_match_data(&pdev->dev);
> 
> Do we even need this cast?

Don't think so, will remove.

Best regards,
Nick Chan