[PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi All,

This series really consists of 2 series, patches 1-5 add support for
interesting ACPI tables which describe multiple i2c chips in a single
fwnode, sometimes multiple cases of the same chip on different addresses,
sometimes a bunch of related chips.

Andy Shevchenko has come up with the solution of adding a quirk based
on the ACPI HID of the fwnode for these devices which makes the
drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices
for each I2cSerialBusV2 in the fwnode. I agree with him that this is
the best (least ugly) solution for this.

I've been testing this solution on a device if mine which needs a solution
for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode
with a HID of BSG1160 which describes 3 different i2c sensors in an accel /
magneto / gyro sensor cluster on the tablet. This has let to some extra
prep. patches and some fixes to Andy's patches.

Patches 6-9 use the new functionality creating  one i2c-client per
I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and
are posted as part of this series to show how this functionality can be
used.

Assuming everyone is ok with this series (I'm not expecting anyone to be
really happy about the need for this), then I suggest that patches 1-6
get merged togther through either the ACPI or the i2c tree, I guess the
i2c tree would make somewhat more sense, since most patches are there.

Then once those are accepted patches 7-9 can be merged into the iio tree,
there is no compile time dependency between the 2, so these can be merged
separately. Note merging 7-9 before there is agreement that this is the
right way to fix this is probably not a good idea.

Regards,

Hans

[PATCH 1/9] ACPI: export __acpi_match_device and __acpi_device[_uevent]_modalias

[Hans de Goede]

Normally __acpi_match_device and __acpi_device[_uevent]_modalias are only
called through functions calling acpi_companion_match() so that if their
are multiple devices sharing the ACPI firmware node only one matches /
gets the acpi:ACPIHID modalias.

Some DSDTs defines multiple i2c devices in a single apci_device and in
the i2c-core-acpi code we want to instantiate separate devices for these,
with all devices reporting / matching the acpi_devices's modalias.

This commit exports __acpi_match_device and __acpi_device[_uevent]_modalias
for use in the i2c-core-acpi code only, with a comment added that they
should not be used in normal code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/bus.c          | 11 ++++++-----
 drivers/acpi/device_sysfs.c |  4 +++-
 include/linux/acpi.h        | 11 +++++++++++
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 84b4a62018eb..29a0e8fa2a13 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -767,11 +767,11 @@ static bool __acpi_match_device_cls(const struct acpi_device_id *id,
 	return true;
 }
 
-static bool __acpi_match_device(struct acpi_device *device,
-				const struct acpi_device_id *acpi_ids,
-				const struct of_device_id *of_ids,
-				const struct acpi_device_id **acpi_id,
-				const struct of_device_id **of_id)
+bool __acpi_match_device(struct acpi_device *device,
+			 const struct acpi_device_id *acpi_ids,
+			 const struct of_device_id *of_ids,
+			 const struct acpi_device_id **acpi_id,
+			 const struct of_device_id **of_id)
 {
 	const struct acpi_device_id *id;
 	struct acpi_hardware_id *hwid;
@@ -808,6 +808,7 @@ static bool __acpi_match_device(struct acpi_device *device,
 		*acpi_id = id;
 	return true;
 }
+EXPORT_SYMBOL_GPL(__acpi_match_device);
 
 /**
  * acpi_match_device - Match a struct device against a given list of ACPI IDs
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 545e91420cde..e6d784ef00da 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -276,6 +276,7 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(__acpi_device_uevent_modalias);
 
 /**
  * acpi_device_uevent_modalias - uevent modalias for ACPI-enumerated devices.
@@ -291,7 +292,7 @@ int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 }
 EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
 
-static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
+int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
 {
 	int len, count;
 
@@ -321,6 +322,7 @@ static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
 
 	return len;
 }
+EXPORT_SYMBOL_GPL(__acpi_device_modalias);
 
 /**
  * acpi_device_modalias - modalias sysfs attribute for ACPI-enumerated devices.
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 15bfb15c2fa5..cf97902792a5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -586,12 +586,23 @@ extern int acpi_nvs_for_each_region(int (*func)(__u64, __u64, void *),
 
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev);
+/* Skips the acpi_companion_match() check, normal code must not use this */
+bool __acpi_match_device(struct acpi_device *device,
+			 const struct acpi_device_id *acpi_ids,
+			 const struct of_device_id *of_ids,
+			 const struct acpi_device_id **acpi_id,
+			 const struct of_device_id **of_id);
 
 const void *acpi_device_get_match_data(const struct device *dev);
 extern bool acpi_driver_match_device(struct device *dev,
 				     const struct device_driver *drv);
 int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
+/* Skips the acpi_companion_match() check, normal code must not use this */
+int __acpi_device_uevent_modalias(struct acpi_device *adev,
+				  struct kobj_uevent_env *env);
 int acpi_device_modalias(struct device *, char *, int);
+/* Skips the acpi_companion_match() check, normal code must not use this */
+int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size);
 void acpi_walk_dep_device_list(acpi_handle handle);
 
 struct platform_device *acpi_create_platform_device(struct acpi_device *,
-- 
2.17.0

[PATCH 2/9] i2c: Allow specifying irq-index to be used in i2c_device_probe()

[Hans de Goede]

Some types of interrupts are retrieved in i2c_device_probe() because
getting them might fail with -EPROBE_DEFER.

So far we've always assumed the first IRQ (index 0) in the firmware-node
is the one we want.

At least with ACPI enumerated i2c-clients in some cases the firmware-node
is shared between multiple i2c-clients so we need to be able to specify
the index rather then hardcoding it at 0.

This commit adds a new fwnode_irq_index member to i2c_board_info and
i2c_client which allows specifying the index.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-base.c | 7 +++++--
 include/linux/i2c.h         | 5 +++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 1ba40bb2b966..ae3fda2c96a4 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -312,9 +312,11 @@ static int i2c_device_probe(struct device *dev)
 		} else if (dev->of_node) {
 			irq = of_irq_get_byname(dev->of_node, "irq");
 			if (irq == -EINVAL || irq == -ENODATA)
-				irq = of_irq_get(dev->of_node, 0);
+				irq = of_irq_get(dev->of_node,
+						 client->fwnode_irq_index);
 		} else if (ACPI_COMPANION(dev)) {
-			irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
+			irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev),
+						    client->fwnode_irq_index);
 		}
 		if (irq == -EPROBE_DEFER)
 			return irq;
@@ -724,6 +726,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->flags = info->flags;
 	client->addr = info->addr;
 
+	client->fwnode_irq_index = info->fwnode_irq_index;
 	client->irq = info->irq;
 	if (!client->irq)
 		client->irq = i2c_dev_irq_from_resources(info->resources,
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 44ad14e016b5..7f9506714a5e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -316,6 +316,8 @@ struct i2c_driver {
  *	generic enough to hide second-sourcing and compatible revisions.
  * @adapter: manages the bus segment hosting this I2C device
  * @dev: Driver model device node for the slave.
+ * @fwnode_irq_index: some devices share a single fwnode, this tells
+ *                    i2c_device_probe() to use the Nth irq from the fwnode
  * @irq: indicates the IRQ generated by this device (if any)
  * @detected: member of an i2c_driver.clients list or i2c-core's
  *	userspace_devices list
@@ -334,6 +336,7 @@ struct i2c_client {
 	char name[I2C_NAME_SIZE];
 	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
 	struct device dev;		/* the device structure		*/
+	int fwnode_irq_index;		/* index for irq lookup		*/
 	int irq;			/* irq issued by device		*/
 	struct list_head detected;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -400,6 +403,7 @@ static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
  * @properties: additional device properties for the device
  * @resources: resources associated with the device
  * @num_resources: number of resources in the @resources array
+ * @fwnode_irq_index: stored in i2c_client.fwnode_irq_index
  * @irq: stored in i2c_client.irq
  *
  * I2C doesn't actually support hardware probing, although controllers and
@@ -425,6 +429,7 @@ struct i2c_board_info {
 	const struct property_entry *properties;
 	const struct resource *resources;
 	unsigned int	num_resources;
+	int		fwnode_irq_index;
 	int		irq;
 };
 
-- 
2.17.0

Re: [PATCH 2/9] i2c: Allow specifying irq-index to be used in i2c_device_probe()

[Andy Shevchenko]

On Sun, 2018-05-20 at 15:28 +0200, Hans de Goede wrote:
> Some types of interrupts are retrieved in i2c_device_probe() because
> getting them might fail with -EPROBE_DEFER.
> 
> So far we've always assumed the first IRQ (index 0) in the firmware-
> node
> is the one we want.
> 
> At least with ACPI enumerated i2c-clients in some cases the firmware-
> node
> is shared between multiple i2c-clients so we need to be able to
> specify
> the index rather then hardcoding it at 0.
> 
> This commit adds a new fwnode_irq_index member to i2c_board_info and
> i2c_client which allows specifying the index.
> 

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

P.S. I would go with 'unsigned int' for the index.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/i2c/i2c-core-base.c | 7 +++++--
>  include/linux/i2c.h         | 5 +++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 1ba40bb2b966..ae3fda2c96a4 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -312,9 +312,11 @@ static int i2c_device_probe(struct device *dev)
>  		} else if (dev->of_node) {
>  			irq = of_irq_get_byname(dev->of_node, "irq");
>  			if (irq == -EINVAL || irq == -ENODATA)
> -				irq = of_irq_get(dev->of_node, 0);
> +				irq = of_irq_get(dev->of_node,
> +						 client-
> >fwnode_irq_index);
>  		} else if (ACPI_COMPANION(dev)) {
> -			irq =
> acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> +			irq =
> acpi_dev_gpio_irq_get(ACPI_COMPANION(dev),
> +						    client-
> >fwnode_irq_index);
>  		}
>  		if (irq == -EPROBE_DEFER)
>  			return irq;
> @@ -724,6 +726,7 @@ i2c_new_device(struct i2c_adapter *adap, struct
> i2c_board_info const *info)
>  	client->flags = info->flags;
>  	client->addr = info->addr;
>  
> +	client->fwnode_irq_index = info->fwnode_irq_index;
>  	client->irq = info->irq;
>  	if (!client->irq)
>  		client->irq = i2c_dev_irq_from_resources(info-
> >resources,
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 44ad14e016b5..7f9506714a5e 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -316,6 +316,8 @@ struct i2c_driver {
>   *	generic enough to hide second-sourcing and compatible
> revisions.
>   * @adapter: manages the bus segment hosting this I2C device
>   * @dev: Driver model device node for the slave.
> + * @fwnode_irq_index: some devices share a single fwnode, this tells
> + *                    i2c_device_probe() to use the Nth irq from the
> fwnode
>   * @irq: indicates the IRQ generated by this device (if any)
>   * @detected: member of an i2c_driver.clients list or i2c-core's
>   *	userspace_devices list
> @@ -334,6 +336,7 @@ struct i2c_client {
>  	char name[I2C_NAME_SIZE];
>  	struct i2c_adapter *adapter;	/* the adapter we sit on	
> */
>  	struct device dev;		/* the device structure	
> 	*/
> +	int fwnode_irq_index;		/* index for irq lookup	
> 	*/
>  	int irq;			/* irq issued by device	
> 	*/
>  	struct list_head detected;
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -400,6 +403,7 @@ static inline bool i2c_detect_slave_mode(struct
> device *dev) { return false; }
>   * @properties: additional device properties for the device
>   * @resources: resources associated with the device
>   * @num_resources: number of resources in the @resources array
> + * @fwnode_irq_index: stored in i2c_client.fwnode_irq_index
>   * @irq: stored in i2c_client.irq
>   *
>   * I2C doesn't actually support hardware probing, although
> controllers and
> @@ -425,6 +429,7 @@ struct i2c_board_info {
>  	const struct property_entry *properties;
>  	const struct resource *resources;
>  	unsigned int	num_resources;
> +	int		fwnode_irq_index;
>  	int		irq;
>  };
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/9] i2c: Allow specifying irq-index to be used in i2c_device_probe()

[Andy Shevchenko]

On Mon, 2018-05-21 at 12:07 +0300, Andy Shevchenko wrote:
> On Sun, 2018-05-20 at 15:28 +0200, Hans de Goede wrote:
> > Some types of interrupts are retrieved in i2c_device_probe() because
> > getting them might fail with -EPROBE_DEFER.
> > 
> > So far we've always assumed the first IRQ (index 0) in the firmware-
> > node
> > is the one we want.
> > 
> > At least with ACPI enumerated i2c-clients in some cases the
> > firmware-
> > node
> > is shared between multiple i2c-clients so we need to be able to
> > specify
> > the index rather then hardcoding it at 0.
> > 
> > This commit adds a new fwnode_irq_index member to i2c_board_info and
> > i2c_client which allows specifying the index.
> > 
> 
> FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> P.S. I would go with 'unsigned int' for the index.

Ah, it seems it is needed to be -1 in some cases. Discard my comment.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 3/9] i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper

[Hans de Goede]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Besides current two users one more is coming. Definitely makes sense to
introduce a helper.

No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 31 +++++++++++++++++++------------
 include/linux/acpi.h        | 11 +++++++++++
 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 7c3b4740b94b..7b1b0aeced36 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -45,6 +45,23 @@ struct i2c_acpi_lookup {
 	u32 min_speed;
 };
 
+bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+			       struct acpi_resource_i2c_serialbus **i2c)
+{
+	struct acpi_resource_i2c_serialbus *sb;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return false;
+
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return false;
+
+	*i2c = sb;
+	return true;
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_get_i2c_resource);
+
 static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 {
 	struct i2c_acpi_lookup *lookup = data;
@@ -52,11 +69,7 @@ static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 	struct acpi_resource_i2c_serialbus *sb;
 	acpi_status status;
 
-	if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
-		return 1;
-
-	sb = &ares->data.i2c_serial_bus;
-	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+	if (info->addr || !i2c_acpi_get_i2c_resource(ares, &sb))
 		return 1;
 
 	if (lookup->index != -1 && lookup->n++ != lookup->index)
@@ -516,13 +529,7 @@ i2c_acpi_space_handler(u32 function, acpi_physical_address command,
 		goto err;
 	}
 
-	if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-		ret = AE_BAD_PARAMETER;
-		goto err;
-	}
-
-	sb = &ares->data.i2c_serial_bus;
-	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+	if (!value64 || !i2c_acpi_get_i2c_resource(ares, &sb)) {
 		ret = AE_BAD_PARAMETER;
 		goto err;
 	}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index cf97902792a5..f97c70f8ba4b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1062,6 +1062,17 @@ static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 }
 #endif
 
+#if defined(CONFIG_ACPI) && IS_ENABLED(CONFIG_I2C)
+bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+			       struct acpi_resource_i2c_serialbus **i2c);
+#else
+static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+			       struct acpi_resource_i2c_serialbus **i2c)
+{
+	return false;
+}
+#endif
+
 /* Device properties */
 
 #define MAX_ACPI_REFERENCE_ARGS	8
