| Message ID | 20250610-gpiochip-set-rv-net-v1-3-35668dd1c76f@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: use new GPIO line value setter callbacks | expand |
On 10/06/2025 at 21:37, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > struct gpio_chip now has callbacks for setting line values that return > an integer, allowing to indicate failures. Convert the driver to using > them. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This does not match the address with which you sent the patch: brgl@bgdev.pl > --- > drivers/net/can/spi/mcp251x.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c > index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644 > --- a/drivers/net/can/spi/mcp251x.c > +++ b/drivers/net/can/spi/mcp251x.c > @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip, > return 0; > } > > -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, > - int value) > +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, > + int value) > { > struct mcp251x_priv *priv = gpiochip_get_data(chip); > u8 mask, val; > @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, > > priv->reg_bfpctrl &= ~mask; > priv->reg_bfpctrl |= val; > + > + return 0; mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write() which can fail. For this change to really make sense, the return value of mcp251x_spi_write() should be propagated all the way around. > } > > -static void > +static int > mcp251x_gpio_set_multiple(struct gpio_chip *chip, > unsigned long *maskp, unsigned long *bitsp) > { > @@ -561,7 +563,7 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip, > val = FIELD_PREP(BFPCTRL_BFS_MASK, val); > > if (!mask) > - return; > + return 0; > > mutex_lock(&priv->mcp_lock); > mcp251x_write_bits(priv->spi, BFPCTRL, mask, val); > @@ -569,6 +571,8 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip, > > priv->reg_bfpctrl &= ~mask; > priv->reg_bfpctrl |= val; > + > + return 0; Same as above. > } > > static void mcp251x_gpio_restore(struct spi_device *spi) > @@ -594,8 +598,8 @@ static int mcp251x_gpio_setup(struct mcp251x_priv *priv) > gpio->get_direction = mcp251x_gpio_get_direction; > gpio->get = mcp251x_gpio_get; > gpio->get_multiple = mcp251x_gpio_get_multiple; > - gpio->set = mcp251x_gpio_set; > - gpio->set_multiple = mcp251x_gpio_set_multiple; > + gpio->set_rv = mcp251x_gpio_set; > + gpio->set_multiple_rv = mcp251x_gpio_set_multiple; > gpio->base = -1; > gpio->ngpio = ARRAY_SIZE(mcp251x_gpio_names); > gpio->names = mcp251x_gpio_names; > Yours sincerely, Vincent Mailhol
On Tue, Jun 10, 2025 at 5:48 PM Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > > On 10/06/2025 at 23:05, Bartosz Golaszewski wrote: > > On Tue, Jun 10, 2025 at 3:55 PM Vincent Mailhol > > <mailhol.vincent@wanadoo.fr> wrote: > >> > >> On 10/06/2025 at 21:37, Bartosz Golaszewski wrote: > >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >>> > >>> struct gpio_chip now has callbacks for setting line values that return > >>> an integer, allowing to indicate failures. Convert the driver to using > >>> them. > >>> > >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> > >> This does not match the address with which you sent the patch: brgl@bgdev.pl > >> > >>> --- > >>> drivers/net/can/spi/mcp251x.c | 16 ++++++++++------ > >>> 1 file changed, 10 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c > >>> index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644 > >>> --- a/drivers/net/can/spi/mcp251x.c > >>> +++ b/drivers/net/can/spi/mcp251x.c > >>> @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip, > >>> return 0; > >>> } > >>> > >>> -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, > >>> - int value) > >>> +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, > >>> + int value) > >>> { > >>> struct mcp251x_priv *priv = gpiochip_get_data(chip); > >>> u8 mask, val; > >>> @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, > >>> > >>> priv->reg_bfpctrl &= ~mask; > >>> priv->reg_bfpctrl |= val; > >>> + > >>> + return 0; > >> > >> mcp251x_gpio_set() calls mcp251x_write_bits() which calls mcp251x_spi_write() > >> which can fail. > >> > >> For this change to really make sense, the return value of mcp251x_spi_write() > >> should be propagated all the way around. > >> > > > > I don't know this code so I followed the example of the rest of the > > codebase where the result of this function is never checked - even in > > functions that do return values. I didn't know the reason for this and > > so didn't want to break anything as I have no means of testing it. > > The return value of mcp251x_spi_write() is used in mcp251x_hw_reset(). In other > locations, mcp251x_spi_write() is only used in functions which return void, so > obviously, the return value is not checked. > Wait, after a second look GPIO callbacks (including those that return a value like request()) use mcp251x_write_bits() which has no return value. It probably should propagate what mcp251x_spi_write() returns but that's material for a different series. The goal of this one is to use the new setters treewide and drop the old ones from struct gpio_chip. Bart
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c index ec5c64006a16f703bc816983765584c5f3ac76e8..7545497d14b46c6388f3976c2bf7b9a99e959c1e 100644 --- a/drivers/net/can/spi/mcp251x.c +++ b/drivers/net/can/spi/mcp251x.c @@ -530,8 +530,8 @@ static int mcp251x_gpio_get_multiple(struct gpio_chip *chip, return 0; } -static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, - int value) +static int mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, + int value) { struct mcp251x_priv *priv = gpiochip_get_data(chip); u8 mask, val; @@ -545,9 +545,11 @@ static void mcp251x_gpio_set(struct gpio_chip *chip, unsigned int offset, priv->reg_bfpctrl &= ~mask; priv->reg_bfpctrl |= val; + + return 0; } -static void +static int mcp251x_gpio_set_multiple(struct gpio_chip *chip, unsigned long *maskp, unsigned long *bitsp) { @@ -561,7 +563,7 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip, val = FIELD_PREP(BFPCTRL_BFS_MASK, val); if (!mask) - return; + return 0; mutex_lock(&priv->mcp_lock); mcp251x_write_bits(priv->spi, BFPCTRL, mask, val); @@ -569,6 +571,8 @@ mcp251x_gpio_set_multiple(struct gpio_chip *chip, priv->reg_bfpctrl &= ~mask; priv->reg_bfpctrl |= val; + + return 0; } static void mcp251x_gpio_restore(struct spi_device *spi) @@ -594,8 +598,8 @@ static int mcp251x_gpio_setup(struct mcp251x_priv *priv) gpio->get_direction = mcp251x_gpio_get_direction; gpio->get = mcp251x_gpio_get; gpio->get_multiple = mcp251x_gpio_get_multiple; - gpio->set = mcp251x_gpio_set; - gpio->set_multiple = mcp251x_gpio_set_multiple; + gpio->set_rv = mcp251x_gpio_set; + gpio->set_multiple_rv = mcp251x_gpio_set_multiple; gpio->base = -1; gpio->ngpio = ARRAY_SIZE(mcp251x_gpio_names); gpio->names = mcp251x_gpio_names;