Re: [PATCH v2 05/13] i2c: acpi: Return error pointers from i2c_acpi_new_device()

[Hans de Goede]

Hi,

On 27-11-18 10:04, Mika Westerberg wrote:
> On Mon, Nov 26, 2018 at 05:08:50PM +0200, Andy Shevchenko wrote:
> > The caller would like to know the reason why the i2c_acpi_new_device() fails.
> > For example, if adapter is not available, it might be in the future and we
> > would like to re-probe the clients again. But at the same time we would like to
> > bail out if the error seems unrecoverable, such as out of memory condition.
> > To achieve this, return error pointer in some cases.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> >   drivers/i2c/i2c-core-acpi.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > index 32affd3fa8bd..af4b5bd5d973 100644
> > --- a/drivers/i2c/i2c-core-acpi.c
> > +++ b/drivers/i2c/i2c-core-acpi.c
> > @@ -387,6 +387,7 @@ struct notifier_block i2c_acpi_notifier = {
> >    * Also see i2c_new_device, which this function calls to create the i2c-client.
> >    *
> >    * Returns a pointer to the new i2c-client, or NULL if the adapter is not found.
> > + * In some cases might return an error pointer.
>
> I would rather make it return error pointer always. Then the caller can
> just check for IS_ERR() and not need to deal with the possible NULL. It
> is also more consistent that way than saying "some cases might return an
> error pointer" (but some cases you get NULL or even pointer to the
> created object) ;-)

Good one, that will allow for a nice cleanup of the callers, we can make
i2c_acpi_new_device return -EPROBE_DEFER when the i2c_acpi_find_adapter_by_handle()
call fails, which is exactly the case when we want to defer.

One problem is that i2c_new_device() currently simply returns NULL on all
errors. Andy, you could take a look how much work it is to make that return
an ERR_PTR too, or just check its return value and return ERR_PTR(-ENXIO) if
it fails for now...

Regards,

Hans