Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

2014-01-16 Thread Zhang Rui
On Wed, 2014-01-15 at 17:08 +0200, Mika Westerberg wrote:
 On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
  diff --git a/drivers/base/platform.c b/drivers/base/platform.c
  index 3a94b79..2f4aea2 100644
  --- a/drivers/base/platform.c
  +++ b/drivers/base/platform.c
  @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, 
  struct device_attribute *a,
   char *buf)
   {
  struct platform_device  *pdev = to_platform_device(dev);
  -   int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
  +   int len;
  +
  +   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
 
 Here you have PAGE_SIZE -1...
 
  +   if (len != -ENODEV)
  +   return len;
  +
  +   len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
   
  return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
   }
  @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct 
  kobj_uevent_env *env)
  if (rc != -ENODEV)
  return rc;
   
  +   rc = acpi_device_uevent_modalias(dev, env);
  +   if (rc != -ENODEV)
  +   return rc;
  +
  add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX,
  pdev-name);
  return 0;
  diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
  index d74c0b3..c4c5588 100644
  --- a/drivers/i2c/i2c-core.c
  +++ b/drivers/i2c/i2c-core.c
  @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct 
  device_driver *drv)
   static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env 
  *env)
   {
  struct i2c_client   *client = to_i2c_client(dev);
  +   int rc;
  +
  +   rc = acpi_device_uevent_modalias(dev, env);
  +   if (rc != -ENODEV)
  +   return rc;
   
  if (add_uevent_var(env, MODALIAS=%s%s,
 I2C_MODULE_PREFIX, client-name))
  @@ -409,6 +414,12 @@ static ssize_t
   show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
   {
  struct i2c_client *client = to_i2c_client(dev);
  +   int len;
  +
  +   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
 
 and here
 
  +   if (len != -ENODEV)
  +   return len;
  +
  return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name);
   }
   
  diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
  index 349ebba..ab70eda 100644
  --- a/drivers/spi/spi.c
  +++ b/drivers/spi/spi.c
  @@ -58,6 +58,11 @@ static ssize_t
   modalias_show(struct device *dev, struct device_attribute *a, char *buf)
   {
  const struct spi_device *spi = to_spi_device(dev);
  +   int len;
  +
  +   len = acpi_device_modalias(dev, buf, PAGE_SIZE);
 
 but here it is PAGE_SIZE.
 
good catch.

 Perhaps it should be PAGE_SIZE in all sites?

dev_attr_show() will give a warning message if modalias_show() returns
PAGE_SIZE, thus I'd prefer to use PAGE_SIZE - 1 for all sites.

thanks,
rui


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

2014-01-16 Thread Zhang Rui
On Thu, 2014-01-16 at 12:28 +, Mark Brown wrote:
 On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
  ACPI enumerated devices has ACPI style _HID and _CID strings,
  all of these strings can be used for both driver loading and matching.
  
  Currently, in Platform, I2C and SPI bus, the ACPI style driver matching
  is supported by invoking acpi_driver_match_device() in bus .match() 
  callback.
  But, the module autoloading is still broken.
 
 Acked-by: Mark Brown broo...@linaro.org
 
 modulo the PAGE_SIZE stuff Mika noted - unless this changes radically
 please just assume I'm OK with it.

thanks.

-rui


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

2014-01-16 Thread Zhang Rui
On Thu, 2014-01-16 at 13:27 +0100, Wolfram Sang wrote:
  diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
  index d74c0b3..c4c5588 100644
  --- a/drivers/i2c/i2c-core.c
  +++ b/drivers/i2c/i2c-core.c
  @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct 
  device_driver *drv)
   static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env 
  *env)
   {
  struct i2c_client   *client = to_i2c_client(dev);
  +   int rc;
  +
  +   rc = acpi_device_uevent_modalias(dev, env);
  +   if (rc != -ENODEV)
  +   return rc;
   
  if (add_uevent_var(env, MODALIAS=%s%s,
 I2C_MODULE_PREFIX, client-name))
 
 I wonder why we don't have/need that with CONFIG_OF? Because probably
 nobody is using modules with i2c devices there?
 
This seems a gap to me but I'm not 100% sure.
I saw Grant Likely introduced the OF style MODALIAS to platform bus, and
OF style registration/binding to i2c bus, maybe he has an answer for
this.

thanks,
rui


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

2014-01-16 Thread Zhang Rui
On Fri, 2014-01-17 at 02:28 +0100, Rafael J. Wysocki wrote:
 On Thursday, January 16, 2014 04:04:35 PM Zhang Rui wrote:
  On Wed, 2014-01-15 at 17:08 +0200, Mika Westerberg wrote:
   On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..2f4aea2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, 
struct device_attribute *a,
 char *buf)
 {
struct platform_device  *pdev = to_platform_device(dev);
-   int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
   
   Here you have PAGE_SIZE -1...
   
+   if (len != -ENODEV)
+   return len;
+
+   len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
 
return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
 }
@@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, 
struct kobj_uevent_env *env)
if (rc != -ENODEV)
return rc;
 
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
+
add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX,
pdev-name);
return 0;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d74c0b3..c4c5588 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, 
struct device_driver *drv)
 static int i2c_device_uevent(struct device *dev, struct 
kobj_uevent_env *env)
 {
struct i2c_client   *client = to_i2c_client(dev);
+   int rc;
+
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
 
if (add_uevent_var(env, MODALIAS=%s%s,
   I2C_MODULE_PREFIX, client-name))
@@ -409,6 +414,12 @@ static ssize_t
 show_modalias(struct device *dev, struct device_attribute *attr, char 
*buf)
 {
struct i2c_client *client = to_i2c_client(dev);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
   
   and here
   
+   if (len != -ENODEV)
+   return len;
+
return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name);
 }
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 349ebba..ab70eda 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -58,6 +58,11 @@ static ssize_t
 modalias_show(struct device *dev, struct device_attribute *a, char 
*buf)
 {
const struct spi_device *spi = to_spi_device(dev);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE);
   
   but here it is PAGE_SIZE.
   
  good catch.
  
   Perhaps it should be PAGE_SIZE in all sites?
  
  dev_attr_show() will give a warning message if modalias_show() returns
  PAGE_SIZE, thus I'd prefer to use PAGE_SIZE - 1 for all sites.
 
 So I changed the PAGE_SIZE to PAGE_SIZE - 1 in the last instance and queued
 up the whole series for 3.14 in linux-pm.git/linux-next.  Please have a look
 at that and let me know if there's anything wrong with it.
 
the change looks okay to me.

thanks,
rui
 Thanks!
 


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

2014-01-15 Thread Zhang, Rui


 -Original Message-
 From: Rob Herring [mailto:robherri...@gmail.com]
 Sent: Wednesday, January 15, 2014 9:45 PM
 To: Zhang, Rui
 Cc: linux-ker...@vger.kernel.org; linux-a...@vger.kernel.org; linux-
 i...@vger.kernel.org; linux-...@vger.kernel.org; w...@the-dreams.de; Mark
 Brown; Greg Kroah-Hartman; Wysocki, Rafael J; Grant Likely; Rob Herring;
 jarkko.nik...@linux.intel.com; mika.westerb...@linux.intel.com;
 devicet...@vger.kernel.org
 Subject: Re: [PATCH 4/4] OF: introduce OF style 'modalias' support for
 platform bus.
 Importance: High
 
 On Tue, Jan 14, 2014 at 2:46 AM, Zhang Rui rui.zh...@intel.com wrote:
  Fix a problem that, the platform bus supports the OF style modalias
 in
  .uevent() call, but not in its device 'modalias' sysfs attribute.
 
  cc: devicet...@vger.kernel.org
  Signed-off-by: Zhang Rui rui.zh...@intel.com
 
 Acked-by: Rob Herring r...@kernel.org
 
 As there doesn't appear any dependency with the rest of this series, I
 can take this.
 
Thanks.

-rui
 One minor nit below.
 
  ---
   drivers/base/platform.c   |4 
   drivers/of/device.c   |3 +++
   include/linux/of_device.h |6 ++
   3 files changed, 13 insertions(+)
 
  diff --git a/drivers/base/platform.c b/drivers/base/platform.c index
  2f4aea2..bc78848 100644
  --- a/drivers/base/platform.c
  +++ b/drivers/base/platform.c
  @@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev,
 struct device_attribute *a,
  struct platform_device  *pdev = to_platform_device(dev);
  int len;
 
  +   len = of_device_get_modalias(dev, buf, PAGE_SIZE -1);
  +   if (len != -ENODEV)
  +   return len;
  +
  len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
  if (len != -ENODEV)
  return len;
  diff --git a/drivers/of/device.c b/drivers/of/device.c index
  f685e55..dafb973 100644
  --- a/drivers/of/device.c
  +++ b/drivers/of/device.c
  @@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev,
 char *str, ssize_t len)
  int cplen, i;
  ssize_t tsize, csize, repend;
 
  +   if ((!dev) || (!dev-of_node))\
 
 Don't need the parentheses here.
 
  +   return -ENODEV;
  +
  /* Name  Type */
  csize = snprintf(str, len, of:N%sT%s, dev-of_node-name,
   dev-of_node-type); diff --git
  a/include/linux/of_device.h b/include/linux/of_device.h index
  82ce324..8d7dd67 100644
  --- a/include/linux/of_device.h
  +++ b/include/linux/of_device.h
  @@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct
  device *dev,  static inline void of_device_uevent(struct device *dev,
  struct kobj_uevent_env *env) { }
 
  +static inline int of_device_get_modalias(struct device *dev,
  +  char *str, ssize_t len) {
  +   return -ENODEV;
  +}
  +
   static inline int of_device_uevent_modalias(struct device *dev,
 struct kobj_uevent_env *env)  {
  --
  1.7.9.5
 
  --
  To unsubscribe from this list: send the line unsubscribe devicetree
  in the body of a message to majord...@vger.kernel.org More majordomo
  info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

2014-01-14 Thread Zhang Rui
On Mon, 2014-01-13 at 17:35 +, Mark Brown wrote:
 On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote:
  ACPI enumerated devices has ACPI style _HID and _CID strings,
  all of these strings can be used for both driver loading and matching.
 
  But currently, in Platform, I2C and SPI bus, only the ACPI style
  driver matching is supported by invoking acpi_driver_match_device()
  in bus .match() callback.
 
 I don't understand what this means, sorry.

sorry, I should be clearer about this.

   As far as I can tell ACPI
 style _HID and _CID strings are something different to ACPI style
 driver matching but what that actually means is not at all clear to me
 so I don't know what problem this is intended to fix.
 
I gave a more detailed description about the problem in the changelog of
patch 2/4.

Take the following piece of code for example,
static const struct acpi_device_id xxx_acpi_match[] = {
{ INTABCD, 0 },
{ }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

this can be seen in a platform/I2C/SPI driver that supports an ACPI
enumerated device, right?

If this piece of code is used in a platform driver for
an ACPI enumerated platform device, the platform DRIVER module_alias
is acpi:INTABCD, but the uevent attribute of its platform device node
is platform:INTABCD:00 (PREFIX:platform_device-name).
If this piece of code is used in an I2C driver for an ACPI enumerated
i2c device, the i2c driver module_alias is acpi:INTABCD, but
the uevent of its i2c device node is
i2c:INTABCD:00 (PREFIX:i2c_client-name).
If this piece of code is used in an *SPI* driver for an ACPI enumerated
spi device, the spi driver module_alias is acpi:INTABCD, but
the uevent of its spi device node is
spi:INTABCD (PREFIX:spi_device-modalias).

thus when the device node is created, the driver will not be loaded
automatically because their modalias do not match.

 Please also always remember to CC maintainers on patches.
 
okay, will resend the patches later.

thanks,
rui
  diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
  index 349ebba..ab70eda 100644
  --- a/drivers/spi/spi.c
  +++ b/drivers/spi/spi.c
  @@ -58,6 +58,11 @@ static ssize_t
   modalias_show(struct device *dev, struct device_attribute *a, char *buf)
   {
  const struct spi_device *spi = to_spi_device(dev);
  +   int len;
  +
  +   len = acpi_device_modalias(dev, buf, PAGE_SIZE);
  +   if (len != -ENODEV)
  +   return len;
   
  return sprintf(buf, %s%s\n, SPI_MODULE_PREFIX, spi-modalias);
   }
 
 What does this do and why can't acpi_driver_match_device() handle this
 like it does for other ACPI devices?  We don't need to add such code for
 other firmware interfaces...


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 0/4] module autoloading fixes

2014-01-14 Thread Zhang Rui
Hi, all,

This patch set fixes a couple of module autoloading problem.

Patch 1/4 fixes a bug in ACPI device 'modalias' and 'uevent' attributes,
  although the bug can rarely be reproduced (only if there is an
  output error of snprintf, or the ids are longer than 1024 bytes)
Patch 2/4 introduces two new APIs for exporting ACPI style 'modalias' and
  'uevent' attributes in other buses.
Patch 3/4 introduce support for ACPI style 'modalias' and 'uevent'
  attributes in platform, I2C and SPI bus.
Patch 4/4 add OF style 'modalias' support for platform bus.

I did some tests and can confirm that the code for ACPI enumerated platform
bus device works well.
I tried with a patch with convert ACPI Fan device/driver to platform bus,
and can confirm that the code for ACPI enumerated platform device works well,
both the platform Fan driver and device show their modalias as acpi:PNP0C0B.

thanks,
rui


Zhang Rui (4):
  ACPI: fix create_modalias() return value handling
  ACPI: add module autoloading support for ACPI enumerated devices
  fix module autoloading for ACPI enumerated devices
  OF: introduce OF style 'modalias' support for platform bus.

 drivers/acpi/scan.c   |   73 +
 drivers/base/platform.c   |   16 +-
 drivers/i2c/i2c-core.c|   11 +++
 drivers/of/device.c   |3 ++
 drivers/spi/spi.c |   10 +++
 include/linux/acpi.h  |   15 ++
 include/linux/of_device.h |6 
 7 files changed, 127 insertions(+), 7 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 3/4] fix module autoloading for ACPI enumerated devices

