[PATCH v2 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[PATCH v2 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[Hans de Goede]

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(-)

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

[PATCH v2 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Hans de Goede]

From: Jeremy Cline <jeremy@jcline.org>

Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI
device. Normally we would handle this by letting the special
drivers/platform/x86/i2c-multi-instantiate.c driver handle the BOSC0200
ACPI id and let it instantiate 2 bmc150_accel type i2c_client-s for us.

But doing so changes the modalias for the first accelerometer
(which is already supported and used on many devices) from
acpi:BOSC0200 to i2c:bmc150_accel. The modalias is not only used
to load the driver, but is also used by hwdb matches in
/lib/udev/hwdb.d/60-sensor.hwdb which provide a mountmatrix to
userspace by setting the ACCEL_MOUNT_MATRIX udev property.

Switching the handling of the BOSC0200 over to i2c-multi-instantiate.c
will break the hwdb matches causing the ACCEL_MOUNT_MATRIX udev prop
to no longer be set. So switching over to i2c-multi-instantiate.c is
not an option.

Changes by Hans de Goede:
-Add explanation to the commit message why i2c-multi-instantiate.c
 cannot be used
-Also set the dev_name, fwnode and irq i2c_board_info struct members
 for the 2nd client

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=198671
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Drop unnecessary if guarding i2c_unregister_device(second_dev)
- Add a comment explaining that the i2c_board_info dev_name being static is
  ok as there should never be more then 1 BOSC0200 ACPI node declaring 2
  accelerometers.
---
 drivers/iio/accel/bmc150-accel-core.c | 21 +++++++++++++++
 drivers/iio/accel/bmc150-accel-i2c.c  | 38 +++++++++++++++++++++++++--
 drivers/iio/accel/bmc150-accel.h      |  2 ++
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 088716d55855..2976aefad89b 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -203,6 +203,7 @@ struct bmc150_accel_data {
 	int ev_enable_state;
 	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	const struct bmc150_accel_chip_info *chip_info;
+	struct i2c_client *second_device;
 	struct iio_mount_matrix orientation;
 };
 
@@ -1659,6 +1660,26 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 }
 EXPORT_SYMBOL_GPL(bmc150_accel_core_probe);
 
+struct i2c_client *bmc150_get_second_device(struct i2c_client *client)
+{
+	struct bmc150_accel_data *data = i2c_get_clientdata(client);
+
+	if (!data)
+		return NULL;
+
+	return data->second_device;
+}
+EXPORT_SYMBOL_GPL(bmc150_get_second_device);
+
+void bmc150_set_second_device(struct i2c_client *client)
+{
+	struct bmc150_accel_data *data = i2c_get_clientdata(client);
+
+	if (data)
+		data->second_device = client;
+}
+EXPORT_SYMBOL_GPL(bmc150_set_second_device);
+
 int bmc150_accel_core_remove(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 06021c8685a7..96ded8cd2bb8 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -29,6 +29,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
 		i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
 		i2c_check_functionality(client->adapter,
 					I2C_FUNC_SMBUS_READ_I2C_BLOCK);
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+	int ret;
 
 	regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf);
 	if (IS_ERR(regmap)) {
@@ -39,12 +41,44 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	if (id)
 		name = id->name;
 
-	return bmc150_accel_core_probe(&client->dev, regmap, client->irq, name,
-				       block_supported);
+	ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, block_supported);
+	if (ret)
+		return ret;
+
+	/*
+	 * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI
+	 * device, try instantiating a second i2c_client for an I2cSerialBusV2
+	 * ACPI resource with index 1. The !id check avoids recursion when
+	 * bmc150_accel_probe() gets called for the second client.
+	 */
+	if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) {
+		struct i2c_board_info board_info = {
+			.type = "bmc150_accel",
+			/*
+			 * The 2nd accel sits in the base of 2-in-1s. Note this
+			 * name is static, as there should never be more then 1
+			 * BOSC0200 ACPI node with 2 accelerometers in it.
+			 */
+			.dev_name = "BOSC0200:base",
+			.fwnode = client->dev.fwnode,
+			.irq = -ENOENT,
+		};
+		struct i2c_client *second_dev;
+
+		second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
+		if (!IS_ERR(second_dev))
+			bmc150_set_second_device(second_dev);
+	}
+
+	return 0;
 }
 
 static int bmc150_accel_remove(struct i2c_client *client)
 {
+	struct i2c_client *second_dev = bmc150_get_second_device(client);
+
+	i2c_unregister_device(second_dev);
+
 	return bmc150_accel_core_remove(&client->dev);
 }
 
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index ae6118ae11b1..6e965a3ca322 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -16,6 +16,8 @@ enum {
 int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 			    const char *name, bool block_supported);
 int bmc150_accel_core_remove(struct device *dev);
+struct i2c_client *bmc150_get_second_device(struct i2c_client *second_device);
+void bmc150_set_second_device(struct i2c_client *second_device);
 extern const struct dev_pm_ops bmc150_accel_pm_ops;
 extern const struct regmap_config bmc150_regmap_conf;
 
