On Tue, Nov 16, 2021 at 06:30:39PM +0100, Uwe Kleine-König wrote:
> 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.
Either this was not convincing or this patch fell through the cracks.
Whatever it was, nobody replied and the patch isn't applied either.
Would you please (re)consider this patch?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