2014-01-14 Thread Zhang Rui
ACPI enumerated devices has ACPI style _HID and _CID strings,
all of these strings can be used for both driver loading and matching.

Currently, in Platform, I2C and SPI bus, the ACPI style driver matching
is supported by invoking acpi_driver_match_device() in bus .match() callback.
But, the module autoloading is still broken.

For example, there is any ACPI device with _HID INTABCD that is
enumerated to platform bus, and we have a driver that can probe it.

The driver exports its module_alias as acpi:INTABCD use the following code
static const struct acpi_device_id xxx_acpi_match[] = {
{ INTABCD, 0 },
{ }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

But, unfortunately, the device' modalias is shown as platform:INTABCD:00,
please refer to modalias_show() and platform_uevent() in
drivers/base/platform.c.
This results in that the driver will not be loaded automatically when the
device node is created, because their modalias do not match.

This also applies to I2C and SPI bus.

With this patch, the device' modalias will be shown as acpi:INTABCD as well.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/base/platform.c |   12 +++-
 drivers/i2c/i2c-core.c  |   11 +++
 drivers/spi/spi.c   |   10 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..2f4aea2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct 
device_attribute *a,
 char *buf)
 {
struct platform_device  *pdev = to_platform_device(dev);
-   int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+   if (len != -ENODEV)
+   return len;
+
+   len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
 
return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
 }
@@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct 
kobj_uevent_env *env)
if (rc != -ENODEV)
return rc;
 
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
+
add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX,
pdev-name);
return 0;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d74c0b3..c4c5588 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct 
device_driver *drv)
 static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
struct i2c_client   *client = to_i2c_client(dev);
+   int rc;
+
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
 
if (add_uevent_var(env, MODALIAS=%s%s,
   I2C_MODULE_PREFIX, client-name))
@@ -409,6 +414,12 @@ static ssize_t
 show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct i2c_client *client = to_i2c_client(dev);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+   if (len != -ENODEV)
+   return len;
+
return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name);
 }
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 349ebba..ab70eda 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -58,6 +58,11 @@ static ssize_t
 modalias_show(struct device *dev, struct device_attribute *a, char *buf)
 {
const struct spi_device *spi = to_spi_device(dev);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE);
+   if (len != -ENODEV)
+   return len;
 
return sprintf(buf, %s%s\n, SPI_MODULE_PREFIX, spi-modalias);
 }
@@ -114,6 +119,11 @@ static int spi_match_device(struct device *dev, struct 
device_driver *drv)
 static int spi_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
const struct spi_device *spi = to_spi_device(dev);
+   int rc;
+
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
 
add_uevent_var(env, MODALIAS=%s%s, SPI_MODULE_PREFIX, spi-modalias);
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 1/4] ACPI: fix create_modalias() return value handling

2014-01-14 Thread Zhang Rui
Currently, create_modalias() handles the output truncated case in
an improper way (return -EINVAL).
Plus, acpi_device_uevent() and acpi_device_modalias_show() do
improper check for the create_modalias() return value as well.

This patch fixes create_modalias() to
return -EINVAL if there is an output error,
return -ENOMEM if the output is truncated,
and also fixes both acpi_device_uevent() and acpi_device_modalias_show()
to do proper return value check.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/acpi/scan.c |   16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index fd39459..c925321 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -85,6 +85,9 @@ int acpi_scan_add_handler_with_hotplug(struct 
acpi_scan_handler *handler,
  * Creates hid/cid(s) string needed for modalias and uevent
  * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get:
  * char *modalias: acpi:IBM0001:ACPI0001
+ * Return: 0: no _HID and no _CID
+ * -EINVAL: output error
+ * -ENOMEM: output is truncated
 */
 static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
   int size)
@@ -101,8 +104,10 @@ static int create_modalias(struct acpi_device *acpi_dev, 
char *modalias,
 
list_for_each_entry(id, acpi_dev-pnp.ids, list) {
count = snprintf(modalias[len], size, %s:, id-id);
-   if (count  0 || count = size)
-   return -EINVAL;
+   if (count  0)
+   return EINVAL;
+   if (count = size)
+   return -ENOMEM;
len += count;
size -= count;
}
@@ -116,10 +121,9 @@ acpi_device_modalias_show(struct device *dev, struct 
device_attribute *attr, cha
struct acpi_device *acpi_dev = to_acpi_device(dev);
int len;
 
-   /* Device has no HID and no CID or string is 1024 */
len = create_modalias(acpi_dev, buf, 1024);
if (len = 0)
-   return 0;
+   return len;
buf[len++] = '\n';
return len;
 }
@@ -782,8 +786,8 @@ static int acpi_device_uevent(struct device *dev, struct 
kobj_uevent_env *env)
return -ENOMEM;
len = create_modalias(acpi_dev, env-buf[env-buflen - 1],
  sizeof(env-buf) - env-buflen);
-   if (len = (sizeof(env-buf) - env-buflen))
-   return -ENOMEM;
+   if (len = 0)
+   return len;
env-buflen += len;
return 0;
 }
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

2014-01-14 Thread Zhang Rui
Fix a problem that, the platform bus supports the OF style modalias
in .uevent() call, but not in its device 'modalias' sysfs attribute.

cc: devicet...@vger.kernel.org
Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/base/platform.c   |4 
 drivers/of/device.c   |3 +++
 include/linux/of_device.h |6 ++
 3 files changed, 13 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 2f4aea2..bc78848 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct 
device_attribute *a,
struct platform_device  *pdev = to_platform_device(dev);
int len;
 
+   len = of_device_get_modalias(dev, buf, PAGE_SIZE -1);
+   if (len != -ENODEV)
+   return len;
+
len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
if (len != -ENODEV)
return len;
diff --git a/drivers/of/device.c b/drivers/of/device.c
index f685e55..dafb973 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, 
ssize_t len)
int cplen, i;
ssize_t tsize, csize, repend;
 
+   if ((!dev) || (!dev-of_node))
+   return -ENODEV;
+
/* Name  Type */
csize = snprintf(str, len, of:N%sT%s, dev-of_node-name,
 dev-of_node-type);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 82ce324..8d7dd67 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev,
 static inline void of_device_uevent(struct device *dev,
struct kobj_uevent_env *env) { }
 
+static inline int of_device_get_modalias(struct device *dev,
+  char *str, ssize_t len)
+{
+   return -ENODEV;
+}
+
 static inline int of_device_uevent_modalias(struct device *dev,
   struct kobj_uevent_env *env)
 {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/4] ACPI: add module autoloading support for ACPI enumerated devices

2014-01-14 Thread Zhang Rui
An ACPI enumerated device may have its compatible id strings.

To support the compatible ACPI ids (acpi_device-pnp.ids),
we introduced acpi_driver_match_device() to match
the driver-acpi_match_table and acpi_device-pnp.ids.

For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to
exports the driver module alias in the format of
acpi:device_compatible_ids.
But in the mean time, the current code does not export the
ACPI compatible strings as part of the module_alias for the
ACPI enumerated devices, which will break the module autoloading.

Take the following piece of code for example,
static const struct acpi_device_id xxx_acpi_match[] = {
{ INTABCD, 0 },
{ }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

If this piece of code is used in a platform driver for
an ACPI enumerated platform device, the platform driver module_alias
is acpi:INTABCD, but the uevent attribute of its platform device node
is platform:INTABCD:00 (PREFIX:platform_device-name).
If this piece of code is used in an i2c driver for an ACPI enumerated
i2c device, the i2c driver module_alias is acpi:INTABCD, but
the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:i2c_client-name).
If this piece of code is used in an spi driver for an ACPI enumerated
spi device, the spi driver module_alias is acpi:INTABCD, but
the uevent of its spi device node is spi:INTABCD 
(PREFIX:spi_device-modalias).

The reason why the module autoloading is not broken for now is that
the uevent file of the ACPI device node is acpi:INTABCD.
Thus it is the ACPI device node creation that loads the platform/i2c/spi driver.

So this is a problem that will affect us the day when the ACPI bus
is removed from device model.

This patch introduces two new APIs,
one for exporting ACPI ids in uevent MODALIAS field,
and another for exporting ACPI ids in device' modalias sysfs attribute.

For any bus that supports ACPI enumerated devices, it needs to invoke
these two functions for their uevent and modalias attribute.

Signed-off-by: Zhang Rui rui.zh...@intel.com
Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com
---
 drivers/acpi/scan.c  |   57 ++
 include/linux/acpi.h |   15 +
 2 files changed, 72 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c925321..680bb56 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -116,6 +116,63 @@ static int create_modalias(struct acpi_device *acpi_dev, 
char *modalias,
return len;
 }
 
+/*
+ * Creates uevent modalias field for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs  CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * acpi:IBM0001:ACPI0001
+ */
+int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env 
*env)
+{
+   struct acpi_device *acpi_dev;
+   int len;
+
+   acpi_dev = ACPI_COMPANION(dev);
+   if (!acpi_dev)
+   return -ENODEV;
+
+   /* Fall back to bus specific way of modalias exporting */
+   if (list_empty(acpi_dev-pnp.ids))
+   return -ENODEV;
+
+   if (add_uevent_var(env, MODALIAS=))
+   return -ENOMEM;
+   len = create_modalias(acpi_dev, env-buf[env-buflen - 1],
+   sizeof(env-buf) - env-buflen);
+   if (len = 0)
+   return len;
+   env-buflen += len;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
+
+/*
+ * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs  CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * acpi:IBM0001:ACPI0001
+ */
+int acpi_device_modalias(struct device *dev, char *buf, int size)
+{
+   struct acpi_device *acpi_dev;
+   int len;
+
+   acpi_dev = ACPI_COMPANION(dev);
+   if (!acpi_dev)
+   return -ENODEV;
+
+   /* Fall back to bus specific way of modalias exporting */
+   if (list_empty(acpi_dev-pnp.ids))
+   return -ENODEV;
+
+   len = create_modalias(acpi_dev, buf, size -1);
+   if (len = 0)
+   return len;
+   buf[len++] = '\n';
+   return len;
+}
+EXPORT_SYMBOL_GPL(acpi_device_modalias);
+
 static ssize_t
 acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, 
char *buf) {
struct acpi_device *acpi_dev = to_acpi_device(dev);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d9099b1..22325ed 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -409,6 +409,9 @@ static inline bool acpi_driver_match_device(struct device 
*dev,
return !!acpi_match_device(drv-acpi_match_table, dev);
 }
 
+int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
+int acpi_device_modalias(struct device *, char *, int);
+
 #define ACPI_PTR(_ptr) (_ptr)
 
 #else  /* !CONFIG_ACPI */
@@ -488,6 +491,18 @@ static inline bool

[PATCH 0/4] module autoloading fixes

2014-01-13 Thread Zhang Rui
Hi, all,

This patch set fixes a couple of module autoloading problem.

Patch 1/4 fixes a bug in ACPI device 'modalias' and 'uevent' attributes,
  although the bug can rarely be reproduced (only if there is an
  output error of snprintf, or the ids are longer than 1024 bytes)
Patch 2/4 introduces two new APIs for exporting ACPI style 'modalias' and
  'uevent' attributes in other buses.
Patch 3/4 introduce support for ACPI style 'modalias' and 'uevent'
  attributes in platform, I2C and SPI bus.
Patch 4/4 add OF style 'modalias' support for platform bus.

I did some tests and can confirm that the code for ACPI enumerated platform
bus device works well.
I tried with a patch with convert ACPI Fan device/driver to platform bus,
and can confirm that the code for ACPI enumerated platform device works well,
both the platform Fan driver and device show their modalias as acpi:PNP0C0B.

thanks,
rui


Zhang Rui (4):
  ACPI: fix create_modalias() return value handling
  ACPI: add module autoloading support for ACPI enumerated devices
  fix module autoloading for ACPI enumerated devices
  OF: introduce OF style 'modalias' support for platform bus.

 drivers/acpi/scan.c   |   73 +
 drivers/base/platform.c   |   16 +-
 drivers/i2c/i2c-core.c|   11 +++
 drivers/of/device.c   |3 ++
 drivers/spi/spi.c |   10 +++
 include/linux/acpi.h  |   15 ++
 include/linux/of_device.h |6 
 7 files changed, 127 insertions(+), 7 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/4] ACPI: add module autoloading support for ACPI enumerated devices

