| Message ID | 20211011132754.2479853-14-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Make some spi device drivers return zero in .remove() | expand |
On Mon, 2021-10-11 at 15:27 +0200, Uwe Kleine-König wrote: > Up to now st33zp24_remove() returns zero unconditionally. Make it return > void instead which makes it easier to see in the callers that there is > no error to handle. So, void is not a return value. > > Also the return value of i2c and spi remove callbacks is ignored anyway. ~~~ ~~~ I2C SPI > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/char/tpm/st33zp24/i2c.c | 5 +---- > drivers/char/tpm/st33zp24/spi.c | 5 +---- > drivers/char/tpm/st33zp24/st33zp24.c | 3 +-- > drivers/char/tpm/st33zp24/st33zp24.h | 2 +- > 4 files changed, 4 insertions(+), 11 deletions(-) > > diff --git a/drivers/char/tpm/st33zp24/i2c.c b/drivers/char/tpm/st33zp24/i2c.c > index 7c617edff4ca..3170d59d660c 100644 > --- a/drivers/char/tpm/st33zp24/i2c.c > +++ b/drivers/char/tpm/st33zp24/i2c.c > @@ -267,11 +267,8 @@ static int st33zp24_i2c_probe(struct i2c_client *client, > static int st33zp24_i2c_remove(struct i2c_client *client) > { > struct tpm_chip *chip = i2c_get_clientdata(client); > - int ret; > > - ret = st33zp24_remove(chip); > - if (ret) > - return ret; > + st33zp24_remove(chip); > > return 0; > } > diff --git a/drivers/char/tpm/st33zp24/spi.c b/drivers/char/tpm/st33zp24/spi.c > index a75dafd39445..ccd9e42b8eab 100644 > --- a/drivers/char/tpm/st33zp24/spi.c > +++ b/drivers/char/tpm/st33zp24/spi.c > @@ -384,11 +384,8 @@ static int st33zp24_spi_probe(struct spi_device *dev) > static int st33zp24_spi_remove(struct spi_device *dev) > { > struct tpm_chip *chip = spi_get_drvdata(dev); > - int ret; > > - ret = st33zp24_remove(chip); > - if (ret) > - return ret; > + st33zp24_remove(chip); > > return 0; > } > diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c > index 4ec10ab5e576..2b63654c38d6 100644 > --- a/drivers/char/tpm/st33zp24/st33zp24.c > +++ b/drivers/char/tpm/st33zp24/st33zp24.c > @@ -588,10 +588,9 @@ EXPORT_SYMBOL(st33zp24_probe); > * @param: tpm_data, the tpm phy. > * @return: 0 in case of success. > */ > -int st33zp24_remove(struct tpm_chip *chip) > +void st33zp24_remove(struct tpm_chip *chip) > { > tpm_chip_unregister(chip); > - return 0; > } > EXPORT_SYMBOL(st33zp24_remove); > > diff --git a/drivers/char/tpm/st33zp24/st33zp24.h b/drivers/char/tpm/st33zp24/st33zp24.h > index 6747be1e2502..b387a476c555 100644 > --- a/drivers/char/tpm/st33zp24/st33zp24.h > +++ b/drivers/char/tpm/st33zp24/st33zp24.h > @@ -34,5 +34,5 @@ int st33zp24_pm_resume(struct device *dev); > > int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops, > struct device *dev, int irq, int io_lpcpd); > -int st33zp24_remove(struct tpm_chip *chip); > +void st33zp24_remove(struct tpm_chip *chip); > #endif /* __LOCAL_ST33ZP24_H__ */ Even though this does not improve things at run-time, this does help to understand what the code does, so in that sense I do think that this a sane change to make. With an appropriate commit message, this can be applied. /Jarkko
On Tue, Oct 12, 2021 at 07:47:22PM +0300, Jarkko Sakkinen wrote: > On Mon, 2021-10-11 at 15:27 +0200, Uwe Kleine-König wrote: > > Up to now st33zp24_remove() returns zero unconditionally. Make it return > > void instead which makes it easier to see in the callers that there is > > no error to handle. > > So, void is not a return value. Hmm, would you be more happy with "Make it return no value"? I think this is less understandable than "Make it return void". Do you have a constructive suggestion how to formulate. > > Also the return value of i2c and spi remove callbacks is ignored anyway. > ~~~ ~~~ > I2C SPI I don't agree. "I2C" is fine if you mean the protocol. For the framework names I consider the small letters better, as it matches the directory name below drivers/ and also matches the function name prefix and what is usually used for short log prefixes. Ditto for spi. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/char/tpm/st33zp24/i2c.c | 5 +---- > > drivers/char/tpm/st33zp24/spi.c | 5 +---- > > drivers/char/tpm/st33zp24/st33zp24.c | 3 +-- > > drivers/char/tpm/st33zp24/st33zp24.h | 2 +- > > 4 files changed, 4 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/char/tpm/st33zp24/i2c.c b/drivers/char/tpm/st33zp24/i2c.c > > index 7c617edff4ca..3170d59d660c 100644 > > --- a/drivers/char/tpm/st33zp24/i2c.c > > +++ b/drivers/char/tpm/st33zp24/i2c.c > > @@ -267,11 +267,8 @@ static int st33zp24_i2c_probe(struct i2c_client *client, > > static int st33zp24_i2c_remove(struct i2c_client *client) > > { > > struct tpm_chip *chip = i2c_get_clientdata(client); > > - int ret; > > > > - ret = st33zp24_remove(chip); > > - if (ret) > > - return ret; > > + st33zp24_remove(chip); > > > > return 0; > > } > > diff --git a/drivers/char/tpm/st33zp24/spi.c b/drivers/char/tpm/st33zp24/spi.c > > index a75dafd39445..ccd9e42b8eab 100644 > > --- a/drivers/char/tpm/st33zp24/spi.c > > +++ b/drivers/char/tpm/st33zp24/spi.c > > @@ -384,11 +384,8 @@ static int st33zp24_spi_probe(struct spi_device *dev) > > static int st33zp24_spi_remove(struct spi_device *dev) > > { > > struct tpm_chip *chip = spi_get_drvdata(dev); > > - int ret; > > > > - ret = st33zp24_remove(chip); > > - if (ret) > > - return ret; > > + st33zp24_remove(chip); > > > > return 0; > > } > > diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c > > index 4ec10ab5e576..2b63654c38d6 100644 > > --- a/drivers/char/tpm/st33zp24/st33zp24.c > > +++ b/drivers/char/tpm/st33zp24/st33zp24.c > > @@ -588,10 +588,9 @@ EXPORT_SYMBOL(st33zp24_probe); > > * @param: tpm_data, the tpm phy. > > * @return: 0 in case of success. > > */ > > -int st33zp24_remove(struct tpm_chip *chip) > > +void st33zp24_remove(struct tpm_chip *chip) > > { > > tpm_chip_unregister(chip); > > - return 0; > > } > > EXPORT_SYMBOL(st33zp24_remove); > > > > diff --git a/drivers/char/tpm/st33zp24/st33zp24.h b/drivers/char/tpm/st33zp24/st33zp24.h > > index 6747be1e2502..b387a476c555 100644 > > --- a/drivers/char/tpm/st33zp24/st33zp24.h > > +++ b/drivers/char/tpm/st33zp24/st33zp24.h > > @@ -34,5 +34,5 @@ int st33zp24_pm_resume(struct device *dev); > > > > int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops, > > struct device *dev, int irq, int io_lpcpd); > > -int st33zp24_remove(struct tpm_chip *chip); > > +void st33zp24_remove(struct tpm_chip *chip); > > #endif /* __LOCAL_ST33ZP24_H__ */ > > Even though this does not improve things at run-time, this does > help to understand what the code does, That is for focus for now. As written in the cover letter the long term goal is to make struct spi_driver::remove return void, too. > so in that sense I do > think that this a sane change to make. Thanks Uwe
On Tue, 2021-10-12 at 19:40 +0200, Uwe Kleine-König wrote: > On Tue, Oct 12, 2021 at 07:47:22PM +0300, Jarkko Sakkinen wrote: > > On Mon, 2021-10-11 at 15:27 +0200, Uwe Kleine-König wrote: > > > Up to now st33zp24_remove() returns zero unconditionally. Make it return > > > void instead which makes it easier to see in the callers that there is > > > no error to handle. > > > > So, void is not a return value. > > Hmm, would you be more happy with "Make it return no value"? I think > this is less understandable than "Make it return void". Do you have a > constructive suggestion how to formulate. I think it is semantically more correct to say that it does not return any value, given that it does not do that :-) You can just state e.g. "Make st33zp24_remove() a void function.", it is semantically sound and not really that confusing. /Jarkko
diff --git a/drivers/char/tpm/st33zp24/i2c.c b/drivers/char/tpm/st33zp24/i2c.c index 7c617edff4ca..3170d59d660c 100644 --- a/drivers/char/tpm/st33zp24/i2c.c +++ b/drivers/char/tpm/st33zp24/i2c.c @@ -267,11 +267,8 @@ static int st33zp24_i2c_probe(struct i2c_client *client, static int st33zp24_i2c_remove(struct i2c_client *client) { struct tpm_chip *chip = i2c_get_clientdata(client); - int ret; - ret = st33zp24_remove(chip); - if (ret) - return ret; + st33zp24_remove(chip); return 0; } diff --git a/drivers/char/tpm/st33zp24/spi.c b/drivers/char/tpm/st33zp24/spi.c index a75dafd39445..ccd9e42b8eab 100644 --- a/drivers/char/tpm/st33zp24/spi.c +++ b/drivers/char/tpm/st33zp24/spi.c @@ -384,11 +384,8 @@ static int st33zp24_spi_probe(struct spi_device *dev) static int st33zp24_spi_remove(struct spi_device *dev) { struct tpm_chip *chip = spi_get_drvdata(dev); - int ret; - ret = st33zp24_remove(chip); - if (ret) - return ret; + st33zp24_remove(chip); return 0; } diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c index 4ec10ab5e576..2b63654c38d6 100644 --- a/drivers/char/tpm/st33zp24/st33zp24.c +++ b/drivers/char/tpm/st33zp24/st33zp24.c @@ -588,10 +588,9 @@ EXPORT_SYMBOL(st33zp24_probe); * @param: tpm_data, the tpm phy. * @return: 0 in case of success. */ -int st33zp24_remove(struct tpm_chip *chip) +void st33zp24_remove(struct tpm_chip *chip) { tpm_chip_unregister(chip); - return 0; } EXPORT_SYMBOL(st33zp24_remove); diff --git a/drivers/char/tpm/st33zp24/st33zp24.h b/drivers/char/tpm/st33zp24/st33zp24.h index 6747be1e2502..b387a476c555 100644 --- a/drivers/char/tpm/st33zp24/st33zp24.h +++ b/drivers/char/tpm/st33zp24/st33zp24.h @@ -34,5 +34,5 @@ int st33zp24_pm_resume(struct device *dev); int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops, struct device *dev, int irq, int io_lpcpd); -int st33zp24_remove(struct tpm_chip *chip); +void st33zp24_remove(struct tpm_chip *chip); #endif /* __LOCAL_ST33ZP24_H__ */
Up to now st33zp24_remove() returns zero unconditionally. Make it return void instead which makes it easier to see in the callers that there is no error to handle. Also the return value of i2c and spi remove callbacks is ignored anyway. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/char/tpm/st33zp24/i2c.c | 5 +---- drivers/char/tpm/st33zp24/spi.c | 5 +---- drivers/char/tpm/st33zp24/st33zp24.c | 3 +-- drivers/char/tpm/st33zp24/st33zp24.h | 2 +- 4 files changed, 4 insertions(+), 11 deletions(-)