[PATCH 0/2] Fixes for the mcp2221 gpiochip get_* calls

[PATCH 0/2] Fixes for the mcp2221 gpiochip get_* calls

[Louis Morhet]

The mcp2221 HID driver exposes a gpiochip device.
While gpioset seemed to work fine, gpioget always returned 1 on all 4
GPIOs of the component (0x01 for input in the field "direction",
according to the documentation).

This patch series addresses this issue by fixing the order of the fields
in the driver's representation of the chip answer, and adding
consistency to the way the callbacks prepare their command and the way
the hid event handler gets these fields.
The set callbacks use a similar mechanism, but work for now because
setting a direction/value only requires gpio-specific positioning in the
command preparation, and not in the raw_event handler.

The core of this issue being a discrepancy in the way the command and
the answer fetch their fields of interest, I would also like to ask if
we should uniformize a bit the way this driver handles gpio, and how.
I thought about several possible implementations for that:
Use mcp->gp_idx as the base offset of the gpio for set too, and modify
the raw_event handler to fetch all relevant data based on that; or set
the buffers in the mcp structure as unions of the various commands
handled and use gp_idx simply as the gpio index to access relevant data
directly from the structured representation everywhere; or go back to ye
old defines to ensure portability...

Louis Morhet (2):
  HID: mcp2221: fix report layout for gpio get
  HID: mcp2221: fix get and get_direction for gpio

 drivers/hid/hid-mcp2221.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/2] HID: mcp2221: fix report layout for gpio get

[Louis Morhet]

The documentation of the component (section 3.1.12 GET GPIO VALUES)
describes the hid report structure with two fields per gpio:
its value, followed by its direction.

However, the driver describes it with a wrong order:
direction followed by value.

Fix the structure representing the report answered by the chip to the
GET GPIO VALUES command.

Fixes commit 567b8e9fed8a ("HID: mcp2221: Fix GPIO output handling")

Signed-off-by: Louis Morhet <lmorhet@kalrayinc.com>
---
 drivers/hid/hid-mcp2221.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index f74a977cf8f8..fa20ed4d395a 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -79,8 +79,8 @@ struct mcp_get_gpio {
 	u8 cmd;
 	u8 dummy;
 	struct {
-		u8 direction;
 		u8 value;
+		u8 direction;
 	} gpio[MCP_NGPIO];
 } __packed;
 
-- 
2.17.1

[PATCH 2/2] HID: mcp2221: fix get and get_direction for gpio

[Louis Morhet]

The mcp2221_raw_event retrieves the value and direction of gpio on the
same command, by setting the value on mcp->status and the direction on
mcp->gpio_dir; and the offset at which they are read is based on
mcp->gp_idx, set by the gpiochip callbacks.

However, the individual gpiochip calls set the index to look for
directly on the field they want to track. This create a "double offset"
in the final read in the response report.

Align the behaviour of mcp2221_raw_event and
mcp_gpio_get/mcp_gpio_get_direction by putting gp_idx on those calls to
the base offset of the gpio status struct.

Signed-off-by: Louis Morhet <lmorhet@kalrayinc.com>
---
 drivers/hid/hid-mcp2221.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index fa20ed4d395a..72883e0ce757 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -594,7 +594,7 @@ static int mcp_gpio_get(struct gpio_chip *gc,
 
 	mcp->txbuf[0] = MCP2221_GPIO_GET;
 
-	mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].value);
+	mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset]);
 
 	mutex_lock(&mcp->lock);
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
@@ -675,7 +675,7 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc,
 
 	mcp->txbuf[0] = MCP2221_GPIO_GET;
 
-	mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].direction);
+	mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset]);
 
 	mutex_lock(&mcp->lock);
 	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
-- 
2.17.1

Re: [PATCH 0/2] Fixes for the mcp2221 gpiochip get_* calls

[Benjamin Tissoires]