2014-01-13 Thread Zhang Rui
An ACPI enumerated device may have its compatible id strings.

To support the compatible ACPI ids (acpi_device-pnp.ids),
we introduced acpi_driver_match_device() to match
the driver-acpi_match_table and acpi_device-pnp.ids.

For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to
exports the driver module alias in the format of
acpi:device_compatible_ids.
But in the mean time, the current code does not export the
ACPI compatible strings as part of the module_alias for the
ACPI enumerated devices, which will break the module autoloading.

Take the following piece of code for example,
static const struct acpi_device_id xxx_acpi_match[] = {
{ INTABCD, 0 },
{ }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

If this piece of code is used in a platform driver for
an ACPI enumerated platform device, the platform driver module_alias
is acpi:INTABCD, but the uevent attribute of its platform device node
is platform:INTABCD:00 (PREFIX:platform_device-name).
If this piece of code is used in an i2c driver for an ACPI enumerated
i2c device, the i2c driver module_alias is acpi:INTABCD, but
the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:i2c_client-name).
If this piece of code is used in an spi driver for an ACPI enumerated
spi device, the spi driver module_alias is acpi:INTABCD, but
the uevent of its spi device node is spi:INTABCD 
(PREFIX:spi_device-modalias).

The reason why the module autoloading is not broken for now is that
the uevent file of the ACPI device node is acpi:INTABCD.
Thus it is the ACPI device node creation that loads the platform/i2c/spi driver.

So this is a problem that will affect us the day when the ACPI bus
is removed from device model.

This patch introduces two new APIs,
one for exporting ACPI ids in uevent MODALIAS field,
and another for exporting ACPI ids in device' modalias sysfs attribute.

For any bus that supports ACPI enumerated devices, it needs to invoke
these two functions for their uevent and modalias attribute.

Signed-off-by: Zhang Rui rui.zh...@intel.com
Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com
---
 drivers/acpi/scan.c  |   57 ++
 include/linux/acpi.h |   15 +
 2 files changed, 72 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c925321..680bb56 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -116,6 +116,63 @@ static int create_modalias(struct acpi_device *acpi_dev, 
char *modalias,
return len;
 }
 
+/*
+ * Creates uevent modalias field for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs  CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * acpi:IBM0001:ACPI0001
+ */
+int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env 
*env)
+{
+   struct acpi_device *acpi_dev;
+   int len;
+
+   acpi_dev = ACPI_COMPANION(dev);
+   if (!acpi_dev)
+   return -ENODEV;
+
+   /* Fall back to bus specific way of modalias exporting */
+   if (list_empty(acpi_dev-pnp.ids))
+   return -ENODEV;
+
+   if (add_uevent_var(env, MODALIAS=))
+   return -ENOMEM;
+   len = create_modalias(acpi_dev, env-buf[env-buflen - 1],
+   sizeof(env-buf) - env-buflen);
+   if (len = 0)
+   return len;
+   env-buflen += len;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
+
+/*
+ * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs  CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * acpi:IBM0001:ACPI0001
+ */
+int acpi_device_modalias(struct device *dev, char *buf, int size)
+{
+   struct acpi_device *acpi_dev;
+   int len;
+
+   acpi_dev = ACPI_COMPANION(dev);
+   if (!acpi_dev)
+   return -ENODEV;
+
+   /* Fall back to bus specific way of modalias exporting */
+   if (list_empty(acpi_dev-pnp.ids))
+   return -ENODEV;
+
+   len = create_modalias(acpi_dev, buf, size -1);
+   if (len = 0)
+   return len;
+   buf[len++] = '\n';
+   return len;
+}
+EXPORT_SYMBOL_GPL(acpi_device_modalias);
+
 static ssize_t
 acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, 
char *buf) {
struct acpi_device *acpi_dev = to_acpi_device(dev);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d9099b1..22325ed 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -409,6 +409,9 @@ static inline bool acpi_driver_match_device(struct device 
*dev,
return !!acpi_match_device(drv-acpi_match_table, dev);
 }
 
+int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
+int acpi_device_modalias(struct device *, char *, int);
+
 #define ACPI_PTR(_ptr) (_ptr)
 
 #else  /* !CONFIG_ACPI */
@@ -488,6 +491,18 @@ static inline bool

[PATCH 1/4] ACPI: fix create_modalias() return value handling

2014-01-13 Thread Zhang Rui
Currently, create_modalias() handles the output truncated case in
an improper way (return -EINVAL).
Plus, acpi_device_uevent() and acpi_device_modalias_show() do
improper check for the create_modalias() return value as well.

This patch fixes create_modalias() to
return -EINVAL if there is an output error,
return -ENOMEM if the output is truncated,
and also fixes both acpi_device_uevent() and acpi_device_modalias_show()
to do proper return value check.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/acpi/scan.c |   16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index fd39459..c925321 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -85,6 +85,9 @@ int acpi_scan_add_handler_with_hotplug(struct 
acpi_scan_handler *handler,
  * Creates hid/cid(s) string needed for modalias and uevent
  * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get:
  * char *modalias: acpi:IBM0001:ACPI0001
+ * Return: 0: no _HID and no _CID
+ * -EINVAL: output error
+ * -ENOMEM: output is truncated
 */
 static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
   int size)
@@ -101,8 +104,10 @@ static int create_modalias(struct acpi_device *acpi_dev, 
char *modalias,
 
list_for_each_entry(id, acpi_dev-pnp.ids, list) {
count = snprintf(modalias[len], size, %s:, id-id);
-   if (count  0 || count = size)
-   return -EINVAL;
+   if (count  0)
+   return EINVAL;
+   if (count = size)
+   return -ENOMEM;
len += count;
size -= count;
}
@@ -116,10 +121,9 @@ acpi_device_modalias_show(struct device *dev, struct 
device_attribute *attr, cha
struct acpi_device *acpi_dev = to_acpi_device(dev);
int len;
 
-   /* Device has no HID and no CID or string is 1024 */
len = create_modalias(acpi_dev, buf, 1024);
if (len = 0)
-   return 0;
+   return len;
buf[len++] = '\n';
return len;
 }
@@ -782,8 +786,8 @@ static int acpi_device_uevent(struct device *dev, struct 
kobj_uevent_env *env)
return -ENOMEM;
len = create_modalias(acpi_dev, env-buf[env-buflen - 1],
  sizeof(env-buf) - env-buflen);
-   if (len = (sizeof(env-buf) - env-buflen))
-   return -ENOMEM;
+   if (len = 0)
+   return len;
env-buflen += len;
return 0;
 }
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 3/4] fix module autoloading for ACPI enumerated devices

2014-01-13 Thread Zhang Rui
ACPI enumerated devices has ACPI style _HID and _CID strings,
all of these strings can be used for both driver loading and matching.

But currently, in Platform, I2C and SPI bus, only the ACPI style
driver matching is supported by invoking acpi_driver_match_device()
in bus .match() callback.

This patch fixes module autoloading on those buses for ACPI enumerated devices.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/base/platform.c |   12 +++-
 drivers/i2c/i2c-core.c  |   11 +++
 drivers/spi/spi.c   |   10 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..2f4aea2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct 
device_attribute *a,
 char *buf)
 {
struct platform_device  *pdev = to_platform_device(dev);
-   int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+   if (len != -ENODEV)
+   return len;
+
+   len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);
 
return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
 }
@@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct 
kobj_uevent_env *env)
if (rc != -ENODEV)
return rc;
 
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
+
add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX,
pdev-name);
return 0;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d74c0b3..c4c5588 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct 
device_driver *drv)
 static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
struct i2c_client   *client = to_i2c_client(dev);
+   int rc;
+
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
 
if (add_uevent_var(env, MODALIAS=%s%s,
   I2C_MODULE_PREFIX, client-name))
@@ -409,6 +414,12 @@ static ssize_t
 show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct i2c_client *client = to_i2c_client(dev);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+   if (len != -ENODEV)
+   return len;
+
return sprintf(buf, %s%s\n, I2C_MODULE_PREFIX, client-name);
 }
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 349ebba..ab70eda 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -58,6 +58,11 @@ static ssize_t
 modalias_show(struct device *dev, struct device_attribute *a, char *buf)
 {
const struct spi_device *spi = to_spi_device(dev);
+   int len;
+
+   len = acpi_device_modalias(dev, buf, PAGE_SIZE);
+   if (len != -ENODEV)
+   return len;
 
return sprintf(buf, %s%s\n, SPI_MODULE_PREFIX, spi-modalias);
 }
@@ -114,6 +119,11 @@ static int spi_match_device(struct device *dev, struct 
device_driver *drv)
 static int spi_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
const struct spi_device *spi = to_spi_device(dev);
+   int rc;
+
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
 
add_uevent_var(env, MODALIAS=%s%s, SPI_MODULE_PREFIX, spi-modalias);
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

2014-01-13 Thread Zhang Rui
Fix a problem that, the platform bus supports the OF style modalias
in .uevent() call, but not in its device' modalias sysfs attribute.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/base/platform.c   |4 
 drivers/of/device.c   |3 +++
 include/linux/of_device.h |6 ++
 3 files changed, 13 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 2f4aea2..bc78848 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct 
device_attribute *a,
struct platform_device  *pdev = to_platform_device(dev);
int len;
 
+   len = of_device_get_modalias(dev, buf, PAGE_SIZE -1);
+   if (len != -ENODEV)
+   return len;
+
len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
if (len != -ENODEV)
return len;
diff --git a/drivers/of/device.c b/drivers/of/device.c
index f685e55..dafb973 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, 
ssize_t len)
int cplen, i;
ssize_t tsize, csize, repend;
 
+   if ((!dev) || (!dev-of_node))
+   return -ENODEV;
+
/* Name  Type */
csize = snprintf(str, len, of:N%sT%s, dev-of_node-name,
 dev-of_node-type);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 82ce324..8d7dd67 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev,
 static inline void of_device_uevent(struct device *dev,
struct kobj_uevent_env *env) { }
 
+static inline int of_device_get_modalias(struct device *dev,
+  char *str, ssize_t len)
+{
+   return -ENODEV;
+}
+
 static inline int of_device_uevent_modalias(struct device *dev,
   struct kobj_uevent_env *env)
 {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices

2013-10-15 Thread Zhang Rui
On Mon, 2013-10-14 at 20:47 +0800, Zhang Rui wrote:
 On Mon, 2013-10-14 at 14:18 +0300, Jarkko Nikula wrote:
  On 10/14/2013 12:23 PM, Zhang Rui wrote:
   Hi,
  
   On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote:
   There is a minor fault about ACPI enumerated I2C devices with their 
   modalias
   attribute. Now modalias is set by device instance not by hardware ID.
   For example i2c:INTABCD:00, i2c:INTABCD:01 etc.
  
   This means each device instance gets different modalias which does match
   with generated modules.alias. Currently this is not problem as matching 
   can
   happen also with acpi:INTABCD modalias.
  
   IMO, this is not the proper fix for the modalias problem because ACPI
   enumerated I2C device may have compatible ids.
   Instead, we should export all the compatible ids as the modules alias of
   the ACPI enumerated I2C device.
  
   can you please take a look at the patch I sent out earlier?
   https://patchwork.kernel.org/patch/3034991/
   https://patchwork.kernel.org/patch/3035041/
   https://patchwork.kernel.org/patch/3035021/
  I see. This makes sense as it avoids that same device has two different 
  modaliases from both acpi and other subsystem.
  
  How about modalias nodes in sysfs, should they also reflect what is 
  matching uvent?
  
 good catch, will fix modalias as well in next version.

Hi,

I have a question about the device uevent and modalias sysfs
attributes.
what is the relationship between these two?
Am I right to say that, if there is the MODALIAS field in uevent file,
this field must be consistent with the content in modalias attribute?

I checked the code in drivers/base/platform.c,
static ssize_t modalias_show(struct device *dev, struct device_attribute
*a,
 char *buf)
{
struct platform_device  *pdev = to_platform_device(dev);
int len = snprintf(buf, PAGE_SIZE, platform:%s\n, pdev-name);

return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
}

static int platform_uevent(struct device *dev, struct kobj_uevent_env
*env)
{
struct platform_device  *pdev = to_platform_device(dev);
int rc;

/* Some devices have extra OF data and an OF-style MODALIAS */
rc = of_device_uevent_modalias(dev, env);
if (rc != -ENODEV)
return rc;

add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX,
pdev-name);
return 0;
}