-- 
2.17.0

[PATCH 4/9] i2c: acpi: Allow get info by index in i2c_acpi_get_info()

[Hans de Goede]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

For some devices we need to get info based on index. As a preparation of
support such, slightly modify i2c_acpi_get_info() helper.

While here, assume that interrupt resources are provided in the same amount
with 1:1 mapping to serial bus resources. This will not affect existing
behaviour because only first resource of each type is considered.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 7b1b0aeced36..75352b3744e5 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -129,17 +129,19 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
 
 static int i2c_acpi_get_info(struct acpi_device *adev,
 			     struct i2c_board_info *info,
+			     int index,
 			     struct i2c_adapter *adapter,
 			     acpi_handle *adapter_handle)
 {
 	struct list_head resource_list;
 	struct resource_entry *entry;
 	struct i2c_acpi_lookup lookup;
+	unsigned int n;
 	int ret;
 
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.info = info;
-	lookup.index = -1;
+	lookup.index = index;
 
 	ret = i2c_acpi_do_lookup(adev, &lookup);
 	if (ret)
@@ -170,10 +172,13 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
 	if (ret < 0)
 		return -EINVAL;
 
+	n = 0;
 	resource_list_for_each_entry(entry, &resource_list) {
 		if (resource_type(entry->res) == IORESOURCE_IRQ) {
-			info->irq = entry->res->start;
-			break;
+			if (index == -1 || n++ == index) {
+				info->irq = entry->res->start;
+				break;
+			}
 		}
 	}
 
@@ -210,7 +215,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
 	if (acpi_bus_get_device(handle, &adev))
 		return AE_OK;
 
-	if (i2c_acpi_get_info(adev, &info, adapter, NULL))
+	if (i2c_acpi_get_info(adev, &info, -1, adapter, NULL))
 		return AE_OK;
 
 	i2c_acpi_register_device(adapter, adev, &info);
@@ -356,7 +361,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
 
 	switch (value) {
 	case ACPI_RECONFIG_DEVICE_ADD:
-		if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
+		if (i2c_acpi_get_info(adev, &info, -1, NULL, &adapter_handle))
 			break;
 
 		adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
-- 
2.17.0

[PATCH 5/9] i2c: acpi: Enumerate several instances out of one device

[Hans de Goede]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Some broken BIOSes provide one ACPI node for several same I2C devices.
Introduce a quirk to handle this case with a list of such devices.

[hdegoede@redhat.com: Add and use i2c_acpi_device_uevent() and
 i2c_acpi_device_modalias() helpers]
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 101 ++++++++++++++++++++++++++++++++++--
 drivers/i2c/i2c-core-base.c |   6 +--
 drivers/i2c/i2c-core.h      |  13 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 75352b3744e5..f2f9048caffd 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -105,8 +105,10 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
 	struct list_head resource_list;
 	int ret;
 
-	if (acpi_bus_get_status(adev) || !adev->status.present ||
-	    acpi_device_enumerated(adev))
+	if (acpi_bus_get_status(adev) || !adev->status.present)
+		return -EINVAL;
+
+	if (lookup->index < 0 && acpi_device_enumerated(adev))
 		return -EINVAL;
 
 	if (acpi_match_device_ids(adev, i2c_acpi_ignored_device_ids) == 0)
@@ -187,6 +189,8 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
 	acpi_set_modalias(adev, dev_name(&adev->dev), info->type,
 			  sizeof(info->type));
 
+	info->fwnode_irq_index = index == -1 ? 0 : index;
+
 	return 0;
 }
 
@@ -205,20 +209,73 @@ static void i2c_acpi_register_device(struct i2c_adapter *adapter,
 	}
 }
 
+static const struct acpi_device_id i2c_acpi_multiple_devices_ids[] = {
+	/*
+	 * Some devices are defined as a single ACPI entry while
+	 * providing more than one instance of the same IP. Try to
+	 * enumerate them all.
+	 */
+	{ "BOSC0200", 0 },
+	{ "INT3515", 0 },
+	{}
+};
+
+static int i2c_acpi_check_resource(struct acpi_resource *ares, void *data)
+{
+	struct acpi_resource_i2c_serialbus *sb;
+	int *count = data;
+
+	if (i2c_acpi_get_i2c_resource(ares, &sb))
+		*count = *count + 1;
+
+	return 1;
+}
+
+static int i2c_acpi_count_resource(struct acpi_device *adev)
+{
+	LIST_HEAD(r);
+	int count = 0;
+	int ret;
+
+	ret = acpi_dev_get_resources(adev, &r, i2c_acpi_check_resource, &count);
+	if (ret < 0)
+		return ret;
+
+	acpi_dev_free_resource_list(&r);
+	return count;
+}
+
 static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
 				       void *data, void **return_value)
 {
 	struct i2c_adapter *adapter = data;
 	struct acpi_device *adev;
 	struct i2c_board_info info;
+	int index, count;
+	char name[16];
 
 	if (acpi_bus_get_device(handle, &adev))
 		return AE_OK;
 
-	if (i2c_acpi_get_info(adev, &info, -1, adapter, NULL))
-		return AE_OK;
+	if (!acpi_match_device_ids(adev, i2c_acpi_multiple_devices_ids)) {
+		count = i2c_acpi_count_resource(adev);
+		if (count < 0)
+			return AE_OK;
+	} else {
+		count = 1;
+	}
+
+	for (index = 0; index < count; index++) {
+		if (i2c_acpi_get_info(adev, &info, index, adapter, NULL))
+			return AE_OK;
+
+		if (count > 1) {
+			sprintf(name, "%s.%d", acpi_dev_name(adev), index);
+			info.dev_name = name;
+		}
 
-	i2c_acpi_register_device(adapter, adev, &info);
+		i2c_acpi_register_device(adapter, adev, &info);
+	}
 
 	return AE_OK;
 }
@@ -252,12 +309,46 @@ const struct acpi_device_id *
 i2c_acpi_match_device(const struct acpi_device_id *matches,
 		      struct i2c_client *client)
 {
+	const struct acpi_device_id *id = NULL;
+	struct acpi_device *adev;
+
 	if (!(client && matches))
 		return NULL;
 
+	adev = ACPI_COMPANION(&client->dev);
+
+	/* Std acpi_match_device() only works for the first physical device */
+	if (!acpi_match_device_ids(adev, i2c_acpi_multiple_devices_ids)) {
+		__acpi_match_device(adev, matches, NULL, &id, NULL);
+		return id;
+	}
+
 	return acpi_match_device(matches, &client->dev);
 }
 
