Message-ID: <20140611105300.GE1657@lahna.fi.intel.com>
Date:	Wed, 11 Jun 2014 13:53:00 +0300
From:	Mika Westerberg <mika.westerberg@...ux.intel.com>
To:	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:	wsa@...-dreams.de, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH 1/2] i2c/ACPI: Support for multiple serial bus addresses

On Tue, Jun 10, 2014 at 08:09:22AM -0700, Srinivas Pandruvada wrote:
> On 06/10/2014 02:24 AM, Mika Westerberg wrote:
> >On Thu, Apr 10, 2014 at 09:15:16PM -0700, Srinivas Pandruvada wrote:
> >>ACPI specification allows multiple i2c addresses defined under one
> >>ACPI device object. These addresses are defined using _CRS method.
> >>The current implementation will pickup the last entry in _CRS, and
> >>create an i2C device, ignoring all other previous entries for addresses.
> >>
> >>The ACPI specification does not define, whether these addresses for one
> >>i2c device or for multiple i2c devices, which are defined under the same
> >>ACPI device object. We need some appoach where i2c clients can enumerate
> >>on each of the i2C address and/or have access to those extra addresses.
> >>
> >>This change addresses both cases:
> >>- Create a new i2c device for each i2c address
> >>- Allow each i2 client to get addresses of all other devices under
> >>the same ACPI device object (companions or siblings)
> >>
> >>Example 1:
> >>Here in the following example, there are two i2C address in _CRS.
> >>They belong to two different physical chipsets, with two different i2c
> >>address but part of a module.
> >>Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> >>{
> >>         Name (RBUF, ResourceTemplate ()
> >>         {
> >>		I2cSerialBus (0x0068, ControllerInitiated, 0x00061A80,
> >>			AddressingMode7Bit, "\\_SB.I2C5",
> >>			0x00, ResourceConsumer, ,
> >>		)
> >>		I2cSerialBus (0x000C, ControllerInitiated, 0x00061A80,
> >>			AddressingMode7Bit, "\\_SB.I2C5",
> >>			0x00, ResourceConsumer, ,
> >>		)
> >>		Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
> >>		{
> >>			0x00000044,
> >>		}
> >>	})
> >>	Return (RBUF)
> >>}
> >>This change adds i2c_new_device for each i2c address. Here contents of
> >>/sys/bus/i2c/devices will
> >>	i2c-some_acpi_dev_name:00:068
> >>	i2c-some_acpi_dev_name::00:00c
> >>
> >>Example 2
> >>
> >>Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> >>                 {
> >>                     Name (SBUF, ResourceTemplate ()
> >>                     {
> >>                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> >>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> >>                             )
> >>                             {   // Pin list
> >>                                 0x0018
> >>                             }
> >>                         I2cSerialBus (0x0010, ControllerInitiated, 0x00061A80,
> >>                             AddressingMode7Bit, "\\_SB.I2C4",
> >>                             0x00, ResourceConsumer, ,
> >>                             )
> >>                         I2cSerialBus (0x000C, ControllerInitiated, 0x00061A80,
> >>                             AddressingMode7Bit, "\\_SB.I2C4",
> >>                             0x00, ResourceConsumer, ,
> >>                             )
> >>                         I2cSerialBus (0x0054, ControllerInitiated, 0x00061A80,
> >>                             AddressingMode7Bit, "\\_SB.I2C4",
> >>                             0x00, ResourceConsumer, ,
> >>                             )
> >>                     })
> >>                     Return (SBUF) /* \_SB_.I2C4.CAM1._CRS.SBUF */
> >>                 }
> >>}
> >>Here there are three i2c addresses for this device.
> >
> >How does the client driver see this?
> >
> >If I have a device that has multiple addresses but it is the same
> >physical device, how does the driver deal with it? For example how
> >drivers/misc/eeprom/at24.c could take advantage of this?
> >
> 
> Use i2c_for_each_acpi_comp_client. I use similar approach as
> i2c_for_each_dev already defined in i2c.h.