This means that the OF-style MODALIAS is not shown in modalias sysfs
attribute.
is this a bug?

thanks,
rui

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/3] Platform: fix module autoloading for ACPI enumerated devices

2013-10-14 Thread Zhang Rui
Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/base/platform.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4f8bef3..80d2250 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -690,6 +690,10 @@ static int platform_uevent(struct device *dev, struct 
kobj_uevent_env *env)
if (rc != -ENODEV)
return rc;
 
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
+
add_uevent_var(env, MODALIAS=%s%s, PLATFORM_MODULE_PREFIX,
pdev-name);
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 1/3] ACPI: add module autoloading support for ACPI enumerated devices

2013-10-14 Thread Zhang Rui
An ACPI enumerated device may have its compatible id strings.

To support the compatible ACPI ids (acpi_device-pnp.ids),
we introduced acpi_driver_match_device() to match
the driver-acpi_match_table and acpi_device-pnp.ids.

For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to
exports the driver module alias in the format of
acpi:device_compatible_ids.
But in the mean time, the current code does not export the
ACPI compatible strings as part of the module_alias for the
ACPI enumerated devices, which will break the module autoloading.

Take the following piece of code for example,
static const struct acpi_device_id xxx_acpi_match[] = {
{ INTABCD, 0 },
{ }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

If this piece of code is used in a platform driver for
an ACPI enumerated platform device, the platform driver module_alias
is acpi:INTABCD, but the uevent attribute of its platform device node
is platform:INTABCD:00 (PREFIX:pdev-name).
If this piece of code is used in an i2c driver for an ACPI enumerated
i2c device, the i2c driver module_alias is acpi:INTABCD, but
the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:client-name).

The reason why the module autoloading is not broken for now is that
the uevent file of the ACPI device node is acpi:INTABCD.
Thus it is the ACPI device node creation that loads the platform/i2c driver.

So this is a problem that will affect us the day when the ACPI bus
is removed from device model.

This patch introduces a new function to exporting ACPI ids as the
module_alias, for the ACPI enumerate devices.

For any drivers using MODULE_DEVICE_TALBE(acpi, blabla), the uevent file
of its associated device must be fixed by invoking this function.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/acpi/scan.c  |   24 
 include/linux/acpi.h |8 
 2 files changed, 32 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 407ad13..db6f879 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -111,6 +111,30 @@ static int create_modalias(struct acpi_device *acpi_dev, 
char *modalias,
return len;
 }
 
+int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env 
*env)
+{
+   struct acpi_device *acpi_dev;
+   int result;
+   int len;
+
+   result = acpi_bus_get_device(ACPI_HANDLE(dev), acpi_dev);
+   if (result)
+   return result;
+
+   if (list_empty(acpi_dev-pnp.ids))
+   return 0;
+
+   if (add_uevent_var(env, MODALIAS=))
+   return -ENOMEM;
+   len = create_modalias(acpi_dev, env-buf[env-buflen - 1],
+   sizeof(env-buf) - env-buflen);
+   if (len = (sizeof(env-buf) - env-buflen))
+   return -ENOMEM;
+   env-buflen += len;
+   return 0;
+}
+EXPORT_SYMBOL(acpi_device_uevent_modalias);
+
 static ssize_t
 acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, 
char *buf) {
struct acpi_device *acpi_dev = to_acpi_device(dev);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a5db4ae..117fa26 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -401,6 +401,8 @@ static inline bool acpi_driver_match_device(struct device 
*dev,
return !!acpi_match_device(drv-acpi_match_table, dev);
 }
 
+int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
+
 #define ACPI_PTR(_ptr) (_ptr)
 
 #else  /* !CONFIG_ACPI */
@@ -471,6 +473,12 @@ static inline bool acpi_driver_match_device(struct device 
*dev,
return false;
 }
 
+static inline int acpi_device_uevent_modalias(struct device *dev,
+   struct kobj_uevent_env *env)
+{
+   return -ENODEV;
+}
+
 #define ACPI_PTR(_ptr) (NULL)
 
 #endif /* !CONFIG_ACPI */
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 3/3] I2C: fix module autoloading for ACPI enumerated devices

2013-10-14 Thread Zhang Rui
Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/i2c/i2c-core.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3be58f8..d75b679 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct 
device_driver *drv)
 static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
struct i2c_client   *client = to_i2c_client(dev);
+   int rc;
+
+   rc = acpi_device_uevent_modalias(dev, env);
+   if (rc != -ENODEV)
+   return rc;
 
if (add_uevent_var(env, MODALIAS=%s%s,
   I2C_MODULE_PREFIX, client-name))
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices

2013-10-14 Thread Zhang Rui
Hi,

On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote:
 There is a minor fault about ACPI enumerated I2C devices with their modalias
 attribute. Now modalias is set by device instance not by hardware ID.
 For example i2c:INTABCD:00, i2c:INTABCD:01 etc.
 
 This means each device instance gets different modalias which does match
 with generated modules.alias. Currently this is not problem as matching can
 happen also with acpi:INTABCD modalias.
 
IMO, this is not the proper fix for the modalias problem because ACPI
enumerated I2C device may have compatible ids.
Instead, we should export all the compatible ids as the modules alias of
the ACPI enumerated I2C device.

can you please take a look at the patch I sent out earlier?
https://patchwork.kernel.org/patch/3034991/
https://patchwork.kernel.org/patch/3035041/
https://patchwork.kernel.org/patch/3035021/

thanks,
rui

 Fix this by using ACPI hardware ID.
 
 Signed-off-by: Jarkko Nikula jarkko.nik...@linux.intel.com
 Cc: Mika Westerberg mika.westerb...@linux.intel.com


 ---
 Generated on top of v3.12-rc4-29-g0e7a3ed.
 ---
  drivers/i2c/i2c-core.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
 index 29d3f04..6dd0c53 100644
 --- a/drivers/i2c/i2c-core.c
 +++ b/drivers/i2c/i2c-core.c
 @@ -,7 +,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle 
 handle, u32 level,
   if (ret  0 || !info.addr)
   return AE_OK;
  
 - strlcpy(info.type, dev_name(adev-dev), sizeof(info.type));
 + strlcpy(info.type, acpi_device_hid(adev), sizeof(info.type));
   if (!i2c_new_device(adapter, info)) {
   dev_err(adapter-dev,
   failed to add I2C device %s from ACPI\n,


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] ACPI: add module autoloading support for ACPI enumerated devices

2013-10-14 Thread Zhang Rui
On Mon, 2013-10-14 at 13:14 +0300, Mika Westerberg wrote:
 On Mon, Oct 14, 2013 at 05:10:32PM +0800, Zhang Rui wrote:
  An ACPI enumerated device may have its compatible id strings.
  
  To support the compatible ACPI ids (acpi_device-pnp.ids),
  we introduced acpi_driver_match_device() to match
  the driver-acpi_match_table and acpi_device-pnp.ids.
  
  For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to
  exports the driver module alias in the format of
  acpi:device_compatible_ids.
  But in the mean time, the current code does not export the
  ACPI compatible strings as part of the module_alias for the
  ACPI enumerated devices, which will break the module autoloading.
  
  Take the following piece of code for example,
  static const struct acpi_device_id xxx_acpi_match[] = {
  { INTABCD, 0 },
  { }
  };
  MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);
  
  If this piece of code is used in a platform driver for
  an ACPI enumerated platform device, the platform driver module_alias
  is acpi:INTABCD, but the uevent attribute of its platform device node
  is platform:INTABCD:00 (PREFIX:pdev-name).
  If this piece of code is used in an i2c driver for an ACPI enumerated
  i2c device, the i2c driver module_alias is acpi:INTABCD, but
  the uevent of its i2c device node is i2c:INTABCD:00 (PREFIX:client-name).
  
  The reason why the module autoloading is not broken for now is that
  the uevent file of the ACPI device node is acpi:INTABCD.
  Thus it is the ACPI device node creation that loads the platform/i2c driver.
  
  So this is a problem that will affect us the day when the ACPI bus
  is removed from device model.
  
  This patch introduces a new function to exporting ACPI ids as the
  module_alias, for the ACPI enumerate devices.
  
  For any drivers using MODULE_DEVICE_TALBE(acpi, blabla), the uevent file
 ^ TABLE?
 
  of its associated device must be fixed by invoking this function.
  
  Signed-off-by: Zhang Rui rui.zh...@intel.com
 
 Makes sense. There is one comment below but other than that, you can add
 my,
 
 Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com
 
Thanks for reviewing.

 For the three patches.
 
 I wonder if we should do the same for SPI bus as well?
 
yes, you are right.
Actually, I should check for buses that supports
acpi_driver_match_device() and fix them all.
Will add it in next version.

thanks,
rui
  ---
   drivers/acpi/scan.c  |   24 
   include/linux/acpi.h |8 
   2 files changed, 32 insertions(+)
  
  diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
  index 407ad13..db6f879 100644
  --- a/drivers/acpi/scan.c
  +++ b/drivers/acpi/scan.c
  @@ -111,6 +111,30 @@ static int create_modalias(struct acpi_device 
  *acpi_dev, char *modalias,
  return len;
   }
   
  +int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env 
  *env)
  +{
  +   struct acpi_device *acpi_dev;
  +   int result;
  +   int len;
  +
  +   result = acpi_bus_get_device(ACPI_HANDLE(dev), acpi_dev);
  +   if (result)
  +   return result;
  +
  +   if (list_empty(acpi_dev-pnp.ids))
  +   return 0;
  +
  +   if (add_uevent_var(env, MODALIAS=))
  +   return -ENOMEM;
  +   len = create_modalias(acpi_dev, env-buf[env-buflen - 1],
  +   sizeof(env-buf) - env-buflen);
  +   if (len = (sizeof(env-buf) - env-buflen))
  +   return -ENOMEM;
  +   env-buflen += len;
  +   return 0;
  +}
  +EXPORT_SYMBOL(acpi_device_uevent_modalias);
 
 _GPL?


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] i2c: Fix modalias for ACPI enumerated I2C devices

2013-10-14 Thread Zhang Rui
On Mon, 2013-10-14 at 14:18 +0300, Jarkko Nikula wrote:
 On 10/14/2013 12:23 PM, Zhang Rui wrote:
  Hi,
 
  On Thu, 2013-10-10 at 17:17 +0300, Jarkko Nikula wrote:
  There is a minor fault about ACPI enumerated I2C devices with their 
  modalias
  attribute. Now modalias is set by device instance not by hardware ID.
  For example i2c:INTABCD:00, i2c:INTABCD:01 etc.
 
  This means each device instance gets different modalias which does match
  with generated modules.alias. Currently this is not problem as matching can
  happen also with acpi:INTABCD modalias.
 
  IMO, this is not the proper fix for the modalias problem because ACPI
  enumerated I2C device may have compatible ids.
  Instead, we should export all the compatible ids as the modules alias of
  the ACPI enumerated I2C device.
 
  can you please take a look at the patch I sent out earlier?
  https://patchwork.kernel.org/patch/3034991/
  https://patchwork.kernel.org/patch/3035041/
  https://patchwork.kernel.org/patch/3035021/
 I see. This makes sense as it avoids that same device has two different 
 modaliases from both acpi and other subsystem.
 
 How about modalias nodes in sysfs, should they also reflect what is 
 matching uvent?
 
good catch, will fix modalias as well in next version.

thanks,
rui

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RFC PATCH 1/6] Introduce acpi_match_device_id().

2012-10-01 Thread Zhang, Rui


 -Original Message-
 From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
 Sent: Monday, October 01, 2012 2:38 PM
 To: Zhang, Rui
 Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
 Rafael J. Wysocki; Grant Likely; Dirk Brandewie
 Subject: Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().
 Importance: High
 
 On Sat, Sep 29, 2012 at 01:31:52PM +, Zhang, Rui wrote:
 
+{
+   acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
  
   If the device is not bound to an ACPI handle this will return NULL.
   And I don't see you doing that in this series meaning that..
  
 
 
  You're right, I should set pdev-archdata.acpi_handle to the I2C
  controller in i2c_root.c.
 
 There already is an API for that - check drivers/acpi/glue.c.

Do you mean acpi_bind?
acpi_bind_one will bind the physical device node to the ACPI device,
But for the i2c controller ACPI device, the physical node is the I2C
adapter in the I2C bus.
If we introduce I2C bus ACPI binding, which is what we're doing now,
the i2c_adapter-dev.archdata.acpi_handle will be set to the ACPI
i2c controller handle by acpi_bind_one.

Thanks,
rui
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RFC PATCH 2/6] Introduce ACPI style match in platform_match

2012-10-01 Thread Zhang, Rui


 -Original Message-
 From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi-
 ow...@vger.kernel.org] On Behalf Of Mika Westerberg
 Sent: Monday, October 01, 2012 2:47 PM
 To: Zhang, Rui
 Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
 Rafael J. Wysocki; Grant Likely; Dirk Brandewie
 Subject: Re: [RFC PATCH 2/6] Introduce ACPI style match in
 platform_match
 Importance: High
 
 On Fri, Sep 28, 2012 at 03:39:15PM +0800, Zhang Rui wrote:
  From 5d7ecd12c2994b8c5905d52718c2870c3b62746e Mon Sep 17 00:00:00
  2001
  From: Zhang Rui rui.zh...@intel.com
  Date: Fri, 28 Sep 2012 14:51:03 +0800
  Subject: [RFC PATCH 2/6] Introduce ACPI style match in platform_match
 
  Signed-off-by: Zhang Rui rui.zh...@intel.com
  ---
   drivers/base/platform.c |8 
   1 files changed, 8 insertions(+), 0 deletions(-)
 
  diff --git a/drivers/base/platform.c b/drivers/base/platform.c index
  a1a7225..90e64c6f 100644
  --- a/drivers/base/platform.c
  +++ b/drivers/base/platform.c
  @@ -20,6 +20,7 @@
   #include linux/err.h
   #include linux/slab.h
   #include linux/pm_runtime.h
  +#include linux/acpi.h
 
   #include base.h
 
  @@ -635,6 +636,13 @@ static const struct platform_device_id
 *platform_match_id(
  struct platform_device *pdev)
   {
  while (id-name[0]) {
  +#ifdef CONFIG_ACPI
 
 I don't think the above is needed as you stub the acpi_match_device_id()
 out when !CONFIG_ACPI.
 
You're right, I'll remove this.

 How about I2C and SPI slave devices?
 
We're introduce the I2C/SPI bus ACPI binding, and the i2c/spi bus .match
method can be redirected to acpi callbacks.

Thanks,
rui 

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver

2012-10-01 Thread Zhang, Rui


 -Original Message-
 From: linux-i2c-ow...@vger.kernel.org [mailto:linux-i2c-
 ow...@vger.kernel.org] On Behalf Of Mika Westerberg
 Sent: Monday, October 01, 2012 2:55 PM
 To: Zhang, Rui
 Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
 Rafael J. Wysocki; Grant Likely; Dirk Brandewie
 Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
 enumeration driver
 Importance: High
 
 On Fri, Sep 28, 2012 at 03:40:32PM +0800, Zhang Rui wrote:
  +acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level,
  +  void *data, void **return_value) {
  +   int result;
  +   acpi_status status;
  +   struct acpi_buffer buffer;
  +   struct acpi_resource *resource;
  +   struct acpi_resource_gpio *gpio;
  +   struct acpi_resource_i2c_serialbus *i2c;
  +   int i;
  +   struct acpi_i2c_root *root = data;
  +   struct i2c_board_info info;
  +   struct acpi_device *device;
  +
  +   if (acpi_bus_get_device(handle, device))
  +   return AE_OK;
  +
  +   status = acpi_get_current_resources(handle, buffer);
  +   if (ACPI_FAILURE(status)) {
  +   dev_err(device-dev, Failed to get ACPI resources\n);
  +   return AE_OK;
  +   }
  +
  +   for (i = 0; i  buffer.length; i += sizeof(struct acpi_resource))
 {
  +   resource = (struct acpi_resource *)(buffer.pointer + i);
  +
  +   switch (resource-type) {
  +   case ACPI_RESOURCE_TYPE_GPIO:
  +   gpio = resource-data.gpio;
  +
  +   if (gpio-connection_type ==
 ACPI_RESOURCE_GPIO_TYPE_INT) {
  +   result =
  +   acpi_device_get_gpio_irq
  +   (gpio-resource_source.string_ptr,
  +gpio-pin_table[0], info.irq);
 
 acpi_device_get_gpio_irq() is not defined in this patch series?
 
ACPI GPIO controller patch has already been sent out, but in ACPI mailing list 
only.

 Also you need to do the gpio_request()/gpio_to_irq() things somewhere.
 Are they handled in acpi_device_get_gpio_irq()?

Yep.
 
 How about GpioIo resources?
 
This is not covered in this patch set, but will be in the next patch set.

  +   if (result)
  +   dev_err(device-dev,
  +   Failed to get IRQ\n);
  +   }
  +   break;
  +   case ACPI_RESOURCE_TYPE_SERIAL_BUS:
  +   i2c = resource-data.i2c_serial_bus;
  +
  +   info.addr = i2c-slave_address;
  +   break;
  +   default:
  +   break;
  +   }
  +   }
  +
  +   add_slave(root, info);
  +
  +   kfree(buffer.pointer);
  +   return AE_OK;
  +}
  +
  +static int __devinit acpi_i2c_root_add(struct acpi_device *device) {
  +   acpi_status status;
  +   struct acpi_i2c_root *root;
  +   struct resource *resources;
  +   int result;
  +
  +   if (!device-pnp.unique_id) {
  +   dev_err(device-dev,
  +   Unsupported ACPI I2C controller. No UID\n);
 
 Where does this restriction come from? As far as I understand UID is
 optional.


_UID is optional.
But it seems to be required for SPB buses that need ACPI device enumeration.
At least this is true for the ACPI 5 compatible ACPI tables I have.

Thanks,
rui
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal

2012-10-01 Thread Zhang, Rui


 -Original Message-
 From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
 Sent: Monday, October 01, 2012 2:45 PM
 To: Zhang, Rui
 Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
 Rafael J. Wysocki; Grant Likely; Dirk Brandewie
 Subject: Re: [RFC PATCH 0/6] ACPI: ACPI 5.0 device enumeration proposal
 Importance: High
 
 On Fri, Sep 28, 2012 at 03:37:43PM +0800, Zhang Rui wrote:
 
  the main idea is that, for Serial Buses like I2C and SPI, we
 enumerate
  the controller as a platform device, and then enumerate the slaves
 via
  i2c/spi_register_board_info. And then, when the controller is really
  probed and enabled in the platform driver, the SPI/I2C bus code will
  enumerate I2C/SPI slaves automatically.
  And for the other devices, we will enumerate all of them as platform
  devices, which is not covered in this patch set yet.
 
 Can you show some example how we could use this new code for example
 with an existing I2C/SPI slave driver?

This is just prototype patch set that I want to illustrate my idea
on ACPI device enumeration, to get more thoughts on this.
So no example driver so far.

 Let's say the device uses few
 GPIOs, one for interrupt and other for triggering firmware download. In
 addition to that it needs a special parameters that can be extracted
 running the _DSM method of the device.
 
 Normally the driver would get this stuff from the platform data or from
 Device Tree but how it is done with these patches?

Can you show me an example driver that gets the special parameters from Device 
Tree?

Thanks,
rui
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RFC PATCH 1/6] Introduce acpi_match_device_id().

2012-09-29 Thread Zhang, Rui


 -Original Message-
 From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
 Sent: Friday, September 28, 2012 10:14 PM
 To: Zhang, Rui
 Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
 Rafael J. Wysocki; Grant Likely; Dirk Brandewie
 Subject: Re: [RFC PATCH 1/6] Introduce acpi_match_device_id().
 Importance: High
 
 On Fri, Sep 28, 2012 at 03:38:30PM +0800, Zhang Rui wrote:
  From 72df5d1f51fb27a4ba7f70a3b07df759d32b8288 Mon Sep 17 00:00:00
  2001
  From: Zhang Rui rui.zh...@intel.com
  Date: Thu, 27 Sep 2012 15:11:55 +0800
  Subject: [RFC PATCH 1/6] Introduce acpi_match_device_id().
 
  This API is used to check if a device id string is compatible with an
  ACPI device, either PNP id exported via _HID or compatible ids
  exported via _CID control method.
 
  Signed-off-by: Zhang Rui rui.zh...@intel.com
  ---
   drivers/acpi/scan.c |   22 ++
   include/acpi/acpi_bus.h |6 ++
   2 files changed, 28 insertions(+), 0 deletions(-)
 
  diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index
  d1ecca2..936a7c9 100644
  --- a/drivers/acpi/scan.c
  +++ b/drivers/acpi/scan.c
  @@ -312,6 +312,28 @@ int acpi_match_device_ids(struct acpi_device
  *device,  }  EXPORT_SYMBOL(acpi_match_device_ids);
 
  +int acpi_match_device_id(const struct device *dev, const char *id)
 
 Would be good idea to implement this in terms of of_match_device() so
 that it returns pointer to the matched id. This way drivers can get the
 -driver_data pretty easily if needed.

Good idea.
Will do that in v2.
 
  +{
  +   acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
 
 If the device is not bound to an ACPI handle this will return NULL. And
 I don't see you doing that in this series meaning that..
 


You're right, I should set pdev-archdata.acpi_handle to the I2C controller in 
i2c_root.c.

Thanks,
rui
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver

2012-09-29 Thread Zhang, Rui


 -Original Message-
 From: Alan Cox [mailto:a...@lxorguk.ukuu.org.uk]
 Sent: Friday, September 28, 2012 8:54 PM
 To: Zhang, Rui
 Cc: LKML; linux-pm; linux-i2c; linux-a...@vger.kernel.org; Len, Brown;
 Rafael J. Wysocki; Grant Likely; Dirk Brandewie
 Subject: Re: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
 enumeration driver
 Importance: High
 
 On Fri, 28 Sep 2012 15:40:32 +0800
 Zhang Rui rui.zh...@intel.com wrote:
 
  From 6077a62f2865201ab6727ca7d628ee5e43aa57e1 Mon Sep 17 00:00:00
  2001
  From: Zhang Rui rui.zh...@intel.com
  Date: Fri, 24 Aug 2012 15:18:25 +0800
  Subject: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller
  enumeration  driver
 
  This driver is able to
  1) enumerate I2C controller via ACPI namespace
 and register it as a platform device.
  2) enumerate I2C slave devices via ACPI namespace.
 
 Will this also trigger and work with the ACPI4 based devices that seem
 to have I²C tables that Linux currently doesn't understand (eq some
 GMA600/Oaktrail platforms) ?

I'm not aware of this. I think the answer is probably No
because these PNPids are pretty new.
But we can check FADT.revision to make the driver work
on ACPI5 platforms only.

Thanks,
rui



[RFC PATCH 1/6] Introduce acpi_match_device_id().

2012-09-28 Thread Zhang Rui
From 72df5d1f51fb27a4ba7f70a3b07df759d32b8288 Mon Sep 17 00:00:00 2001
From: Zhang Rui rui.zh...@intel.com
Date: Thu, 27 Sep 2012 15:11:55 +0800
Subject: [RFC PATCH 1/6] Introduce acpi_match_device_id().

This API is used to check if a device id string is compatible
with an ACPI device,
either PNP id exported via _HID or compatible ids exported
via _CID control method.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/acpi/scan.c |   22 ++
 include/acpi/acpi_bus.h |6 ++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index d1ecca2..936a7c9 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -312,6 +312,28 @@ int acpi_match_device_ids(struct acpi_device *device,
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
+int acpi_match_device_id(const struct device *dev, const char *id)
+{
+   acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
+   struct acpi_device *device;
+   struct acpi_hardware_id *hwid;
+   acpi_status status;
+
+   if (!handle || !id)
+   return -ENODEV;
+
+   status = acpi_bus_get_device(handle, device);
+   if (ACPI_FAILURE(status))
+   return -ENODEV;
+
+   list_for_each_entry(hwid, device-pnp.ids, list)
+   if (!strcmp(id, hwid-id))
+   return 0;
+
+   return -ENODEV;
+}
+EXPORT_SYMBOL(acpi_match_device_id);
+
 static void acpi_free_ids(struct acpi_device *device)
 {
struct acpi_hardware_id *id, *tmp;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index bde976e..8b5b124 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -378,6 +378,7 @@ int acpi_bus_start(struct acpi_device *device);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,
  const struct acpi_device_id *ids);
+int acpi_match_device_id(const struct device *, const char *);
 int acpi_create_dir(struct acpi_device *);
 void acpi_remove_dir(struct acpi_device *);
 
@@ -448,6 +449,11 @@ static inline int acpi_pm_device_sleep_wake(struct device 
*dev, bool enable)
 
 static inline int register_acpi_bus_type(void *bus) { return 0; }
 static inline int unregister_acpi_bus_type(void *bus) { return 0; }
+static inline int acpi_match_device_id(const struct device *device,
+   const char *name)
+{
+   return -ENODEV;
+}
 
 #endif /* CONFIG_ACPI */
 
-- 
1.7.7.6



--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC PATCH 3/6] ACPI: introduce acpi_get_generic_resources

2012-09-28 Thread Zhang Rui
From 9a851d177794129a89f720c7122cb39fd163126b Mon Sep 17 00:00:00 2001
From: Zhang Rui rui.zh...@intel.com
Date: Fri, 28 Sep 2012 08:34:05 +0800
Subject: [RFC PATCH 3/6] ACPI: introduce acpi_get_generic_resources

