| Message ID | 20181126150858.16901-6-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | i2c-multi-instantiate: Adapt for INT3515 and alike | expand |
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@linux.intel.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > --- > 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) ;-)
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@linux.intel.com> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> --- >> 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
On Tue, Nov 27, 2018 at 10:16:25AM +0100, Hans de Goede wrote: > 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... I would use -ENODEV here and -EINVAL in case there is no ACPI companion :)
On Tue, Nov 27, 2018 at 01:49:53PM +0200, Mika Westerberg wrote: > On Tue, Nov 27, 2018 at 10:16:25AM +0100, Hans de Goede wrote: > > 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... > > I would use -ENODEV here and -EINVAL in case there is no ACPI companion :) Sounds more traditional than ENXIO. I would go the way Mika proposed if there is no objection.
Hi, On 27-11-18 14:46, Andy Shevchenko wrote: > On Tue, Nov 27, 2018 at 01:49:53PM +0200, Mika Westerberg wrote: >> On Tue, Nov 27, 2018 at 10:16:25AM +0100, Hans de Goede wrote: >>> 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... >> >> I would use -ENODEV here and -EINVAL in case there is no ACPI companion :) > > Sounds more traditional than ENXIO. > I would go the way Mika proposed if there is no objection. Works for me, go for it. Regards, Han
On Tue, Nov 27, 2018 at 04:24:41PM +0100, Hans de Goede wrote: > On 27-11-18 14:46, Andy Shevchenko wrote: > > On Tue, Nov 27, 2018 at 01:49:53PM +0200, Mika Westerberg wrote: > > > On Tue, Nov 27, 2018 at 10:16:25AM +0100, Hans de Goede wrote: > > > > 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... > > > > > > I would use -ENODEV here and -EINVAL in case there is no ACPI companion :) > > > > Sounds more traditional than ENXIO. > > I would go the way Mika proposed if there is no objection. > > Works for me, go for it. Just sent a new version, but dropped your Rb in that very patch. Please, check if everything is okay.
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. */ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, struct i2c_board_info *info) @@ -399,7 +400,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, adev = ACPI_COMPANION(dev); if (!adev) - return NULL; + return ERR_PTR(-ENODEV); memset(&lookup, 0, sizeof(lookup)); lookup.info = info; @@ -409,9 +410,11 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, ret = acpi_dev_get_resources(adev, &resource_list, i2c_acpi_fill_info, &lookup); acpi_dev_free_resource_list(&resource_list); + if (ret < 0) + return ERR_PTR(ret); - if (ret < 0 || !info->addr) - return NULL; + if (!info->addr) + return ERR_PTR(-EADDRNOTAVAIL); adapter = i2c_acpi_find_adapter_by_handle(lookup.adapter_handle); if (!adapter)