Hello,
On Thu, Mar 31, 2022 at 03:22:31PM +0200, Uwe Kleine-König wrote:
> 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?
More than three weeks later still no feedback :-\
Please consider applying the patch.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