But now probe is called multiple times (with a different i2c_client
passed in so I need to keep some account about that, right? Doing that
makes the drivers more complex.

> 
> >I also think that this needs to documented somewhere under
> >Documentation/i2c/* or so.
> >
> >>When there is only I2cSerialBus address is present then there is no
> >>change. The device name will not include address. In this way if
> >>some device is using hardcoded path, this change will not impact them.
> >>
> >>Here any i2c client driver simply need to add ACPI bindings. They will
> >>be probed multiple times, the client will bind to correct i2c device,
> >>based on checking its identity and returning error for other.
> >>At the same time, any i2c client can call i2c_for_each_acpi_comp_client
> >>to get the i2c client instances of companion addresses defined
> >>under the same ACPI device.
> >>
> >>Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> >>---
> >>  drivers/i2c/i2c-core.c | 111 ++++++++++++++++++++++++++++++++++++++++++-------
> >>  include/linux/i2c.h    |  13 ++++++
> >>  2 files changed, 109 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >>index 5fb80b8..a69a7b4 100644
> >>--- a/drivers/i2c/i2c-core.c
> >>+++ b/drivers/i2c/i2c-core.c
> >>@@ -627,7 +627,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
> >>  	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> >>
> >>  	if (adev) {
> >>-		dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
> >>+		dev_set_name(&client->dev, "i2c-%s", client->name);
> >>  		return;
> >>  	}
> >>
> >>@@ -1080,24 +1080,44 @@ static void of_i2c_register_devices(struct i2c_adapter *adap) { }
> >>  /* ACPI support code */
> >>
> >>  #if IS_ENABLED(CONFIG_ACPI)
> >>+/*
> >>+ * acpi_i2c_add_resource is a callback, which is called while walking
> >>+ * name spaces, adding a limit allows for faster processing, without
> >>+ * using reallocation,
> >>+ * Adding some limit for max adresses in resource. Currently we see
> >>+ * max only 3 addresses, so 20 is more than enough
> >>+ */
> >>+#define MAX_CRS_ELEMENTS	20
> >>+struct i2c_resource_info {
> >>+	unsigned short addr[MAX_CRS_ELEMENTS];
> >>+	unsigned short flags[MAX_CRS_ELEMENTS];
> >>+	int count;
> >>+	int common_irq;
> >>+};
> >>+
> >>  static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data)
> >>  {
> >>-	struct i2c_board_info *info = data;
> >>+	struct i2c_resource_info *rcs_info = data;
> >>
> >>  	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
> >>  		struct acpi_resource_i2c_serialbus *sb;
> >>
> >>  		sb = &ares->data.i2c_serial_bus;
> >>  		if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> >>-			info->addr = sb->slave_address;
> >>+			if (rcs_info->count >= MAX_CRS_ELEMENTS)
> >>+				return 1; /* Simply ignore adding */
> >>+			rcs_info->addr[rcs_info->count] =
> >>+							sb->slave_address;
> >>  			if (sb->access_mode == ACPI_I2C_10BIT_MODE)
> >>-				info->flags |= I2C_CLIENT_TEN;
> >>+				rcs_info->flags[rcs_info->count] =
> >>+								I2C_CLIENT_TEN;
> >>+			rcs_info->count++;
> >>  		}
> >>-	} else if (info->irq < 0) {
> >>+	} else if (rcs_info->common_irq < 0) {
> >>  		struct resource r;
> >>
> >>  		if (acpi_dev_resource_interrupt(ares, 0, &r))
> >>-			info->irq = r.start;
> >>+			rcs_info->common_irq = r.start;
> >>  	}
> >>
> >>  	/* Tell the ACPI core to skip this resource */
> >>@@ -1111,7 +1131,10 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
> >>  	struct list_head resource_list;
> >>  	struct i2c_board_info info;
> >>  	struct acpi_device *adev;
> >>+	struct i2c_resource_info rcs_info;
> >>+	struct i2c_client *i2c_client;
> >>  	int ret;
> >>+	int i;
> >>
> >>  	if (acpi_bus_get_device(handle, &adev))
> >>  		return AE_OK;
> >>@@ -1120,23 +1143,42 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
> >>
> >>  	memset(&info, 0, sizeof(info));
> >>  	info.acpi_node.companion = adev;
> >>-	info.irq = -1;
> >>+
> >>+	memset(&rcs_info, 0, sizeof(rcs_info));
> >>+	rcs_info.common_irq = -1;
> >>
> >>  	INIT_LIST_HEAD(&resource_list);
> >>  	ret = acpi_dev_get_resources(adev, &resource_list,
> >>-				     acpi_i2c_add_resource, &info);
> >>+				     acpi_i2c_add_resource, &rcs_info);
> >>  	acpi_dev_free_resource_list(&resource_list);
> >>
> >>-	if (ret < 0 || !info.addr)
> >>+	if (ret < 0)
> >>  		return AE_OK;
> >>
> >>  	adev->power.flags.ignore_parent = true;
> >>-	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
> >>-	if (!i2c_new_device(adapter, &info)) {
> >>-		adev->power.flags.ignore_parent = false;
> >>-		dev_err(&adapter->dev,
> >>-			"failed to add I2C device %s from ACPI\n",
> >>-			dev_name(&adev->dev));
> >>+	info.irq = rcs_info.common_irq;
> >>+	for (i = 0; i < rcs_info.count; ++i) {
> >
> >Instead of this, would it make sense if you do the device creation in
> >acpi_i2c_add_resource() for each enumerated device?
> >
> 
> You can't. Because the interrupt resource is common. You may see that
> definition (as above examples), you have to go through the list before, to
> find out irq number, which is needed to create i2c device, if there
> is any IRQ.

Indeed. So how about first looking for the IRQ and then add the devices?
Or something like that.

> 
> >At least it would look better than this ;-)
> >
> >>+		if (!rcs_info.addr[i])
> >>+			continue;
> >>+		info.addr = rcs_info.addr[i];
> >>+		info.flags = rcs_info.flags[i];
> >>+		/* Status quo, when only one address is present */
> >>+		if (rcs_info.count > 1)
> >>+			snprintf(info.type, sizeof(info.type), "%s:%03x",
> >>+						dev_name(&adev->dev),
> >>+						info.addr);
> >>+		else
> >>+			strlcpy(info.type, dev_name(&adev->dev),
> >>+							sizeof(info.type));
> >>+		i2c_client = i2c_new_device(adapter, &info);
> >>+		if (!i2c_client) {
> >>+			if (!i)
> >>+				adev->power.flags.ignore_parent = false;
> >>+			dev_err(&adapter->dev,
> >>+				"failed to add I2C device %s from ACPI\n",
> >>+					dev_name(&adev->dev));
> >>+			break;
> >>+		}
> >>  	}
> >>
> >>  	return AE_OK;
> >>@@ -1168,6 +1210,45 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap)
> >>  	if (ACPI_FAILURE(status))
> >>  		dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
> >>  }
> >>+
> >>+/* Helper functions to get companion addresses */
> >>+struct acpi_comp_arg {
> >>+	struct acpi_device *acpi_dev;
> >>+	void (*callback)(struct i2c_client *);
> >>+};
> >>+
> >>+static int i2c_acpi_companion(struct device *dev, void *_arg)
> >>+{
> >>+	struct acpi_comp_arg *arg = _arg;
> >>+
> >>+	if (arg->acpi_dev == ACPI_COMPANION(dev)) {
> >>+		struct i2c_client *client;
> >>+
> >>+		client = to_i2c_client(dev);
> >>+		arg->callback(client);
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >
> >Missing kernel doc here.
> >
> >>+int i2c_for_each_acpi_comp_client(struct i2c_client *client,
> >>+					void (*callback)(struct i2c_client *))
> >>+{
> >>+	struct acpi_comp_arg arg;
> >>+	int found;
> >>+
> >>+	if (!client)
> >>+		return -EINVAL;
> >>+
> >>+	arg.acpi_dev = ACPI_COMPANION(&client->dev);
> >>+	arg.callback = callback;
> >>+	found = device_for_each_child(&client->adapter->dev, &arg,
> >>+							i2c_acpi_companion);
> >>+
> >>+	return found;
> >>+}
> >>+EXPORT_SYMBOL_GPL(i2c_for_each_acpi_comp_client);
> >
> >Since devices with multiple addresses seem to exist in real world, I
> >think it would be beneficial to add a bit more generic implementation of
> >the above to the I2C core instead. Then non-ACPI systems could also take
> >advantage of it.
> >
> What is your suggestion? i2c already use similar method for finding each
> i2c_for_each_dev, I felt similar method would be sufficient.

Well, how about linking relative i2c_clients? So that you can walk the
list if you have multi-address device.

> 
> >>+
> >>  #else
> >>  static inline void acpi_i2c_register_devices(struct i2c_adapter *adap) {}
> >>  #endif /* CONFIG_ACPI */
> >>diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> >>index deddeb8..ce75c73 100644
> >>--- a/include/linux/i2c.h
> >>+++ b/include/linux/i2c.h
> >>@@ -323,6 +323,19 @@ extern struct i2c_client *
> >>  i2c_new_dummy(struct i2c_adapter *adap, u16 address);
> >>
> >>  extern void i2c_unregister_device(struct i2c_client *);
> >>+
> >>+#if IS_ENABLED(CONFIG_ACPI)
> >>+/* Callback to get i2c_client instance for ACPI i2 resource */
> >>+int i2c_for_each_acpi_comp_client(struct i2c_client *client,
> >>+					void (*callback)(struct i2c_client *));
> >>+#else
> >>+int i2c_for_each_acpi_comp_client(struct i2c_client *client,
> >>+					int (*callback)(struct i2c_client *))
> >>+{
> >>+	return 0;
> >>+}
> >>+#endif
> >>+
> >>  #endif /* I2C */
> >>
> >>  /* Mainboard arch_initcall() code should register all its I2C devices.
> >>--
> >>1.7.11.7
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >>the body of a message to majordomo@...r.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>Please read the FAQ at  http://www.tux.org/lkml/
> >
> Thanks,
> Srinivas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists