[PATCH] platform/chrome: Add support for Acer C720

[PATCH] platform/chrome: Add support for Acer C720

[Mika Westerberg]

Acer C720 has touchpad and light sensor connected to a separate I2C buses.
Since the designware I2C host controller driver has two instances on this
particular machine we need a way to match the correct instance. Add support
for this and then register both C720 touchpad and light sensor.

This code is based on following patch from Benson Leung:

https://patchwork.kernel.org/patch/3074411/

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Benson Leung <bleung@chromium.org>
---
 drivers/platform/chrome/chromeos_laptop.c | 45 ++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/chrome/chromeos_laptop.c b/drivers/platform/chrome/chromeos_laptop.c
index 7f3aad0e115c..44806c5dc26f 100644
--- a/drivers/platform/chrome/chromeos_laptop.c
+++ b/drivers/platform/chrome/chromeos_laptop.c
@@ -45,6 +45,8 @@ static const char *i2c_adapter_names[] = {
 	"SMBus I801 adapter",
 	"i915 gmbus vga",
 	"i915 gmbus panel",
+	"i2c-designware-pci",
+	"i2c-designware-pci",
 };
 
 /* Keep this enum consistent with i2c_adapter_names */
@@ -52,6 +54,8 @@ enum i2c_adapter_type {
 	I2C_ADAPTER_SMBUS = 0,
 	I2C_ADAPTER_VGADDC,
 	I2C_ADAPTER_PANEL,
+	I2C_ADAPTER_DESIGNWARE_0,
+	I2C_ADAPTER_DESIGNWARE_1,
 };
 
 struct i2c_peripheral {
@@ -183,29 +187,42 @@ static struct i2c_client *__add_probed_i2c_device(
 	return client;
 }
 
+struct i2c_lookup {
+	const char *name;
+	int instance;
+	int n;
+};
+
 static int __find_i2c_adap(struct device *dev, void *data)
 {
-	const char *name = data;
+	struct i2c_lookup *lookup = data;
 	static const char *prefix = "i2c-";
 	struct i2c_adapter *adapter;
 	if (strncmp(dev_name(dev), prefix, strlen(prefix)) != 0)
 		return 0;
 	adapter = to_i2c_adapter(dev);
-	return (strncmp(adapter->name, name, strlen(name)) == 0);
+	if (strncmp(adapter->name, lookup->name, strlen(lookup->name)) == 0 &&
+	    lookup->n++ == lookup->instance)
+		return 1;
+	return 0;
 }
 
 static int find_i2c_adapter_num(enum i2c_adapter_type type)
 {
 	struct device *dev = NULL;
 	struct i2c_adapter *adapter;
-	const char *name = i2c_adapter_names[type];
+	struct i2c_lookup lookup;
+
+	memset(&lookup, 0, sizeof(lookup));
+	lookup.name = i2c_adapter_names[type];
+	lookup.instance = (type == I2C_ADAPTER_DESIGNWARE_1) ? 1 : 0;
+
 	/* find the adapter by name */
-	dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
-			      __find_i2c_adap);
+	dev = bus_find_device(&i2c_bus_type, NULL, &lookup, __find_i2c_adap);
 	if (!dev) {
 		/* Adapters may appear later. Deferred probing will retry */
 		pr_notice("%s: i2c adapter %s not found on system.\n", __func__,
-			  name);
+			  lookup.name);
 		return -ENODEV;
 	}
 	adapter = to_i2c_adapter(dev);
@@ -388,6 +405,15 @@ static struct chromeos_laptop acer_ac700 = {
 	},
 };
 
+static struct chromeos_laptop acer_c720 = {
+	.i2c_peripherals = {
+		/* Touchpad. */
+		{ .add = setup_cyapa_tp, I2C_ADAPTER_DESIGNWARE_0 },
+		/* Light Sensor. */
+		{ .add = setup_isl29018_als, I2C_ADAPTER_DESIGNWARE_1 },
+	},
+};
+
 static struct chromeos_laptop hp_pavilion_14_chromebook = {
 	.i2c_peripherals = {
 		/* Touchpad. */
@@ -445,6 +471,13 @@ static struct dmi_system_id chromeos_laptop_dmi_table[] __initdata = {
 		_CBDD(acer_ac700),
 	},
 	{
+		.ident = "Acer C720",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "Peppy"),
+		},
+		_CBDD(acer_c720),
+	},
+	{
 		.ident = "HP Pavilion 14 Chromebook",
 		.matches = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Butterfly"),
-- 
2.0.0.rc4

Re: [PATCH] platform/chrome: Add support for Acer C720

[Benson Leung]

Hi Mika!

Thanks for putting this patch together. It looks like this does the
job of distinguishing the two busses apart. Thanks!


On Tue, Jun 3, 2014 at 6:02 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Acer C720 has touchpad and light sensor connected to a separate I2C buses.
> Since the designware I2C host controller driver has two instances on this
> particular machine we need a way to match the correct instance. Add support
> for this and then register both C720 touchpad and light sensor.
>
> This code is based on following patch from Benson Leung:
>
> https://patchwork.kernel.org/patch/3074411/
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Benson Leung <bleung@chromium.org>

Since it was based on my original:
Signed-off-by: Benson Leung <bleung@chromium.org>

> ---
>  drivers/platform/chrome/chromeos_laptop.c | 45 ++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/chrome/chromeos_laptop.c b/drivers/platform/chrome/chromeos_laptop.c
> index 7f3aad0e115c..44806c5dc26f 100644
> --- a/drivers/platform/chrome/chromeos_laptop.c
> +++ b/drivers/platform/chrome/chromeos_laptop.c
> @@ -45,6 +45,8 @@ static const char *i2c_adapter_names[] = {
>         "SMBus I801 adapter",
>         "i915 gmbus vga",
>         "i915 gmbus panel",
> +       "i2c-designware-pci",
> +       "i2c-designware-pci",
>  };
>
>  /* Keep this enum consistent with i2c_adapter_names */
> @@ -52,6 +54,8 @@ enum i2c_adapter_type {
>         I2C_ADAPTER_SMBUS = 0,
>         I2C_ADAPTER_VGADDC,
>         I2C_ADAPTER_PANEL,
> +       I2C_ADAPTER_DESIGNWARE_0,
> +       I2C_ADAPTER_DESIGNWARE_1,
>  };
>
>  struct i2c_peripheral {
> @@ -183,29 +187,42 @@ static struct i2c_client *__add_probed_i2c_device(
>         return client;
>  }
>
> +struct i2c_lookup {
> +       const char *name;
> +       int instance;
> +       int n;
> +};
> +
>  static int __find_i2c_adap(struct device *dev, void *data)
>  {
> -       const char *name = data;
> +       struct i2c_lookup *lookup = data;
>         static const char *prefix = "i2c-";
>         struct i2c_adapter *adapter;
>         if (strncmp(dev_name(dev), prefix, strlen(prefix)) != 0)
>                 return 0;
>         adapter = to_i2c_adapter(dev);
> -       return (strncmp(adapter->name, name, strlen(name)) == 0);
> +       if (strncmp(adapter->name, lookup->name, strlen(lookup->name)) == 0 &&
> +           lookup->n++ == lookup->instance)
> +               return 1;
> +       return 0;

minor nit. This could retain the original form with a single return.

return  (strncmp(adapter->name, name, strlen(name)) == 0) &&
            lookup->n++ == lookup->instance);

>  }
>
>  static int find_i2c_adapter_num(enum i2c_adapter_type type)
>  {
>         struct device *dev = NULL;
>         struct i2c_adapter *adapter;
> -       const char *name = i2c_adapter_names[type];
> +       struct i2c_lookup lookup;
> +
> +       memset(&lookup, 0, sizeof(lookup));
> +       lookup.name = i2c_adapter_names[type];
> +       lookup.instance = (type == I2C_ADAPTER_DESIGNWARE_1) ? 1 : 0;
> +
>         /* find the adapter by name */
> -       dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
> -                             __find_i2c_adap);
> +       dev = bus_find_device(&i2c_bus_type, NULL, &lookup, __find_i2c_adap);
>         if (!dev) {
>                 /* Adapters may appear later. Deferred probing will retry */
>                 pr_notice("%s: i2c adapter %s not found on system.\n", __func__,
> -                         name);
> +                         lookup.name);
>                 return -ENODEV;
>         }
>         adapter = to_i2c_adapter(dev);
> @@ -388,6 +405,15 @@ static struct chromeos_laptop acer_ac700 = {
>         },
>  };
>
> +static struct chromeos_laptop acer_c720 = {
> +       .i2c_peripherals = {
> +               /* Touchpad. */
> +               { .add = setup_cyapa_tp, I2C_ADAPTER_DESIGNWARE_0 },
> +               /* Light Sensor. */
> +               { .add = setup_isl29018_als, I2C_ADAPTER_DESIGNWARE_1 },
> +       },
> +};
> +
>  static struct chromeos_laptop hp_pavilion_14_chromebook = {
>         .i2c_peripherals = {
>                 /* Touchpad. */
> @@ -445,6 +471,13 @@ static struct dmi_system_id chromeos_laptop_dmi_table[] __initdata = {
>                 _CBDD(acer_ac700),
>         },
>         {
> +               .ident = "Acer C720",
> +               .matches = {
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "Peppy"),
> +               },
> +               _CBDD(acer_c720),
> +       },
> +       {
>                 .ident = "HP Pavilion 14 Chromebook",
>                 .matches = {
>                         DMI_MATCH(DMI_PRODUCT_NAME, "Butterfly"),
> --
> 2.0.0.rc4
>

By the way, I can create patches on top of this to support the other
systems that we missed as well, the HP 14, the Dell 11, the Toshiba,
etc.

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] platform/chrome: Add support for Acer C720

[Mika Westerberg]

On Tue, Jun 03, 2014 at 12:46:00PM -0700, Benson Leung wrote:
> Hi Mika!
> 
> Thanks for putting this patch together. It looks like this does the
> job of distinguishing the two busses apart. Thanks!
> 
> 
> On Tue, Jun 3, 2014 at 6:02 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Acer C720 has touchpad and light sensor connected to a separate I2C buses.
> > Since the designware I2C host controller driver has two instances on this
> > particular machine we need a way to match the correct instance. Add support
> > for this and then register both C720 touchpad and light sensor.
> >
> > This code is based on following patch from Benson Leung:
> >
> > https://patchwork.kernel.org/patch/3074411/
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Benson Leung <bleung@chromium.org>
> 
> Since it was based on my original:
> Signed-off-by: Benson Leung <bleung@chromium.org>

Thanks.

> 
> > ---
> >  drivers/platform/chrome/chromeos_laptop.c | 45 ++++++++++++++++++++++++++-----
> >  1 file changed, 39 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/chromeos_laptop.c b/drivers/platform/chrome/chromeos_laptop.c
> > index 7f3aad0e115c..44806c5dc26f 100644
> > --- a/drivers/platform/chrome/chromeos_laptop.c
> > +++ b/drivers/platform/chrome/chromeos_laptop.c
> > @@ -45,6 +45,8 @@ static const char *i2c_adapter_names[] = {
> >         "SMBus I801 adapter",
> >         "i915 gmbus vga",
> >         "i915 gmbus panel",
> > +       "i2c-designware-pci",
> > +       "i2c-designware-pci",
> >  };
> >
> >  /* Keep this enum consistent with i2c_adapter_names */
> > @@ -52,6 +54,8 @@ enum i2c_adapter_type {
> >         I2C_ADAPTER_SMBUS = 0,
> >         I2C_ADAPTER_VGADDC,
> >         I2C_ADAPTER_PANEL,
> > +       I2C_ADAPTER_DESIGNWARE_0,
> > +       I2C_ADAPTER_DESIGNWARE_1,
> >  };
> >
> >  struct i2c_peripheral {
> > @@ -183,29 +187,42 @@ static struct i2c_client *__add_probed_i2c_device(
> >         return client;
> >  }
> >
> > +struct i2c_lookup {
> > +       const char *name;
> > +       int instance;
> > +       int n;
> > +};
> > +
> >  static int __find_i2c_adap(struct device *dev, void *data)
> >  {
> > -       const char *name = data;
> > +       struct i2c_lookup *lookup = data;
> >         static const char *prefix = "i2c-";
> >         struct i2c_adapter *adapter;
> >         if (strncmp(dev_name(dev), prefix, strlen(prefix)) != 0)
> >                 return 0;
> >         adapter = to_i2c_adapter(dev);
> > -       return (strncmp(adapter->name, name, strlen(name)) == 0);
> > +       if (strncmp(adapter->name, lookup->name, strlen(lookup->name)) == 0 &&
> > +           lookup->n++ == lookup->instance)
> > +               return 1;
> > +       return 0;
> 
> minor nit. This could retain the original form with a single return.
> 
> return  (strncmp(adapter->name, name, strlen(name)) == 0) &&
>             lookup->n++ == lookup->instance);
> 
> >  }
> >
> >  static int find_i2c_adapter_num(enum i2c_adapter_type type)
> >  {
> >         struct device *dev = NULL;
> >         struct i2c_adapter *adapter;
> > -       const char *name = i2c_adapter_names[type];
> > +       struct i2c_lookup lookup;
> > +
> > +       memset(&lookup, 0, sizeof(lookup));
> > +       lookup.name = i2c_adapter_names[type];
> > +       lookup.instance = (type == I2C_ADAPTER_DESIGNWARE_1) ? 1 : 0;
> > +
> >         /* find the adapter by name */
> > -       dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
> > -                             __find_i2c_adap);
> > +       dev = bus_find_device(&i2c_bus_type, NULL, &lookup, __find_i2c_adap);
> >         if (!dev) {
> >                 /* Adapters may appear later. Deferred probing will retry */
> >                 pr_notice("%s: i2c adapter %s not found on system.\n", __func__,
> > -                         name);
> > +                         lookup.name);
> >                 return -ENODEV;
> >         }
> >         adapter = to_i2c_adapter(dev);
> > @@ -388,6 +405,15 @@ static struct chromeos_laptop acer_ac700 = {
> >         },
> >  };
> >
> > +static struct chromeos_laptop acer_c720 = {
> > +       .i2c_peripherals = {
> > +               /* Touchpad. */
> > +               { .add = setup_cyapa_tp, I2C_ADAPTER_DESIGNWARE_0 },
> > +               /* Light Sensor. */
> > +               { .add = setup_isl29018_als, I2C_ADAPTER_DESIGNWARE_1 },
> > +       },
> > +};
> > +
> >  static struct chromeos_laptop hp_pavilion_14_chromebook = {
> >         .i2c_peripherals = {
> >                 /* Touchpad. */
> > @@ -445,6 +471,13 @@ static struct dmi_system_id chromeos_laptop_dmi_table[] __initdata = {
> >                 _CBDD(acer_ac700),
> >         },
> >         {
> > +               .ident = "Acer C720",
> > +               .matches = {
> > +                       DMI_MATCH(DMI_PRODUCT_NAME, "Peppy"),
> > +               },
> > +               _CBDD(acer_c720),
> > +       },
> > +       {
> >                 .ident = "HP Pavilion 14 Chromebook",
> >                 .matches = {
> >                         DMI_MATCH(DMI_PRODUCT_NAME, "Butterfly"),
> > --
> > 2.0.0.rc4
> >
> 
> By the way, I can create patches on top of this to support the other
> systems that we missed as well, the HP 14, the Dell 11, the Toshiba,
> etc.

Cool :)

Do you mind including $subject patch (with your suggested change above)
with your patch series once v3.16-rc1 is released? Please let me know if
you want me to do the change and send the patch myself.

Re: [PATCH] platform/chrome: Add support for Acer C720

[Benson Leung]

Hi Mika,


On Wed, Jun 4, 2014 at 1:37 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Do you mind including $subject patch (with your suggested change above)
> with your patch series once v3.16-rc1 is released? Please let me know if
> you want me to do the change and send the patch myself.

I'll include your Acer C720 patch along with the rest of my patches
when v3.16-rc1 is released.

Thanks!

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

[PATCH 0/1] platform/chrome: Probe multiple i2c adapters of the same name

[Scot Doyle]

Would a generalized approach to the "multiple adapters with same name" 
problem be desirable? If so, would this approach work?

I don't have access to an Acer C720. This patch has only been tested on a 
Toshiba Chromebook using the "i2c: designware-pci: Add Haswell PCI IDs" 
patch from Mika applied to the 3.15-rc8 release.

Thanks,
Scot

[PATCH 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Scot Doyle]

The chromeos_laptop module probes the first i2c adapter with a specific 
name for expected hardware. However, the Acer C720 Chromebook has two i2c 
adapters with the same name. This patch probes each i2c adapter with a 
specific name in turn, until locating the expected hardware.

Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
CC: Benson Leung <bleung@chromium.org>
CC: Mika Westerberg <mika.westerberg@linux.intel.com>
---
diff --git a/drivers/platform/chrome/chromeos_laptop.c b/drivers/platform/chrome/chromeos_laptop.c
index 7f3aad0..3a3382e 100644
--- a/drivers/platform/chrome/chromeos_laptop.c
+++ b/drivers/platform/chrome/chromeos_laptop.c
@@ -173,7 +173,7 @@ static struct i2c_client *__add_probed_i2c_device(
  	/* add the i2c device */
  	client = i2c_new_probed_device(adapter, info, addrs, NULL);
  	if (!client)
-		pr_err("%s failed to register device %d-%02x\n",
+		pr_debug("%s did not add i2c device %d-%02x\n",
  		       __func__, bus, info->addr);
  	else
  		pr_debug("%s added i2c device %d-%02x\n",
@@ -194,13 +194,14 @@ static int __find_i2c_adap(struct device *dev, void *data)
  	return (strncmp(adapter->name, name, strlen(name)) == 0);
  }

-static int find_i2c_adapter_num(enum i2c_adapter_type type)
+static int find_i2c_next_adapter_num(enum i2c_adapter_type type,
+		struct device *start)
  {
  	struct device *dev = NULL;
  	struct i2c_adapter *adapter;
  	const char *name = i2c_adapter_names[type];
-	/* find the adapter by name */
-	dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
+	/* find the next adapter by name */
+	dev = bus_find_device(&i2c_bus_type, start, (void *)name,
  			      __find_i2c_adap);
  	if (!dev) {
  		/* Adapters may appear later. Deferred probing will retry */
@@ -226,10 +227,17 @@ static struct i2c_client *add_probed_i2c_device(
  		struct i2c_board_info *info,
  		const unsigned short *addrs)
  {
-	return __add_probed_i2c_device(name,
-				       find_i2c_adapter_num(type),
-				       info,
-				       addrs);
+	struct i2c_client *client = NULL;
+	struct device *start = NULL;
+	int adapter_num = 0;
+	while (!client && adapter_num > -1) {
+		adapter_num = find_i2c_next_adapter_num(type, start);
+		client = __add_probed_i2c_device(name,
+						 adapter_num,
+						 info,
+						 addrs);
+	}
+	return client;
  }

  /*
@@ -242,10 +250,10 @@ static struct i2c_client *add_i2c_device(const char *name,
  						struct i2c_board_info *info)
  {
  	const unsigned short addr_list[] = { info->addr, I2C_CLIENT_END };
-	return __add_probed_i2c_device(name,
-				       find_i2c_adapter_num(type),
-				       info,
-				       addr_list);
+	return add_probed_i2c_device(name,
+				     type,
+				     info,
+				     addr_list);
  }

  static int setup_cyapa_tp(enum i2c_adapter_type type)

Re: [PATCH 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Mika Westerberg]

On Fri, Jun 06, 2014 at 11:25:14PM +0100, Scot Doyle wrote:
> The chromeos_laptop module probes the first i2c adapter with a specific name
> for expected hardware. However, the Acer C720 Chromebook has two i2c
> adapters with the same name. This patch probes each i2c adapter with a
> specific name in turn, until locating the expected hardware.
> 
> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
> CC: Benson Leung <bleung@chromium.org>
> CC: Mika Westerberg <mika.westerberg@linux.intel.com>

I don't have anything against this approach.

One comment though, if you happen to have two devices with the same
address but in different buses this can go wrong. Not sure how likely
that will happen.

Re: [PATCH 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Scot Doyle]

That's a good point, I'll submit a new patch which documents this 
requirement. If this isn't a sufficiently robust solution, I 
understand.

On Mon, 9 Jun 2014, Mika Westerberg wrote:

> On Fri, Jun 06, 2014 at 11:25:14PM +0100, Scot Doyle wrote:
>> The chromeos_laptop module probes the first i2c 
adapter with a specific name
>> for expected hardware. However, the Acer C720 Chromebook has two i2c
>> adapters with the same name. This patch probes each i2c adapter with a
>> specific name in turn, until locating the expected hardware.
>>
>> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
>> CC: Benson Leung <bleung@chromium.org>
>> CC: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> I don't have anything against this approach.
>
> One comment though, if you happen to have two devices with the same
> address but in different buses this can go wrong. Not sure how likely
> that will happen.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH v2 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Scot Doyle]

The chromeos_laptop module probes the first i2c adapter with a specific 
name for expected hardware. However, the Acer C720 Chromebook has two i2c 
adapters with the same name. This patch probes each i2c adapter with a 
specific name in turn, until locating the expected hardware.

Thanks to Mika Westerberg for identifying the need for unique bus 
addresses within each set of identically-named adapters.

Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
CC: Benson Leung <bleung@chromium.org>
CC: Mika Westerberg <mika.westerberg@linux.intel.com>
---
diff --git a/drivers/platform/chrome/chromeos_laptop.c 
b/drivers/platform/chrome/chromeos_laptop.c
index 7f3aad0..8382315 100644
--- a/drivers/platform/chrome/chromeos_laptop.c
+++ b/drivers/platform/chrome/chromeos_laptop.c
@@ -29,6 +29,8 @@
  #include <linux/module.h>
  #include <linux/platform_device.h>

+/* Note: Bus addresses must be unique within each set of identically-named
+	 adapters on a board, otherwise device probing may fail. */
  #define ATMEL_TP_I2C_ADDR	0x4b
  #define ATMEL_TP_I2C_BL_ADDR	0x25
  #define ATMEL_TS_I2C_ADDR	0x4a
@@ -173,7 +175,7 @@ static struct i2c_client *__add_probed_i2c_device(
  	/* add the i2c device */
  	client = i2c_new_probed_device(adapter, info, addrs, NULL);
  	if (!client)
-		pr_err("%s failed to register device %d-%02x\n",
+		pr_debug("%s did not add i2c device %d-%02x\n",
  		       __func__, bus, info->addr);
  	else
  		pr_debug("%s added i2c device %d-%02x\n",
@@ -194,13 +196,14 @@ static int __find_i2c_adap(struct device *dev, void *data)
  	return (strncmp(adapter->name, name, strlen(name)) == 0);
  }

-static int find_i2c_adapter_num(enum i2c_adapter_type type)
+static int find_i2c_next_adapter_num(enum i2c_adapter_type type,
+				     struct device *start)
  {
  	struct device *dev = NULL;
  	struct i2c_adapter *adapter;
  	const char *name = i2c_adapter_names[type];
-	/* find the adapter by name */
-	dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
+	/* find the next adapter by name */
+	dev = bus_find_device(&i2c_bus_type, start, (void *)name,
  			      __find_i2c_adap);
  	if (!dev) {
  		/* Adapters may appear later. Deferred probing will retry */
@@ -226,10 +229,17 @@ static struct i2c_client *add_probed_i2c_device(
  		struct i2c_board_info *info,
  		const unsigned short *addrs)
  {
-	return __add_probed_i2c_device(name,
-				       find_i2c_adapter_num(type),
-				       info,
-				       addrs);
+	struct i2c_client *client = NULL;
+	struct device *start = NULL;
+	int adapter_num = 0;
+	while (!client && adapter_num > -1) {
+		adapter_num = find_i2c_next_adapter_num(type, start);
+		client = __add_probed_i2c_device(name,
+						 adapter_num,
+						 info,
+						 addrs);
+	}
+	return client;
  }

  /*
@@ -242,10 +252,10 @@ static struct i2c_client *add_i2c_device(const char *name,
  						struct i2c_board_info *info)
  {
  	const unsigned short addr_list[] = { info->addr, I2C_CLIENT_END };
-	return __add_probed_i2c_device(name,
-				       find_i2c_adapter_num(type),
-				       info,
-				       addr_list);
+	return add_probed_i2c_device(name,
+				     type,
+				     info,
+				     addr_list);
  }

  static int setup_cyapa_tp(enum i2c_adapter_type type)

Re: [PATCH v2 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Mika Westerberg]

On Tue, Jun 10, 2014 at 06:17:35AM +0100, Scot Doyle wrote:
> The chromeos_laptop module probes the first i2c adapter with a specific name
> for expected hardware. However, the Acer C720 Chromebook has two i2c
> adapters with the same name. This patch probes each i2c adapter with a
> specific name in turn, until locating the expected hardware.
> 
> Thanks to Mika Westerberg for identifying the need for unique bus addresses
> within each set of identically-named adapters.
> 
> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
> CC: Benson Leung <bleung@chromium.org>
> CC: Mika Westerberg <mika.westerberg@linux.intel.com>

Few comments about style:

> ---
> diff --git a/drivers/platform/chrome/chromeos_laptop.c
> b/drivers/platform/chrome/chromeos_laptop.c
> index 7f3aad0..8382315 100644
> --- a/drivers/platform/chrome/chromeos_laptop.c
> +++ b/drivers/platform/chrome/chromeos_laptop.c
> @@ -29,6 +29,8 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> 
> +/* Note: Bus addresses must be unique within each set of identically-named
> +	 adapters on a board, otherwise device probing may fail. */

Check Documentation/CodingStyle about format of block comments.

>  #define ATMEL_TP_I2C_ADDR	0x4b
>  #define ATMEL_TP_I2C_BL_ADDR	0x25
>  #define ATMEL_TS_I2C_ADDR	0x4a
> @@ -173,7 +175,7 @@ static struct i2c_client *__add_probed_i2c_device(
>  	/* add the i2c device */
>  	client = i2c_new_probed_device(adapter, info, addrs, NULL);
>  	if (!client)
> -		pr_err("%s failed to register device %d-%02x\n",
> +		pr_debug("%s did not add i2c device %d-%02x\n",
>  		       __func__, bus, info->addr);
>  	else
>  		pr_debug("%s added i2c device %d-%02x\n",
> @@ -194,13 +196,14 @@ static int __find_i2c_adap(struct device *dev, void *data)
>  	return (strncmp(adapter->name, name, strlen(name)) == 0);
>  }
> 
> -static int find_i2c_adapter_num(enum i2c_adapter_type type)
> +static int find_i2c_next_adapter_num(enum i2c_adapter_type type,
> +				     struct device *start)
>  {
>  	struct device *dev = NULL;
>  	struct i2c_adapter *adapter;
>  	const char *name = i2c_adapter_names[type];
> -	/* find the adapter by name */
> -	dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
> +	/* find the next adapter by name */
> +	dev = bus_find_device(&i2c_bus_type, start, (void *)name,
>  			      __find_i2c_adap);
>  	if (!dev) {
>  		/* Adapters may appear later. Deferred probing will retry */
> @@ -226,10 +229,17 @@ static struct i2c_client *add_probed_i2c_device(
>  		struct i2c_board_info *info,
>  		const unsigned short *addrs)
>  {
> -	return __add_probed_i2c_device(name,
> -				       find_i2c_adapter_num(type),
> -				       info,
> -				       addrs);
> +	struct i2c_client *client = NULL;
> +	struct device *start = NULL;
> +	int adapter_num = 0;

Please add blank line here.

> +	while (!client && adapter_num > -1) {
> +		adapter_num = find_i2c_next_adapter_num(type, start);
> +		client = __add_probed_i2c_device(name,
> +						 adapter_num,
> +						 info,
> +						 addrs);

Don't the above fit into one line, specifically if you call adapter_num
just num or something like that.

> +	}
> +	return client;
>  }
> 
>  /*
> @@ -242,10 +252,10 @@ static struct i2c_client *add_i2c_device(const char *name,
>  						struct i2c_board_info *info)
>  {
>  	const unsigned short addr_list[] = { info->addr, I2C_CLIENT_END };
> -	return __add_probed_i2c_device(name,
> -				       find_i2c_adapter_num(type),
> -				       info,
> -				       addr_list);
> +	return add_probed_i2c_device(name,
> +				     type,
> +				     info,
> +				     addr_list);

Same here.

>  }
> 
>  static int setup_cyapa_tp(enum i2c_adapter_type type)

Re: [PATCH v3 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Benson Leung]

On Wed, Jun 11, 2014 at 10:51 AM, Scot Doyle <lkml14@scotdoyle.com> wrote:
> The chromeos_laptop module probes the first i2c adapter with a specific name
> for expected hardware. However, the Acer C720 Chromebook has two i2c
> adapters with the same name. This patch probes each i2c adapter with a
> specific name until locating the expected hardware.
>
> Thanks to Mika Westerberg for formatting tips and identifying the need for
> unique bus addresses within each set of identically-named adapters.


Scot,

Is your intention with this patch to replace the one that Mika wrote
that specifically special cased the designware i2c busses? He already
solved this problem in this patch.
https://patchwork.kernel.org/patch/4288591/

I'm not sure about your approach here. It looks like if there's some
ambiguity in the names of the busses, you try to probe the device on
every bus with that name.

We know that bus 0 is going to have the touchpad, and we know that bus
1 is going to have the touchscreen and light sensor. I'd prefer not to
send extra probes for all of the combination of devices that don't
exist.

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

[PATCH v3 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Scot Doyle]

The chromeos_laptop module probes the first i2c adapter with a specific 
name for expected hardware. However, the Acer C720 Chromebook has two i2c 
adapters with the same name. This patch probes each i2c adapter with a 
specific name until locating the expected hardware.

Thanks to Mika Westerberg for formatting tips and identifying the need for 
unique bus addresses within each set of identically-named adapters.

Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
CC: Benson Leung <bleung@chromium.org>
CC: Mika Westerberg <mika.westerberg@linux.intel.com>
---
diff --git a/drivers/platform/chrome/chromeos_laptop.c b/drivers/platform/chrome/chromeos_laptop.c
index 7f3aad0..82ae5e7 100644
--- a/drivers/platform/chrome/chromeos_laptop.c
+++ b/drivers/platform/chrome/chromeos_laptop.c
@@ -29,6 +29,10 @@
  #include <linux/module.h>
  #include <linux/platform_device.h>

+/*
+ * Note: Bus addresses must be unique within each set of identically-named
+ *       adapters on a board, otherwise device probing may fail.
+ */
  #define ATMEL_TP_I2C_ADDR	0x4b
  #define ATMEL_TP_I2C_BL_ADDR	0x25
  #define ATMEL_TS_I2C_ADDR	0x4a
@@ -173,7 +177,7 @@ static struct i2c_client *__add_probed_i2c_device(
  	/* add the i2c device */
  	client = i2c_new_probed_device(adapter, info, addrs, NULL);
  	if (!client)
-		pr_err("%s failed to register device %d-%02x\n",
+		pr_debug("%s did not add i2c device %d-%02x\n",
  		       __func__, bus, info->addr);
  	else
  		pr_debug("%s added i2c device %d-%02x\n",
@@ -194,13 +198,14 @@ static int __find_i2c_adap(struct device *dev, void *data)
  	return (strncmp(adapter->name, name, strlen(name)) == 0);
  }

-static int find_i2c_adapter_num(enum i2c_adapter_type type)
+static int find_i2c_next_adapter_num(enum i2c_adapter_type type,
+				     struct device *start)
  {
  	struct device *dev = NULL;
  	struct i2c_adapter *adapter;
  	const char *name = i2c_adapter_names[type];
-	/* find the adapter by name */
-	dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
+	/* find the next adapter by name */
+	dev = bus_find_device(&i2c_bus_type, start, (void *)name,
  			      __find_i2c_adap);
  	if (!dev) {
  		/* Adapters may appear later. Deferred probing will retry */
@@ -226,10 +231,15 @@ static struct i2c_client *add_probed_i2c_device(
  		struct i2c_board_info *info,
  		const unsigned short *addrs)
  {
-	return __add_probed_i2c_device(name,
-				       find_i2c_adapter_num(type),
-				       info,
-				       addrs);
+	struct i2c_client *client = NULL;
+	struct device *start = NULL;
+	int num = 0;
+
+	while (!client && num > -1) {
+		num = find_i2c_next_adapter_num(type, start);
+		client = __add_probed_i2c_device(name, num, info, addrs);
+	}
+	return client;
  }

  /*
@@ -242,10 +252,7 @@ static struct i2c_client *add_i2c_device(const char *name,
  						struct i2c_board_info *info)
  {
  	const unsigned short addr_list[] = { info->addr, I2C_CLIENT_END };
-	return __add_probed_i2c_device(name,
-				       find_i2c_adapter_num(type),
-				       info,
-				       addr_list);
+	return add_probed_i2c_device(name, type, info, addr_list);
  }

  static int setup_cyapa_tp(enum i2c_adapter_type type)

Re: [PATCH v3 1/1] platform/chrome: Probe multiple i2c adapters of the same name

[Scot Doyle]

On Wed, 11 Jun 2014, Benson Leung wrote:
> On Wed, Jun 11, 2014 at 10:51 AM, Scot Doyle <lkml14@scotdoyle.com> wrote:
>> The chromeos_laptop module probes the first i2c adapter with a specific name
>> for expected hardware. However, the Acer C720 Chromebook has two i2c
>> adapters with the same name. This patch probes each i2c adapter with a
>> specific name until locating the expected hardware.
>>
>> Thanks to Mika Westerberg for formatting tips and identifying the need for
>> unique bus addresses within each set of identically-named adapters.
>
> Scot,
>
> Is your intention with this patch to replace the one that Mika wrote
> that specifically special cased the designware i2c busses? He already
> solved this problem in this patch.
> https://patchwork.kernel.org/patch/4288591/
>
> I'm not sure about your approach here. It looks like if there's some
> ambiguity in the names of the busses, you try to probe the device on
> every bus with that name.
>
> We know that bus 0 is going to have the touchpad, and we know that bus
> 1 is going to have the touchscreen and light sensor. I'd prefer not to
> send extra probes for all of the combination of devices that don't
> exist.

In the case of the Acer C720, will the touchpad always be the first device 
iterated by the bus_find_device function?

My quite possibly incorrect understanding is that:
- the i2c bus numbers assigned are dynamic
- dynamic bus numbers depend on the order of device registration
- there is no guarantee of the ordering of device registration
- bus_find_device iterates by insertion order (as opposed to ascending pci 
device id or ascending pci hardware address)