On Wednesday 01 October 2014 04:17:02 Rafael J. Wysocki wrote:
> Index: linux-pm/drivers/leds/leds-gpio.c
> ===================================================================
> --- linux-pm.orig/drivers/leds/leds-gpio.c
> +++ linux-pm/drivers/leds/leds-gpio.c
> @@ -231,6 +231,13 @@ static const struct of_device_id of_gpio
>
> MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
>
> +static const struct acpi_device_id acpi_gpio_leds_match[] = {
> + { "PRP0001" }, /* Device Tree shoehorned into ACPI */
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match);
> +
> static int gpio_led_probe(struct platform_device *pdev)
> {
> struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -286,6 +293,7 @@ static struct platform_driver gpio_led_d
> .name = "leds-gpio",
> .owner = THIS_MODULE,
> .of_match_table = of_gpio_leds_match,
> + .acpi_match_table = acpi_gpio_leds_match,
> },
> };
Is this something you'd have to do in every driver you want to support
_PRP based probing? For the ".acpi_match_table =" reference, I think
you could actually provide a generic acpi_device_id table exported from
core code that you refer to, so each driver just does
.acpi_match_table = acpi_match_by_of_compatible,
(or whatever you want to call it).
Regarding the MODULE_DEVICE_TABLE, I suspect the above won't work the
way you are hoping for, because once you get to dozens or hundreds of
drivers doing this, each device will show up with the same string,
so udev will try to load all the modules that list "PRP0001". That
doesn't look right. With the code from patch 3, you can probably drop
the acpi MODULE_DEVICE_TABLE() entirely and get the correct behavior.
Arnd
On Wed, Oct 01, 2014 at 10:13:04AM +0200, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 04:17:02 Rafael J. Wysocki wrote:
> > Index: linux-pm/drivers/leds/leds-gpio.c
> > ===================================================================
> > --- linux-pm.orig/drivers/leds/leds-gpio.c
> > +++ linux-pm/drivers/leds/leds-gpio.c
> > @@ -231,6 +231,13 @@ static const struct of_device_id of_gpio
> >
> > MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
> >
> > +static const struct acpi_device_id acpi_gpio_leds_match[] = {
> > + { "PRP0001" }, /* Device Tree shoehorned into ACPI */
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match);
> > +
> > static int gpio_led_probe(struct platform_device *pdev)
> > {
> > struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > @@ -286,6 +293,7 @@ static struct platform_driver gpio_led_d
> > .name = "leds-gpio",
> > .owner = THIS_MODULE,
> > .of_match_table = of_gpio_leds_match,
> > + .acpi_match_table = acpi_gpio_leds_match,
> > },
> > };
>
> Is this something you'd have to do in every driver you want to support
> _PRP based probing? For the ".acpi_match_table =" reference, I think
> you could actually provide a generic acpi_device_id table exported from
> core code that you refer to, so each driver just does
>
> .acpi_match_table = acpi_match_by_of_compatible,
>
> (or whatever you want to call it).
That's a good idea.
> Regarding the MODULE_DEVICE_TABLE, I suspect the above won't work the
> way you are hoping for, because once you get to dozens or hundreds of
> drivers doing this, each device will show up with the same string,
> so udev will try to load all the modules that list "PRP0001". That
> doesn't look right. With the code from patch 3, you can probably drop
> the acpi MODULE_DEVICE_TABLE() entirely and get the correct behavior.
It actually works like this now:
# cd /sys/bus/platform/devices/PRP0001\:00/
DRIVER=leds-gpio
MODALIAS=of:Nprp0001TacpiCgpio-leds
# cat modalias
of:Nprp0001TacpiCgpio-leds
In other words the modalias changes to be of:Nprp0001Tacpi, e.g
name=prp0001, type=acpi and then list of compatible values.
Udev then loads only module that matches the modalias so it should not
load everything listing PRP0001 in their MODULE_DEVICE_TABLE().
On Wednesday 01 October 2014 12:13:09 Mika Westerberg wrote:
>
> > Regarding the MODULE_DEVICE_TABLE, I suspect the above won't work the
> > way you are hoping for, because once you get to dozens or hundreds of
> > drivers doing this, each device will show up with the same string,
> > so udev will try to load all the modules that list "PRP0001". That
> > doesn't look right. With the code from patch 3, you can probably drop
> > the acpi MODULE_DEVICE_TABLE() entirely and get the correct behavior.
>
> It actually works like this now:
>
> # cd /sys/bus/platform/devices/PRP0001\:00/
> DRIVER=leds-gpio
> MODALIAS=of:Nprp0001TacpiCgpio-leds
>
> # cat modalias
> of:Nprp0001TacpiCgpio-leds
>
> In other words the modalias changes to be of:Nprp0001Tacpi, e.g
> name=prp0001, type=acpi and then list of compatible values.
>
> Udev then loads only module that matches the modalias so it should not
> load everything listing PRP0001 in their MODULE_DEVICE_TABLE().
I'm not completely following yet. I can see how this works now, but
how is this better than just using the existing modalias for OF?
Arnd
On Wed, Oct 01, 2014 at 12:01:34PM +0200, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 12:13:09 Mika Westerberg wrote:
> >
> > > Regarding the MODULE_DEVICE_TABLE, I suspect the above won't work the
> > > way you are hoping for, because once you get to dozens or hundreds of
> > > drivers doing this, each device will show up with the same string,
> > > so udev will try to load all the modules that list "PRP0001". That
> > > doesn't look right. With the code from patch 3, you can probably drop
> > > the acpi MODULE_DEVICE_TABLE() entirely and get the correct behavior.
> >
> > It actually works like this now:
> >
> > # cd /sys/bus/platform/devices/PRP0001\:00/
> > DRIVER=leds-gpio
> > MODALIAS=of:Nprp0001TacpiCgpio-leds
> >
> > # cat modalias
> > of:Nprp0001TacpiCgpio-leds
> >
> > In other words the modalias changes to be of:Nprp0001Tacpi, e.g
> > name=prp0001, type=acpi and then list of compatible values.
> >
> > Udev then loads only module that matches the modalias so it should not
> > load everything listing PRP0001 in their MODULE_DEVICE_TABLE().
>
> I'm not completely following yet. I can see how this works now, but
> how is this better than just using the existing modalias for OF?
You mean using just what of_device_get_modalias() would create? In that
case, what do we put to name and type fields?
On Wednesday 01 October 2014 14:59:01 Mika Westerberg wrote:
> On Wed, Oct 01, 2014 at 12:01:34PM +0200, Arnd Bergmann wrote:
> > On Wednesday 01 October 2014 12:13:09 Mika Westerberg wrote:
> > >
> > > > Regarding the MODULE_DEVICE_TABLE, I suspect the above won't work the
> > > > way you are hoping for, because once you get to dozens or hundreds of
> > > > drivers doing this, each device will show up with the same string,
> > > > so udev will try to load all the modules that list "PRP0001". That
> > > > doesn't look right. With the code from patch 3, you can probably drop
> > > > the acpi MODULE_DEVICE_TABLE() entirely and get the correct behavior.
> > >
> > > It actually works like this now:
> > >
> > > # cd /sys/bus/platform/devices/PRP0001\:00/
> > > DRIVER=leds-gpio
> > > MODALIAS=of:Nprp0001TacpiCgpio-leds
> > >
> > > # cat modalias
> > > of:Nprp0001TacpiCgpio-leds
> > >
> > > In other words the modalias changes to be of:Nprp0001Tacpi, e.g
> > > name=prp0001, type=acpi and then list of compatible values.
> > >
> > > Udev then loads only module that matches the modalias so it should not
> > > load everything listing PRP0001 in their MODULE_DEVICE_TABLE().
> >
> > I'm not completely following yet. I can see how this works now, but
> > how is this better than just using the existing modalias for OF?
>
> You mean using just what of_device_get_modalias() would create? In that
> case, what do we put to name and type fields?
Sorry, I think we're still both misunderstanding one another. You were
talking about the modalias created by the device scanning above, while
I meant the one in the MODULE_DEVICE_TABLE.
With the entry you create in create_modalias(), you will only ever
match against the MODULE_DEVICE_TABLE(of, of_gpio_leds_match)
line, not against the MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match),
so I think you can just drop the latter.
On the question what to put into the name and type fields, that is
unrelated. The type is supposed to be for the 'device_type' property
in DT, which we should never rely on in a driver that supports both
APCI and DT. In Linux we only use that for "pci", "cpu" and "memory",
all of which have their own way of getting probed in ACPI.
The "name" is normally ignored in DT as well, except for backwards
compatibility with old bindings, but I would argue that you should not
just put "prp0001" in there. Either leave it empty like type, or use
the name of the device as it appears in the ACPI tables, such as "DEV0"
or "PWM".
Arnd
On Wed, Oct 01, 2014 at 03:52:42PM +0200, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 14:59:01 Mika Westerberg wrote:
> > On Wed, Oct 01, 2014 at 12:01:34PM +0200, Arnd Bergmann wrote:
> > > On Wednesday 01 October 2014 12:13:09 Mika Westerberg wrote:
> > > >
> > > > > Regarding the MODULE_DEVICE_TABLE, I suspect the above won't work the
> > > > > way you are hoping for, because once you get to dozens or hundreds of
> > > > > drivers doing this, each device will show up with the same string,
> > > > > so udev will try to load all the modules that list "PRP0001". That
> > > > > doesn't look right. With the code from patch 3, you can probably drop
> > > > > the acpi MODULE_DEVICE_TABLE() entirely and get the correct behavior.
> > > >
> > > > It actually works like this now:
> > > >
> > > > # cd /sys/bus/platform/devices/PRP0001\:00/
> > > > DRIVER=leds-gpio
> > > > MODALIAS=of:Nprp0001TacpiCgpio-leds
> > > >
> > > > # cat modalias
> > > > of:Nprp0001TacpiCgpio-leds
> > > >
> > > > In other words the modalias changes to be of:Nprp0001Tacpi, e.g
> > > > name=prp0001, type=acpi and then list of compatible values.
> > > >
> > > > Udev then loads only module that matches the modalias so it should not
> > > > load everything listing PRP0001 in their MODULE_DEVICE_TABLE().
> > >
> > > I'm not completely following yet. I can see how this works now, but
> > > how is this better than just using the existing modalias for OF?
> >
> > You mean using just what of_device_get_modalias() would create? In that
> > case, what do we put to name and type fields?
>
> Sorry, I think we're still both misunderstanding one another. You were
> talking about the modalias created by the device scanning above, while
> I meant the one in the MODULE_DEVICE_TABLE.
Right, got it now.
> With the entry you create in create_modalias(), you will only ever
> match against the MODULE_DEVICE_TABLE(of, of_gpio_leds_match)
> line, not against the MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match),
> so I think you can just drop the latter.
Indeed.
> On the question what to put into the name and type fields, that is
> unrelated. The type is supposed to be for the 'device_type' property
> in DT, which we should never rely on in a driver that supports both
> APCI and DT. In Linux we only use that for "pci", "cpu" and "memory",
> all of which have their own way of getting probed in ACPI.
> The "name" is normally ignored in DT as well, except for backwards
> compatibility with old bindings, but I would argue that you should not
> just put "prp0001" in there. Either leave it empty like type, or use
> the name of the device as it appears in the ACPI tables, such as "DEV0"
> or "PWM".
OK, I think it makes sense to leave them empty. I remember I tried that
at some point but it didn't work without N and T fields. Is there some
example what to put there in case of empty?
Something like "of:N*T*Cgpio-leds" perhaps?
On Wednesday 01 October 2014 17:04:41 Mika Westerberg wrote:
>
> > On the question what to put into the name and type fields, that is
> > unrelated. The type is supposed to be for the 'device_type' property
> > in DT, which we should never rely on in a driver that supports both
> > APCI and DT. In Linux we only use that for "pci", "cpu" and "memory",
> > all of which have their own way of getting probed in ACPI.
> > The "name" is normally ignored in DT as well, except for backwards
> > compatibility with old bindings, but I would argue that you should not
> > just put "prp0001" in there. Either leave it empty like type, or use
> > the name of the device as it appears in the ACPI tables, such as "DEV0"
> > or "PWM".
>
> OK, I think it makes sense to leave them empty. I remember I tried that
> at some point but it didn't work without N and T fields. Is there some
> example what to put there in case of empty?
>
> Something like "of:N*T*Cgpio-leds" perhaps?
Sorry, don't know. If I read the code right, the type field in DT ends
up being "<NULL>" for any device that doesn't set the device_type
property, but that seems a bit silly and probably isn't worth copying.
Arnd
On Wed, Oct 01, 2014 at 10:13:04AM +0200, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 04:17:02 Rafael J. Wysocki wrote:
> > Index: linux-pm/drivers/leds/leds-gpio.c
> > ===================================================================
> > --- linux-pm.orig/drivers/leds/leds-gpio.c
> > +++ linux-pm/drivers/leds/leds-gpio.c
> > @@ -231,6 +231,13 @@ static const struct of_device_id of_gpio
> >
> > MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
> >
> > +static const struct acpi_device_id acpi_gpio_leds_match[] = {
> > + { "PRP0001" }, /* Device Tree shoehorned into ACPI */
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match);
> > +
> > static int gpio_led_probe(struct platform_device *pdev)
> > {
> > struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > @@ -286,6 +293,7 @@ static struct platform_driver gpio_led_d
> > .name = "leds-gpio",
> > .owner = THIS_MODULE,
> > .of_match_table = of_gpio_leds_match,
> > + .acpi_match_table = acpi_gpio_leds_match,
> > },
> > };
>
> Is this something you'd have to do in every driver you want to support
> _PRP based probing? For the ".acpi_match_table =" reference, I think
> you could actually provide a generic acpi_device_id table exported from
> core code that you refer to, so each driver just does
>
> .acpi_match_table = acpi_match_by_of_compatible,
No, I think in absence of drv->acpi_match_table ACPI core should just go and
use drv->of_match_table to do the matching and be done with it.
Thanks.
--
Dmitry
On 10/1/14, 9:30, "Dmitry Torokhov" <[email protected]> wrote:
>On Wed, Oct 01, 2014 at 10:13:04AM +0200, Arnd Bergmann wrote:
>> On Wednesday 01 October 2014 04:17:02 Rafael J. Wysocki wrote:
>> > Index: linux-pm/drivers/leds/leds-gpio.c
>> > ===================================================================
>> > --- linux-pm.orig/drivers/leds/leds-gpio.c
>> > +++ linux-pm/drivers/leds/leds-gpio.c
>> > @@ -231,6 +231,13 @@ static const struct of_device_id of_gpio
>> >
>> > MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
>> >
>> > +static const struct acpi_device_id acpi_gpio_leds_match[] = {
>> > + { "PRP0001" }, /* Device Tree shoehorned into ACPI */
>> > + {},
>> > +};
>> > +
>> > +MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match);
>> > +
>> > static int gpio_led_probe(struct platform_device *pdev)
>> > {
>> > struct gpio_led_platform_data *pdata =
>>dev_get_platdata(&pdev->dev);
>> > @@ -286,6 +293,7 @@ static struct platform_driver gpio_led_d
>> > .name = "leds-gpio",
>> > .owner = THIS_MODULE,
>> > .of_match_table = of_gpio_leds_match,
>> > + .acpi_match_table = acpi_gpio_leds_match,
>> > },
>> > };
>>
>> Is this something you'd have to do in every driver you want to support
>> _PRP based probing? For the ".acpi_match_table =" reference, I think
>> you could actually provide a generic acpi_device_id table exported from
>> core code that you refer to, so each driver just does
>>
>> .acpi_match_table = acpi_match_by_of_compatible,
>
>No, I think in absence of drv->acpi_match_table ACPI core should just go
>and
>use drv->of_match_table to do the matching and be done with it.
But then you will match drivers that have of-only support that don't know
anything about ACPI and haven't been updated to use the new API. Worse,
some of those drivers will assume of node structs and such and potentially
panic. Unless I'm sorry mistaken here....
--
Darren Hart
Intel Open Source Technology Center
On Wed, Oct 01, 2014 at 11:11:46AM -0700, Darren Hart wrote:
> On 10/1/14, 9:30, "Dmitry Torokhov" <[email protected]> wrote:
>
> >On Wed, Oct 01, 2014 at 10:13:04AM +0200, Arnd Bergmann wrote:
> >> On Wednesday 01 October 2014 04:17:02 Rafael J. Wysocki wrote:
> >> > Index: linux-pm/drivers/leds/leds-gpio.c
> >> > ===================================================================
> >> > --- linux-pm.orig/drivers/leds/leds-gpio.c
> >> > +++ linux-pm/drivers/leds/leds-gpio.c
> >> > @@ -231,6 +231,13 @@ static const struct of_device_id of_gpio
> >> >
> >> > MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
> >> >
> >> > +static const struct acpi_device_id acpi_gpio_leds_match[] = {
> >> > + { "PRP0001" }, /* Device Tree shoehorned into ACPI */
> >> > + {},
> >> > +};
> >> > +
> >> > +MODULE_DEVICE_TABLE(acpi, acpi_gpio_leds_match);
> >> > +
> >> > static int gpio_led_probe(struct platform_device *pdev)
> >> > {
> >> > struct gpio_led_platform_data *pdata =
> >>dev_get_platdata(&pdev->dev);
> >> > @@ -286,6 +293,7 @@ static struct platform_driver gpio_led_d
> >> > .name = "leds-gpio",
> >> > .owner = THIS_MODULE,
> >> > .of_match_table = of_gpio_leds_match,
> >> > + .acpi_match_table = acpi_gpio_leds_match,
> >> > },
> >> > };
> >>
> >> Is this something you'd have to do in every driver you want to support
> >> _PRP based probing? For the ".acpi_match_table =" reference, I think
> >> you could actually provide a generic acpi_device_id table exported from
> >> core code that you refer to, so each driver just does
> >>
> >> .acpi_match_table = acpi_match_by_of_compatible,
> >
> >No, I think in absence of drv->acpi_match_table ACPI core should just go
> >and
> >use drv->of_match_table to do the matching and be done with it.
>
> But then you will match drivers that have of-only support that don't know
> anything about ACPI and haven't been updated to use the new API. Worse,
> some of those drivers will assume of node structs and such and potentially
> panic. Unless I'm sorry mistaken here....
Does ACPI set dev->of_node pointer? I'd expect them to fail right there...
--
Dmitry
On Wednesday 01 October 2014 11:11:46 Darren Hart wrote:
> >
> >No, I think in absence of drv->acpi_match_table ACPI core should just go
> >and
> >use drv->of_match_table to do the matching and be done with it.
>
> But then you will match drivers that have of-only support that don't know
> anything about ACPI and haven't been updated to use the new API. Worse,
> some of those drivers will assume of node structs and such and potentially
> panic. Unless I'm sorry mistaken here....
>
I don't think that is a huge danger: most drivers tend to check for
the presence of dev->of_node before calling any of the DT interfaces,
you'd only ever enter the probe function if the compatible string matches
(i.e. an old kernel with a new ACPI table), and most users of ACPI
systems will disable CONFIG_OF at compile time, so the accessors looking
at the of_node are not there.
In theory it's possible that something goes wrong here, but it's not
very likely to ever cause problems.
Arnd
On Wed, Oct 01, 2014 at 04:14:20PM +0200, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 17:04:41 Mika Westerberg wrote:
> >
> > > On the question what to put into the name and type fields, that is
> > > unrelated. The type is supposed to be for the 'device_type' property
> > > in DT, which we should never rely on in a driver that supports both
> > > APCI and DT. In Linux we only use that for "pci", "cpu" and "memory",
> > > all of which have their own way of getting probed in ACPI.
> > > The "name" is normally ignored in DT as well, except for backwards
> > > compatibility with old bindings, but I would argue that you should not
> > > just put "prp0001" in there. Either leave it empty like type, or use
> > > the name of the device as it appears in the ACPI tables, such as "DEV0"
> > > or "PWM".
> >
> > OK, I think it makes sense to leave them empty. I remember I tried that
> > at some point but it didn't work without N and T fields. Is there some
> > example what to put there in case of empty?
> >
> > Something like "of:N*T*Cgpio-leds" perhaps?
>
> Sorry, don't know. If I read the code right, the type field in DT ends
> up being "<NULL>" for any device that doesn't set the device_type
> property, but that seems a bit silly and probably isn't worth copying.
OK, I checked and udev wants to have both N and T but they can be left
empty. If there are no objections the modalias will look like this:
[root@mnw01 ~]# cat /sys/bus/spi/devices/spi-PRP0001\:00/modalias
of:Nat25TCatmel,at25
In other words name is the ACPI device name (AT25) in lower case, T is
left empty and C is the compatible property.
On Thursday 02 October 2014 12:55:36 Mika Westerberg wrote:
> On Wed, Oct 01, 2014 at 04:14:20PM +0200, Arnd Bergmann wrote:
> > On Wednesday 01 October 2014 17:04:41 Mika Westerberg wrote:
> > >
> > > > On the question what to put into the name and type fields, that is
> > > > unrelated. The type is supposed to be for the 'device_type' property
> > > > in DT, which we should never rely on in a driver that supports both
> > > > APCI and DT. In Linux we only use that for "pci", "cpu" and "memory",
> > > > all of which have their own way of getting probed in ACPI.
> > > > The "name" is normally ignored in DT as well, except for backwards
> > > > compatibility with old bindings, but I would argue that you should not
> > > > just put "prp0001" in there. Either leave it empty like type, or use
> > > > the name of the device as it appears in the ACPI tables, such as "DEV0"
> > > > or "PWM".
> > >
> > > OK, I think it makes sense to leave them empty. I remember I tried that
> > > at some point but it didn't work without N and T fields. Is there some
> > > example what to put there in case of empty?
> > >
> > > Something like "of:N*T*Cgpio-leds" perhaps?
> >
> > Sorry, don't know. If I read the code right, the type field in DT ends
> > up being "<NULL>" for any device that doesn't set the device_type
> > property, but that seems a bit silly and probably isn't worth copying.
>
> OK, I checked and udev wants to have both N and T but they can be left
> empty. If there are no objections the modalias will look like this:
>
> [root@mnw01 ~]# cat /sys/bus/spi/devices/spi-PRP0001\:00/modalias
> of:Nat25TCatmel,at25
>
> In other words name is the ACPI device name (AT25) in lower case, T is
> left empty and C is the compatible property.
Yes, seems ok to me.
Arnd