| Message ID | 1381414669-26115-1-git-send-email-jarkko.nikula@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
+Rafael On Thu, Oct 10, 2013 at 05:17:49PM +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. I think that this is intentional. We don't want that the i2c modalias matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI IDs that are added to the driver to match with the ACPI device. > 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. > > Fix this by using ACPI hardware ID. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Cc: Mika Westerberg <mika.westerberg@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 > @@ -1111,7 +1111,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", > -- > 1.8.4.rc3 -- 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
On Friday, October 11, 2013 05:49:46 PM Mika Westerberg wrote: > +Rafael > > On Thu, Oct 10, 2013 at 05:17:49PM +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. > > I think that this is intentional. We don't want that the i2c modalias > matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI > IDs that are added to the driver to match with the ACPI device. Well, I'm not really sure this was intentional, but I wonder how other bus types work in that respect? > > 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. > > > > Fix this by using ACPI hardware ID. > > > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > > Cc: Mika Westerberg <mika.westerberg@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 > > @@ -1111,7 +1111,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", >
On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote: > > I think that this is intentional. We don't want that the i2c modalias > > matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI > > IDs that are added to the driver to match with the ACPI device. > > Well, I'm not really sure this was intentional, but I wonder how other bus > types work in that respect? We have the same for platform bus, if that's what you are asking. It probably doesn't hurt to have this patch applied but it might cause inadvertent match if for some reason there is an I2C client driver that happens to have INTABCD I2C id in its list. -- 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
On Saturday, October 12, 2013 08:04:13 AM Mika Westerberg wrote: > On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote: > > > I think that this is intentional. We don't want that the i2c modalias > > > matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI > > > IDs that are added to the driver to match with the ACPI device. > > > > Well, I'm not really sure this was intentional, but I wonder how other bus > > types work in that respect? > > We have the same for platform bus, if that's what you are asking. > > It probably doesn't hurt to have this patch applied but it might cause > inadvertent match if for some reason there is an I2C client driver that > happens to have INTABCD I2C id in its list. Well, if they have that id in their lists, they are supposed to be able to handle this device, aren't they? What other reason may be there for them to put that id into their lists?
On Sat, Oct 12, 2013 at 03:45:15PM +0200, Rafael J. Wysocki wrote: > On Saturday, October 12, 2013 08:04:13 AM Mika Westerberg wrote: > > On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote: > > > > I think that this is intentional. We don't want that the i2c modalias > > > > matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI > > > > IDs that are added to the driver to match with the ACPI device. > > > > > > Well, I'm not really sure this was intentional, but I wonder how other bus > > > types work in that respect? > > > > We have the same for platform bus, if that's what you are asking. > > > > It probably doesn't hurt to have this patch applied but it might cause > > inadvertent match if for some reason there is an I2C client driver that > > happens to have INTABCD I2C id in its list. > > Well, if they have that id in their lists, they are supposed to be able to > handle this device, aren't they? What other reason may be there for them > to put that id into their lists? If we have two ACPI enumerated devices, they have following modalias: i2c-device0: i2c:INTABCD:00 acpi:INTABCD i2c-device1: i2c:INTABCD:01 acpi:INTABCD Likelihood that some random I2C driver has INTABCD:00 or INTABCD:01 ids in their list is minimal. However, when you turn it to this: i2c-device0: i2c:INTABCD acpi:INTABCD i2c-device1: i2c:INTABCD acpi:INTABCD It might be possible that we get a match that isn't supposed to happen. Well, OK it is pretty remote but anyway :-) -- 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
On 10/12/2013 08:04 AM, Mika Westerberg wrote: > On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote: >>> I think that this is intentional. We don't want that the i2c modalias >>> matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI >>> IDs that are added to the driver to match with the ACPI device. >> Well, I'm not really sure this was intentional, but I wonder how other bus >> types work in that respect? > We have the same for platform bus, if that's what you are asking. > Do we? I don't recall seeing per device modaliases on other platforms on their platform buses. And actually I don't see that happening in drivers/base/platform.c: platform_uevent() either where just pdev->name is used but not pdev->id (which is used with pdev->name for dev_set_name()). This makes me thinking that perhaps "pdevinfo.name = dev_name(&adev->dev);" in drivers/acpi/acpi_platform.c: acpi_create_platform_device() should be fixed too as now modalias for ACPI registered platform devices differ from platform devices that are registered in other subsystems (e.g. regulatory, pcspkr, alarmtimer, etc devices)? I can send a patch for that.
On 10/12/2013 07:18 PM, Mika Westerberg wrote: > If we have two ACPI enumerated devices, they have following modalias: > > i2c-device0: i2c:INTABCD:00 > acpi:INTABCD > > i2c-device1: i2c:INTABCD:01 > acpi:INTABCD > > Likelihood that some random I2C driver has INTABCD:00 or INTABCD:01 ids in > their list is minimal. However, when you turn it to this: > > > i2c-device0: i2c:INTABCD > acpi:INTABCD > > i2c-device1: i2c:INTABCD > acpi:INTABCD > > It might be possible that we get a match that isn't supposed to happen. > Well, OK it is pretty remote but anyway :-) Well, name conflicts could occur of course but still I don't think we should generate illegal or wrong modaliases. I'm not an udev expert but I suppose trying to load nonexisting drivers (i2c_INTABCD:xy) could slow booting a little and perhaps pollute needlessly error log compared to if it can see that driver is already loaded or tries to load the same driver again. I don't think name conflicts can pose too big risk as they are trivial to fix in sources and can be queued to stable too.
On Mon, Oct 14, 2013 at 09:34:39AM +0300, Jarkko Nikula wrote: > On 10/12/2013 08:04 AM, Mika Westerberg wrote: > >On Sat, Oct 12, 2013 at 12:16:02AM +0200, Rafael J. Wysocki wrote: > >>>I think that this is intentional. We don't want that the i2c modalias > >>>matches with the ACPI device (like with the i2c:INTABCD). Instead use ACPI > >>>IDs that are added to the driver to match with the ACPI device. > >>Well, I'm not really sure this was intentional, but I wonder how other bus > >>types work in that respect? > >We have the same for platform bus, if that's what you are asking. > > > Do we? I don't recall seeing per device modaliases on other > platforms on their platform buses. I mean for platform devices enumerated from ACPI. > And actually I don't see that happening in drivers/base/platform.c: > platform_uevent() either where just pdev->name is used but not > pdev->id (which is used with pdev->name for dev_set_name()). > > This makes me thinking that perhaps "pdevinfo.name = > dev_name(&adev->dev);" in drivers/acpi/acpi_platform.c: > acpi_create_platform_device() should be fixed too as now modalias > for ACPI registered platform devices differ from platform devices > that are registered in other subsystems (e.g. regulatory, pcspkr, > alarmtimer, etc devices)? Well, if you think that it doesn't hit us back later if we get a match that isn't supposed to happen. -- 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
On Mon, Oct 14, 2013 at 09:45:53AM +0300, Jarkko Nikula wrote: > On 10/12/2013 07:18 PM, Mika Westerberg wrote: > >If we have two ACPI enumerated devices, they have following modalias: > > > > i2c-device0: i2c:INTABCD:00 > > acpi:INTABCD > > > > i2c-device1: i2c:INTABCD:01 > > acpi:INTABCD > > > >Likelihood that some random I2C driver has INTABCD:00 or INTABCD:01 ids in > >their list is minimal. However, when you turn it to this: > > > > > > i2c-device0: i2c:INTABCD > > acpi:INTABCD > > > > i2c-device1: i2c:INTABCD > > acpi:INTABCD > > > >It might be possible that we get a match that isn't supposed to happen. > >Well, OK it is pretty remote but anyway :-) > Well, name conflicts could occur of course but still I don't think > we should generate illegal or wrong modaliases. I'm not an udev > expert but I suppose trying to load nonexisting drivers > (i2c_INTABCD:xy) could slow booting a little and perhaps pollute > needlessly error log compared to if it can see that driver is > already loaded or tries to load the same driver again. > > I don't think name conflicts can pose too big risk as they are > trivial to fix in sources and can be queued to stable too. OK, you got me convinced :-) No further objections from me. -- 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
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.nikula@linux.intel.com> > Cc: Mika Westerberg <mika.westerberg@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 > @@ -1111,7 +1111,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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: > 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? I would consider that as a bug, but I'm not sure what the recommended practice is. Greg?
On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: > > 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? Well, if it isn't, it's pretty pointless, right? > > 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? > > I would consider that as a bug, but I'm not sure what the recommended practice > is. Greg? I have no idea how the OF stuff is working, and honestly, I really have no wish to ever know anything about it. Especially when it comes to platform devices/drivers, something that I personally hate and wish would be deleted. So go ask the OF maintainers/developers, this is their domain :) sorry, greg k-h -- 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
On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: > > > 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? > > Well, if it isn't, it's pretty pointless, right? > > > > 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? > > > > I would consider that as a bug, but I'm not sure what the recommended practice > > is. Greg? > > I have no idea how the OF stuff is working, and honestly, I really have > no wish to ever know anything about it. Especially when it comes to > platform devices/drivers, something that I personally hate and wish > would be deleted. > > So go ask the OF maintainers/developers, this is their domain :) Well, OK. Whom in particular? Rafael -- 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
On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: > > > 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? > > Well, if it isn't, it's pretty pointless, right? > > > > 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? > > > > I would consider that as a bug, but I'm not sure what the recommended practice > > is. Greg? > > I have no idea how the OF stuff is working, and honestly, I really have > no wish to ever know anything about it. Especially when it comes to > platform devices/drivers, something that I personally hate and wish > would be deleted. > > So go ask the OF maintainers/developers, this is their domain :) So Rui, let's consider that as a bug and please prepare a patch to fix it. Thanks!
On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote: > On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: > > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: > > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: > > > > 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? > > > > Well, if it isn't, it's pretty pointless, right? > > > > > > 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? > > > > > > I would consider that as a bug, but I'm not sure what the recommended practice > > > is. Greg? > > > > I have no idea how the OF stuff is working, and honestly, I really have > > no wish to ever know anything about it. Especially when it comes to > > platform devices/drivers, something that I personally hate and wish > > would be deleted. > > > > So go ask the OF maintainers/developers, this is their domain :) > > Well, OK. Whom in particular? The "OPEN FIRMWARE AND FLATTENED DEVICE TREE" entries in MAINTAINERS? -- 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
On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote: > On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote: > > On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: > > > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: > > > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: > > > > > 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? > > > > > > Well, if it isn't, it's pretty pointless, right? > > > > > > > > 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? > > > > > > > > I would consider that as a bug, but I'm not sure what the recommended practice > > > > is. Greg? > > > > > > I have no idea how the OF stuff is working, and honestly, I really have > > > no wish to ever know anything about it. Especially when it comes to > > > platform devices/drivers, something that I personally hate and wish > > > would be deleted. > > > > > > So go ask the OF maintainers/developers, this is their domain :) > > > > Well, OK. Whom in particular? > > The "OPEN FIRMWARE AND FLATTENED DEVICE TREE" entries in MAINTAINERS? Hmm, that looks like Grant and Rob Herring. Grant, Rob, do we want the OF-style MODALIAS to be shown in "modalias" sysfs attributes of platform devices? Rafael -- 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
On Wed, Oct 16, 2013 at 12:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote: >> On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote: >> > On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: >> > > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: >> > > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: >> > > > > 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? >> > > >> > > Well, if it isn't, it's pretty pointless, right? >> > > >> > > > > 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? >> > > > >> > > > I would consider that as a bug, but I'm not sure what the recommended practice >> > > > is. Greg? >> > > >> > > I have no idea how the OF stuff is working, and honestly, I really have >> > > no wish to ever know anything about it. Especially when it comes to >> > > platform devices/drivers, something that I personally hate and wish >> > > would be deleted. <digress>Greg, I've heard you say that a lot, but regardless of what platform devices/drivers were originally designed for, it is pretty much exactly what we need for non-discoverable memory mapped busses. I've yet to heard a viable alternative proposed. I've heard the proposal of creating new bus types and new driver binding to that bus type for each variant of a non-discoverable memory mapped bus, but I think it is a non-starter. There are too many combinations. What /might/ work to replace the platform_bus_type would be to have a mechanism for drivers to transparently bind to multiple bus types, but then I suspect that it will end up looking an awful lot like the existing platform_bus_type. Probably worth discussing over beer next week</digress> >> > > >> > > So go ask the OF maintainers/developers, this is their domain :) >> > >> > Well, OK. Whom in particular? >> >> The "OPEN FIRMWARE AND FLATTENED DEVICE TREE" entries in MAINTAINERS? > > Hmm, that looks like Grant and Rob Herring. > > Grant, Rob, do we want the OF-style MODALIAS to be shown in "modalias" sysfs > attributes of platform devices? I've not actually had to deal with modutils and modalias on any of the platforms I've worked with so I've only looked at it superficially. Yes, that does look like a bug, but Ben would probably have a much better idea [cc'd]. g. -- 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
On Wed, Oct 16, 2013 at 01:04:02AM +0100, Grant Likely wrote: > On Wed, Oct 16, 2013 at 12:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote: > >> On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote: > >> > On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: > >> > > On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: > >> > > > On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: > >> > > > > 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? > >> > > > >> > > Well, if it isn't, it's pretty pointless, right? > >> > > > >> > > > > 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? > >> > > > > >> > > > I would consider that as a bug, but I'm not sure what the recommended practice > >> > > > is. Greg? > >> > > > >> > > I have no idea how the OF stuff is working, and honestly, I really have > >> > > no wish to ever know anything about it. Especially when it comes to > >> > > platform devices/drivers, something that I personally hate and wish > >> > > would be deleted. > > <digress>Greg, I've heard you say that a lot, but regardless of what > platform devices/drivers were originally designed for, it is pretty > much exactly what we need for non-discoverable memory mapped busses. > I've yet to heard a viable alternative proposed. I've heard the > proposal of creating new bus types and new driver binding to that bus > type for each variant of a non-discoverable memory mapped bus, but I > think it is a non-starter. There are too many combinations. What > /might/ work to replace the platform_bus_type would be to have a > mechanism for drivers to transparently bind to multiple bus types, but > then I suspect that it will end up looking an awful lot like the > existing platform_bus_type. > > Probably worth discussing over beer next week</digress> I think we have a whole session about this at the ARM summit next week, and if someone wants to bring beer into it, that will make me happy! -- 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
On 10/16/2013 03:04 AM, Grant Likely wrote: > On Wed, Oct 16, 2013 at 12:47 AM, Rafael J. Wysocki<rjw@sisk.pl> wrote: >> On Tuesday, October 15, 2013 04:31:43 PM Greg Kroah-Hartman wrote: >>> On Tue, Oct 15, 2013 at 11:24:01PM +0200, Rafael J. Wysocki wrote: >>>> On Tuesday, October 15, 2013 01:48:29 PM Greg Kroah-Hartman wrote: >>>>> On Tue, Oct 15, 2013 at 10:37:02PM +0200, Rafael J. Wysocki wrote: >>>>>> On Tuesday, October 15, 2013 07:44:44 PM Zhang Rui wrote: >>>>>>> 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? >>>>> Well, if it isn't, it's pretty pointless, right? >>>>>>> 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? Here is an example from one DT based system: cat /sys/bus/platform/devices/48070000.i2c/uevent DRIVER=omap_i2c OF_NAME=i2c OF_FULLNAME=/ocp/i2c@48070000 OF_COMPATIBLE_0=ti,omap4-i2c OF_COMPATIBLE_N=1 MODALIAS=of:Ni2cT<NULL>Cti,omap4-i2c cat /sys/bus/platform/devices/48070000.i2c/modalias platform:48070000.i2c And a device on that I2C bus: cat /sys/bus/platform/devices/rtc.11/uevent DRIVER=twl_rtc OF_NAME=rtc OF_FULLNAME=/ocp/i2c@48070000/twl@48/rtc OF_COMPATIBLE_0=ti,twl4030-rtc OF_COMPATIBLE_N=1 MODALIAS=of:NrtcT<NULL>Cti,twl4030-rtc cat /sys/bus/platform/devices/rtc.11/modalias platform:rtc.11 Unfortunately I cannot debug above example further at the moment is there failing or needless modprobe calls. Maybe device tree experts know better?
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 @@ -1111,7 +1111,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",
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. Fix this by using ACPI hardware ID. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> Cc: Mika Westerberg <mika.westerberg@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(-)