diff mbox

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

Message ID 1389620911-3890-1-git-send-email-rui.zhang@intel.com
State Superseded
Headers show

Commit Message

Zhang, Rui Jan. 13, 2014, 1:48 p.m. UTC 
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.zhang@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(-)

Comments

Mark Brown Jan. 13, 2014, 5:35 p.m. UTC | #1 
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.  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.

Please also always remember to CC maintainers on patches.

> 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...
Zhang, Rui Jan. 14, 2014, 8 a.m. UTC | #2 
On Mon, 2014-01-13 at 17:35 +0000, 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Jan. 14, 2014, 2:41 p.m. UTC | #3 
On Tue, Jan 14, 2014 at 04:00:17PM +0800, Zhang Rui wrote:
> On Mon, 2014-01-13 at 17:35 +0000, 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.

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

OK that makes sense, but what does this have to do with the _HID and
_CID methods?  Surely we're just replacing spi: with acpi: in the uevent?
Zhang, Rui Jan. 15, 2014, 1:16 a.m. UTC | #4 
On Tue, 2014-01-14 at 14:41 +0000, Mark Brown wrote:
> On Tue, Jan 14, 2014 at 04:00:17PM +0800, Zhang Rui wrote:
> > On Mon, 2014-01-13 at 17:35 +0000, 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.
> 
> > 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).
> 
> OK that makes sense, but what does this have to do with the _HID and
> _CID methods?

If an ACPI enumerated SPI device has a _HID and a _CID,
both of them need to be exposed in 'uevent', so that a driver that
matches _CID can also have a chance to be probed. This can not be done
by the current code, thus we need special handling of ACPI enumerated
SPI devices.

>   Surely we're just replacing spi: with acpi: in the uevent?

yes.

thanks,
rui

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

Patch

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;