-- 
2.28.0

[PATCH v2 3/3] iio: accel: bmc150: Get mount-matrix from ACPI

[Hans de Goede]

bmc150 accelerometers with an ACPI hardware-id of BOSC0200 have an ACPI
method providing their mount-matrix, add support for retrieving this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add a big comment adding some more info about when which ACPI object is
  used to get the mount-matrix and an (incomplete) list of devices known to
  use this
---
 drivers/iio/accel/bmc150-accel-core.c | 107 +++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 2976aefad89b..d61cc0ec4cfd 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -410,6 +410,103 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
 }
 #endif
 
+#ifdef CONFIG_ACPI
+/*
+ * Support for getting accelerometer information from BOSC0200 ACPI nodes.
+ *
+ * There are 2 variants of the BOSC0200 ACPI node. Some 2-in-1s with 360 degree
+ * hinges declare 2 I2C ACPI-resources for 2 accelerometers, 1 in the display
+ * and 1 in the base of the 2-in-1. On these 2-in-1s the ROMS ACPI object
+ * contains the mount-matrix for the sensor in the display and ROMK contains
+ * the mount-matrix for the sensor in the base. On devices using a single
+ * sensor there is a ROTM ACPI object which contains the mount-matrix.
+ *
+ * Here is an incomplete list of devices known to use 1 of these setups:
+ *
+ * Yoga devices with 2 accelerometers using ROMS + ROMK for the mount-matrices:
+ * Lenovo Thinkpad Yoga 11e 3th gen
+ * Lenovo Thinkpad Yoga 11e 4th gen
+ *
+ * Tablets using a single accelerometer using ROTM for the mount-matrix:
+ * Chuwi Hi8 Pro (CWI513)
+ * Chuwi Vi8 Plus (CWI519)
+ * Chuwi Hi13
+ * Irbis TW90
+ * Jumper EZpad mini 3
+ * Onda V80 plus
+ * Predia Basic Tablet
+ */
+static bool bmc150_apply_acpi_orientation(struct device *dev,
+					  struct iio_mount_matrix *orientation)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	union acpi_object *obj, *elements;
+	char *name, *alt_name, *str;
+	acpi_status status;
+	int i, j, val[3];
+
+	if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
+		return false;
+
+	if (strcmp(dev_name(dev), "i2c-BOSC0200:base") == 0)
+		alt_name = "ROMK";
+	else
+		alt_name = "ROMS";
+
+	if (acpi_has_method(adev->handle, "ROTM"))
+		name = "ROTM";
+	else if (acpi_has_method(adev->handle, alt_name))
+		name = alt_name;
+	else
+		return false;
+
+	status = acpi_evaluate_object(adev->handle, name, NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(dev, "Failed to get ACPI mount matrix: %d\n", status);
+		return false;
+	}
+
+	obj = buffer.pointer;
+	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3)
+		goto unknown_format;
+
+	elements = obj->package.elements;
+	for (i = 0; i < 3; i++) {
+		if (elements[i].type != ACPI_TYPE_STRING)
+			goto unknown_format;
+
+		str = elements[i].string.pointer;
+		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3)
+			goto unknown_format;
+
+		for (j = 0; j < 3; j++) {
+			switch (val[j]) {
+			case -1: str = "-1"; break;
+			case 0:  str = "0";  break;
+			case 1:  str = "1";  break;
+			default: goto unknown_format;
+			}
+			orientation->rotation[i * 3 + j] = str;
+		}
+	}
+
+	kfree(buffer.pointer);
+	return true;
+
+unknown_format:
+	dev_warn(dev, "Unknown ACPI mount matrix format, ignoring\n");
+	kfree(buffer.pointer);
+	return false;
+}
+#else
+static bool bmc150_apply_acpi_orientation(struct device *dev,
+					  struct iio_mount_matrix *orientation)
+{
+	return false;
+}
+#endif
+
 static const struct bmc150_accel_interrupt_info {
 	u8 map_reg;
 	u8 map_bitmask;
@@ -1571,10 +1668,12 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 
 	data->regmap = regmap;
 
-	ret = iio_read_mount_matrix(dev, "mount-matrix",
-				     &data->orientation);
-	if (ret)
-		return ret;
+	if (!bmc150_apply_acpi_orientation(dev, &data->orientation)) {
+		ret = iio_read_mount_matrix(dev, "mount-matrix",
+					     &data->orientation);
+		if (ret)
+			return ret;
+	}
 
 	ret = bmc150_accel_chip_init(data);
 	if (ret < 0)
-- 
2.28.0

Re: [PATCH v2 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[Andy Shevchenko]

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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[Jonathan Cameron]

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
> >  
> 
> 

Re: [PATCH v2 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[Hans de Goede]

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
>>>  
>>
>>
> 

Re: [PATCH v2 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[Hans de Goede]

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
>>>  
>>
>>
> 

Re: [PATCH v2 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko