Re: [PATCH 13/13] tpm: st33zp24: Make st33zp24_remove() return void

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]