| Message ID | 20211025094119.82967-9-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data | expand |
On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The discrete.c code is not the only code which needs to lookup the > acpi_device and device-name for the sensor for which the INT3472 > ACPI-device is a GPIO/clk/regulator provider. > > The tps68470.c code also needs this functionality, so factor this > out into a new get_sensor_adev_and_name() helper. ... > +int skl_int3472_get_sensor_adev_and_name(struct device *dev, > + struct acpi_device **sensor_adev_ret, > + const char **name_ret) > +{ > + struct acpi_device *adev = ACPI_COMPANION(dev); > + struct acpi_device *sensor; > + int ret = 0; > + > + sensor = acpi_dev_get_first_consumer_dev(adev); > + if (!sensor) { > + dev_err(dev, "INT3472 seems to have no dependents.\n"); > + return -ENODEV; > + } > + > + *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, > + acpi_dev_name(sensor)); > + if (!*name_ret) > + ret = -ENOMEM; > + > + if (ret == 0 && sensor_adev_ret) > + *sensor_adev_ret = sensor; > + else > + acpi_dev_put(sensor); > + > + return ret; The error path is twisted a bit including far staying ret=0 assignment. Can it be int ret; ... *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(sensor)); if (!*name_ret) { acpi_dev_put(sensor); return -ENOMEM; } if (sensor_adev_ret) *sensor_adev_ret = sensor; return 0; ? > +}
Hi, On 10/25/21 13:31, Andy Shevchenko wrote: > On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> The discrete.c code is not the only code which needs to lookup the >> acpi_device and device-name for the sensor for which the INT3472 >> ACPI-device is a GPIO/clk/regulator provider. >> >> The tps68470.c code also needs this functionality, so factor this >> out into a new get_sensor_adev_and_name() helper. > > ... > >> +int skl_int3472_get_sensor_adev_and_name(struct device *dev, >> + struct acpi_device **sensor_adev_ret, >> + const char **name_ret) >> +{ >> + struct acpi_device *adev = ACPI_COMPANION(dev); >> + struct acpi_device *sensor; >> + int ret = 0; >> + >> + sensor = acpi_dev_get_first_consumer_dev(adev); >> + if (!sensor) { >> + dev_err(dev, "INT3472 seems to have no dependents.\n"); >> + return -ENODEV; >> + } >> + >> + *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, >> + acpi_dev_name(sensor)); >> + if (!*name_ret) >> + ret = -ENOMEM; >> + >> + if (ret == 0 && sensor_adev_ret) >> + *sensor_adev_ret = sensor; >> + else >> + acpi_dev_put(sensor); >> + >> + return ret; > > The error path is twisted a bit including far staying ret=0 assignment. > > Can it be > > int ret; > ... > *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, > acpi_dev_name(sensor)); > if (!*name_ret) { > acpi_dev_put(sensor); > return -ENOMEM; > } > > if (sensor_adev_ret) > *sensor_adev_ret = sensor; > > return 0; > > ? That misses an acpi_dev_put(sensor) when sensor_adev_ret == NULL. Regards, Hans
On Mon, Nov 1, 2021 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 10/25/21 13:31, Andy Shevchenko wrote: > > On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> +int skl_int3472_get_sensor_adev_and_name(struct device *dev, > >> + struct acpi_device **sensor_adev_ret, > >> + const char **name_ret) > >> +{ > >> + struct acpi_device *adev = ACPI_COMPANION(dev); > >> + struct acpi_device *sensor; > >> + int ret = 0; > >> + > >> + sensor = acpi_dev_get_first_consumer_dev(adev); > >> + if (!sensor) { > >> + dev_err(dev, "INT3472 seems to have no dependents.\n"); > >> + return -ENODEV; > >> + } > >> + > >> + *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, > >> + acpi_dev_name(sensor)); > >> + if (!*name_ret) > >> + ret = -ENOMEM; > >> + > >> + if (ret == 0 && sensor_adev_ret) > >> + *sensor_adev_ret = sensor; > >> + else > >> + acpi_dev_put(sensor); > >> + > >> + return ret; > > > > The error path is twisted a bit including far staying ret=0 assignment. > > > > Can it be > > > > int ret; > > ... > > *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, > > acpi_dev_name(sensor)); > > if (!*name_ret) { > > acpi_dev_put(sensor); > > return -ENOMEM; > > } > > > > if (sensor_adev_ret) > > *sensor_adev_ret = sensor; > > > > return 0; > > > > ? > > That misses an acpi_dev_put(sensor) when sensor_adev_ret == NULL. else acpi_dev_put(...); ?
On Mon, Nov 1, 2021 at 12:44 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Nov 1, 2021 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 10/25/21 13:31, Andy Shevchenko wrote: > > > On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > > >> + if (ret == 0 && sensor_adev_ret) > > >> + *sensor_adev_ret = sensor; > > >> + else > > >> + acpi_dev_put(sensor); > > >> + > > >> + return ret; ... > > > if (sensor_adev_ret) > > > *sensor_adev_ret = sensor; > > > > > > return 0; > > > > > > ? > > > > That misses an acpi_dev_put(sensor) when sensor_adev_ret == NULL. > > else > acpi_dev_put(...); > > ? Hmm... But then in the original code and with this proposal the acpi_dev_put() seems a bit strange to me. If we are fine (no error code returned) why would the caller (note _er_) go different paths?
Hi, On 11/1/21 11:44, Andy Shevchenko wrote: > On Mon, Nov 1, 2021 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 10/25/21 13:31, Andy Shevchenko wrote: >>> On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>>> +int skl_int3472_get_sensor_adev_and_name(struct device *dev, >>>> + struct acpi_device **sensor_adev_ret, >>>> + const char **name_ret) >>>> +{ >>>> + struct acpi_device *adev = ACPI_COMPANION(dev); >>>> + struct acpi_device *sensor; >>>> + int ret = 0; >>>> + >>>> + sensor = acpi_dev_get_first_consumer_dev(adev); >>>> + if (!sensor) { >>>> + dev_err(dev, "INT3472 seems to have no dependents.\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, >>>> + acpi_dev_name(sensor)); >>>> + if (!*name_ret) >>>> + ret = -ENOMEM; >>>> + >>>> + if (ret == 0 && sensor_adev_ret) >>>> + *sensor_adev_ret = sensor; >>>> + else >>>> + acpi_dev_put(sensor); >>>> + >>>> + return ret; >>> >>> The error path is twisted a bit including far staying ret=0 assignment. >>> >>> Can it be >>> >>> int ret; >>> ... >>> *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, >>> acpi_dev_name(sensor)); >>> if (!*name_ret) { >>> acpi_dev_put(sensor); >>> return -ENOMEM; >>> } >>> >>> if (sensor_adev_ret) >>> *sensor_adev_ret = sensor; >>> >>> return 0; >>> >>> ? >> >> That misses an acpi_dev_put(sensor) when sensor_adev_ret == NULL. > > else > acpi_dev_put(...); Then we have 2 acpi_dev_put() paths, IMHO the original code which clearly states that we keep the ref: if (success && returning-the-ref) and put the ref in all other cases is better then having 2 separate put paths. Regards, Hans
Hi, On 11/1/21 11:46, Andy Shevchenko wrote: > On Mon, Nov 1, 2021 at 12:44 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Mon, Nov 1, 2021 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> On 10/25/21 13:31, Andy Shevchenko wrote: >>>> On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>>>> + if (ret == 0 && sensor_adev_ret) >>>>> + *sensor_adev_ret = sensor; >>>>> + else >>>>> + acpi_dev_put(sensor); >>>>> + >>>>> + return ret; > > ... > >>>> if (sensor_adev_ret) >>>> *sensor_adev_ret = sensor; >>>> >>>> return 0; >>>> >>>> ? >>> >>> That misses an acpi_dev_put(sensor) when sensor_adev_ret == NULL. >> >> else >> acpi_dev_put(...); >> >> ? > > Hmm... But then in the original code and with this proposal the > acpi_dev_put() seems a bit strange to me. > If we are fine (no error code returned) why would the caller (note > _er_) go different paths? We always need to get the dev to get the name, but some callers are only interested in the name, so they pass NULL for sensor_adev_ret, this helps to keep the code calling this clean, which is the whole idea of having a helper for this. Regards, Hans
On Mon, Nov 1, 2021 at 12:49 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/1/21 11:46, Andy Shevchenko wrote: > > On Mon, Nov 1, 2021 at 12:44 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: ... > > Hmm... But then in the original code and with this proposal the > > acpi_dev_put() seems a bit strange to me. > > If we are fine (no error code returned) why would the caller (note > > _er_) go different paths? > > We always need to get the dev to get the name, but some callers are > only interested in the name, so they pass NULL for sensor_adev_ret, > this helps to keep the code calling this clean, which is the whole > idea of having a helper for this. OK. (Not that I'm very happy with this, but... the function needs a comment about this alternative behaviour, I forgot if it already has, though)
diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c index 350655a9515b..77cf058e4168 100644 --- a/drivers/platform/x86/intel/int3472/common.c +++ b/drivers/platform/x86/intel/int3472/common.c @@ -52,3 +52,31 @@ int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb) kfree(obj); return ret; } + +/* sensor_adev_ret may be NULL, name_ret must not be NULL */ +int skl_int3472_get_sensor_adev_and_name(struct device *dev, + struct acpi_device **sensor_adev_ret, + const char **name_ret) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + struct acpi_device *sensor; + int ret = 0; + + sensor = acpi_dev_get_first_consumer_dev(adev); + if (!sensor) { + dev_err(dev, "INT3472 seems to have no dependents.\n"); + return -ENODEV; + } + + *name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT, + acpi_dev_name(sensor)); + if (!*name_ret) + ret = -ENOMEM; + + if (ret == 0 && sensor_adev_ret) + *sensor_adev_ret = sensor; + else + acpi_dev_put(sensor); + + return ret; +} diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h index d14944ee8586..53270d19c73a 100644 --- a/drivers/platform/x86/intel/int3472/common.h +++ b/drivers/platform/x86/intel/int3472/common.h @@ -108,6 +108,9 @@ struct int3472_discrete_device { union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev, char *id); int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb); +int skl_int3472_get_sensor_adev_and_name(struct device *dev, + struct acpi_device **sensor_adev_ret, + const char **name_ret); int skl_int3472_register_clock(struct int3472_discrete_device *int3472); void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472); diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index d2e8a87a077e..ff2bdbb8722c 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -363,19 +363,10 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev) int3472->dev = &pdev->dev; platform_set_drvdata(pdev, int3472); - int3472->sensor = acpi_dev_get_first_consumer_dev(adev); - if (!int3472->sensor) { - dev_err(&pdev->dev, "INT3472 seems to have no dependents.\n"); - return -ENODEV; - } - - int3472->sensor_name = devm_kasprintf(int3472->dev, GFP_KERNEL, - I2C_DEV_NAME_FORMAT, - acpi_dev_name(int3472->sensor)); - if (!int3472->sensor_name) { - ret = -ENOMEM; - goto err_put_sensor; - } + ret = skl_int3472_get_sensor_adev_and_name(&pdev->dev, &int3472->sensor, + &int3472->sensor_name); + if (ret) + return ret; /* * Initialising this list means we can call gpiod_remove_lookup_table() @@ -390,11 +381,6 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev) } return 0; - -err_put_sensor: - acpi_dev_put(int3472->sensor); - - return ret; } static int skl_int3472_discrete_remove(struct platform_device *pdev)
The discrete.c code is not the only code which needs to lookup the acpi_device and device-name for the sensor for which the INT3472 ACPI-device is a GPIO/clk/regulator provider. The tps68470.c code also needs this functionality, so factor this out into a new get_sensor_adev_and_name() helper. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/intel/int3472/common.c | 28 +++++++++++++++++++ drivers/platform/x86/intel/int3472/common.h | 3 ++ drivers/platform/x86/intel/int3472/discrete.c | 22 +++------------ 3 files changed, 35 insertions(+), 18 deletions(-)