Introduce acpi_get_generic_resources() to convert
ACPI style resources to struct resource.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/acpi/Makefile   |1 +
 drivers/acpi/resource.c |  165 +++
 include/acpi/acpi_bus.h |1 +
 3 files changed, 167 insertions(+), 0 deletions(-)
 create mode 100644 drivers/acpi/resource.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 6b1d535..4b65608 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -46,6 +46,7 @@ acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 ifdef CONFIG_ACPI_VIDEO
 acpi-y += video_detect.o
 endif
+acpi-y += resource.o
 
 # These are (potentially) separate modules
 obj-$(CONFIG_ACPI_AC)  += ac.o
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
new file mode 100644
index 000..30a5204
--- /dev/null
+++ b/drivers/acpi/resource.c
@@ -0,0 +1,165 @@
+/*
+ * resource.c -- convert ACPI resource to generic resource
+ *
+ * Copyright (c) 2012 Zhang Rui rui.zh...@intel.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ */
+#include linux/kernel.h
+#include linux/export.h
+#include linux/acpi.h
+
+static int irq_flags(int triggering, int polarity, int sharable)
+{
+   int flags;
+
+   if (triggering == ACPI_LEVEL_SENSITIVE) {
+   if (polarity == ACPI_ACTIVE_LOW)
+   flags = IORESOURCE_IRQ_LOWLEVEL;
+   else
+   flags = IORESOURCE_IRQ_HIGHLEVEL;
+   } else {
+   if (polarity == ACPI_ACTIVE_LOW)
+   flags = IORESOURCE_IRQ_LOWEDGE;
+   else
+   flags = IORESOURCE_IRQ_HIGHEDGE;
+   }
+
+   if (sharable == ACPI_SHARED)
+   flags |= IORESOURCE_IRQ_SHAREABLE;
+
+   return flags;
+}
+
+static void acpi_get_irq_resource(struct acpi_resource *res,
+ struct resource *resource)
+{
+   struct acpi_resource_irq *irq = res-data.irq;
+   int t, p;
+
+   if (irq-interrupt_count == 0)
+   resource-flags = IORESOURCE_DISABLED;
+
+   if (!acpi_get_override_irq(irq-interrupts[0], t, p)) {
+   t = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
+   p = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
+
+   if (irq-triggering != t || irq-polarity != p) {
+   irq-triggering = t;
+   irq-polarity = p;
+   }
+   }
+
+   resource-flags =
+   irq_flags(irq-triggering, irq-polarity, irq-sharable);
+   resource-flags |= IORESOURCE_IRQ;
+   resource-start = irq-interrupts[0];
+   resource-end = irq-interrupts[0];
+}
+
+static void acpi_get_extended_irq_resource(struct acpi_resource *res,
+  struct resource *resource)
+{
+   struct acpi_resource_extended_irq *irq = res-data.extended_irq;
+   int t, p;
+
+   if (irq-interrupt_count == 0)
+   resource-flags = IORESOURCE_DISABLED;
+
+   if (!acpi_get_override_irq(irq-interrupts[0], t, p)) {
+   t = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
+   p = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
+
+   if (irq-triggering != t || irq-polarity != p) {
+   irq-triggering = t;
+   irq-polarity = p;
+   }
+   }
+
+   resource-flags =
+   irq_flags(irq-triggering, irq-polarity, irq-sharable);
+   resource-flags |= IORESOURCE_IRQ;
+   resource-start = irq-interrupts[0];
+   resource-end = irq-interrupts[0];
+}
+
+static void acpi_get_mem_resource(struct acpi_resource *res,
+ struct resource *resource)
+{
+   struct acpi_resource_fixed_memory32 *mem = res-data.fixed_memory32;
+
+   if (mem-address_length == 0)
+   resource-flags |= IORESOURCE_DISABLED;
+   if (mem-write_protect == ACPI_READ_WRITE_MEMORY)
+   resource-flags |= IORESOURCE_MEM_WRITEABLE;
+
+   resource-flags |= IORESOURCE_MEM;
+   resource-start = mem-address;
+   resource-end = mem-address + mem-address_length - 1;
+}
+
+int acpi_get_generic_resources(struct acpi_device *device,
+  struct resource

[RFC PATCH 4/6] Change i2c_register_board_info from __init to __devinit

2012-09-28 Thread Zhang Rui
From 34aa38e12c04544d89af2eae46de284dc8a03b8d Mon Sep 17 00:00:00 2001
From: Zhang Rui rui.zh...@intel.com
Date: Thu, 27 Sep 2012 15:42:23 +0800
Subject: [RFC PATCH 4/6] Change i2c_register_board_info from __init to
 __devinit.

ACPI 5 supports enumerating I2C adapter and its slaves
via ACPI namespace, and this needs to be done at runtime,
after the ACPI I2C controller driver being loaded.

detailed usage of this API can be found in next patch.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/i2c/i2c-boardinfo.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/i2c-boardinfo.c b/drivers/i2c/i2c-boardinfo.c
index f24cc64..1ecbbdc 100644
--- a/drivers/i2c/i2c-boardinfo.c
+++ b/drivers/i2c/i2c-boardinfo.c
@@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(__i2c_first_dynamic_bus_num);
  * The board info passed can safely be __initdata, but be careful of embedded
  * pointers (for platform_data, functions, etc) since that won't be copied.
  */
-int __init
+int __devinit
 i2c_register_board_info(int busnum,
struct i2c_board_info const *info, unsigned len)
 {
-- 
1.7.7.6



--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration driver

2012-09-28 Thread Zhang Rui
From 6077a62f2865201ab6727ca7d628ee5e43aa57e1 Mon Sep 17 00:00:00 2001
From: Zhang Rui rui.zh...@intel.com
Date: Fri, 24 Aug 2012 15:18:25 +0800
Subject: [RFC PATCH 5/6] ACPI: Introduce ACPI I2C controller enumeration
 driver

This driver is able to
1) enumerate I2C controller via ACPI namespace
   and register it as a platform device.
2) enumerate I2C slave devices via ACPI namespace.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/acpi/Makefile   |1 +
 drivers/acpi/i2c_root.c |  229 +++
 drivers/acpi/sysfs.c|1 +
 include/acpi/acpi_drivers.h |1 +
 4 files changed, 232 insertions(+), 0 deletions(-)
 create mode 100644 drivers/acpi/i2c_root.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 4b65608..5b14f05 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -35,6 +35,7 @@ acpi-y+= scan.o
 acpi-y += processor_core.o
 acpi-y += ec.o
 acpi-y += gpio.o
+acpi-y += i2c_root.o
 acpi-$(CONFIG_ACPI_DOCK)   += dock.o
 acpi-y += pci_root.o pci_link.o pci_irq.o pci_bind.o
 acpi-y += power.o
diff --git a/drivers/acpi/i2c_root.c b/drivers/acpi/i2c_root.c
new file mode 100644
index 000..b9a042b
--- /dev/null
+++ b/drivers/acpi/i2c_root.c
@@ -0,0 +1,229 @@
+/*
+ *  i2c_root.c - ACPI I2C controller Driver ($Revision: 40 $)
+ *
+ *  Copyright (C) 2012 Intel Corp
+ *  Copyright (C) 2012 Zhang Rui rui.zh...@intel.com
+ *
+ * ~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/init.h
+#include linux/types.h
+#include linux/spinlock.h
+#include linux/pm.h
+#include linux/pm_runtime.h
+#include linux/platform_device.h
+#include linux/i2c.h
+#include linux/acpi.h
+#include linux/slab.h
+#include acpi/acpi_bus.h
+#include acpi/acpi_drivers.h
+#include acpi/apei.h
+
+#define PREFIX ACPI: 
+
+#define _COMPONENT ACPI_SPB_COMPONENT
+ACPI_MODULE_NAME(i2c_root);
+#define ACPI_I2C_ROOT_CLASSi2c_root
+#define ACPI_I2C_ROOT_DEVICE_NAME  I2C Controller
+
+static int acpi_i2c_root_add(struct acpi_device *device);
+static int acpi_i2c_root_remove(struct acpi_device *device, int type);
+
+static const struct acpi_device_id root_device_ids[] = {
+   {INT33B1, 0},
+   {, 0},
+};
+
+MODULE_DEVICE_TABLE(acpi, root_device_ids);
+
+static struct acpi_driver acpi_i2c_root_driver = {
+   .name = i2c_root,
+   .class = ACPI_I2C_ROOT_CLASS,
+   .ids = root_device_ids,
+   .ops = {
+   .add = acpi_i2c_root_add,
+   .remove = acpi_i2c_root_remove,
+   },
+};
+
+struct acpi_i2c_root {
+   struct acpi_device *device;
+   struct platform_device *pdev;
+   int busnum;
+   int slaves;
+   struct i2c_board_info *info;
+};
+
+static int add_slave(struct acpi_i2c_root *root, struct i2c_board_info *info)
+{
+   struct i2c_board_info *p;
+
+   if (!info)
+   return 0;
+
+   p = kzalloc(sizeof(*p) * (root-slaves + 1), GFP_KERNEL);
+   if (!p)
+   return -ENOMEM;
+
+   memcpy(p, info, sizeof(*p));
+   if (root-info)
+   memcpy(p + 1, root-info, sizeof(*p) * root-slaves);
+
+   kfree(root-info);
+   root-info = p;
+   root-slaves++;
+   return 0;
+}
+
+static int register_slaves(struct acpi_i2c_root *root)
+{
+   return i2c_register_board_info(root-busnum, root-info, root-slaves);
+}
+
+/*
+ * The i2c info registering call back for each i2c slave device
+ */
+acpi_status __init i2c_enumerate_slave(acpi_handle handle, u32 level,
+  void *data, void **return_value)
+{
+   int result;
+   acpi_status status;
+   struct acpi_buffer buffer;
+   struct acpi_resource *resource;
+   struct acpi_resource_gpio *gpio;
+   struct acpi_resource_i2c_serialbus *i2c;
+   int i;
+   struct acpi_i2c_root *root = data;
+   struct i2c_board_info info;
+   struct acpi_device *device

[RFC PATCH 6/6] Introduce INT33B1 I2C controller driver

2012-09-28 Thread Zhang Rui
From 817d814ecae91862f42a0447f455dae7f74cba27 Mon Sep 17 00:00:00 2001
From: Zhang Rui rui.zh...@intel.com
Date: Fri, 24 Aug 2012 15:20:38 +0800
Subject: [RFC PATCH 6/6] Introduce INT33B1 I2C controller driver

This is a dummy platform device driver to illustrate my idea about
how a really I2C controller should work on ACPI 5 platforms. 
It just probes the INT33B1 I2C controller which is enumerated by ACPI.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 drivers/i2c/busses/Kconfig|8 +++
 drivers/i2c/busses/Makefile   |1 +
 drivers/i2c/busses/i2c-33b1.c |  117 +
 3 files changed, 126 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-33b1.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b4aaa1b..536a19c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -257,6 +257,14 @@ config I2C_SCMI
  To compile this driver as a module, choose M here:
  the module will be called i2c-scmi.
 
+config I2C_33B1
+   tristate INT33B1 I2C controller
+   help
+ This driver supports the ACPI enumerated INT33B1 I2C controller.
+
+ To compile this driver as a module, choose M here:
+ the module will be called i2c-scmi.
+
 endif # ACPI
 
 comment Mac SMBus host controller drivers
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ce3c2be..8e478bd 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -4,6 +4,7 @@
 
 # ACPI drivers
 obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
+obj-$(CONFIG_I2C_33B1) += i2c-33b1.o
 
 # PC SMBus host controller drivers
 obj-$(CONFIG_I2C_ALI1535)  += i2c-ali1535.o
diff --git a/drivers/i2c/busses/i2c-33b1.c b/drivers/i2c/busses/i2c-33b1.c
new file mode 100644
index 000..152c3e4
--- /dev/null
+++ b/drivers/i2c/busses/i2c-33b1.c
@@ -0,0 +1,117 @@
+/*
+ *  i2c_33b1.c -INT33B1 i2c Controller Driver
+ *
+ *  Copyright (c) 2012 Intel Corp
+ *  Copyright (c) 2012 Zhang Rui rui.zh...@intel.com
+ *
+ * ~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ * ~~
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/init.h
+#include linux/ioport.h
+#include linux/delay.h
+#include linux/errno.h
+#include linux/i2c.h
+#include linux/io.h
+#include linux/platform_device.h
+
+
+struct int33b1_i2c_private {
+struct i2c_adapter adap;
+void __iomem *iobase;
+};
+
+
+static int int33b1_i2c_xfer(struct i2c_adapter *adap,
+   struct i2c_msg *msgs, int num)
+{
+   return 0;
+}
+
+static u32 int33b1_func(struct i2c_adapter *adap)
+{
+   /* Emulate SMBUS over I2C */
+   return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
+}
+
+static struct i2c_algorithm int33b1_i2c_algo = {
+   .master_xfer= int33b1_i2c_xfer,
+   .functionality  = int33b1_func,
+};
+
+static int __devinit int33b1_i2c_probe(struct platform_device *pdev)
+{
+   struct int33b1_i2c_private  *priv;
+   struct resource *res;
+   int ret;
+
+   priv = devm_kzalloc(pdev-dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv)
+   return -ENOMEM;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   priv-iobase = devm_request_and_ioremap(pdev-dev, res);
+   if (!priv-iobase) {
+   dev_err(pdev-dev, devm_request_and_ioremap failed\n);
+   return -EBUSY;
+   }
+
+   priv-adap.dev.parent = pdev-dev;
+   priv-adap.owner= THIS_MODULE;
+   priv-adap.algo_data= priv;
+   priv-adap.algo = int33b1_i2c_algo;
+   priv-adap.nr   = pdev-id;
+   priv-adap.class= I2C_CLASS_HWMON;
+   snprintf(priv-adap.name, sizeof(priv-adap.name), int33b1-i2c);
+
+   i2c_set_adapdata(priv-adap, priv);
+   ret = i2c_add_numbered_adapter(priv-adap);
+   if (ret  0) {
+   dev_err(priv-adap.dev, Failed to add i2c bus.\n);
+   return ret;
+   }
+
+   platform_set_drvdata(pdev, priv);
+   dev_info(priv-adap.dev, Added I2C Bus.\n);
+   return 0;
+}
+
+static int __devexit int33b1_i2c_remove(struct platform_device *pdev)
+{
+   struct int33b1_i2c_private *priv;
+
+   priv = platform_get_drvdata(pdev);
+   i2c_del_adapter(priv-adap);
+   platform_set_drvdata(pdev, NULL);
+   return 0;
+}
+
+static struct platform_driver

