| Message ID | 20201130141954.339805-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member | expand |
On Mon, Nov 30, 2020 at 4:20 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The bmc150_accel_dat struct irq member is only ever used inside > bmc150_accel_core_probe, drop it and just use the function argument > directly. FWIW, for all three Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/iio/accel/bmc150-accel-core.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > index 48435865fdaf..088716d55855 100644 > --- a/drivers/iio/accel/bmc150-accel-core.c > +++ b/drivers/iio/accel/bmc150-accel-core.c > @@ -183,7 +183,6 @@ enum bmc150_accel_trigger_id { > > struct bmc150_accel_data { > struct regmap *regmap; > - int irq; > struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS]; > struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS]; > struct mutex mutex; > @@ -1568,7 +1567,6 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > > data = iio_priv(indio_dev); > dev_set_drvdata(dev, indio_dev); > - data->irq = irq; > > data->regmap = regmap; > > @@ -1599,9 +1597,8 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > return ret; > } > > - if (data->irq > 0) { > - ret = devm_request_threaded_irq( > - dev, data->irq, > + if (irq > 0) { > + ret = devm_request_threaded_irq(dev, irq, > bmc150_accel_irq_handler, > bmc150_accel_irq_thread_handler, > IRQF_TRIGGER_RISING, > -- > 2.28.0 >
On Mon, 30 Nov 2020 16:32:21 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Nov 30, 2020 at 4:20 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > The bmc150_accel_dat struct irq member is only ever used inside > > bmc150_accel_core_probe, drop it and just use the function argument > > directly. > > FWIW, for all three > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> This crossed with a series adding regulator control to this driver, but I'm fairly sure that won't cause any problems so I've dealt with the fuzz and applied it anyway. However... drivers/iio/accel/bmc150-accel-i2c.c: In function ‘bmc150_accel_probe’: drivers/iio/accel/bmc150-accel-i2c.c:55:28: error: implicit declaration of function ‘acpi_device_hid’; did you mean ‘dmi_device_id’? [-Werror=implicit-function-declarati on] 55 | if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { | ^~~~~~~~~~~~~~~ | dmi_device_id drivers/iio/accel/bmc150-accel-i2c.c:55:28: warning: passing argument 1 of ‘strcmp’ makes pointer from integer without a cast [-Wint-conversion] 55 | if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { | ^~~~~~~~~~~~~~~~~~~~~ | | | int I've added #ifdef CONFIG_ACPI around the relevant block and shuffled around assignment of adev + added a __maybe_unused marking to it. Perhaps I should have pulled that block out into another function but it seemed more trouble than it was worth. I'm slightly confused on how I ended up with a test .config that doesn't have CONFIG_ACPI but that's another story and handy on this occasion as we didn't have to wait for 0-day to notice this. Please sanity check I didn't mess it up. Series applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to see what else we missed. Thanks Jonathan > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/iio/accel/bmc150-accel-core.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > > index 48435865fdaf..088716d55855 100644 > > --- a/drivers/iio/accel/bmc150-accel-core.c > > +++ b/drivers/iio/accel/bmc150-accel-core.c > > @@ -183,7 +183,6 @@ enum bmc150_accel_trigger_id { > > > > struct bmc150_accel_data { > > struct regmap *regmap; > > - int irq; > > struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS]; > > struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS]; > > struct mutex mutex; > > @@ -1568,7 +1567,6 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > > > > data = iio_priv(indio_dev); > > dev_set_drvdata(dev, indio_dev); > > - data->irq = irq; > > > > data->regmap = regmap; > > > > @@ -1599,9 +1597,8 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > > return ret; > > } > > > > - if (data->irq > 0) { > > - ret = devm_request_threaded_irq( > > - dev, data->irq, > > + if (irq > 0) { > > + ret = devm_request_threaded_irq(dev, irq, > > bmc150_accel_irq_handler, > > bmc150_accel_irq_thread_handler, > > IRQF_TRIGGER_RISING, > > -- > > 2.28.0 > > > >
Hi, On 11/30/20 9:46 PM, Jonathan Cameron wrote: > On Mon, 30 Nov 2020 16:32:21 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Mon, Nov 30, 2020 at 4:20 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> The bmc150_accel_dat struct irq member is only ever used inside >>> bmc150_accel_core_probe, drop it and just use the function argument >>> directly. >> >> FWIW, for all three >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > This crossed with a series adding regulator control to this driver, but I'm fairly > sure that won't cause any problems so I've dealt with the fuzz and applied it anyway. > > However... > > drivers/iio/accel/bmc150-accel-i2c.c: In function ‘bmc150_accel_probe’: > drivers/iio/accel/bmc150-accel-i2c.c:55:28: error: implicit declaration of function ‘acpi_device_hid’; did you mean ‘dmi_device_id’? [-Werror=implicit-function-declarati > on] > 55 | if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > | ^~~~~~~~~~~~~~~ > | dmi_device_id > drivers/iio/accel/bmc150-accel-i2c.c:55:28: warning: passing argument 1 of ‘strcmp’ makes pointer from integer without a cast [-Wint-conversion] > 55 | if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > | ^~~~~~~~~~~~~~~~~~~~~ > | | > | int > > > I've added #ifdef CONFIG_ACPI around the relevant block and shuffled around assignment of > adev + added a __maybe_unused marking to it. Perhaps I should have pulled that block > out into another function but it seemed more trouble than it was worth. Thanks, the fixed up version looks good. And sorry for not catching this myself, some of the ACPI functions are stubbed when CONFIG_ACPI is not set and I was relying on this here. But I missed acpi_device_hid() not being stubbed. I was aware I was using some unstubbed things in patch 3/3 so that already had the #if CONFIG_ACPI. > Please sanity check I didn't mess it up. Done, I'll also add the final version to my local tree so that its get tested when I spin that up on one of the affected devices. > Series applied to the togreg branch of iio.git and pushed out as testing for > the autobuilders to see what else we missed. Thanks & Regards, Hans >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/iio/accel/bmc150-accel-core.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c >>> index 48435865fdaf..088716d55855 100644 >>> --- a/drivers/iio/accel/bmc150-accel-core.c >>> +++ b/drivers/iio/accel/bmc150-accel-core.c >>> @@ -183,7 +183,6 @@ enum bmc150_accel_trigger_id { >>> >>> struct bmc150_accel_data { >>> struct regmap *regmap; >>> - int irq; >>> struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS]; >>> struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS]; >>> struct mutex mutex; >>> @@ -1568,7 +1567,6 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, >>> >>> data = iio_priv(indio_dev); >>> dev_set_drvdata(dev, indio_dev); >>> - data->irq = irq; >>> >>> data->regmap = regmap; >>> >>> @@ -1599,9 +1597,8 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, >>> return ret; >>> } >>> >>> - if (data->irq > 0) { >>> - ret = devm_request_threaded_irq( >>> - dev, data->irq, >>> + if (irq > 0) { >>> + ret = devm_request_threaded_irq(dev, irq, >>> bmc150_accel_irq_handler, >>> bmc150_accel_irq_thread_handler, >>> IRQF_TRIGGER_RISING, >>> -- >>> 2.28.0 >>> >> >> >
Hi, On 11/30/20 9:46 PM, Jonathan Cameron wrote: > On Mon, 30 Nov 2020 16:32:21 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Mon, Nov 30, 2020 at 4:20 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> The bmc150_accel_dat struct irq member is only ever used inside >>> bmc150_accel_core_probe, drop it and just use the function argument >>> directly. >> >> FWIW, for all three >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > This crossed with a series adding regulator control to this driver, but I'm fairly > sure that won't cause any problems so I've dealt with the fuzz and applied it anyway. > > However... > > drivers/iio/accel/bmc150-accel-i2c.c: In function ‘bmc150_accel_probe’: > drivers/iio/accel/bmc150-accel-i2c.c:55:28: error: implicit declaration of function ‘acpi_device_hid’; did you mean ‘dmi_device_id’? [-Werror=implicit-function-declarati > on] > 55 | if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > | ^~~~~~~~~~~~~~~ > | dmi_device_id > drivers/iio/accel/bmc150-accel-i2c.c:55:28: warning: passing argument 1 of ‘strcmp’ makes pointer from integer without a cast [-Wint-conversion] > 55 | if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > | ^~~~~~~~~~~~~~~~~~~~~ > | | > | int > > > I've added #ifdef CONFIG_ACPI around the relevant block and shuffled around assignment of > adev + added a __maybe_unused marking to it. Perhaps I should have pulled that block > out into another function but it seemed more trouble than it was worth. > > I'm slightly confused on how I ended up with a test .config that doesn't have CONFIG_ACPI > but that's another story and handy on this occasion as we didn't have to wait for 0-day > to notice this. > > Please sanity check I didn't mess it up. FWIW I decided to go the full mile and since I cherry-picked the final versions of the patches (including the regulator changes) into my local tree anyway I decided to spin it up on a Thinkpad Yoga 11e 4th gen which has the dual accelerometer setup. I'm happy to report that the final version of the patches work as advertised there. Regards, Hans >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/iio/accel/bmc150-accel-core.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c >>> index 48435865fdaf..088716d55855 100644 >>> --- a/drivers/iio/accel/bmc150-accel-core.c >>> +++ b/drivers/iio/accel/bmc150-accel-core.c >>> @@ -183,7 +183,6 @@ enum bmc150_accel_trigger_id { >>> >>> struct bmc150_accel_data { >>> struct regmap *regmap; >>> - int irq; >>> struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS]; >>> struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS]; >>> struct mutex mutex; >>> @@ -1568,7 +1567,6 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, >>> >>> data = iio_priv(indio_dev); >>> dev_set_drvdata(dev, indio_dev); >>> - data->irq = irq; >>> >>> data->regmap = regmap; >>> >>> @@ -1599,9 +1597,8 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, >>> return ret; >>> } >>> >>> - if (data->irq > 0) { >>> - ret = devm_request_threaded_irq( >>> - dev, data->irq, >>> + if (irq > 0) { >>> + ret = devm_request_threaded_irq(dev, irq, >>> bmc150_accel_irq_handler, >>> bmc150_accel_irq_thread_handler, >>> IRQF_TRIGGER_RISING, >>> -- >>> 2.28.0 >>> >> >> >
On Tue, Dec 1, 2020 at 2:23 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/30/20 9:46 PM, Jonathan Cameron wrote: ... > And sorry for not catching this myself, some of the ACPI functions are stubbed > when CONFIG_ACPI is not set and I was relying on this here. But I missed acpi_device_hid() > not being stubbed. > > I was aware I was using some unstubbed things in patch 3/3 so that already had the #if CONFIG_ACPI. acpi_dev_hid_uid_match() has a stub.
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c index 48435865fdaf..088716d55855 100644 --- a/drivers/iio/accel/bmc150-accel-core.c +++ b/drivers/iio/accel/bmc150-accel-core.c @@ -183,7 +183,6 @@ enum bmc150_accel_trigger_id { struct bmc150_accel_data { struct regmap *regmap; - int irq; struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS]; struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS]; struct mutex mutex; @@ -1568,7 +1567,6 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, data = iio_priv(indio_dev); dev_set_drvdata(dev, indio_dev); - data->irq = irq; data->regmap = regmap; @@ -1599,9 +1597,8 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, return ret; } - if (data->irq > 0) { - ret = devm_request_threaded_irq( - dev, data->irq, + if (irq > 0) { + ret = devm_request_threaded_irq(dev, irq, bmc150_accel_irq_handler, bmc150_accel_irq_thread_handler, IRQF_TRIGGER_RISING,
The bmc150_accel_dat struct irq member is only ever used inside bmc150_accel_core_probe, drop it and just use the function argument directly. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/iio/accel/bmc150-accel-core.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)