Re: [PATCH v5] hid-mcp2200: added driver for GPIOs of MCP2200

From: Johannes Roith <johannes@gnu-linux.rocks>
To: sergeantsagara@protonmail.com
Cc: ak@it-klinger.de, andi.shyti@kernel.org,
	benjamin.tissoires@redhat.com, christophe.jaillet@wanadoo.fr,
	jikos@kernel.org, johannes@gnu-linux.rocks,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	rdunlap@infradead.org
Subject: Re: [PATCH v5] hid-mcp2200: added driver for GPIOs of MCP2200
Date: Thu, 31 Aug 2023 20:53:43 +0200	[thread overview]
Message-ID: <20230831185343.211651-1-johannes@gnu-linux.rocks> (raw)
In-Reply-To: <87a5unt25h.fsf@protonmail.com>

Hi Rahul,

thanks for the feedback, I will add it to the driver.

> My personal recommendation is to just have a single DMA buffer allocated
> for the mcp2200 instance rather than having to call the allocator and
> release the memory per command.

I added an 16-byte Array hid_report to the mcp2000 struct. When I need the
report, I do the following:

struct mcp_set_clear_outputs *cmd;

mutex_lock(&mcp->lock);
cmd = (struct mcp_set_clear_outputs *) mcp->hid_report

Do you think this is a valid implementation or do I really have to add a
pointer to the mcp2200 struct instead of the 16 byte array and allocate 
another 16 byte for it in the probe function?

> The reason you run into this is likely because of the action added to
> devm conflicting with hid_device_remove....
> 
> I recommend not depending on devm for teardown rather than making a stub
> remove function to work around the issue.

I am not sure, if I have understand this correctly, but basically I already
have a stub remove function (which is empty). First the remove function gets
called, then the unregister function and everything is cleaned up correctly.
Did I get this right or do you have any other recommendation for me?

So, do I need any adaptions, or can we go with the empty remove function?

Best regards,
Johannes