On Tue, 04 Apr 2023 14:14:58 +0200, Louis Morhet wrote:
> The mcp2221 HID driver exposes a gpiochip device.
> While gpioset seemed to work fine, gpioget always returned 1 on all 4
> GPIOs of the component (0x01 for input in the field "direction",
> according to the documentation).
> 
> This patch series addresses this issue by fixing the order of the fields
> in the driver's representation of the chip answer, and adding
> consistency to the way the callbacks prepare their command and the way
> the hid event handler gets these fields.
> The set callbacks use a similar mechanism, but work for now because
> setting a direction/value only requires gpio-specific positioning in the
> command preparation, and not in the raw_event handler.
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.4/mcp2221), thanks!

[1/2] HID: mcp2221: fix report layout for gpio get
      https://git.kernel.org/hid/hid/c/e36c31f8cac5
[2/2] HID: mcp2221: fix get and get_direction for gpio
      https://git.kernel.org/hid/hid/c/ca6961d8a851

Cheers,
-- 
Benjamin Tissoires <benjamin.tissoires@redhat.com>

Re: [PATCH 0/2] Fixes for the mcp2221 gpiochip get_* calls

[Benjamin Tissoires]

On Apr 04 2023, Louis Morhet wrote:
> The mcp2221 HID driver exposes a gpiochip device.
> While gpioset seemed to work fine, gpioget always returned 1 on all 4
> GPIOs of the component (0x01 for input in the field "direction",
> according to the documentation).
> 
> This patch series addresses this issue by fixing the order of the fields
> in the driver's representation of the chip answer, and adding
> consistency to the way the callbacks prepare their command and the way
> the hid event handler gets these fields.
> The set callbacks use a similar mechanism, but work for now because
> setting a direction/value only requires gpio-specific positioning in the
> command preparation, and not in the raw_event handler.

As you should have seen in the automatic replied, I took that series in
because it seems to fix a rather worrying bug.


> 
> The core of this issue being a discrepancy in the way the command and
> the answer fetch their fields of interest, I would also like to ask if
> we should uniformize a bit the way this driver handles gpio, and how.
> I thought about several possible implementations for that:
> Use mcp->gp_idx as the base offset of the gpio for set too, and modify
> the raw_event handler to fetch all relevant data based on that; or set
> the buffers in the mcp structure as unions of the various commands
> handled and use gp_idx simply as the gpio index to access relevant data
> directly from the structured representation everywhere; or go back to ye
> old defines to ensure portability...

Honestly, it's hard to make a choice here. You haven't got a replied
from the other mcp2221 folks in almost 10 days so I am not sure you'll
get feedback directly.

May I suggest that you work on any one of these idea, and then submit a
series? Generally, having the code ready makes it way easier to decide
if this is a good solution or not, while having 3 different vague
suggestions makes it hard to see the implications of them.

Also, just a side note, this driver is very limited in term of scope, as
it only touches one dedicated device. Which means that whatever solution
feels the more elegant to you has a good chance of being accepted :)

Cheers,
Benjamin

> 
> Louis Morhet (2):
>   HID: mcp2221: fix report layout for gpio get
>   HID: mcp2221: fix get and get_direction for gpio
> 
>  drivers/hid/hid-mcp2221.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
> 
> 
> 

Re: [PATCH 0/2] Fixes for the mcp2221 gpiochip get_* calls

[Louis Morhet]

On Thu, Apr 13, 2023 at 04:49:13PM +0200, Benjamin Tissoires wrote:
> As you should have seen in the automatic replied, I took that series in
> because it seems to fix a rather worrying bug.
Thanks!

> Also, just a side note, this driver is very limited in term of scope, as
> it only touches one dedicated device. Which means that whatever solution
> feels the more elegant to you has a good chance of being accepted :)
Well, using a struct to describe the layout of a message seems more
elegant; but if I'm not mistaken, taking offsets of fields in a packed
struct is UB... so I was surprised to even see that in Linux, that's why
I was wondering if I should pursue in that direction.


--
Louis