| Message ID | 20201104220223.293253-1-lars.povlsen@microchip.com |
|---|---|
| State | Accepted |
| Commit | 567b8e9fed8add9e20885be38ecd73bb0e07406b |
| Headers | show |
| Series | HID: mcp2221: Fix GPIO output handling | expand |
On Thu, Nov 5, 2020 at 3:32 AM Lars Povlsen <lars.povlsen@microchip.com> wrote: > > The mcp2221 driver GPIO output handling has has several issues. > > * A wrong value is used for the GPIO direction. > > * Wrong offsets are calculated for some GPIO set value/set direction > operations, when offset is larger than 0. > > This has been fixed by introducing proper manifest constants for the > direction encoding, and using 'offsetof' when calculating GPIO > register offsets. > > The updated driver has been tested with the Sparx5 pcb134/pcb135 > board, which has the mcp2221 device with several (output) GPIO's. > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > --- > drivers/hid/hid-mcp2221.c | 48 +++++++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 9 deletions(-) > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c > index 0d27ccb55dd9..4211b9839209 100644 > --- a/drivers/hid/hid-mcp2221.c > +++ b/drivers/hid/hid-mcp2221.c > @@ -49,6 +49,36 @@ enum { > MCP2221_ALT_F_NOT_GPIOD = 0xEF, > }; > > +/* MCP GPIO direction encoding */ > +enum { > + MCP2221_DIR_OUT = 0x00, > + MCP2221_DIR_IN = 0x01, > +}; > + > +#define MCP_NGPIO 4 > + > +/* MCP GPIO set command layout */ > +struct mcp_set_gpio { > + u8 cmd; > + u8 dummy; > + struct { > + u8 change_value; > + u8 value; > + u8 change_direction; > + u8 direction; > + } gpio[MCP_NGPIO]; > +} __packed; > + > +/* MCP GPIO get command layout */ > +struct mcp_get_gpio { > + u8 cmd; > + u8 dummy; > + struct { > + u8 direction; > + u8 value; > + } gpio[MCP_NGPIO]; > +} __packed; > + > /* > * There is no way to distinguish responses. Therefore next command > * is sent only after response to previous has been received. Mutex > @@ -542,7 +572,7 @@ static int mcp_gpio_get(struct gpio_chip *gc, > > mcp->txbuf[0] = MCP2221_GPIO_GET; > > - mcp->gp_idx = (offset + 1) * 2; > + mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].value); > > mutex_lock(&mcp->lock); > ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); > @@ -559,7 +589,7 @@ static void mcp_gpio_set(struct gpio_chip *gc, > memset(mcp->txbuf, 0, 18); > mcp->txbuf[0] = MCP2221_GPIO_SET; > > - mcp->gp_idx = ((offset + 1) * 4) - 1; > + mcp->gp_idx = offsetof(struct mcp_set_gpio, gpio[offset].value); > > mcp->txbuf[mcp->gp_idx - 1] = 1; > mcp->txbuf[mcp->gp_idx] = !!value; > @@ -575,7 +605,7 @@ static int mcp_gpio_dir_set(struct mcp2221 *mcp, > memset(mcp->txbuf, 0, 18); > mcp->txbuf[0] = MCP2221_GPIO_SET; > > - mcp->gp_idx = (offset + 1) * 5; > + mcp->gp_idx = offsetof(struct mcp_set_gpio, gpio[offset].direction); > > mcp->txbuf[mcp->gp_idx - 1] = 1; > mcp->txbuf[mcp->gp_idx] = val; > @@ -590,7 +620,7 @@ static int mcp_gpio_direction_input(struct gpio_chip *gc, > struct mcp2221 *mcp = gpiochip_get_data(gc); > > mutex_lock(&mcp->lock); > - ret = mcp_gpio_dir_set(mcp, offset, 0); > + ret = mcp_gpio_dir_set(mcp, offset, MCP2221_DIR_IN); > mutex_unlock(&mcp->lock); > > return ret; > @@ -603,7 +633,7 @@ static int mcp_gpio_direction_output(struct gpio_chip *gc, > struct mcp2221 *mcp = gpiochip_get_data(gc); > > mutex_lock(&mcp->lock); > - ret = mcp_gpio_dir_set(mcp, offset, 1); > + ret = mcp_gpio_dir_set(mcp, offset, MCP2221_DIR_OUT); > mutex_unlock(&mcp->lock); > > /* Can't configure as output, bailout early */ > @@ -623,7 +653,7 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc, > > mcp->txbuf[0] = MCP2221_GPIO_GET; > > - mcp->gp_idx = (offset + 1) * 2; > + mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].direction); > > mutex_lock(&mcp->lock); > ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); > @@ -632,7 +662,7 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc, > if (ret) > return ret; > > - if (mcp->gpio_dir) > + if (mcp->gpio_dir == MCP2221_DIR_IN) > return GPIO_LINE_DIRECTION_IN; > > return GPIO_LINE_DIRECTION_OUT; > @@ -758,7 +788,7 @@ static int mcp2221_raw_event(struct hid_device *hdev, > mcp->status = -ENOENT; > } else { > mcp->status = !!data[mcp->gp_idx]; > - mcp->gpio_dir = !!data[mcp->gp_idx + 1]; > + mcp->gpio_dir = data[mcp->gp_idx + 1]; > } > break; > default: > @@ -860,7 +890,7 @@ static int mcp2221_probe(struct hid_device *hdev, > mcp->gc->get_direction = mcp_gpio_get_direction; > mcp->gc->set = mcp_gpio_set; > mcp->gc->get = mcp_gpio_get; > - mcp->gc->ngpio = 4; > + mcp->gc->ngpio = MCP_NGPIO; > mcp->gc->base = -1; > mcp->gc->can_sleep = 1; > mcp->gc->parent = &hdev->dev; > -- > 2.25.1 > Reviewed-by: Rishi Gupta <gupt21@gmail.com> Regards, Rishi
On Wed, 4 Nov 2020, Lars Povlsen wrote: > The mcp2221 driver GPIO output handling has has several issues. > > * A wrong value is used for the GPIO direction. > > * Wrong offsets are calculated for some GPIO set value/set direction > operations, when offset is larger than 0. > > This has been fixed by introducing proper manifest constants for the > direction encoding, and using 'offsetof' when calculating GPIO > register offsets. > > The updated driver has been tested with the Sparx5 pcb134/pcb135 > board, which has the mcp2221 device with several (output) GPIO's. I believe we want also Fixes: 328de1c519c5c092 ("HID: mcp2221: add GPIO functionality support") here, right? I'll add that and apply, thanks. > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > --- > drivers/hid/hid-mcp2221.c | 48 +++++++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 9 deletions(-) > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c > index 0d27ccb55dd9..4211b9839209 100644 > --- a/drivers/hid/hid-mcp2221.c > +++ b/drivers/hid/hid-mcp2221.c > @@ -49,6 +49,36 @@ enum { > MCP2221_ALT_F_NOT_GPIOD = 0xEF, > }; > > +/* MCP GPIO direction encoding */ > +enum { > + MCP2221_DIR_OUT = 0x00, > + MCP2221_DIR_IN = 0x01, > +}; > + > +#define MCP_NGPIO 4 > + > +/* MCP GPIO set command layout */ > +struct mcp_set_gpio { > + u8 cmd; > + u8 dummy; > + struct { > + u8 change_value; > + u8 value; > + u8 change_direction; > + u8 direction; > + } gpio[MCP_NGPIO]; > +} __packed; > + > +/* MCP GPIO get command layout */ > +struct mcp_get_gpio { > + u8 cmd; > + u8 dummy; > + struct { > + u8 direction; > + u8 value; > + } gpio[MCP_NGPIO]; > +} __packed; > + > /* > * There is no way to distinguish responses. Therefore next command > * is sent only after response to previous has been received. Mutex > @@ -542,7 +572,7 @@ static int mcp_gpio_get(struct gpio_chip *gc, > > mcp->txbuf[0] = MCP2221_GPIO_GET; > > - mcp->gp_idx = (offset + 1) * 2; > + mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].value); > > mutex_lock(&mcp->lock); > ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); > @@ -559,7 +589,7 @@ static void mcp_gpio_set(struct gpio_chip *gc, > memset(mcp->txbuf, 0, 18); > mcp->txbuf[0] = MCP2221_GPIO_SET; > > - mcp->gp_idx = ((offset + 1) * 4) - 1; > + mcp->gp_idx = offsetof(struct mcp_set_gpio, gpio[offset].value); > > mcp->txbuf[mcp->gp_idx - 1] = 1; > mcp->txbuf[mcp->gp_idx] = !!value; > @@ -575,7 +605,7 @@ static int mcp_gpio_dir_set(struct mcp2221 *mcp, > memset(mcp->txbuf, 0, 18); > mcp->txbuf[0] = MCP2221_GPIO_SET; > > - mcp->gp_idx = (offset + 1) * 5; > + mcp->gp_idx = offsetof(struct mcp_set_gpio, gpio[offset].direction); > > mcp->txbuf[mcp->gp_idx - 1] = 1; > mcp->txbuf[mcp->gp_idx] = val; > @@ -590,7 +620,7 @@ static int mcp_gpio_direction_input(struct gpio_chip *gc, > struct mcp2221 *mcp = gpiochip_get_data(gc); > > mutex_lock(&mcp->lock); > - ret = mcp_gpio_dir_set(mcp, offset, 0); > + ret = mcp_gpio_dir_set(mcp, offset, MCP2221_DIR_IN); > mutex_unlock(&mcp->lock); > > return ret; > @@ -603,7 +633,7 @@ static int mcp_gpio_direction_output(struct gpio_chip *gc, > struct mcp2221 *mcp = gpiochip_get_data(gc); > > mutex_lock(&mcp->lock); > - ret = mcp_gpio_dir_set(mcp, offset, 1); > + ret = mcp_gpio_dir_set(mcp, offset, MCP2221_DIR_OUT); > mutex_unlock(&mcp->lock); > > /* Can't configure as output, bailout early */ > @@ -623,7 +653,7 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc, > > mcp->txbuf[0] = MCP2221_GPIO_GET; > > - mcp->gp_idx = (offset + 1) * 2; > + mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].direction); > > mutex_lock(&mcp->lock); > ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); > @@ -632,7 +662,7 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc, > if (ret) > return ret; > > - if (mcp->gpio_dir) > + if (mcp->gpio_dir == MCP2221_DIR_IN) > return GPIO_LINE_DIRECTION_IN; > > return GPIO_LINE_DIRECTION_OUT; > @@ -758,7 +788,7 @@ static int mcp2221_raw_event(struct hid_device *hdev, > mcp->status = -ENOENT; > } else { > mcp->status = !!data[mcp->gp_idx]; > - mcp->gpio_dir = !!data[mcp->gp_idx + 1]; > + mcp->gpio_dir = data[mcp->gp_idx + 1]; > } > break; > default: > @@ -860,7 +890,7 @@ static int mcp2221_probe(struct hid_device *hdev, > mcp->gc->get_direction = mcp_gpio_get_direction; > mcp->gc->set = mcp_gpio_set; > mcp->gc->get = mcp_gpio_get; > - mcp->gc->ngpio = 4; > + mcp->gc->ngpio = MCP_NGPIO; > mcp->gc->base = -1; > mcp->gc->can_sleep = 1; > mcp->gc->parent = &hdev->dev; > -- > 2.25.1 > -- Jiri Kosina SUSE Labs
[+cc Takayuki, thanks for your report!] On Wed, Nov 04, 2020 at 11:02:23PM +0100, Lars Povlsen wrote: > The mcp2221 driver GPIO output handling has has several issues. > > * A wrong value is used for the GPIO direction. > > * Wrong offsets are calculated for some GPIO set value/set direction > operations, when offset is larger than 0. > > This has been fixed by introducing proper manifest constants for the > direction encoding, and using 'offsetof' when calculating GPIO > register offsets. > > The updated driver has been tested with the Sparx5 pcb134/pcb135 > board, which has the mcp2221 device with several (output) GPIO's. > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> > --- > drivers/hid/hid-mcp2221.c | 48 +++++++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 9 deletions(-) > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c > index 0d27ccb55dd9..4211b9839209 100644 > --- a/drivers/hid/hid-mcp2221.c > +++ b/drivers/hid/hid-mcp2221.c > +/* MCP GPIO get command layout */ > +struct mcp_get_gpio { > + u8 cmd; > + u8 dummy; > + struct { > + u8 direction; > + u8 value; > + } gpio[MCP_NGPIO]; > +} __packed; This bug report: https://bugzilla.kernel.org/show_bug.cgi?id=216736 suggests that direction and value may be reversed here. Mentioning here in case nobody actively monitors the bugzilla. Bjorn
On Tue, 29 Nov 2022, Bjorn Helgaas wrote: > [+cc Takayuki, thanks for your report!] > > On Wed, Nov 04, 2020 at 11:02:23PM +0100, Lars Povlsen wrote: >> The mcp2221 driver GPIO output handling has has several issues. >> >> * A wrong value is used for the GPIO direction. >> >> * Wrong offsets are calculated for some GPIO set value/set direction >> operations, when offset is larger than 0. >> >> This has been fixed by introducing proper manifest constants for the >> direction encoding, and using 'offsetof' when calculating GPIO >> register offsets. >> >> The updated driver has been tested with the Sparx5 pcb134/pcb135 >> board, which has the mcp2221 device with several (output) GPIO's. >> >> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> >> --- >> drivers/hid/hid-mcp2221.c | 48 +++++++++++++++++++++++++++++++-------- >> 1 file changed, 39 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c >> index 0d27ccb55dd9..4211b9839209 100644 >> --- a/drivers/hid/hid-mcp2221.c >> +++ b/drivers/hid/hid-mcp2221.c > >> +/* MCP GPIO get command layout */ >> +struct mcp_get_gpio { >> + u8 cmd; >> + u8 dummy; >> + struct { >> + u8 direction; >> + u8 value; >> + } gpio[MCP_NGPIO]; >> +} __packed; > > This bug report: https://bugzilla.kernel.org/show_bug.cgi?id=216736 > suggests that direction and value may be reversed here. > > Mentioning here in case nobody actively monitors the bugzilla. > > Bjorn > I can confirm that the order is reversed when compared to both the data sheet and what I can observe in the hid reports. The patch from the linked bugzilla bug fixes the issue for me. > --- a/drivers/hid/hid-mcp2221.c > +++ b/drivers/hid/hid-mcp2221.c > @@ -74,8 +74,8 @@ struct mcp_get_gpio { > u8 cmd; > u8 dummy; > struct { > - u8 direction; > u8 value; > + u8 direction; > } gpio[MCP_NGPIO]; > } __packed; Tested-by: Sven Zühlsdorf <sven.zuehlsdorf@vigem.de>
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c index 0d27ccb55dd9..4211b9839209 100644 --- a/drivers/hid/hid-mcp2221.c +++ b/drivers/hid/hid-mcp2221.c @@ -49,6 +49,36 @@ enum { MCP2221_ALT_F_NOT_GPIOD = 0xEF, }; +/* MCP GPIO direction encoding */ +enum { + MCP2221_DIR_OUT = 0x00, + MCP2221_DIR_IN = 0x01, +}; + +#define MCP_NGPIO 4 + +/* MCP GPIO set command layout */ +struct mcp_set_gpio { + u8 cmd; + u8 dummy; + struct { + u8 change_value; + u8 value; + u8 change_direction; + u8 direction; + } gpio[MCP_NGPIO]; +} __packed; + +/* MCP GPIO get command layout */ +struct mcp_get_gpio { + u8 cmd; + u8 dummy; + struct { + u8 direction; + u8 value; + } gpio[MCP_NGPIO]; +} __packed; + /* * There is no way to distinguish responses. Therefore next command * is sent only after response to previous has been received. Mutex @@ -542,7 +572,7 @@ static int mcp_gpio_get(struct gpio_chip *gc, mcp->txbuf[0] = MCP2221_GPIO_GET; - mcp->gp_idx = (offset + 1) * 2; + mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].value); mutex_lock(&mcp->lock); ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); @@ -559,7 +589,7 @@ static void mcp_gpio_set(struct gpio_chip *gc, memset(mcp->txbuf, 0, 18); mcp->txbuf[0] = MCP2221_GPIO_SET; - mcp->gp_idx = ((offset + 1) * 4) - 1; + mcp->gp_idx = offsetof(struct mcp_set_gpio, gpio[offset].value); mcp->txbuf[mcp->gp_idx - 1] = 1; mcp->txbuf[mcp->gp_idx] = !!value; @@ -575,7 +605,7 @@ static int mcp_gpio_dir_set(struct mcp2221 *mcp, memset(mcp->txbuf, 0, 18); mcp->txbuf[0] = MCP2221_GPIO_SET; - mcp->gp_idx = (offset + 1) * 5; + mcp->gp_idx = offsetof(struct mcp_set_gpio, gpio[offset].direction); mcp->txbuf[mcp->gp_idx - 1] = 1; mcp->txbuf[mcp->gp_idx] = val; @@ -590,7 +620,7 @@ static int mcp_gpio_direction_input(struct gpio_chip *gc, struct mcp2221 *mcp = gpiochip_get_data(gc); mutex_lock(&mcp->lock); - ret = mcp_gpio_dir_set(mcp, offset, 0); + ret = mcp_gpio_dir_set(mcp, offset, MCP2221_DIR_IN); mutex_unlock(&mcp->lock); return ret; @@ -603,7 +633,7 @@ static int mcp_gpio_direction_output(struct gpio_chip *gc, struct mcp2221 *mcp = gpiochip_get_data(gc); mutex_lock(&mcp->lock); - ret = mcp_gpio_dir_set(mcp, offset, 1); + ret = mcp_gpio_dir_set(mcp, offset, MCP2221_DIR_OUT); mutex_unlock(&mcp->lock); /* Can't configure as output, bailout early */ @@ -623,7 +653,7 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc, mcp->txbuf[0] = MCP2221_GPIO_GET; - mcp->gp_idx = (offset + 1) * 2; + mcp->gp_idx = offsetof(struct mcp_get_gpio, gpio[offset].direction); mutex_lock(&mcp->lock); ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); @@ -632,7 +662,7 @@ static int mcp_gpio_get_direction(struct gpio_chip *gc, if (ret) return ret; - if (mcp->gpio_dir) + if (mcp->gpio_dir == MCP2221_DIR_IN) return GPIO_LINE_DIRECTION_IN; return GPIO_LINE_DIRECTION_OUT; @@ -758,7 +788,7 @@ static int mcp2221_raw_event(struct hid_device *hdev, mcp->status = -ENOENT; } else { mcp->status = !!data[mcp->gp_idx]; - mcp->gpio_dir = !!data[mcp->gp_idx + 1]; + mcp->gpio_dir = data[mcp->gp_idx + 1]; } break; default: @@ -860,7 +890,7 @@ static int mcp2221_probe(struct hid_device *hdev, mcp->gc->get_direction = mcp_gpio_get_direction; mcp->gc->set = mcp_gpio_set; mcp->gc->get = mcp_gpio_get; - mcp->gc->ngpio = 4; + mcp->gc->ngpio = MCP_NGPIO; mcp->gc->base = -1; mcp->gc->can_sleep = 1; mcp->gc->parent = &hdev->dev;
The mcp2221 driver GPIO output handling has has several issues. * A wrong value is used for the GPIO direction. * Wrong offsets are calculated for some GPIO set value/set direction operations, when offset is larger than 0. This has been fixed by introducing proper manifest constants for the direction encoding, and using 'offsetof' when calculating GPIO register offsets. The updated driver has been tested with the Sparx5 pcb134/pcb135 board, which has the mcp2221 device with several (output) GPIO's. Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com> --- drivers/hid/hid-mcp2221.c | 48 +++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 9 deletions(-)