+int i2c_acpi_device_uevent(struct i2c_client *client,
+			   struct kobj_uevent_env *env)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+
+	/* Std ACPI function only works for the first physical device */
+	if (!acpi_match_device_ids(adev, i2c_acpi_multiple_devices_ids))
+		return __acpi_device_uevent_modalias(adev, env);
+
+	return acpi_device_uevent_modalias(&client->dev, env);
+}
+
+int i2c_acpi_device_modalias(struct i2c_client *client, char *buf, int size)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+
+	/* Std ACPI function only works for the first physical device */
+	if (!acpi_match_device_ids(adev, i2c_acpi_multiple_devices_ids))
+		return __acpi_device_modalias(adev, buf, size);
+
+	return acpi_device_modalias(&client->dev, buf, size);
+}
+
 static acpi_status i2c_acpi_lookup_speed(acpi_handle handle, u32 level,
 					   void *data, void **return_value)
 {
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ae3fda2c96a4..7bf9d8d1f8e7 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -109,7 +109,7 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 		return 1;
 
 	/* Then ACPI style match */
-	if (acpi_driver_match_device(dev, drv))
+	if (i2c_acpi_match_device(drv->acpi_match_table, client))
 		return 1;
 
 	driver = to_i2c_driver(drv);
@@ -130,7 +130,7 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (rc != -ENODEV)
 		return rc;
 
-	rc = acpi_device_uevent_modalias(dev, env);
+	rc = i2c_acpi_device_uevent(client, env);
 	if (rc != -ENODEV)
 		return rc;
 
@@ -451,7 +451,7 @@ show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 	if (len != -ENODEV)
 		return len;
 
-	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+	len = i2c_acpi_device_modalias(client, buf, PAGE_SIZE - 1);
 	if (len != -ENODEV)
 		return len;
 
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 37576f50fe20..887353e1d58d 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -33,6 +33,9 @@ int i2c_check_7bit_addr_validity_strict(unsigned short addr);
 const struct acpi_device_id *
 i2c_acpi_match_device(const struct acpi_device_id *matches,
 		      struct i2c_client *client);
+int i2c_acpi_device_uevent(struct i2c_client *client,
+			   struct kobj_uevent_env *env);
+int i2c_acpi_device_modalias(struct i2c_client *client, char *buf, int size);
 void i2c_acpi_register_devices(struct i2c_adapter *adap);
 #else /* CONFIG_ACPI */
 static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
@@ -42,6 +45,16 @@ i2c_acpi_match_device(const struct acpi_device_id *matches,
 {
 	return NULL;
 }
+static inline int i2c_acpi_device_uevent(struct i2c_client *client,
+					 struct kobj_uevent_env *env)
+{
+	return -ENODEV;
+}
+static inline int i2c_acpi_device_modalias(struct i2c_client *client,
+					   char *buf, int size)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_ACPI */
 extern struct notifier_block i2c_acpi_notifier;
 
-- 
2.17.0

[PATCH 6/9] i2c: acpi: Add BSG1160 to i2c_acpi_multiple_devices_ids

[Hans de Goede]

The BSG1160 ACPI HID is a HID describing a sensor complex consisting of
3 sensors:

Accel: BMC150A at addr 0x11
Gyro: BMG160 at addr 0x68
Magneto: BMC150B at addr 0x13

The ACPI resources table contains I2cSerialBusV2 resources for each,
add the BSG1160 id to the i2c_acpi_multiple_devices_ids list, so that
an i2c_client gets instantiated for each sensor.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index f2f9048caffd..7fb622b269db 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -216,6 +216,7 @@ static const struct acpi_device_id i2c_acpi_multiple_devices_ids[] = {
 	 * enumerate them all.
 	 */
 	{ "BOSC0200", 0 },
+	{ "BSG1160", 0 },
 	{ "INT3515", 0 },
 	{}
 };
-- 
2.17.0

[PATCH 7/9] iio: accel: bmc150: Add support for BSG1160 ACPI HID

[Hans de Goede]

The BSG1160 ACPI HID is a HID describing a sensor complex consisting of
3 sensors:

Accel: BMC150A at addr 0x11
Gyro: BMG160 at addr 0x68
Magneto: BMC150B at addr 0x13

A previous patch on this series has added the BSG1160 HID to the
i2c_acpi_multiple_devices_ids list, so that one i2c_client gets
instantiated per sensor.

This commit not only adds the BSG1160 HID to the bmc150 code, but it also
makes the i2c probe function check the client address for devices with a
BSG1160 HID and only bind to the one at address 0x11.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-i2c.c | 8 ++++++++
 drivers/iio/accel/bmc150-accel.h     | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 8ffc308d5fd0..e0d7b507e397 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -31,13 +31,20 @@
 static int bmc150_accel_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
+	const struct acpi_device_id *acpi_id;
 	struct regmap *regmap;
+	struct device *dev = &client->dev;
 	const char *name = NULL;
 	bool block_supported =
 		i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
 		i2c_check_functionality(client->adapter,
 					I2C_FUNC_SMBUS_READ_I2C_BLOCK);
 
+	/* The BSG1160 ACPI id describes multiple sensors, only bind to ours */
+	acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (acpi_id && acpi_id->driver_data == bsg1160 && client->addr != 0x11)
+		return -ENODEV;
+
 	regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf);
 	if (IS_ERR(regmap)) {
 		dev_err(&client->dev, "Failed to initialize i2c regmap\n");
@@ -64,6 +71,7 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
 	{"BMA250E",	bma250e},
 	{"BMA222E",	bma222e},
 	{"BMA0280",	bma280},
+	{"BSG1160",	bsg1160},
 	{"BOSC0200"},
 	{ },
 };
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index ae6118ae11b1..ac540a775d54 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -11,6 +11,7 @@ enum {
 	bma250e,
 	bma222e,
 	bma280,
+	bsg1160,
 };
 
 int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
-- 
2.17.0

[PATCH 8/9] iio: gyro: bmg160: Add support for BSG1160 ACPI HID

[Hans de Goede]

The BSG1160 ACPI HID is a HID describing a sensor complex consisting of
3 sensors:

Accel: BMC150A at addr 0x11
Gyro: BMG160 at addr 0x68
Magneto: BMC150B at addr 0x13

A previous patch on this series has added the BSG1160 HID to the
i2c_acpi_multiple_devices_ids list, so that one i2c_client gets
instantiated per sensor.

This commit not only adds the BSG1160 HID to the bmg160 code, but it also
makes the i2c probe function check the client address for devices with a
BSG1160 HID and only bind to the one at address 0x68.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/gyro/bmg160_i2c.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/gyro/bmg160_i2c.c b/drivers/iio/gyro/bmg160_i2c.c
index 90126a5a7663..2f72f41e33bb 100644
--- a/drivers/iio/gyro/bmg160_i2c.c
+++ b/drivers/iio/gyro/bmg160_i2c.c
@@ -15,9 +15,17 @@ static const struct regmap_config bmg160_regmap_i2c_conf = {
 static int bmg160_i2c_probe(struct i2c_client *client,
 			    const struct i2c_device_id *id)
 {
+	const struct acpi_device_id *acpi_id;
 	struct regmap *regmap;
+	struct device *dev = &client->dev;
 	const char *name = NULL;
 
+	/* The BSG1160 ACPI id describes multiple sensors, only bind to ours */
+	acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (acpi_id && acpi_id->driver_data &&
+	    client->addr != acpi_id->driver_data)
+		return -ENODEV;
+
 	regmap = devm_regmap_init_i2c(client, &bmg160_regmap_i2c_conf);
 	if (IS_ERR(regmap)) {
 		dev_err(&client->dev, "Failed to register i2c regmap %d\n",
@@ -41,6 +49,7 @@ static int bmg160_i2c_remove(struct i2c_client *client)
 static const struct acpi_device_id bmg160_acpi_match[] = {
 	{"BMG0160", 0},
 	{"BMI055B", 0},
+	{"BSG1160", 0x68},
 	{},
 };
 
-- 
2.17.0

[PATCH 9/9] iio: magnetometer: bmc150: Add support for BSG1160 ACPI HID

[Hans de Goede]

The BSG1160 ACPI HID is a HID describing a sensor complex consisting of
3 sensors:

Accel: BMC150A at addr 0x11
Gyro: BMG160 at addr 0x68
Magneto: BMC150B at addr 0x13

A previous patch on this series has added the BSG1160 HID to the
i2c_acpi_multiple_devices_ids list, so that one i2c_client gets
instantiated per sensor.

This commit not only adds the BSG1160 HID to the bmc150 code, but it also
makes the i2c probe function check the client address for devices with a
BSG1160 HID and only bind to the one at address 0x13.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/magnetometer/bmc150_magn_i2c.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/magnetometer/bmc150_magn_i2c.c b/drivers/iio/magnetometer/bmc150_magn_i2c.c
index 57e40dd1222e..c8a1e4178462 100644
--- a/drivers/iio/magnetometer/bmc150_magn_i2c.c
+++ b/drivers/iio/magnetometer/bmc150_magn_i2c.c
@@ -27,9 +27,17 @@
 static int bmc150_magn_i2c_probe(struct i2c_client *client,
 				 const struct i2c_device_id *id)
 {
+	const struct acpi_device_id *acpi_id;
 	struct regmap *regmap;
+	struct device *dev = &client->dev;
 	const char *name = NULL;
 
+	/* The BSG1160 ACPI id describes multiple sensors, only bind to ours */
+	acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
+	if (acpi_id && acpi_id->driver_data &&
+	    client->addr != acpi_id->driver_data)
+		return -ENODEV;
+
 	regmap = devm_regmap_init_i2c(client, &bmc150_magn_regmap_config);
 	if (IS_ERR(regmap)) {
 		dev_err(&client->dev, "Failed to initialize i2c regmap\n");
@@ -51,6 +59,7 @@ static const struct acpi_device_id bmc150_magn_acpi_match[] = {
 	{"BMC150B", 0},
 	{"BMC156B", 0},
 	{"BMM150B", 0},
+	{"BSG1160", 0x13},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, bmc150_magn_acpi_match);
-- 
2.17.0

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Jonathan Cameron]

On Sun, 20 May 2018 15:28:48 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi All,
> 
> This series really consists of 2 series, patches 1-5 add support for
> interesting ACPI tables which describe multiple i2c chips in a single
> fwnode, sometimes multiple cases of the same chip on different addresses,
> sometimes a bunch of related chips.
> 
> Andy Shevchenko has come up with the solution of adding a quirk based
> on the ACPI HID of the fwnode for these devices which makes the
> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices
> for each I2cSerialBusV2 in the fwnode. I agree with him that this is
> the best (least ugly) solution for this.
> 
> I've been testing this solution on a device if mine which needs a solution
> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode
> with a HID of BSG1160 which describes 3 different i2c sensors in an accel /
> magneto / gyro sensor cluster on the tablet. This has let to some extra
> prep. patches and some fixes to Andy's patches.
> 
> Patches 6-9 use the new functionality creating  one i2c-client per
> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and
> are posted as part of this series to show how this functionality can be
> used.
> 
> Assuming everyone is ok with this series (I'm not expecting anyone to be
> really happy about the need for this), then I suggest that patches 1-6
> get merged togther through either the ACPI or the i2c tree, I guess the
> i2c tree would make somewhat more sense, since most patches are there.
> 
> Then once those are accepted patches 7-9 can be merged into the iio tree,
> there is no compile time dependency between the 2, so these can be merged
> separately. Note merging 7-9 before there is agreement that this is the
> right way to fix this is probably not a good idea.

It's hideous, but I can live with it as better than anything else anyone
has come up with.  I just hope we don't get a huge number of these
'interesting' ACPI cases going forwards.

Jonathan

> 
> Regards,
> 
> Hans
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Lars-Peter Clausen]

On 05/20/2018 06:23 PM, Jonathan Cameron wrote:
> On Sun, 20 May 2018 15:28:48 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi All,
>>
>> This series really consists of 2 series, patches 1-5 add support for
>> interesting ACPI tables which describe multiple i2c chips in a single
>> fwnode, sometimes multiple cases of the same chip on different addresses,
>> sometimes a bunch of related chips.
>>
>> Andy Shevchenko has come up with the solution of adding a quirk based
>> on the ACPI HID of the fwnode for these devices which makes the
>> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices
>> for each I2cSerialBusV2 in the fwnode. I agree with him that this is
>> the best (least ugly) solution for this.
>>
>> I've been testing this solution on a device if mine which needs a solution
>> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode
>> with a HID of BSG1160 which describes 3 different i2c sensors in an accel /
>> magneto / gyro sensor cluster on the tablet. This has let to some extra
>> prep. patches and some fixes to Andy's patches.
>>
>> Patches 6-9 use the new functionality creating  one i2c-client per
>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and
>> are posted as part of this series to show how this functionality can be
>> used.
>>
>> Assuming everyone is ok with this series (I'm not expecting anyone to be
>> really happy about the need for this), then I suggest that patches 1-6
>> get merged togther through either the ACPI or the i2c tree, I guess the
>> i2c tree would make somewhat more sense, since most patches are there.
>>
>> Then once those are accepted patches 7-9 can be merged into the iio tree,
>> there is no compile time dependency between the 2, so these can be merged
>> separately. Note merging 7-9 before there is agreement that this is the
>> right way to fix this is probably not a good idea.
> 
> It's hideous, but I can live with it as better than anything else anyone
> has come up with.  I just hope we don't get a huge number of these
> 'interesting' ACPI cases going forwards.

It's ACPI, you know this is just the beginning ;)

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Andy Shevchenko]

On Sun, 2018-05-20 at 15:28 +0200, Hans de Goede wrote:
> Hi All,
> 
> This series really consists of 2 series, patches 1-5 add support for
> interesting ACPI tables which describe multiple i2c chips in a single
> fwnode, sometimes multiple cases of the same chip on different
> addresses,
> sometimes a bunch of related chips.
> 
> Andy Shevchenko has come up with the solution of adding a quirk based
> on the ACPI HID of the fwnode for these devices which makes the
> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client
> devices
> for each I2cSerialBusV2 in the fwnode. I agree with him that this is
> the best (least ugly) solution for this.
> 
> I've been testing this solution on a device if mine which needs a
> solution
> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device /
> fwnode
> with a HID of BSG1160 which describes 3 different i2c sensors in an
> accel /
> magneto / gyro sensor cluster on the tablet. This has let to some
> extra
> prep. patches and some fixes to Andy's patches.

Thanks for taking care!

> Patches 6-9 use the new functionality creating  one i2c-client per
> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work
> and
> are posted as part of this series to show how this functionality can
> be
> used.

I suppose it's better to do an "MFD" type of IIO driver for that chip.
Check, for example, drivers/iio/imu/bmi160/bmi160_core.c

> 
> Assuming everyone is ok with this series (I'm not expecting anyone to
> be
> really happy about the need for this), then I suggest that patches 1-6
> get merged togther through either the ACPI or the i2c tree, I guess
> the
> i2c tree would make somewhat more sense, since most patches are there.
> 
> Then once those are accepted patches 7-9 can be merged into the iio
> tree,
> there is no compile time dependency between the 2, so these can be
> merged
> separately. Note merging 7-9 before there is agreement that this is
> the
> right way to fix this is probably not a good idea.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi,

On 21-05-18 11:19, Andy Shevchenko wrote:
> On Sun, 2018-05-20 at 15:28 +0200, Hans de Goede wrote:
>> Hi All,
>>
>> This series really consists of 2 series, patches 1-5 add support for
>> interesting ACPI tables which describe multiple i2c chips in a single
>> fwnode, sometimes multiple cases of the same chip on different
>> addresses,
>> sometimes a bunch of related chips.
>>
>> Andy Shevchenko has come up with the solution of adding a quirk based
>> on the ACPI HID of the fwnode for these devices which makes the
>> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client
>> devices
>> for each I2cSerialBusV2 in the fwnode. I agree with him that this is
>> the best (least ugly) solution for this.
>>
>> I've been testing this solution on a device if mine which needs a
>> solution
>> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device /
>> fwnode
>> with a HID of BSG1160 which describes 3 different i2c sensors in an
>> accel /
>> magneto / gyro sensor cluster on the tablet. This has let to some
>> extra
>> prep. patches and some fixes to Andy's patches.
> 
> Thanks for taking care!
> 
>> Patches 6-9 use the new functionality creating  one i2c-client per
>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work
>> and
>> are posted as part of this series to show how this functionality can
>> be
>> used.
> 
> I suppose it's better to do an "MFD" type of IIO driver for that chip.
> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c

That seems to be a single chip listening on a single i2c address / spi
chip-select.

In the BSG1160 case the 3 sensors are listening on 3 different i2c addresses.

We could use the drivers/mfd framework, but the we get platform devices
and we would need to patch all 3 existing drivers to support platform
bindings and get a regmap from there (converting them to regmap where
necessary).

Regards,

Hans





> 
>>
>> Assuming everyone is ok with this series (I'm not expecting anyone to
>> be
>> really happy about the need for this), then I suggest that patches 1-6
>> get merged togther through either the ACPI or the i2c tree, I guess
>> the
>> i2c tree would make somewhat more sense, since most patches are there.
>>
>> Then once those are accepted patches 7-9 can be merged into the iio
>> tree,
>> there is no compile time dependency between the 2, so these can be
>> merged
>> separately. Note merging 7-9 before there is agreement that this is
>> the
>> right way to fix this is probably not a good idea.
> 

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Andy Shevchenko]

On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
> On 21-05-18 11:19, Andy Shevchenko wrote:

> > > Patches 6-9 use the new functionality creating  one i2c-client per
> > > I2cSerialBusV2 resource to make the sensor cluster on the HP X2
> > > work
> > > and
> > > are posted as part of this series to show how this functionality
> > > can
> > > be
> > > used.
> > 
> > I suppose it's better to do an "MFD" type of IIO driver for that
> > chip.
> > Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
> 
> That seems to be a single chip listening on a single i2c address / spi
> chip-select.

Ooops, wrong reference.

> In the BSG1160 case the 3 sensors are listening on 3 different i2c
> addresses.

There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
two independent drivers for them. Luckily for ACPI they have different
IDs (on the platforms where it's used like that).

So, my series targeting the series of same IPs under one device...

> We could use the drivers/mfd framework, but the we get platform
> devices
> and we would need to patch all 3 existing drivers to support platform
> bindings and get a regmap from there (converting them to regmap where
> necessary).

...and in your case MFD sounds better. Though why do you need to have a
common regmap?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Lars-Peter Clausen]

On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>> On 21-05-18 11:19, Andy Shevchenko wrote:
> 
>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>> work
>>>> and
>>>> are posted as part of this series to show how this functionality
>>>> can
>>>> be
>>>> used.
>>>
>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>> chip.
>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>
>> That seems to be a single chip listening on a single i2c address / spi
>> chip-select.
> 
> Ooops, wrong reference.
> 
>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>> addresses.
> 
> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
> two independent drivers for them. Luckily for ACPI they have different
> IDs (on the platforms where it's used like that).
> 
> So, my series targeting the series of same IPs under one device...
> 
>> We could use the drivers/mfd framework, but the we get platform
>> devices
>> and we would need to patch all 3 existing drivers to support platform
>> bindings and get a regmap from there (converting them to regmap where
>> necessary).
> 
> ...and in your case MFD sounds better. Though why do you need to have a
> common regmap?

I'm not convinced MFD is the right place. You wouldn't really utilize
anything of the MFD subsystem. And in a sense it is not a multi-function
device. It's just multiple devices that are described by the same firmware
description table entry.

But I think some kind of board driver might be useful here that translates
the ACPI description into something more reasonable. I.e. bind to the ACPI
ID and then instantiate the 3 child I2C devices on the same bus. Those do
not have to be platform drivers and you do not have to use regmap.

The current approach adds board specific workarounds to each of the device
drivers. It might be easier to have that managed in a central place.

The problem with ACPI is that the description in the tables is often for
vendor device drivers that ship together with the hardware. If you want to
use the same tables with upstream drivers some kind of translation table
might be required.

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi,

On 21-05-18 15:31, Lars-Peter Clausen wrote:
> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>>> On 21-05-18 11:19, Andy Shevchenko wrote:
>>
>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>>> work
>>>>> and
>>>>> are posted as part of this series to show how this functionality
>>>>> can
>>>>> be
>>>>> used.
>>>>
>>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>>> chip.
>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>>
>>> That seems to be a single chip listening on a single i2c address / spi
>>> chip-select.
>>
>> Ooops, wrong reference.
>>
>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>>> addresses.
>>
>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
>> two independent drivers for them. Luckily for ACPI they have different
>> IDs (on the platforms where it's used like that).
>>
>> So, my series targeting the series of same IPs under one device...
>>
>>> We could use the drivers/mfd framework, but the we get platform
>>> devices
>>> and we would need to patch all 3 existing drivers to support platform
>>> bindings and get a regmap from there (converting them to regmap where
>>> necessary).
>>
>> ...and in your case MFD sounds better. Though why do you need to have a
>> common regmap?
> 
> I'm not convinced MFD is the right place. You wouldn't really utilize
> anything of the MFD subsystem. And in a sense it is not a multi-function
> device. It's just multiple devices that are described by the same firmware
> description table entry.
> 
> But I think some kind of board driver might be useful here that translates
> the ACPI description into something more reasonable. I.e. bind to the ACPI
> ID and then instantiate the 3 child I2C devices on the same bus. Those do
> not have to be platform drivers and you do not have to use regmap.
> 
> The current approach adds board specific workarounds to each of the device
> drivers. It might be easier to have that managed in a central place.

Right, I considered that, and I'm actually doing pretty much that for
a somewhat similar ACPI case, see:

drivers/platform/x86/intel_cht_int33fe.c

But there things were more complicated and we also needed to attach
device-properties, while at the same time we were also somewhat lucky,
because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
and we only care about 2-4, so we can have an i2c-driver in
platform/drivers/x86 bind to the 1st resource and then have it
instantiate i2c clients for I2cSerialBusV2 resources 2-4.

The problem with the BSG1160 case is that we want to also have an
iio driver bind to the first i2c-client and that will not work
if an i2c-driver in platform/drivers/x86 binds to the first
i2c-client and the i2c-subsys will rightfully not let us create another
i2c-client at the same address.

About the "board specific workarounds for each of the drivers", I could
check if they are all checking an id register and if so if I could just
let all 3 of them try to bind without issues. This will likely still
require a change to log the id not matching add a less severe log-level.

Regards,

Hans

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi,

On 21-05-18 15:40, Hans de Goede wrote:
> Hi,
> 
> On 21-05-18 15:31, Lars-Peter Clausen wrote:
>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>>>> On 21-05-18 11:19, Andy Shevchenko wrote:
>>>
>>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>>>> work
>>>>>> and
>>>>>> are posted as part of this series to show how this functionality
>>>>>> can
>>>>>> be
>>>>>> used.
>>>>>
>>>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>>>> chip.
>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>>>
>>>> That seems to be a single chip listening on a single i2c address / spi
>>>> chip-select.
>>>
>>> Ooops, wrong reference.
>>>
>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>>>> addresses.
>>>
>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
>>> two independent drivers for them. Luckily for ACPI they have different
>>> IDs (on the platforms where it's used like that).
>>>
>>> So, my series targeting the series of same IPs under one device...
>>>
>>>> We could use the drivers/mfd framework, but the we get platform
>>>> devices
>>>> and we would need to patch all 3 existing drivers to support platform
>>>> bindings and get a regmap from there (converting them to regmap where
>>>> necessary).
>>>
>>> ...and in your case MFD sounds better. Though why do you need to have a
>>> common regmap?
>>
>> I'm not convinced MFD is the right place. You wouldn't really utilize
>> anything of the MFD subsystem. And in a sense it is not a multi-function
>> device. It's just multiple devices that are described by the same firmware
>> description table entry.
>>
>> But I think some kind of board driver might be useful here that translates
>> the ACPI description into something more reasonable. I.e. bind to the ACPI
>> ID and then instantiate the 3 child I2C devices on the same bus. Those do
>> not have to be platform drivers and you do not have to use regmap.
>>
>> The current approach adds board specific workarounds to each of the device
>> drivers. It might be easier to have that managed in a central place.
> 
> Right, I considered that, and I'm actually doing pretty much that for
> a somewhat similar ACPI case, see:
> 
> drivers/platform/x86/intel_cht_int33fe.c
> 
> But there things were more complicated and we also needed to attach
> device-properties, while at the same time we were also somewhat lucky,
> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
> and we only care about 2-4, so we can have an i2c-driver in
> platform/drivers/x86 bind to the 1st resource and then have it
> instantiate i2c clients for I2cSerialBusV2 resources 2-4.
> 
> The problem with the BSG1160 case is that we want to also have an
> iio driver bind to the first i2c-client and that will not work
> if an i2c-driver in platform/drivers/x86 binds to the first
> i2c-client and the i2c-subsys will rightfully not let us create another
> i2c-client at the same address.
> 
> About the "board specific workarounds for each of the drivers", I could
> check if they are all checking an id register and if so if I could just
> let all 3 of them try to bind without issues. This will likely still
> require a change to log the id not matching add a less severe log-level.

p.s.

Also there seems to be a pattern here where this is happening more
often, e.g. see also:

https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
Search for BOSC0200 to find a single Device() blurb describing
2 bma250 accelerometers at 2 different addresses.

And having to write a whole new driver each time this happens is
going to become tedious pretty quick and also seems undesirable.

Just adding a HID to an id-table OTOH for each case seems like a
better (less sucking) solution.

So I think we should not focus too much on the BSG1160 example
and more try to come up with a generic solution for this as
Andy has done.

Regards,

Hans

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Lars-Peter Clausen]

On 05/21/2018 03:44 PM, Hans de Goede wrote:
> Hi,
> 
> On 21-05-18 15:40, Hans de Goede wrote:
>> Hi,
>>
>> On 21-05-18 15:31, Lars-Peter Clausen wrote:
>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>>>>> On 21-05-18 11:19, Andy Shevchenko wrote:
>>>>
>>>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>>>>> work
>>>>>>> and
>>>>>>> are posted as part of this series to show how this functionality
>>>>>>> can
>>>>>>> be
>>>>>>> used.
>>>>>>
>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>>>>> chip.
>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>>>>
>>>>> That seems to be a single chip listening on a single i2c address / spi
>>>>> chip-select.
>>>>
>>>> Ooops, wrong reference.
>>>>
>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>>>>> addresses.
>>>>
>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
>>>> two independent drivers for them. Luckily for ACPI they have different
>>>> IDs (on the platforms where it's used like that).
>>>>
>>>> So, my series targeting the series of same IPs under one device...
>>>>
>>>>> We could use the drivers/mfd framework, but the we get platform
>>>>> devices
>>>>> and we would need to patch all 3 existing drivers to support platform
>>>>> bindings and get a regmap from there (converting them to regmap where
>>>>> necessary).
>>>>
>>>> ...and in your case MFD sounds better. Though why do you need to have a
>>>> common regmap?
>>>
>>> I'm not convinced MFD is the right place. You wouldn't really utilize
>>> anything of the MFD subsystem. And in a sense it is not a multi-function
>>> device. It's just multiple devices that are described by the same firmware
>>> description table entry.
>>>
>>> But I think some kind of board driver might be useful here that translates
>>> the ACPI description into something more reasonable. I.e. bind to the ACPI
>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do
>>> not have to be platform drivers and you do not have to use regmap.
>>>
>>> The current approach adds board specific workarounds to each of the device
>>> drivers. It might be easier to have that managed in a central place.
>>
>> Right, I considered that, and I'm actually doing pretty much that for
>> a somewhat similar ACPI case, see:
>>
>> drivers/platform/x86/intel_cht_int33fe.c
>>
>> But there things were more complicated and we also needed to attach
>> device-properties, while at the same time we were also somewhat lucky,
>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
>> and we only care about 2-4, so we can have an i2c-driver in
>> platform/drivers/x86 bind to the 1st resource and then have it
>> instantiate i2c clients for I2cSerialBusV2 resources 2-4.
>>
>> The problem with the BSG1160 case is that we want to also have an
>> iio driver bind to the first i2c-client and that will not work
>> if an i2c-driver in platform/drivers/x86 binds to the first
>> i2c-client and the i2c-subsys will rightfully not let us create another
>> i2c-client at the same address.
>>
>> About the "board specific workarounds for each of the drivers", I could
>> check if they are all checking an id register and if so if I could just
>> let all 3 of them try to bind without issues. This will likely still
>> require a change to log the id not matching add a less severe log-level.
> 
> p.s.
> 
> Also there seems to be a pattern here where this is happening more
> often, e.g. see also:
> 
> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
> Search for BOSC0200 to find a single Device() blurb describing
> 2 bma250 accelerometers at 2 different addresses.
> 
> And having to write a whole new driver each time this happens is
> going to become tedious pretty quick and also seems undesirable.
> 
> Just adding a HID to an id-table OTOH for each case seems like a
> better (less sucking) solution.

I'd use the same argument to argue for the opposite. The fact that is is a
common occurrence means it should not be handled in the device driver,
because it means you'll end up having to add quirks for each and every
vendor binding.

E.g. if you look at the example you provided there is also a mounting matrix
and calibration data for each of the two sensors. You need a way to map
those to the individual devices.

> 
> So I think we should not focus too much on the BSG1160 example
> and more try to come up with a generic solution for this as
> Andy has done.

I agree that a generic solution is the right approach, but I do not think
that adding lots of individual quirks to device drivers is a generic solution.

Maybe we can teach the I2C framework about these hub nodes, so that the
device for the hub itself does not prevent the children from binding to
their I2C addresses. You are already patching the I2C core anyway.

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi,

On 21-05-18 17:07, Lars-Peter Clausen wrote:
> On 05/21/2018 03:44 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 21-05-18 15:40, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 21-05-18 15:31, Lars-Peter Clausen wrote:
>>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
>>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>>>>>> On 21-05-18 11:19, Andy Shevchenko wrote:
>>>>>
>>>>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>>>>>> work
>>>>>>>> and
>>>>>>>> are posted as part of this series to show how this functionality
>>>>>>>> can
>>>>>>>> be
>>>>>>>> used.
>>>>>>>
>>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>>>>>> chip.
>>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>>>>>
>>>>>> That seems to be a single chip listening on a single i2c address / spi
>>>>>> chip-select.
>>>>>
>>>>> Ooops, wrong reference.
>>>>>
>>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>>>>>> addresses.
>>>>>
>>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
>>>>> two independent drivers for them. Luckily for ACPI they have different
>>>>> IDs (on the platforms where it's used like that).
>>>>>
>>>>> So, my series targeting the series of same IPs under one device...
>>>>>
>>>>>> We could use the drivers/mfd framework, but the we get platform
>>>>>> devices
>>>>>> and we would need to patch all 3 existing drivers to support platform
>>>>>> bindings and get a regmap from there (converting them to regmap where
>>>>>> necessary).
>>>>>
>>>>> ...and in your case MFD sounds better. Though why do you need to have a
>>>>> common regmap?
>>>>
>>>> I'm not convinced MFD is the right place. You wouldn't really utilize
>>>> anything of the MFD subsystem. And in a sense it is not a multi-function
>>>> device. It's just multiple devices that are described by the same firmware
>>>> description table entry.
>>>>
>>>> But I think some kind of board driver might be useful here that translates
>>>> the ACPI description into something more reasonable. I.e. bind to the ACPI
>>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do
>>>> not have to be platform drivers and you do not have to use regmap.
>>>>
>>>> The current approach adds board specific workarounds to each of the device
>>>> drivers. It might be easier to have that managed in a central place.
>>>
>>> Right, I considered that, and I'm actually doing pretty much that for
>>> a somewhat similar ACPI case, see:
>>>
>>> drivers/platform/x86/intel_cht_int33fe.c
>>>
>>> But there things were more complicated and we also needed to attach
>>> device-properties, while at the same time we were also somewhat lucky,
>>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
>>> and we only care about 2-4, so we can have an i2c-driver in
>>> platform/drivers/x86 bind to the 1st resource and then have it
>>> instantiate i2c clients for I2cSerialBusV2 resources 2-4.
>>>
>>> The problem with the BSG1160 case is that we want to also have an
>>> iio driver bind to the first i2c-client and that will not work
>>> if an i2c-driver in platform/drivers/x86 binds to the first
>>> i2c-client and the i2c-subsys will rightfully not let us create another
>>> i2c-client at the same address.
>>>
>>> About the "board specific workarounds for each of the drivers", I could
>>> check if they are all checking an id register and if so if I could just
>>> let all 3 of them try to bind without issues. This will likely still
>>> require a change to log the id not matching add a less severe log-level.
>>
>> p.s.
>>
>> Also there seems to be a pattern here where this is happening more
>> often, e.g. see also:
>>
>> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
>> Search for BOSC0200 to find a single Device() blurb describing
>> 2 bma250 accelerometers at 2 different addresses.
>>
>> And having to write a whole new driver each time this happens is
>> going to become tedious pretty quick and also seems undesirable.
>>
>> Just adding a HID to an id-table OTOH for each case seems like a
>> better (less sucking) solution.
> 
> I'd use the same argument to argue for the opposite. The fact that is is a
> common occurrence means it should not be handled in the device driver,
> because it means you'll end up having to add quirks for each and every
> vendor binding.
> 
> E.g. if you look at the example you provided there is also a mounting matrix
> and calibration data for each of the two sensors. You need a way to map
> those to the individual devices.
> 
>>
>> So I think we should not focus too much on the BSG1160 example
>> and more try to come up with a generic solution for this as
>> Andy has done.
> 
> I agree that a generic solution is the right approach, but I do not think
> that adding lots of individual quirks to device drivers is a generic solution.
> 
> Maybe we can teach the I2C framework about these hub nodes, so that the
> device for the hub itself does not prevent the children from binding to
> their I2C addresses. You are already patching the I2C core anyway.

Ok, so thinking more about this I think that we indeed need to solve this
differently. Another argument here is to also not pollute the i2c core
with a whole bunch of extra code, just to handle these corner cases.

So my idea is to have an i2c-driver under platform/x86 which deals with
these special cases where we want multiple i2c-clients instantiated
from a single ACPI fwnode.

The idea is to have a bool no_address_busy_check in i2c_board_info,
with a big fat comment that it is special and should be avoided,
which disables the i2c_check_addr_busy() check in i2c_new_device().

This instantiation driver will use per ACPI-HID driver_data
pointing to an array of:

struct give_my_type_a_proper_name {
	const char *type;
	int irq_index;
}

The probe will then iterate over this array, stopping at a NULL type
pointer and instantiate i2c_clients for each entry in the array
using type as i2c_board_info.type and requesting an interrupt
from the ACPI fwnode resources using irq_index, except when irq_index
is -1 (and setting the special no_address_busy_check bool for the
first instantiation).

The idea is that by having a generic instantiation loop for this
driven by per ACPI-HID driver_data we have a generic solution,
while at the same time having this isolated in a driver which
can be modular and only loaded when one of the special ACPI HIDs
is encountered.

So how does this sound ?  I will give you all some time to reply
and assuming no one shoots this down try to implement this say
next weekend.

Heikki, would this also work for your "INT3515" HID case?

Regards,

Hans

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Heikki Krogerus]

On Mon, May 21, 2018 at 09:12:38PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 21-05-18 17:07, Lars-Peter Clausen wrote:
> > On 05/21/2018 03:44 PM, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 21-05-18 15:40, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 21-05-18 15:31, Lars-Peter Clausen wrote:
> > > > > On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
> > > > > > On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
> > > > > > > On 21-05-18 11:19, Andy Shevchenko wrote:
> > > > > > 
> > > > > > > > > Patches 6-9 use the new functionality creating?? one i2c-client per
> > > > > > > > > I2cSerialBusV2 resource to make the sensor cluster on the HP X2
> > > > > > > > > work
> > > > > > > > > and
> > > > > > > > > are posted as part of this series to show how this functionality
> > > > > > > > > can
> > > > > > > > > be
> > > > > > > > > used.
> > > > > > > > 
> > > > > > > > I suppose it's better to do an "MFD" type of IIO driver for that
> > > > > > > > chip.
> > > > > > > > Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
> > > > > > > 
> > > > > > > That seems to be a single chip listening on a single i2c address / spi
> > > > > > > chip-select.
> > > > > > 
> > > > > > Ooops, wrong reference.
> > > > > > 
> > > > > > > In the BSG1160 case the 3 sensors are listening on 3 different i2c
> > > > > > > addresses.
> > > > > > 
> > > > > > There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
> > > > > > two independent drivers for them. Luckily for ACPI they have different
> > > > > > IDs (on the platforms where it's used like that).
> > > > > > 
> > > > > > So, my series targeting the series of same IPs under one device...
> > > > > > 
> > > > > > > We could use the drivers/mfd framework, but the we get platform
> > > > > > > devices
> > > > > > > and we would need to patch all 3 existing drivers to support platform
> > > > > > > bindings and get a regmap from there (converting them to regmap where
> > > > > > > necessary).
> > > > > > 
> > > > > > ...and in your case MFD sounds better. Though why do you need to have a
> > > > > > common regmap?
> > > > > 
> > > > > I'm not convinced MFD is the right place. You wouldn't really utilize
> > > > > anything of the MFD subsystem. And in a sense it is not a multi-function
> > > > > device. It's just multiple devices that are described by the same firmware
> > > > > description table entry.
> > > > > 
> > > > > But I think some kind of board driver might be useful here that translates
> > > > > the ACPI description into something more reasonable. I.e. bind to the ACPI
> > > > > ID and then instantiate the 3 child I2C devices on the same bus. Those do
> > > > > not have to be platform drivers and you do not have to use regmap.
> > > > > 
> > > > > The current approach adds board specific workarounds to each of the device
> > > > > drivers. It might be easier to have that managed in a central place.
> > > > 
> > > > Right, I considered that, and I'm actually doing pretty much that for
> > > > a somewhat similar ACPI case, see:
> > > > 
> > > > drivers/platform/x86/intel_cht_int33fe.c
> > > > 
> > > > But there things were more complicated and we also needed to attach
> > > > device-properties, while at the same time we were also somewhat lucky,
> > > > because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
> > > > and we only care about 2-4, so we can have an i2c-driver in
> > > > platform/drivers/x86 bind to the 1st resource and then have it
> > > > instantiate i2c clients for I2cSerialBusV2 resources 2-4.
> > > > 
> > > > The problem with the BSG1160 case is that we want to also have an
> > > > iio driver bind to the first i2c-client and that will not work
> > > > if an i2c-driver in platform/drivers/x86 binds to the first
> > > > i2c-client and the i2c-subsys will rightfully not let us create another
> > > > i2c-client at the same address.
> > > > 
> > > > About the "board specific workarounds for each of the drivers", I could
> > > > check if they are all checking an id register and if so if I could just
> > > > let all 3 of them try to bind without issues. This will likely still
> > > > require a change to log the id not matching add a less severe log-level.
> > > 
> > > p.s.
> > > 
> > > Also there seems to be a pattern here where this is happening more
> > > often, e.g. see also:
> > > 
> > > https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
> > > Search for BOSC0200 to find a single Device() blurb describing
> > > 2 bma250 accelerometers at 2 different addresses.
> > > 
> > > And having to write a whole new driver each time this happens is
> > > going to become tedious pretty quick and also seems undesirable.
> > > 
> > > Just adding a HID to an id-table OTOH for each case seems like a
> > > better (less sucking) solution.
> > 
> > I'd use the same argument to argue for the opposite. The fact that is is a
> > common occurrence means it should not be handled in the device driver,
> > because it means you'll end up having to add quirks for each and every
> > vendor binding.
> > 
> > E.g. if you look at the example you provided there is also a mounting matrix
> > and calibration data for each of the two sensors. You need a way to map
> > those to the individual devices.
> > 
> > > 
> > > So I think we should not focus too much on the BSG1160 example
> > > and more try to come up with a generic solution for this as
> > > Andy has done.
> > 
> > I agree that a generic solution is the right approach, but I do not think
> > that adding lots of individual quirks to device drivers is a generic solution.
> > 
> > Maybe we can teach the I2C framework about these hub nodes, so that the
> > device for the hub itself does not prevent the children from binding to
> > their I2C addresses. You are already patching the I2C core anyway.
> 
> Ok, so thinking more about this I think that we indeed need to solve this
> differently. Another argument here is to also not pollute the i2c core
> with a whole bunch of extra code, just to handle these corner cases.
> 
> So my idea is to have an i2c-driver under platform/x86 which deals with
> these special cases where we want multiple i2c-clients instantiated
> from a single ACPI fwnode.
> 
> The idea is to have a bool no_address_busy_check in i2c_board_info,
> with a big fat comment that it is special and should be avoided,
> which disables the i2c_check_addr_busy() check in i2c_new_device().
> 
> This instantiation driver will use per ACPI-HID driver_data
> pointing to an array of:
> 
> struct give_my_type_a_proper_name {
> 	const char *type;
> 	int irq_index;
> }
> 
> The probe will then iterate over this array, stopping at a NULL type
> pointer and instantiate i2c_clients for each entry in the array
> using type as i2c_board_info.type and requesting an interrupt
> from the ACPI fwnode resources using irq_index, except when irq_index
> is -1 (and setting the special no_address_busy_check bool for the
> first instantiation).
> 
> The idea is that by having a generic instantiation loop for this
> driven by per ACPI-HID driver_data we have a generic solution,
> while at the same time having this isolated in a driver which
> can be modular and only loaded when one of the special ACPI HIDs
> is encountered.
> 
> So how does this sound ?  I will give you all some time to reply
> and assuming no one shoots this down try to implement this say
> next weekend.
> 
> Heikki, would this also work for your "INT3515" HID case?

I'm sure it will. I'll test it once you are done.


Thanks,

-- 
heikki

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Jonathan Cameron]

On Mon, 21 May 2018 21:12:38 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 21-05-18 17:07, Lars-Peter Clausen wrote:
> > On 05/21/2018 03:44 PM, Hans de Goede wrote:  
> >> Hi,
> >>
> >> On 21-05-18 15:40, Hans de Goede wrote:  
> >>> Hi,
> >>>
> >>> On 21-05-18 15:31, Lars-Peter Clausen wrote:  
> >>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:  
> >>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:  
> >>>>>> On 21-05-18 11:19, Andy Shevchenko wrote:  
> >>>>>  
> >>>>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
> >>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
> >>>>>>>> work
> >>>>>>>> and
> >>>>>>>> are posted as part of this series to show how this functionality
> >>>>>>>> can
> >>>>>>>> be
> >>>>>>>> used.  
> >>>>>>>
> >>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that
> >>>>>>> chip.
> >>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c  
> >>>>>>
> >>>>>> That seems to be a single chip listening on a single i2c address / spi
> >>>>>> chip-select.  
> >>>>>
> >>>>> Ooops, wrong reference.
> >>>>>  
> >>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
> >>>>>> addresses.  
> >>>>>
> >>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
> >>>>> two independent drivers for them. Luckily for ACPI they have different
> >>>>> IDs (on the platforms where it's used like that).
> >>>>>
> >>>>> So, my series targeting the series of same IPs under one device...
> >>>>>  
> >>>>>> We could use the drivers/mfd framework, but the we get platform
> >>>>>> devices
> >>>>>> and we would need to patch all 3 existing drivers to support platform
> >>>>>> bindings and get a regmap from there (converting them to regmap where
> >>>>>> necessary).  
> >>>>>
> >>>>> ...and in your case MFD sounds better. Though why do you need to have a
> >>>>> common regmap?  
> >>>>
> >>>> I'm not convinced MFD is the right place. You wouldn't really utilize
> >>>> anything of the MFD subsystem. And in a sense it is not a multi-function
> >>>> device. It's just multiple devices that are described by the same firmware
> >>>> description table entry.
> >>>>
> >>>> But I think some kind of board driver might be useful here that translates
> >>>> the ACPI description into something more reasonable. I.e. bind to the ACPI
> >>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do
> >>>> not have to be platform drivers and you do not have to use regmap.
> >>>>
> >>>> The current approach adds board specific workarounds to each of the device
> >>>> drivers. It might be easier to have that managed in a central place.  
> >>>
> >>> Right, I considered that, and I'm actually doing pretty much that for
> >>> a somewhat similar ACPI case, see:
> >>>
> >>> drivers/platform/x86/intel_cht_int33fe.c
> >>>
> >>> But there things were more complicated and we also needed to attach
> >>> device-properties, while at the same time we were also somewhat lucky,
> >>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
> >>> and we only care about 2-4, so we can have an i2c-driver in
> >>> platform/drivers/x86 bind to the 1st resource and then have it
> >>> instantiate i2c clients for I2cSerialBusV2 resources 2-4.
> >>>
> >>> The problem with the BSG1160 case is that we want to also have an
> >>> iio driver bind to the first i2c-client and that will not work
> >>> if an i2c-driver in platform/drivers/x86 binds to the first
> >>> i2c-client and the i2c-subsys will rightfully not let us create another
> >>> i2c-client at the same address.
> >>>
> >>> About the "board specific workarounds for each of the drivers", I could
> >>> check if they are all checking an id register and if so if I could just
> >>> let all 3 of them try to bind without issues. This will likely still
> >>> require a change to log the id not matching add a less severe log-level.  
> >>
> >> p.s.
> >>
> >> Also there seems to be a pattern here where this is happening more
> >> often, e.g. see also:
> >>
> >> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
> >> Search for BOSC0200 to find a single Device() blurb describing
> >> 2 bma250 accelerometers at 2 different addresses.
> >>
> >> And having to write a whole new driver each time this happens is
> >> going to become tedious pretty quick and also seems undesirable.
> >>
> >> Just adding a HID to an id-table OTOH for each case seems like a
> >> better (less sucking) solution.  
> > 
> > I'd use the same argument to argue for the opposite. The fact that is is a
> > common occurrence means it should not be handled in the device driver,
> > because it means you'll end up having to add quirks for each and every
> > vendor binding.
> > 
> > E.g. if you look at the example you provided there is also a mounting matrix
> > and calibration data for each of the two sensors. You need a way to map
> > those to the individual devices.
> >   
> >>
> >> So I think we should not focus too much on the BSG1160 example
> >> and more try to come up with a generic solution for this as
> >> Andy has done.  
> > 
> > I agree that a generic solution is the right approach, but I do not think
> > that adding lots of individual quirks to device drivers is a generic solution.
> > 
> > Maybe we can teach the I2C framework about these hub nodes, so that the
> > device for the hub itself does not prevent the children from binding to
> > their I2C addresses. You are already patching the I2C core anyway.  
> 
> Ok, so thinking more about this I think that we indeed need to solve this
> differently. Another argument here is to also not pollute the i2c core
> with a whole bunch of extra code, just to handle these corner cases.
> 
> So my idea is to have an i2c-driver under platform/x86 which deals with
> these special cases where we want multiple i2c-clients instantiated
> from a single ACPI fwnode.

Don't make it x86 specific (sooner or later someone will do something similar
on an ARM or other platform), other than that it will be interesting to
see how this pans out.

One corner case as well that looks much the same is the package in package
parts.  They really are kind of "one device", so not really broken ACPI in
the same way most of these cases are.

I looked through the ACPI 6.2 spec and concluded that these cases were
already broken. If the opportunity arises I'll have a chat with some people
more heavily involved in that standard than I am and see if we can strengthen
this. May make no difference to what Vendors do but we can try.

We can pragmatically handle them with the same interface, but it seems
a little clunky. For DT we have the option to have odd bindings that
bind the two internal parts separately but we clearly can't control that
for ACPI.

> 
> The idea is to have a bool no_address_busy_check in i2c_board_info,
> with a big fat comment that it is special and should be avoided,
> which disables the i2c_check_addr_busy() check in i2c_new_device().
> 
> This instantiation driver will use per ACPI-HID driver_data
> pointing to an array of:
> 
> struct give_my_type_a_proper_name {
> 	const char *type;
> 	int irq_index;
> }
> 
> The probe will then iterate over this array, stopping at a NULL type
> pointer and instantiate i2c_clients for each entry in the array
> using type as i2c_board_info.type and requesting an interrupt
> from the ACPI fwnode resources using irq_index, except when irq_index
> is -1 (and setting the special no_address_busy_check bool for the
> first instantiation).
> 
> The idea is that by having a generic instantiation loop for this
> driven by per ACPI-HID driver_data we have a generic solution,
> while at the same time having this isolated in a driver which
> can be modular and only loaded when one of the special ACPI HIDs
> is encountered.
> 
> So how does this sound ?  I will give you all some time to reply
> and assuming no one shoots this down try to implement this say
> next weekend.
> 
> Heikki, would this also work for your "INT3515" HID case?
> 
> Regards,
> 
> Hans
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Lars-Peter Clausen]

On 05/21/2018 09:12 PM, Hans de Goede wrote:
> Hi,
> 
> On 21-05-18 17:07, Lars-Peter Clausen wrote:
>> On 05/21/2018 03:44 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 21-05-18 15:40, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 21-05-18 15:31, Lars-Peter Clausen wrote:
>>>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
>>>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>>>>>>> On 21-05-18 11:19, Andy Shevchenko wrote:
>>>>>>
>>>>>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>>>>>>> work
>>>>>>>>> and
>>>>>>>>> are posted as part of this series to show how this functionality
>>>>>>>>> can
>>>>>>>>> be
>>>>>>>>> used.
>>>>>>>>
>>>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>>>>>>> chip.
>>>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>>>>>>
>>>>>>> That seems to be a single chip listening on a single i2c address / spi
>>>>>>> chip-select.
>>>>>>
>>>>>> Ooops, wrong reference.
>>>>>>
>>>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>>>>>>> addresses.
>>>>>>
>>>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
>>>>>> two independent drivers for them. Luckily for ACPI they have different
>>>>>> IDs (on the platforms where it's used like that).
>>>>>>
>>>>>> So, my series targeting the series of same IPs under one device...
>>>>>>
>>>>>>> We could use the drivers/mfd framework, but the we get platform
>>>>>>> devices
>>>>>>> and we would need to patch all 3 existing drivers to support platform
>>>>>>> bindings and get a regmap from there (converting them to regmap where
>>>>>>> necessary).
>>>>>>
>>>>>> ...and in your case MFD sounds better. Though why do you need to have a
>>>>>> common regmap?
>>>>>
>>>>> I'm not convinced MFD is the right place. You wouldn't really utilize
>>>>> anything of the MFD subsystem. And in a sense it is not a multi-function
>>>>> device. It's just multiple devices that are described by the same firmware
>>>>> description table entry.
>>>>>
>>>>> But I think some kind of board driver might be useful here that translates
>>>>> the ACPI description into something more reasonable. I.e. bind to the ACPI
>>>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do
>>>>> not have to be platform drivers and you do not have to use regmap.
>>>>>
>>>>> The current approach adds board specific workarounds to each of the device
>>>>> drivers. It might be easier to have that managed in a central place.
>>>>
>>>> Right, I considered that, and I'm actually doing pretty much that for
>>>> a somewhat similar ACPI case, see:
>>>>
>>>> drivers/platform/x86/intel_cht_int33fe.c
>>>>
>>>> But there things were more complicated and we also needed to attach
>>>> device-properties, while at the same time we were also somewhat lucky,
>>>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
>>>> and we only care about 2-4, so we can have an i2c-driver in
>>>> platform/drivers/x86 bind to the 1st resource and then have it
>>>> instantiate i2c clients for I2cSerialBusV2 resources 2-4.
>>>>
>>>> The problem with the BSG1160 case is that we want to also have an
>>>> iio driver bind to the first i2c-client and that will not work
>>>> if an i2c-driver in platform/drivers/x86 binds to the first
>>>> i2c-client and the i2c-subsys will rightfully not let us create another
>>>> i2c-client at the same address.
>>>>
>>>> About the "board specific workarounds for each of the drivers", I could
>>>> check if they are all checking an id register and if so if I could just
>>>> let all 3 of them try to bind without issues. This will likely still
>>>> require a change to log the id not matching add a less severe log-level.
>>>
>>> p.s.
>>>
>>> Also there seems to be a pattern here where this is happening more
>>> often, e.g. see also:
>>>
>>> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
>>> Search for BOSC0200 to find a single Device() blurb describing
>>> 2 bma250 accelerometers at 2 different addresses.
>>>
>>> And having to write a whole new driver each time this happens is
>>> going to become tedious pretty quick and also seems undesirable.
>>>
>>> Just adding a HID to an id-table OTOH for each case seems like a
>>> better (less sucking) solution.
>>
>> I'd use the same argument to argue for the opposite. The fact that is is a
>> common occurrence means it should not be handled in the device driver,
>> because it means you'll end up having to add quirks for each and every
>> vendor binding.
>>
>> E.g. if you look at the example you provided there is also a mounting matrix
>> and calibration data for each of the two sensors. You need a way to map
>> those to the individual devices.
>>
>>>
>>> So I think we should not focus too much on the BSG1160 example
>>> and more try to come up with a generic solution for this as
>>> Andy has done.
>>
>> I agree that a generic solution is the right approach, but I do not think
>> that adding lots of individual quirks to device drivers is a generic
>> solution.
>>
>> Maybe we can teach the I2C framework about these hub nodes, so that the
>> device for the hub itself does not prevent the children from binding to
>> their I2C addresses. You are already patching the I2C core anyway.
> 
> Ok, so thinking more about this I think that we indeed need to solve this
> differently. Another argument here is to also not pollute the i2c core
> with a whole bunch of extra code, just to handle these corner cases.
> 
> So my idea is to have an i2c-driver under platform/x86 which deals with
> these special cases where we want multiple i2c-clients instantiated
> from a single ACPI fwnode.
> 
> The idea is to have a bool no_address_busy_check in i2c_board_info,
> with a big fat comment that it is special and should be avoided,
> which disables the i2c_check_addr_busy() check in i2c_new_device().

Ideally we'd be able to register the hub as an address-less device and then
have each of the sensors bind to their respective I2C addresses while still
making sure that there are no address duplicates.

Maybe is possible to re-use part of the I2C MUX infrastructure and have the
hub register itself as some kind of MUX device and the sensors as children
to the hub. This way the sensors would still be grouped in the device hierarchy.

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi,

On 22-05-18 13:40, Lars-Peter Clausen wrote:
> On 05/21/2018 09:12 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 21-05-18 17:07, Lars-Peter Clausen wrote:
>>> On 05/21/2018 03:44 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 21-05-18 15:40, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 21-05-18 15:31, Lars-Peter Clausen wrote:
>>>>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
>>>>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>>>>>>>> On 21-05-18 11:19, Andy Shevchenko wrote:
>>>>>>>
>>>>>>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>>>>>>>> work
>>>>>>>>>> and
>>>>>>>>>> are posted as part of this series to show how this functionality
>>>>>>>>>> can
>>>>>>>>>> be
>>>>>>>>>> used.
>>>>>>>>>
>>>>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>>>>>>>> chip.
>>>>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>>>>>>>
>>>>>>>> That seems to be a single chip listening on a single i2c address / spi
>>>>>>>> chip-select.
>>>>>>>
>>>>>>> Ooops, wrong reference.
>>>>>>>
>>>>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>>>>>>>> addresses.
>>>>>>>
>>>>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
>>>>>>> two independent drivers for them. Luckily for ACPI they have different
>>>>>>> IDs (on the platforms where it's used like that).
>>>>>>>
>>>>>>> So, my series targeting the series of same IPs under one device...
>>>>>>>
>>>>>>>> We could use the drivers/mfd framework, but the we get platform
>>>>>>>> devices
>>>>>>>> and we would need to patch all 3 existing drivers to support platform
>>>>>>>> bindings and get a regmap from there (converting them to regmap where
>>>>>>>> necessary).
>>>>>>>
>>>>>>> ...and in your case MFD sounds better. Though why do you need to have a
>>>>>>> common regmap?
>>>>>>
>>>>>> I'm not convinced MFD is the right place. You wouldn't really utilize
>>>>>> anything of the MFD subsystem. And in a sense it is not a multi-function
>>>>>> device. It's just multiple devices that are described by the same firmware
>>>>>> description table entry.
>>>>>>
>>>>>> But I think some kind of board driver might be useful here that translates
>>>>>> the ACPI description into something more reasonable. I.e. bind to the ACPI
>>>>>> ID and then instantiate the 3 child I2C devices on the same bus. Those do
>>>>>> not have to be platform drivers and you do not have to use regmap.
>>>>>>
>>>>>> The current approach adds board specific workarounds to each of the device
>>>>>> drivers. It might be easier to have that managed in a central place.
>>>>>
>>>>> Right, I considered that, and I'm actually doing pretty much that for
>>>>> a somewhat similar ACPI case, see:
>>>>>
>>>>> drivers/platform/x86/intel_cht_int33fe.c
>>>>>
>>>>> But there things were more complicated and we also needed to attach
>>>>> device-properties, while at the same time we were also somewhat lucky,
>>>>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
>>>>> and we only care about 2-4, so we can have an i2c-driver in
>>>>> platform/drivers/x86 bind to the 1st resource and then have it
>>>>> instantiate i2c clients for I2cSerialBusV2 resources 2-4.
>>>>>
>>>>> The problem with the BSG1160 case is that we want to also have an
>>>>> iio driver bind to the first i2c-client and that will not work
>>>>> if an i2c-driver in platform/drivers/x86 binds to the first
>>>>> i2c-client and the i2c-subsys will rightfully not let us create another
>>>>> i2c-client at the same address.
>>>>>
>>>>> About the "board specific workarounds for each of the drivers", I could
>>>>> check if they are all checking an id register and if so if I could just
>>>>> let all 3 of them try to bind without issues. This will likely still
>>>>> require a change to log the id not matching add a less severe log-level.
>>>>
>>>> p.s.
>>>>
>>>> Also there seems to be a pattern here where this is happening more
>>>> often, e.g. see also:
>>>>
>>>> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
>>>> Search for BOSC0200 to find a single Device() blurb describing
>>>> 2 bma250 accelerometers at 2 different addresses.
>>>>
>>>> And having to write a whole new driver each time this happens is
>>>> going to become tedious pretty quick and also seems undesirable.
>>>>
>>>> Just adding a HID to an id-table OTOH for each case seems like a
>>>> better (less sucking) solution.
>>>
>>> I'd use the same argument to argue for the opposite. The fact that is is a
>>> common occurrence means it should not be handled in the device driver,
>>> because it means you'll end up having to add quirks for each and every
>>> vendor binding.
>>>
>>> E.g. if you look at the example you provided there is also a mounting matrix
>>> and calibration data for each of the two sensors. You need a way to map
>>> those to the individual devices.
>>>
>>>>
>>>> So I think we should not focus too much on the BSG1160 example
>>>> and more try to come up with a generic solution for this as
>>>> Andy has done.
>>>
>>> I agree that a generic solution is the right approach, but I do not think
>>> that adding lots of individual quirks to device drivers is a generic
>>> solution.
>>>
>>> Maybe we can teach the I2C framework about these hub nodes, so that the
>>> device for the hub itself does not prevent the children from binding to
>>> their I2C addresses. You are already patching the I2C core anyway.
>>
>> Ok, so thinking more about this I think that we indeed need to solve this
>> differently. Another argument here is to also not pollute the i2c core
>> with a whole bunch of extra code, just to handle these corner cases.
>>
>> So my idea is to have an i2c-driver under platform/x86 which deals with
>> these special cases where we want multiple i2c-clients instantiated
>> from a single ACPI fwnode.
>>
>> The idea is to have a bool no_address_busy_check in i2c_board_info,
>> with a big fat comment that it is special and should be avoided,
>> which disables the i2c_check_addr_busy() check in i2c_new_device().
> 
> Ideally we'd be able to register the hub as an address-less device and then
> have each of the sensors bind to their respective I2C addresses while still
> making sure that there are no address duplicates.
> 
> Maybe is possible to re-use part of the I2C MUX infrastructure and have the
> hub register itself as some kind of MUX device and the sensors as children
> to the hub. This way the sensors would still be grouped in the device hierarchy.

There is no hub, i2c topology wise there are simply 3 separate i2c devices
which for some reason got lumped together in a single ACPI fwnode. Representing
this as a different topology then it actually is seems counter-productive.

I do not know the physicial topology in the HP x2 case, but in the
Lenovo Yoga 11e case there are 2 separate sensors, one in the base and
one in the display, lumped together in a single ACPI fwnode.

Regards,

Hans

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Lars-Peter Clausen]

On 05/22/2018 01:55 PM, Hans de Goede wrote:
> Hi,
> 
> On 22-05-18 13:40, Lars-Peter Clausen wrote:
>> On 05/21/2018 09:12 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 21-05-18 17:07, Lars-Peter Clausen wrote:
>>>> On 05/21/2018 03:44 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 21-05-18 15:40, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 21-05-18 15:31, Lars-Peter Clausen wrote:
>>>>>>> On 05/21/2018 03:13 PM, Andy Shevchenko wrote:
>>>>>>>> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>>>>>>>>> On 21-05-18 11:19, Andy Shevchenko wrote:
>>>>>>>>
>>>>>>>>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>>>>>>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>>>>>>>>> work
>>>>>>>>>>> and
>>>>>>>>>>> are posted as part of this series to show how this functionality
>>>>>>>>>>> can
>>>>>>>>>>> be
>>>>>>>>>>> used.
>>>>>>>>>>
>>>>>>>>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>>>>>>>>> chip.
>>>>>>>>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>>>>>>>>
>>>>>>>>> That seems to be a single chip listening on a single i2c address / spi
>>>>>>>>> chip-select.
>>>>>>>>
>>>>>>>> Ooops, wrong reference.
>>>>>>>>
>>>>>>>>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>>>>>>>>> addresses.
>>>>>>>>
>>>>>>>> There is a Bosh magnetometer + accelerometer chip (BMC150). We have
>>>>>>>> just
>>>>>>>> two independent drivers for them. Luckily for ACPI they have different
>>>>>>>> IDs (on the platforms where it's used like that).
>>>>>>>>
>>>>>>>> So, my series targeting the series of same IPs under one device...
>>>>>>>>
>>>>>>>>> We could use the drivers/mfd framework, but the we get platform
>>>>>>>>> devices
>>>>>>>>> and we would need to patch all 3 existing drivers to support platform
>>>>>>>>> bindings and get a regmap from there (converting them to regmap where
>>>>>>>>> necessary).
>>>>>>>>
>>>>>>>> ...and in your case MFD sounds better. Though why do you need to have a
>>>>>>>> common regmap?
>>>>>>>
>>>>>>> I'm not convinced MFD is the right place. You wouldn't really utilize
>>>>>>> anything of the MFD subsystem. And in a sense it is not a multi-function
>>>>>>> device. It's just multiple devices that are described by the same
>>>>>>> firmware
>>>>>>> description table entry.
>>>>>>>
>>>>>>> But I think some kind of board driver might be useful here that
>>>>>>> translates
>>>>>>> the ACPI description into something more reasonable. I.e. bind to the
>>>>>>> ACPI
>>>>>>> ID and then instantiate the 3 child I2C devices on the same bus.
>>>>>>> Those do
>>>>>>> not have to be platform drivers and you do not have to use regmap.
>>>>>>>
>>>>>>> The current approach adds board specific workarounds to each of the
>>>>>>> device
>>>>>>> drivers. It might be easier to have that managed in a central place.
>>>>>>
>>>>>> Right, I considered that, and I'm actually doing pretty much that for
>>>>>> a somewhat similar ACPI case, see:
>>>>>>
>>>>>> drivers/platform/x86/intel_cht_int33fe.c
>>>>>>
>>>>>> But there things were more complicated and we also needed to attach
>>>>>> device-properties, while at the same time we were also somewhat lucky,
>>>>>> because there are 4 I2cSerialBusV2 resources in the single ACPI fwnode
>>>>>> and we only care about 2-4, so we can have an i2c-driver in
>>>>>> platform/drivers/x86 bind to the 1st resource and then have it
>>>>>> instantiate i2c clients for I2cSerialBusV2 resources 2-4.
>>>>>>
>>>>>> The problem with the BSG1160 case is that we want to also have an
>>>>>> iio driver bind to the first i2c-client and that will not work
>>>>>> if an i2c-driver in platform/drivers/x86 binds to the first
>>>>>> i2c-client and the i2c-subsys will rightfully not let us create another
>>>>>> i2c-client at the same address.
>>>>>>
>>>>>> About the "board specific workarounds for each of the drivers", I could
>>>>>> check if they are all checking an id register and if so if I could just
>>>>>> let all 3 of them try to bind without issues. This will likely still
>>>>>> require a change to log the id not matching add a less severe log-level.
>>>>>
>>>>> p.s.
>>>>>
>>>>> Also there seems to be a pattern here where this is happening more
>>>>> often, e.g. see also:
>>>>>
>>>>> https://fedorapeople.org/~jwrdegoede/lenovo-yoga-11e-dstd.dsl
>>>>> Search for BOSC0200 to find a single Device() blurb describing
>>>>> 2 bma250 accelerometers at 2 different addresses.
>>>>>
>>>>> And having to write a whole new driver each time this happens is
>>>>> going to become tedious pretty quick and also seems undesirable.
>>>>>
>>>>> Just adding a HID to an id-table OTOH for each case seems like a
>>>>> better (less sucking) solution.
>>>>
>>>> I'd use the same argument to argue for the opposite. The fact that is is a
>>>> common occurrence means it should not be handled in the device driver,
>>>> because it means you'll end up having to add quirks for each and every
>>>> vendor binding.
>>>>
>>>> E.g. if you look at the example you provided there is also a mounting
>>>> matrix
>>>> and calibration data for each of the two sensors. You need a way to map
>>>> those to the individual devices.
>>>>
>>>>>
>>>>> So I think we should not focus too much on the BSG1160 example
>>>>> and more try to come up with a generic solution for this as
>>>>> Andy has done.
>>>>
>>>> I agree that a generic solution is the right approach, but I do not think
>>>> that adding lots of individual quirks to device drivers is a generic
>>>> solution.
>>>>
>>>> Maybe we can teach the I2C framework about these hub nodes, so that the
>>>> device for the hub itself does not prevent the children from binding to
>>>> their I2C addresses. You are already patching the I2C core anyway.
>>>
>>> Ok, so thinking more about this I think that we indeed need to solve this
>>> differently. Another argument here is to also not pollute the i2c core
>>> with a whole bunch of extra code, just to handle these corner cases.
>>>
>>> So my idea is to have an i2c-driver under platform/x86 which deals with
>>> these special cases where we want multiple i2c-clients instantiated
>>> from a single ACPI fwnode.
>>>
>>> The idea is to have a bool no_address_busy_check in i2c_board_info,
>>> with a big fat comment that it is special and should be avoided,
>>> which disables the i2c_check_addr_busy() check in i2c_new_device().
>>
>> Ideally we'd be able to register the hub as an address-less device and then
>> have each of the sensors bind to their respective I2C addresses while still
>> making sure that there are no address duplicates.
>>
>> Maybe is possible to re-use part of the I2C MUX infrastructure and have the
>> hub register itself as some kind of MUX device and the sensors as children
>> to the hub. This way the sensors would still be grouped in the device
>> hierarchy.
> 
> There is no hub, i2c topology wise there are simply 3 separate i2c devices
> which for some reason got lumped together in a single ACPI fwnode. Representing
> this as a different topology then it actually is seems counter-productive.
> 
> I do not know the physicial topology in the HP x2 case, but in the
> Lenovo Yoga 11e case there are 2 separate sensors, one in the base and
> one in the display, lumped together in a single ACPI fwnode.

Hm, OK. I was assuming that there was a good reason why they are lumped
together, forming some sort of virtual hub. If they produce uncorrelated
datastreams it does not really matter.

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi,

On 21-05-18 15:13, Andy Shevchenko wrote:
> On Mon, 2018-05-21 at 14:34 +0200, Hans de Goede wrote:
>> On 21-05-18 11:19, Andy Shevchenko wrote:
> 
>>>> Patches 6-9 use the new functionality creating  one i2c-client per
>>>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2
>>>> work
>>>> and
>>>> are posted as part of this series to show how this functionality
>>>> can
>>>> be
>>>> used.
>>>
>>> I suppose it's better to do an "MFD" type of IIO driver for that
>>> chip.
>>> Check, for example, drivers/iio/imu/bmi160/bmi160_core.c
>>
>> That seems to be a single chip listening on a single i2c address / spi
>> chip-select.
> 
> Ooops, wrong reference.
> 
>> In the BSG1160 case the 3 sensors are listening on 3 different i2c
>> addresses.
> 
> There is a Bosh magnetometer + accelerometer chip (BMC150). We have just
> two independent drivers for them. Luckily for ACPI they have different
> IDs (on the platforms where it's used like that).
> 
> So, my series targeting the series of same IPs under one device...
> 
>> We could use the drivers/mfd framework, but the we get platform
>> devices
>> and we would need to patch all 3 existing drivers to support platform
>> bindings and get a regmap from there (converting them to regmap where
>> necessary).
> 
> ...and in your case MFD sounds better. Though why do you need to have a
> common regmap?

Not a common regmap, but a regmap per i2c-address of course, but we need
to have the MFD driver create these regmaps because the MFD-child devices
are platform_devices which know nothing about i2c. And then we need to add
platform_device / driver support to 3 iio devices and have that code
retrieve the regmap. And if not all drivers are using regmap (I did not
check) convert some of them to regmap first.

There really is no MFD device here, as the need to create separate
regmaps shows, usually the whole purpose of the MFD framework is
to share a single i2c-client / regmap between multiple drivers
because the i2c device has multiple function blocks. So the MFD
framework really is a _very_ poor fit here.

To put this differently, I can rewrite the DSDT so that things
just work without needing any kernel modification at all, the
problem is in the *enumeration* here, not in multiple separate function
blocks sharing a single address space.

And using your proposed quirk support for enumeration multiple
i2c-clients from a single acpi_device fixes the enumeration problem.

Regards,

Hans

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Rafael J. Wysocki]

On Sunday, May 20, 2018 3:28:48 PM CEST Hans de Goede wrote:
> Hi All,
> 
> This series really consists of 2 series, patches 1-5 add support for
> interesting ACPI tables which describe multiple i2c chips in a single
> fwnode, sometimes multiple cases of the same chip on different addresses,
> sometimes a bunch of related chips.
> 
> Andy Shevchenko has come up with the solution of adding a quirk based
> on the ACPI HID of the fwnode for these devices which makes the
> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices
> for each I2cSerialBusV2 in the fwnode. I agree with him that this is
> the best (least ugly) solution for this.
> 
> I've been testing this solution on a device if mine which needs a solution
> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode
> with a HID of BSG1160 which describes 3 different i2c sensors in an accel /
> magneto / gyro sensor cluster on the tablet. This has let to some extra
> prep. patches and some fixes to Andy's patches.
> 
> Patches 6-9 use the new functionality creating  one i2c-client per
> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and
> are posted as part of this series to show how this functionality can be
> used.
> 
> Assuming everyone is ok with this series (I'm not expecting anyone to be
> really happy about the need for this), then I suggest that patches 1-6
> get merged togther through either the ACPI or the i2c tree, I guess the
> i2c tree would make somewhat more sense, since most patches are there.
> 
> Then once those are accepted patches 7-9 can be merged into the iio tree,
> there is no compile time dependency between the 2, so these can be merged
> separately. Note merging 7-9 before there is agreement that this is the
> right way to fix this is probably not a good idea.

>From the discussion I gather that this series will be updated.

Thanks,
Rafael

Re: [PATCH 0/9] ACPI/i2c Enumerate several instances out of one fwnode

[Hans de Goede]

Hi,

On 24-05-18 10:55, Rafael J. Wysocki wrote:
> On Sunday, May 20, 2018 3:28:48 PM CEST Hans de Goede wrote:
>> Hi All,
>>
>> This series really consists of 2 series, patches 1-5 add support for
>> interesting ACPI tables which describe multiple i2c chips in a single
>> fwnode, sometimes multiple cases of the same chip on different addresses,
>> sometimes a bunch of related chips.
>>
>> Andy Shevchenko has come up with the solution of adding a quirk based
>> on the ACPI HID of the fwnode for these devices which makes the
>> drivers/i2c/i2c-core-acpi.c code instantiate separate i2c_client devices
>> for each I2cSerialBusV2 in the fwnode. I agree with him that this is
>> the best (least ugly) solution for this.
>>
>> I've been testing this solution on a device if mine which needs a solution
>> for this, the HP Pavilion x2 - 10-n000nd 2-in-1 has an acpi_device / fwnode
>> with a HID of BSG1160 which describes 3 different i2c sensors in an accel /
>> magneto / gyro sensor cluster on the tablet. This has let to some extra
>> prep. patches and some fixes to Andy's patches.
>>
>> Patches 6-9 use the new functionality creating  one i2c-client per
>> I2cSerialBusV2 resource to make the sensor cluster on the HP X2 work and
>> are posted as part of this series to show how this functionality can be
>> used.
>>
>> Assuming everyone is ok with this series (I'm not expecting anyone to be
>> really happy about the need for this), then I suggest that patches 1-6
>> get merged togther through either the ACPI or the i2c tree, I guess the
>> i2c tree would make somewhat more sense, since most patches are there.
>>
>> Then once those are accepted patches 7-9 can be merged into the iio tree,
>> there is no compile time dependency between the 2, so these can be merged
>> separately. Note merging 7-9 before there is agreement that this is the
>> right way to fix this is probably not a good idea.
> 
>  From the discussion I gather that this series will be updated.

More like thrown away and rewritten, but yes this is obsolete.

Regards,

Hans