Re: Fwd: Hid over I2C and ACPI interaction

2012-07-08 Thread Zhang Rui
On 四, 2012-07-05 at 10:44 +0200, Benjamin Tissoires wrote:
 Hi,
 
 Many thanks for these information. It seems like I was on the right
 track, but I didn't saw the hidden part of the iceberg.

yep.

 I've already written the i2c slave part (and the acpi handling to get
 the HID register by using the DSM should work), but I need now the
 whole ACPI pnp drivers...
 
you need the ACPI/PNP I2C controller driver.

 But without a real ACPI 5.0 mainboard, I think it will be quite
 difficult to implement and debug this ACPI stuff.
 
yes, that's the problem I have. I can not send out the code based on
some example ASL code. :)

thanks,
rui

 Cheers,
 Benjamin
 
 On Thu, Jul 5, 2012 at 9:20 AM, Zhang Rui rui.zh...@intel.com wrote:
  Hah, seems I forgot to reply to Benjamin.
 
  On 四, 2012-07-05 at 15:01 +0800, Zhang Rui wrote:
    Original Message 
   Subject: Hid over I2C and ACPI interaction
   Date: Wed, 4 Jul 2012 15:46:35 +0200
   From: Benjamin Tissoires benjamin.tissoi...@gmail.com
   To: Jean Delvare kh...@linux-fr.org, Ben Dooks ben-li...@fluff.org, 
   Wolfram
   Sang w.s...@pengutronix.de, Len Brown l...@kernel.org,
   linux-a...@vger.kernel.org, linux-i2c@vger.kernel.org,
   linux-ker...@vger.kernel.org
   CC: Jiri Kosina jkos...@suse.cz, Stéphane Chatty cha...@enac.fr, JJ 
   Ding
   jj_d...@emc.com.tw
  
   Hi Guys,
  
   I'm the co-author and the maintainer of the hid-multitouch driver. To
   support even more devices, I started the implementation of the HID
   over I2C protocol specification which is introduced by Win8. I'm quite
   comfortable with the hid and the I2C part, but I'm blocked with the
   interaction with the ACPI for the pnp part.
  
   I wanted to have your advice/help on this problem. I've add in the
   recipients list the maintainers of i2c and ACPI, sorry for the noise
   if you don't feel concerned about this.
  
   So, let's go deeper in the problem ;-)
   Microsoft's spec asks the OEM to fill the ACPI DSDT to provide the
   following scope in the ASL layout:
  
begin of ASL
   Scope (\_SB) {
   //
   //  General Purpose I/O, ports 0...127
   //
  
   Device(HIDI2C_DEVICE1) {
Name(_ADR,0)
Name (_HID, MSFT1234”)
Name (_CID, PNP0C50)
Name (_UID, 3)
  
Method(_CRS, 0x0, NotSerialized)
{
Name (RBUF, ResourceTemplate ()
{
// Address 0x07 on I2C-X (OEM selects this address)
   //IHV SPECIFIC I2C3 = I2C Controller; TGD0 = GPIO Controller;
I2CSerialBus (0x07, ControllerInitiated,
   10,AddressingMode7Bit, \\_SB.I2C3)
GpioInt(Level, ActiveLow, Exclusive, PullUp, 0, \\_SB. TGD0,
   0 , ResourceConsumer, , ) {40}
})
 Return(RBUF)
 }
  
 Method(_DSM, 0x4, NotSerialized)
 {
// BreakPoint
Store (Method _DSM begin, Debug)
  
// DSM UUID
switch(ToBuffer(Arg0))
{
// ACPI DSM UUID for HIDI2C
case(ToUUID(3CDFF6F7-4267-4555-AD05-B30A3D8938DE))
{
 // DSM function which returns the HID Descriptor
   Address (skipped)
}
  
default
{
// No other GUIDs supported
Return(Buffer(One) { 0x00 })
}
}
}
   }
end of ASL
  
  yep, this is an ACPI enumerated I2C controller.
 
   Summary:
   - a HID over I2C device has to present the Compatibility ID PNP0C50
   - in the _CRS block, the address, the adapter and the gpioInt are
   defined (or referenced)
   - it presents a Device Specific Method (_DSM) which returns the HID
   Descriptor register address. This register is our entry point for
   retrieving the information about our hid device (so it's mandatory to
   obtain it).
  
   Where am I:
   - I've written a first layer on top of i2c that retrieves the hid
   register (currently the address 0x0001 is hardcoded), then get the
   report desccriptors and the input events, and forward all this stuff
   to the hid layer.
   - It's working with a custom emulated HID over i2c touchpad, while
   waiting for the one a manufacturer should send to me.
   - The detection and the addition to the adapter is done by adding the
   address in the lists and the name through the i2c -detect callback
   (which is not very good, because I don't have the interrupt line
   there).
   - I've written a first acpi implementation that rely on the
   DEVICE_ACPI_HANDLE macro to get the ACPI handle of the device (if
   available).
   - I'm not able to do some tests with the ACPI, as I don't know how to
   implement this DSDT on my computer (I'm missing the I2C part), and the
   manufacturer returned the mainboard with the right DSDT to the OEM.
  
   My questions:
   - will the current acpi implementation handle I2C devices?
 
  you still need to write your own

Re: Fwd: Hid over I2C and ACPI interaction

2012-07-05 Thread Zhang Rui
 driver
for SPI controller and SD/MMC controller.
Third, you need a I2C slave device driver to handle the I2C slave device
in I2C bus.

here is a BKM I wrote, hope it helps.

And also any comments are welcome. :)

From 0a0fa4ff7b4b06c6560de94a78b15c6adfd86e34 Mon Sep 17 00:00:00 2001
From: Zhang Rui rui.zh...@intel.com
Date: Mon, 26 Dec 2011 10:42:04 +0800

 As many SoC IP blocks are not hardware self-enumerable, the
 firmware, aka, ACPI tables, is responsible for
 enumerating/reserving/assigning system resources to these
 devices. This tutorial talks about how to enumerate these
 devices via ACPI namespace.

Signed-off-by: Zhang Rui rui.zh...@intel.com
---
 Documentation/acpi/acpi-device-probing.txt |  466

 1 file changed, 466 insertions(+)
 create mode 100644 Documentation/acpi/acpi-device-probing.txt

diff --git a/Documentation/acpi/acpi-device-probing.txt
b/Documentation/acpi/acpi-device-probing.txt
new file mode 100644
index 000..82efbf3
--- /dev/null
+++ b/Documentation/acpi/acpi-device-probing.txt
@@ -0,0 +1,466 @@
+
+HOWTO enumerate devices via ACPI
+
+Copyright (c) 2011-2012 Intel Corporation
+
+Contrast to hardware self-enumerable devices(e.g. USB, PCI) on PC
platform,
+many SoC IP blocks can not be self enumerated.
+We used to introduce platform specific code for these devices.
+But now, with ACPI 5.0, there is no requirement for the hardware to be
+self-discoverable, enumerable or re-locatable, as the firmware is
responsible
+for enumerating/reserving/assigning system resources (such as address
ranges or
+interrupts) to the device.
+
+This document will show how to enumerate and configure a device via
ACPI.
+If you want to get more details about why and when we need this,
+please refer to ACPI spec 5.0 and
+Intel Architecture Platform Compatibility Definition.
+
+Note that although these are ACPI devices, we prefer to use PnP drivers
for them,
+this is because:
+1. all the non-ACPI-predefined Devices are exported as PnP devices as
well
+2. PnP bus is a well designed bus. Probing via PnP layer saves a lot of
work
+   for the device driver, e.g. getting  parsing ACPI resources.
+
+=
+1. Understand device definition in ACPI namespace
+   [Case study 1] SD/MMC controller
+2. Driver for a leaf device
+   2.1 Make a list of supported PnP ids
+   2.2 Implement .probe/.remove callbacks for the PnP driver
+   2.3 Fill in the pnp_driver structure
+   2.4 Register the PnP driver
+3. Driver for a master device on a non-self-enumerable bus
+   [Case Study 2] SPI controller and its slave device
+   3.1 Probe the master device
+   3.2 Walk ACPI namesapce to get the child devices of the master
device
+   3.3 Register these child devices as slave devices
+   3.4 Write slave device driver
+4. Misc
+=
+
+-
+1. Understand device definition in ACPI namespace
+-
+
+To enumerate a device in ACPI namespace, we need to find out and
understand
+HOW the device is defined in ACPI namespace first.
+
+[Case study 1 ] SD/MMC Controller
+
+Here is an ASL example code for SD/MMC controller definition in ACPI
namespace.
+
+Device (EMMC)
+{
+Name (_ADR, Zero)
+/* I use PNP, an arbitrary string, here, as PnP id is
device specific */
+Name (_HID, PNP)
+Name (_CID, PNP)
+Name (_UID, 4)
+
+Method (_CRS, 0, NotSerialized)
+{
+Name (RBUF, ResourceTemplate ()
+{
+Memory32Fixed (ReadWrite,
+0xFFA5, // Address Base
+0x0100, // Address Length
+)
+Interrupt (ResourceConsumer, Level, ActiveLow,
Exclusive, ,, )
+{
+0x001b,
+}
+})
+Return (RBUF)
+}
+
+Method (_STA, 0, NotSerialized)
+{
+Return (0x0F)
+}
+}
+
+_ADR : the address of this device on its parent bus. Useless in this
case.
+_HID : the PnP id for this device.
+_CID : the compatible PnP id. use this as the PnP id if _HID doesn't
exist.
+_CRS : the system resources currently allocated to this device.
+   the Memory32Fixed part shows an Mem space for the device,
+   and the Interrupt part shows the device interrupt.
+_STA : the current status of the device, e.g. it's
enabled/disabled/removed.
+
+By reading this example ASL code, we should know that there is a SD/MMC
controller
+on this platform, it's mem space base address is 0xFFA5, length is
0x0100,
+and the irq for this device is 0x1b

Re: Fwd: Hid over I2C and ACPI interaction

2012-07-05 Thread Zhang Rui
Hah, seems I forgot to reply to Benjamin.

On 四, 2012-07-05 at 15:01 +0800, Zhang Rui wrote:
   Original Message 
  Subject: Hid over I2C and ACPI interaction
  Date: Wed, 4 Jul 2012 15:46:35 +0200
  From: Benjamin Tissoires benjamin.tissoi...@gmail.com
  To: Jean Delvare kh...@linux-fr.org, Ben Dooks ben-li...@fluff.org, 
  Wolfram 
  Sang w.s...@pengutronix.de, Len Brown l...@kernel.org, 
  linux-a...@vger.kernel.org, linux-i2c@vger.kernel.org, 
  linux-ker...@vger.kernel.org
  CC: Jiri Kosina jkos...@suse.cz, Stéphane Chatty cha...@enac.fr, JJ 
  Ding 
  jj_d...@emc.com.tw
  
  Hi Guys,
  
  I'm the co-author and the maintainer of the hid-multitouch driver. To
  support even more devices, I started the implementation of the HID
  over I2C protocol specification which is introduced by Win8. I'm quite
  comfortable with the hid and the I2C part, but I'm blocked with the
  interaction with the ACPI for the pnp part.
  
  I wanted to have your advice/help on this problem. I've add in the
  recipients list the maintainers of i2c and ACPI, sorry for the noise
  if you don't feel concerned about this.
  
  So, let's go deeper in the problem ;-)
  Microsoft's spec asks the OEM to fill the ACPI DSDT to provide the
  following scope in the ASL layout:
  
   begin of ASL
  Scope (\_SB) {
  //
  //  General Purpose I/O, ports 0...127
  //
  
  Device(HIDI2C_DEVICE1) {
   Name(_ADR,0)
   Name (_HID, MSFT1234”)
   Name (_CID, PNP0C50)
   Name (_UID, 3)
  
   Method(_CRS, 0x0, NotSerialized)
   {
   Name (RBUF, ResourceTemplate ()
   {
   // Address 0x07 on I2C-X (OEM selects this address)
  //IHV SPECIFIC I2C3 = I2C Controller; TGD0 = GPIO Controller;
   I2CSerialBus (0x07, ControllerInitiated,
  10,AddressingMode7Bit, \\_SB.I2C3)
   GpioInt(Level, ActiveLow, Exclusive, PullUp, 0, \\_SB. TGD0,
  0 , ResourceConsumer, , ) {40}
   })
Return(RBUF)
}
  
Method(_DSM, 0x4, NotSerialized)
{
   // BreakPoint
   Store (Method _DSM begin, Debug)
  
   // DSM UUID
   switch(ToBuffer(Arg0))
   {  
   // ACPI DSM UUID for HIDI2C
   case(ToUUID(3CDFF6F7-4267-4555-AD05-B30A3D8938DE))
   {
// DSM function which returns the HID Descriptor
  Address (skipped)
   }
  
   default
   {
   // No other GUIDs supported
   Return(Buffer(One) { 0x00 })
   }
   }
   }
  }
   end of ASL
  
 yep, this is an ACPI enumerated I2C controller.
 
  Summary:
  - a HID over I2C device has to present the Compatibility ID PNP0C50
  - in the _CRS block, the address, the adapter and the gpioInt are
  defined (or referenced)
  - it presents a Device Specific Method (_DSM) which returns the HID
  Descriptor register address. This register is our entry point for
  retrieving the information about our hid device (so it's mandatory to
  obtain it).
  
  Where am I:
  - I've written a first layer on top of i2c that retrieves the hid
  register (currently the address 0x0001 is hardcoded), then get the
  report desccriptors and the input events, and forward all this stuff
  to the hid layer.
  - It's working with a custom emulated HID over i2c touchpad, while
  waiting for the one a manufacturer should send to me.
  - The detection and the addition to the adapter is done by adding the
  address in the lists and the name through the i2c -detect callback
  (which is not very good, because I don't have the interrupt line
  there).
  - I've written a first acpi implementation that rely on the
  DEVICE_ACPI_HANDLE macro to get the ACPI handle of the device (if
  available).
  - I'm not able to do some tests with the ACPI, as I don't know how to
  implement this DSDT on my computer (I'm missing the I2C part), and the
  manufacturer returned the mainboard with the right DSDT to the OEM.
  
  My questions:
  - will the current acpi implementation handle I2C devices?
 
 you still need to write your own device driver for the device.
 
  - it seems to me that the .archdata field is left blank during the i2c
  device initialization in all paths I've seen. Is that true?
  - who puts the name int the struct i2c_board_info? (for hot-plugged
  i2c devices).
  
  - finally, what is the best way of handling ACPI for those I2C devices:
 1) everything is fine, I should have the ACPI handle in .archdata.
 2) someone has to implement the handling of I2C in the pnpACPI layer
  (by adding I2CSerialBus handling and creating there the i2c slave).
 3) I should create an acpi driver which handles PNP0C50 and which
  creates the i2c slaves.
  
 exactly.
 
 As this I2C controller uses the GPIO interrupt, we need an ACPI GPIO
 controller driver for interrupts first.
 I already have such a patch in hand, but have

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

