| Message ID | 20250408-gpiochip-set-rv-powerpc-v1-2-73dc1ebc6ef1@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc: convert board-file GPIO chips to using new value setters | expand |
Le 08/04/2025 à 09:21, Bartosz Golaszewski a écrit : > 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> > --- > arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > index 4d8fa9ed1a67..d4ba6dbb86b2 100644 > --- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > +++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > @@ -92,7 +92,7 @@ static void mcu_power_off(void) > mutex_unlock(&mcu->lock); > } > > -static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > +static int mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > { > struct mcu *mcu = gpiochip_get_data(gc); > u8 bit = 1 << (4 + gpio); > @@ -105,6 +105,8 @@ static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > > i2c_smbus_write_byte_data(mcu->client, MCU_REG_CTRL, mcu->reg_ctrl); > mutex_unlock(&mcu->lock); > + > + return 0; i2c_smbus_write_byte_data() can fail, why not return the value returned by i2c_smbus_write_byte_data() ? > } > > static int mcu_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) > @@ -123,7 +125,7 @@ static int mcu_gpiochip_add(struct mcu *mcu) > gc->can_sleep = 1; > gc->ngpio = MCU_NUM_GPIO; > gc->base = -1; > - gc->set = mcu_gpio_set; > + gc->set_rv = mcu_gpio_set; > gc->direction_output = mcu_gpio_dir_out; > gc->parent = dev; > >
On Wed, Apr 30, 2025 at 7:33 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 08/04/2025 à 09:21, Bartosz Golaszewski a écrit : > > 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> > > --- > > arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > > index 4d8fa9ed1a67..d4ba6dbb86b2 100644 > > --- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > > +++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > > @@ -92,7 +92,7 @@ static void mcu_power_off(void) > > mutex_unlock(&mcu->lock); > > } > > > > -static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > > +static int mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > > { > > struct mcu *mcu = gpiochip_get_data(gc); > > u8 bit = 1 << (4 + gpio); > > @@ -105,6 +105,8 @@ static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > > > > i2c_smbus_write_byte_data(mcu->client, MCU_REG_CTRL, mcu->reg_ctrl); > > mutex_unlock(&mcu->lock); > > + > > + return 0; > > i2c_smbus_write_byte_data() can fail, why not return the value returned > by i2c_smbus_write_byte_data() ? > The calls to i2c_smbus_write_byte_data() in this driver are universally not checked. I cannot test it and wasn't sure if that's on purpose so I decided to stay safe. Someone who has access to this platform could potentially fix it across the file. Bartosz > > } > > > > static int mcu_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) > > @@ -123,7 +125,7 @@ static int mcu_gpiochip_add(struct mcu *mcu) > > gc->can_sleep = 1; > > gc->ngpio = MCU_NUM_GPIO; > > gc->base = -1; > > - gc->set = mcu_gpio_set; > > + gc->set_rv = mcu_gpio_set; > > gc->direction_output = mcu_gpio_dir_out; > > gc->parent = dev; > > > > >
Le 30/04/2025 à 19:37, Bartosz Golaszewski a écrit : > On Wed, Apr 30, 2025 at 7:33 PM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> >> >> Le 08/04/2025 à 09:21, Bartosz Golaszewski a écrit : >>> 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> >>> --- >>> arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c >>> index 4d8fa9ed1a67..d4ba6dbb86b2 100644 >>> --- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c >>> +++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c >>> @@ -92,7 +92,7 @@ static void mcu_power_off(void) >>> mutex_unlock(&mcu->lock); >>> } >>> >>> -static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) >>> +static int mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) >>> { >>> struct mcu *mcu = gpiochip_get_data(gc); >>> u8 bit = 1 << (4 + gpio); >>> @@ -105,6 +105,8 @@ static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) >>> >>> i2c_smbus_write_byte_data(mcu->client, MCU_REG_CTRL, mcu->reg_ctrl); >>> mutex_unlock(&mcu->lock); >>> + >>> + return 0; >> >> i2c_smbus_write_byte_data() can fail, why not return the value returned >> by i2c_smbus_write_byte_data() ? >> > > The calls to i2c_smbus_write_byte_data() in this driver are > universally not checked. I cannot test it and wasn't sure if that's on > purpose so I decided to stay safe. Someone who has access to this > platform could potentially fix it across the file. As far as I can see this function is called three times in this file. First time is in mcu_power_off(), which must return void. Second time is inside a forever loop in shutdown_thread_fn(), and I can't see what could be done with the returned value. Last time is in the function you are changing. Wouldn't it make sense to take the value into account here ? IIUC it is the purpose of the change, isn't it ? Christophe
On Wed, Apr 30, 2025 at 7:47 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 30/04/2025 à 19:37, Bartosz Golaszewski a écrit : > > On Wed, Apr 30, 2025 at 7:33 PM Christophe Leroy > > <christophe.leroy@csgroup.eu> wrote: > >> > >> > >> > >> Le 08/04/2025 à 09:21, Bartosz Golaszewski a écrit : > >>> 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> > >>> --- > >>> arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c | 6 ++++-- > >>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > >>> index 4d8fa9ed1a67..d4ba6dbb86b2 100644 > >>> --- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > >>> +++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c > >>> @@ -92,7 +92,7 @@ static void mcu_power_off(void) > >>> mutex_unlock(&mcu->lock); > >>> } > >>> > >>> -static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > >>> +static int mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > >>> { > >>> struct mcu *mcu = gpiochip_get_data(gc); > >>> u8 bit = 1 << (4 + gpio); > >>> @@ -105,6 +105,8 @@ static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > >>> > >>> i2c_smbus_write_byte_data(mcu->client, MCU_REG_CTRL, mcu->reg_ctrl); > >>> mutex_unlock(&mcu->lock); > >>> + > >>> + return 0; > >> > >> i2c_smbus_write_byte_data() can fail, why not return the value returned > >> by i2c_smbus_write_byte_data() ? > >> > > > > The calls to i2c_smbus_write_byte_data() in this driver are > > universally not checked. I cannot test it and wasn't sure if that's on > > purpose so I decided to stay safe. Someone who has access to this > > platform could potentially fix it across the file. > > As far as I can see this function is called three times in this file. > > First time is in mcu_power_off(), which must return void. > Second time is inside a forever loop in shutdown_thread_fn(), and I > can't see what could be done with the returned value. > > Last time is in the function you are changing. Wouldn't it make sense to > take the value into account here ? IIUC it is the purpose of the change, > isn't it ? > > Christophe > Sure, I can do it. The purpose is first and foremost to convert all drivers so that we can drop the old callbacks but I see what you mean. Bart
diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c index 4d8fa9ed1a67..d4ba6dbb86b2 100644 --- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c +++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c @@ -92,7 +92,7 @@ static void mcu_power_off(void) mutex_unlock(&mcu->lock); } -static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) +static int mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) { struct mcu *mcu = gpiochip_get_data(gc); u8 bit = 1 << (4 + gpio); @@ -105,6 +105,8 @@ static void mcu_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) i2c_smbus_write_byte_data(mcu->client, MCU_REG_CTRL, mcu->reg_ctrl); mutex_unlock(&mcu->lock); + + return 0; } static int mcu_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) @@ -123,7 +125,7 @@ static int mcu_gpiochip_add(struct mcu *mcu) gc->can_sleep = 1; gc->ngpio = MCU_NUM_GPIO; gc->base = -1; - gc->set = mcu_gpio_set; + gc->set_rv = mcu_gpio_set; gc->direction_output = mcu_gpio_dir_out; gc->parent = dev;