On Tue, Nov 16, 2021 at 05:55:35PM +0200, Jarkko Sakkinen wrote:
> On Sat, 2021-11-13 at 22:53 +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Sat, Nov 13, 2021 at 12:53:32PM +0200, Jarkko Sakkinen wrote:
> > > On Fri, 2021-11-12 at 23:53 +0100, Uwe Kleine-König wrote:
> > > > tpm_cr50_i2c_remove() is only called after tpm_cr50_i2c_probe() returned
> > > > successfully. As i2c_get_clientdata() returns driver data for the
> > > > client's device and this was set in tpmm_chip_alloc() it won't return
> > > > NULL.
> > >
> > > This does not make the check obsolete, e.g. it would catch a programming
> > > error elsewhere.
> > >
> > > > Simplify accordingly to prepare changing the prototype of the i2c remove
> > > > callback to return void. Notice that already today returning an error
> > > > code from the remove callback doesn't prevent removal.
> > >
> > > I don't understand what you are trying to say.
> >
> > The eventual goal is the following change:
> >
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index 16119ac1aa97..c7069ebf5a66 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -273,7 +273,7 @@ struct i2c_driver {
> >
> > /* Standard driver model interfaces */
> > int (*probe)(struct i2c_client *client, const struct i2c_device_id *id);
> > - int (*remove)(struct i2c_client *client);
> > + void (*remove)(struct i2c_client *client);
> >
> > /* New driver model interface to aid the seamless removal of the
> > * current probe()'s, more commonly unused than used second parameter.
> >
> > To prepare that I want to change all remove callbacks to unconditionally
> > return 0.
> >
> > The motivation for the above change is that returning an error from an
> > i2c (or spi or platform) remove callback doesn't prevent the device from
> > being removed. So the ability to return an int leads to wrong
> > expectations by driver authors.
> >
> > The only effect a non-zero return code has, is an error message from the
> > i2c core. So if you object to my suggested change, the minimal change I
> > want to convince you of is to replace
> >
> > return -ENODEV;
> >
> > by
> >
> > return 0;
> >
> > .
>
> Please then include it to a patch set, where this happens.
My plan is to do all the preparation before submitting the change to
struct i2c_driver such that in the end coordination is only needed for a
single patch. (As this patch should be easy to review and without side
effects it should only drop "return 0;" (or replace them by "return;",
depending on context) to make this easy to review/verify.
Note that the suggested change has already a benefit today because in
the error case (and without the change) you get two error messages.
Returning 0 suppresses the generic (and so less useful) one.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