2009-10-15 Thread Zhang Rui
On Tue, 2009-10-13 at 01:02 +0800, Jonathan Cameron wrote:
 Jean Delvare wrote:
  Hi Jonathan,
  
  On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote:
  @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
   };
   
   /*
  + * IDR to assign each registered device a unique id
  + */
  +static DEFINE_IDR(tsl2550_idr);
  +static DEFINE_SPINLOCK(tsl2550_idr_lock);
  +#define TSL2550_DEV_MAX 9
  Such an arbitrary limit is simply not acceptable.
  Fair enough, but it is based on restricting the size
  of the char array needed for the name when registering
  with als.  Hence single digit decimal maximum.
  Do you suggest leaving it unrestricted (which makes code
  a little fiddly given variable size of max idr) or some other
  limit?
  
  The name size really isn't an issue. You won't notice 16 bytes instead
  of 9. And it's not like we'll have millions of these devices.
 I agree, it's merely a question of picking some suitable limit. IDR's
 on a 64 bit box will do something in the ball park of 2e18 which might
 be an excessive limit ;) I'll leave this be for next version on basis
 I'm in favour of moving this into the core and hence as you said this
 will be irrelevant here anyway.
  +static int tsl2550_get_id(void)
  +{
  +int ret, val;
  +
  +idr_again:
  +if (unlikely(idr_pre_get(tsl2550_idr, GFP_KERNEL) == 0))
  +return -ENOMEM;
  +spin_lock(tsl2550_idr_lock);
  +ret = idr_get_new(tsl2550_idr, NULL, val);
  +if (unlikely(ret == -EAGAIN))
  +goto idr_again;
  +else if (unlikely(ret))
  +return ret;
  +if (val  TSL2550_DEV_MAX)
  +return -ENOMEM;
  +return val;
  +}
  +
  +static void tsl2550_free_id(int val)
  +{
  +spin_lock(tsl2550_idr_lock);
  +idr_remove(tsl2550_idr, val);
  +spin_unlock(tsl2550_idr_lock);
  +}
  Having to implement this in _every_ ALS driver sounds wrong and
  painful. If uniqueness of any kind must be provided, it should be
  handled by the ALS core and not by individual drivers, otherwise you
  can be certain that at least one driver will get it wrong someday.
  I agree. The reason this originally occurred is that the acpi layer
  apparently doesn't allow for simultaneous probing of multiple drivers
  and hence can get away with a simple counter and no locking.
 
  I'd imaging that als-class devices are simply named als%u. Just like
  hwmon devices are named hwmon%u, input devices are names input%u and
  event%u, etc. I don't know of any class pushing the naming down to its
  drivers, do you? The only example I can remember are i2c drivers back
  in year 1999, when they were part of lm-sensors.I have personally put
  an end to this years ago.
  This debate started in the als thread. Personally I couldn't care less
  either way but it does need to be put to bed before that subsystem merges.
  To my mind either approach is trivially handled in a userspace library
  so it doesn't matter.  I don't suppose you can remember what the original
  reasons for squashing this naming approach were?
  
  Code duplication. Having the same unique-id handling code repeated in
  50 drivers was no fun, as it did not add any value compared to a
  central handling.
 Counter argument placed (cc'd Pavel and Corentin for this point)
 is that having a generic name, e.g. hwmon0 and a name field in sysfs
 is superfluous when we can combine the two.
  
  @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct 
  device *dev,
   return ret;
   }
   
  -static DEVICE_ATTR(lux1_input, S_IRUGO,
  +static DEVICE_ATTR(illuminance, S_IRUGO,
  tsl2550_show_lux1_input, NULL);
  Question: if I have a light sensing chip with two sensors, how are we
  going to handle it? Will we register 2 als class devices? The initial
  naming convention had the advantage that you could have more than one
  sensor per device, but I don't know if it is desirable in practice.
  This only becomes and issue if we have two sensors measuring illuminance
  (or approximation of it).  The only two sensor chips I know of have one
  broadband and one in the infrared tsl2561 and I think the isl chip with
  a driver currently in misc.  The combination of these two are needed to
  calculate illuminance.  Some of the original discussion went into how
  to support separate access to the individual sensors.  Decision was to
  leave that question until it becomes relevant.  Basically we would put
  the current drivers in just supporting illuminance and see if anyone asked
  for furthers support.  One tricky aspect is what the units should be for
  particular frequency ranges. At least illuminance is cleanly defined
  (even if chips are only fairly coarsely approximating it. 
  
  Hmm, this isn't the point I was raising. I was thinking of a light
  sensor chip which would sense light in different locations. Think chip
  with remote sensors. This is 

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

2009-10-15 Thread Zhang Rui
update Pavel's email address.

On Fri, 2009-10-16 at 09:37 +0800, Zhang Rui wrote:
 On Tue, 2009-10-13 at 01:02 +0800, Jonathan Cameron wrote:
  Jean Delvare wrote:
   Hi Jonathan,
   
   On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote:
   @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
};

/*
   + * IDR to assign each registered device a unique id
   + */
   +static DEFINE_IDR(tsl2550_idr);
   +static DEFINE_SPINLOCK(tsl2550_idr_lock);
   +#define TSL2550_DEV_MAX 9
   Such an arbitrary limit is simply not acceptable.
   Fair enough, but it is based on restricting the size
   of the char array needed for the name when registering
   with als.  Hence single digit decimal maximum.
   Do you suggest leaving it unrestricted (which makes code
   a little fiddly given variable size of max idr) or some other
   limit?
   
   The name size really isn't an issue. You won't notice 16 bytes instead
   of 9. And it's not like we'll have millions of these devices.
  I agree, it's merely a question of picking some suitable limit. IDR's
  on a 64 bit box will do something in the ball park of 2e18 which might
  be an excessive limit ;) I'll leave this be for next version on basis
  I'm in favour of moving this into the core and hence as you said this
  will be irrelevant here anyway.
   +static int tsl2550_get_id(void)
   +{
   +  int ret, val;
   +
   +idr_again:
   +  if (unlikely(idr_pre_get(tsl2550_idr, GFP_KERNEL) == 0))
   +  return -ENOMEM;
   +  spin_lock(tsl2550_idr_lock);
   +  ret = idr_get_new(tsl2550_idr, NULL, val);
   +  if (unlikely(ret == -EAGAIN))
   +  goto idr_again;
   +  else if (unlikely(ret))
   +  return ret;
   +  if (val  TSL2550_DEV_MAX)
   +  return -ENOMEM;
   +  return val;
   +}
   +
   +static void tsl2550_free_id(int val)
   +{
   +  spin_lock(tsl2550_idr_lock);
   +  idr_remove(tsl2550_idr, val);
   +  spin_unlock(tsl2550_idr_lock);
   +}
   Having to implement this in _every_ ALS driver sounds wrong and
   painful. If uniqueness of any kind must be provided, it should be
   handled by the ALS core and not by individual drivers, otherwise you
   can be certain that at least one driver will get it wrong someday.
   I agree. The reason this originally occurred is that the acpi layer
   apparently doesn't allow for simultaneous probing of multiple drivers
   and hence can get away with a simple counter and no locking.
  
   I'd imaging that als-class devices are simply named als%u. Just like
   hwmon devices are named hwmon%u, input devices are names input%u and
   event%u, etc. I don't know of any class pushing the naming down to its
   drivers, do you? The only example I can remember are i2c drivers back
   in year 1999, when they were part of lm-sensors.I have personally put
   an end to this years ago.
   This debate started in the als thread. Personally I couldn't care less
   either way but it does need to be put to bed before that subsystem 
   merges.
   To my mind either approach is trivially handled in a userspace library
   so it doesn't matter.  I don't suppose you can remember what the original
   reasons for squashing this naming approach were?
   
   Code duplication. Having the same unique-id handling code repeated in
   50 drivers was no fun, as it did not add any value compared to a
   central handling.
  Counter argument placed (cc'd Pavel and Corentin for this point)
  is that having a generic name, e.g. hwmon0 and a name field in sysfs
  is superfluous when we can combine the two.
   
   @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct 
   device *dev,
  return ret;
}

   -static DEVICE_ATTR(lux1_input, S_IRUGO,
   +static DEVICE_ATTR(illuminance, S_IRUGO,
 tsl2550_show_lux1_input, NULL);
   Question: if I have a light sensing chip with two sensors, how are we
   going to handle it? Will we register 2 als class devices? The initial
   naming convention had the advantage that you could have more than one
   sensor per device, but I don't know if it is desirable in practice.
   This only becomes and issue if we have two sensors measuring illuminance
   (or approximation of it).  The only two sensor chips I know of have one
   broadband and one in the infrared tsl2561 and I think the isl chip with
   a driver currently in misc.  The combination of these two are needed to
   calculate illuminance.  Some of the original discussion went into how
   to support separate access to the individual sensors.  Decision was to
   leave that question until it becomes relevant.  Basically we would put
   the current drivers in just supporting illuminance and see if anyone 
   asked
   for furthers support.  One tricky aspect is what the units should be for
   particular frequency ranges. At least illuminance is cleanly defined
   (even if chips are only fairly coarsely approximating it. 
   
   Hmm