| Message ID | 474bfc72b67e1d0e2a223084a69f04d2baa1dda8.1510822218.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/11/17 10:50, H. Nikolaus Schaller wrote: > The vendor name was "toppoly" but other panels and the vendor list > have defined it as "tpo". So let's fix it in driver and bindings. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > -MODULE_ALIAS("spi:toppoly,td028ttec1"); > +MODULE_ALIAS("spi:tpo,td028ttec1"); Doesn't this mean that the module won't load if you have old bindings? Can't we have two module aliases? Tomi
> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: > > On 16/11/17 10:50, H. Nikolaus Schaller wrote: >> The vendor name was "toppoly" but other panels and the vendor list >> have defined it as "tpo". So let's fix it in driver and bindings. >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- > > >> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >> +MODULE_ALIAS("spi:tpo,td028ttec1"); > > Doesn't this mean that the module won't load if you have old bindings? Hm. Well, I think it can load but doesn't automatically from DT strings which might be unexpected. > Can't we have two module aliases? I think we can. Just a random example: https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 So we should keep both. Should I submit a new version? BR, Nikolaus -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote: > >> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: >> >> On 16/11/17 10:50, H. Nikolaus Schaller wrote: >>> The vendor name was "toppoly" but other panels and the vendor list >>> have defined it as "tpo". So let's fix it in driver and bindings. >>> >>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>> --- >> >> >>> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >>> +MODULE_ALIAS("spi:tpo,td028ttec1"); >> >> Doesn't this mean that the module won't load if you have old bindings? > > Hm. > > Well, I think it can load but doesn't automatically from DT strings which might > be unexpected. > >> Can't we have two module aliases? > > I think we can. Just a random example: > https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 > > So we should keep both. Even better would be to drop both MODULE_ALIAS and let the MODULE_DEVICE_TABLE macro define them for your from the SPI id table. Although it doesn't look like this driver has an SPI id table, you should probably add one, I be interested to see if this module is always being matched through the "spi" or the "of" alias.. > > Should I submit a new version? > > BR, > Nikolaus > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, > Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>: > > On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote: >> >>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: >>> >>> On 16/11/17 10:50, H. Nikolaus Schaller wrote: >>>> The vendor name was "toppoly" but other panels and the vendor list >>>> have defined it as "tpo". So let's fix it in driver and bindings. >>>> >>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>> --- >>> >>> >>>> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >>>> +MODULE_ALIAS("spi:tpo,td028ttec1"); >>> >>> Doesn't this mean that the module won't load if you have old bindings? >> >> Hm. >> >> Well, I think it can load but doesn't automatically from DT strings which might >> be unexpected. >> >>> Can't we have two module aliases? >> >> I think we can. Just a random example: >> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 >> >> So we should keep both. > > Even better would be to drop both MODULE_ALIAS and let the > MODULE_DEVICE_TABLE macro define them for your from the SPI id table. Why would that be better? As far as I see it will need more code and changes than adding one line of MODULE_ALIAS. > Although it doesn't look like this driver has an SPI id table, you > should probably add one, I be interested to see if this module is always > being matched through the "spi" or the "of" alias.. Could you please propose how that code should look like, so that I can test? BR and thanks, Nikolaus Schaller > >> >> Should I submit a new version? >> >> BR, >> Nikolaus >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote: > Hi Andrew, > >> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>: >> >> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote: >>> >>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: >>>> >>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote: >>>>> The vendor name was "toppoly" but other panels and the vendor list >>>>> have defined it as "tpo". So let's fix it in driver and bindings. >>>>> >>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>> --- >>>> >>>> >>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >>>>> +MODULE_ALIAS("spi:tpo,td028ttec1"); >>>> >>>> Doesn't this mean that the module won't load if you have old bindings? >>> >>> Hm. >>> >>> Well, I think it can load but doesn't automatically from DT strings which might >>> be unexpected. >>> >>>> Can't we have two module aliases? >>> >>> I think we can. Just a random example: >>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 >>> >>> So we should keep both. >> >> Even better would be to drop both MODULE_ALIAS and let the >> MODULE_DEVICE_TABLE macro define them for your from the SPI id table. > > Why would that be better? > MODULE_ALIAS is ugly, you already have a table (usually) of device names that are supported by the driver, the module aliases should be generated from that table. This also keeps supported device list in one place. > As far as I see it will need more code and changes than adding one line of > MODULE_ALIAS. > >> Although it doesn't look like this driver has an SPI id table, you >> should probably add one, I be interested to see if this module is always >> being matched through the "spi" or the "of" alias.. > > Could you please propose how that code should look like, so that I can test? > Sure, start with $ udevadm monitor and see what string the kernel is looking for when trying to find a module for this device. If it is only ever looking for "of:toppoly,td028ttec1", then you can drop the MODULE_ALIAS and be done as it was never getting used anyway. What I expect though is "spi:toppoly,td028ttec1", in which case you should add static const struct spi_device_id td028ttec1_ids[] = { { "toppoly,td028ttec1", 0 }, { "tpo,td028ttec1", 0}, { /* sentinel */ } }; MODULE_DEVICE_TABLE(spi, td028ttec1_ids); link to it in the td028ttec1_spi_driver struct: .id_table = td028ttec1_ids, Then test again to see that the module still loads with the new and old DT string. Andrew > BR and thanks, > Nikolaus Schaller > >> >>> >>> Should I submit a new version? >>> >>> BR, >>> Nikolaus >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Am 16.11.2017 um 18:08 schrieb Andrew F. Davis <afd@ti.com>: > > On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote: >> Hi Andrew, >> >>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>: >>> >>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote: >>>> >>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: >>>>> >>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote: >>>>>> The vendor name was "toppoly" but other panels and the vendor list >>>>>> have defined it as "tpo". So let's fix it in driver and bindings. >>>>>> >>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>>> --- >>>>> >>>>> >>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1"); >>>>> >>>>> Doesn't this mean that the module won't load if you have old bindings? >>>> >>>> Hm. >>>> >>>> Well, I think it can load but doesn't automatically from DT strings which might >>>> be unexpected. >>>> >>>>> Can't we have two module aliases? >>>> >>>> I think we can. Just a random example: >>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 >>>> >>>> So we should keep both. >>> >>> Even better would be to drop both MODULE_ALIAS and let the >>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table. >> >> Why would that be better? >> > > MODULE_ALIAS is ugly, you already have a table (usually) of device names > that are supported by the driver, the module aliases should be generated > from that table. This also keeps supported device list in one place. > >> As far as I see it will need more code and changes than adding one line of >> MODULE_ALIAS. >> >>> Although it doesn't look like this driver has an SPI id table, you >>> should probably add one, I be interested to see if this module is always >>> being matched through the "spi" or the "of" alias.. >> >> Could you please propose how that code should look like, so that I can test? >> > > Sure, > > start with > $ udevadm monitor > and see what string the kernel is looking for when trying to find a > module for this device. Well, the module is loaded automatically from DT at boot time well before I can start udevadm. So that is the most tricky part to setup the system to suppress this... > > If it is only ever looking for "of:toppoly,td028ttec1", then you can > drop the MODULE_ALIAS and be done as it was never getting used anyway. Since it is an SPI client, I am sure it looks for "spi:something. > > What I expect though is "spi:toppoly,td028ttec1", in which case you > should add > > static const struct spi_device_id td028ttec1_ids[] = { > { "toppoly,td028ttec1", 0 }, > { "tpo,td028ttec1", 0}, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(spi, td028ttec1_ids); We already have a static const struct of_device_id td028ttec1_of_match[] table with the same information. So we still have two places to keep in sync. Or can we remove the td028ttec1_of_match[]? AFAIK not. > > link to it in the td028ttec1_spi_driver struct: > .id_table = td028ttec1_ids, > > Then test again to see that the module still loads with the new and old > DT string. In total I am not really convinced that adding 7 lines of code is better than one (the "tpo," alias) that is tested and works... And it looks like a lot of unplanned code testing for me which takes more than 5 minutes :) So I'd prefer to leave that exercise of fixing the MODULE_ALIAS/DEVICE_TABLE to someone else... BR and thanks, Nikolaus > > Andrew > >> BR and thanks, >> Nikolaus Schaller >> >>> >>>> >>>> Should I submit a new version? >>>> >>>> BR, >>>> Nikolaus >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/16/2017 12:18 PM, H. Nikolaus Schaller wrote: > >> Am 16.11.2017 um 18:08 schrieb Andrew F. Davis <afd@ti.com>: >> >> On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote: >>> Hi Andrew, >>> >>>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>: >>>> >>>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote: >>>>> >>>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: >>>>>> >>>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote: >>>>>>> The vendor name was "toppoly" but other panels and the vendor list >>>>>>> have defined it as "tpo". So let's fix it in driver and bindings. >>>>>>> >>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>>>> --- >>>>>> >>>>>> >>>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >>>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1"); >>>>>> >>>>>> Doesn't this mean that the module won't load if you have old bindings? >>>>> >>>>> Hm. >>>>> >>>>> Well, I think it can load but doesn't automatically from DT strings which might >>>>> be unexpected. >>>>> >>>>>> Can't we have two module aliases? >>>>> >>>>> I think we can. Just a random example: >>>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 >>>>> >>>>> So we should keep both. >>>> >>>> Even better would be to drop both MODULE_ALIAS and let the >>>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table. >>> >>> Why would that be better? >>> >> >> MODULE_ALIAS is ugly, you already have a table (usually) of device names >> that are supported by the driver, the module aliases should be generated >> from that table. This also keeps supported device list in one place. >> >>> As far as I see it will need more code and changes than adding one line of >>> MODULE_ALIAS. >>> >>>> Although it doesn't look like this driver has an SPI id table, you >>>> should probably add one, I be interested to see if this module is always >>>> being matched through the "spi" or the "of" alias.. >>> >>> Could you please propose how that code should look like, so that I can test? >>> >> >> Sure, >> >> start with >> $ udevadm monitor >> and see what string the kernel is looking for when trying to find a >> module for this device. > > Well, the module is loaded automatically from DT at boot time well before > I can start udevadm. So that is the most tricky part to setup the system > to suppress this... > >> >> If it is only ever looking for "of:toppoly,td028ttec1", then you can >> drop the MODULE_ALIAS and be done as it was never getting used anyway. > > Since it is an SPI client, I am sure it looks for "spi:something. > >> >> What I expect though is "spi:toppoly,td028ttec1", in which case you >> should add >> >> static const struct spi_device_id td028ttec1_ids[] = { >> { "toppoly,td028ttec1", 0 }, >> { "tpo,td028ttec1", 0}, >> { /* sentinel */ } >> }; >> MODULE_DEVICE_TABLE(spi, td028ttec1_ids); > > We already have a static const struct of_device_id td028ttec1_of_match[] > table with the same information. > > So we still have two places to keep in sync. > > Or can we remove the td028ttec1_of_match[]? AFAIK not. > >> >> link to it in the td028ttec1_spi_driver struct: >> .id_table = td028ttec1_ids, >> >> Then test again to see that the module still loads with the new and old >> DT string. > > In total I am not really convinced that adding 7 lines of code is better > than one (the "tpo," alias) that is tested and works... > > And it looks like a lot of unplanned code testing for me which takes more > than 5 minutes :) > > So I'd prefer to leave that exercise of fixing the MODULE_ALIAS/DEVICE_TABLE > to someone else... > That's fine, someday I'll probably get some script to do this for all the drivers that still have MODULE_ALIAS and an existing table. > BR and thanks, > Nikolaus > >> >> Andrew >> >>> BR and thanks, >>> Nikolaus Schaller >>> >>>> >>>>> >>>>> Should I submit a new version? >>>>> >>>>> BR, >>>>> Nikolaus >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, > Am 16.11.2017 um 19:32 schrieb Andrew F. Davis <afd@ti.com>: > > On 11/16/2017 12:18 PM, H. Nikolaus Schaller wrote: >> >>> Am 16.11.2017 um 18:08 schrieb Andrew F. Davis <afd@ti.com>: >>> >>> On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote: >>>> Hi Andrew, >>>> >>>>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>: >>>>> >>>>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote: >>>>>> >>>>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: >>>>>>> >>>>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote: >>>>>>>> The vendor name was "toppoly" but other panels and the vendor list >>>>>>>> have defined it as "tpo". So let's fix it in driver and bindings. >>>>>>>> >>>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>>>>> --- >>>>>>> >>>>>>> >>>>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >>>>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1"); >>>>>>> >>>>>>> Doesn't this mean that the module won't load if you have old bindings? >>>>>> >>>>>> Hm. >>>>>> >>>>>> Well, I think it can load but doesn't automatically from DT strings which might >>>>>> be unexpected. >>>>>> >>>>>>> Can't we have two module aliases? >>>>>> >>>>>> I think we can. Just a random example: >>>>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 >>>>>> >>>>>> So we should keep both. >>>>> >>>>> Even better would be to drop both MODULE_ALIAS and let the >>>>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table. >>>> >>>> Why would that be better? >>>> >>> >>> MODULE_ALIAS is ugly, you already have a table (usually) of device names >>> that are supported by the driver, the module aliases should be generated >>> from that table. This also keeps supported device list in one place. >>> >>>> As far as I see it will need more code and changes than adding one line of >>>> MODULE_ALIAS. >>>> >>>>> Although it doesn't look like this driver has an SPI id table, you >>>>> should probably add one, I be interested to see if this module is always >>>>> being matched through the "spi" or the "of" alias.. >>>> >>>> Could you please propose how that code should look like, so that I can test? >>>> >>> >>> Sure, >>> >>> start with >>> $ udevadm monitor >>> and see what string the kernel is looking for when trying to find a >>> module for this device. >> >> Well, the module is loaded automatically from DT at boot time well before >> I can start udevadm. So that is the most tricky part to setup the system >> to suppress this... >> >>> >>> If it is only ever looking for "of:toppoly,td028ttec1", then you can >>> drop the MODULE_ALIAS and be done as it was never getting used anyway. >> >> Since it is an SPI client, I am sure it looks for "spi:something. >> >>> >>> What I expect though is "spi:toppoly,td028ttec1", in which case you >>> should add >>> >>> static const struct spi_device_id td028ttec1_ids[] = { >>> { "toppoly,td028ttec1", 0 }, >>> { "tpo,td028ttec1", 0}, >>> { /* sentinel */ } >>> }; >>> MODULE_DEVICE_TABLE(spi, td028ttec1_ids); >> >> We already have a static const struct of_device_id td028ttec1_of_match[] >> table with the same information. >> >> So we still have two places to keep in sync. >> >> Or can we remove the td028ttec1_of_match[]? AFAIK not. >> >>> >>> link to it in the td028ttec1_spi_driver struct: >>> .id_table = td028ttec1_ids, >>> >>> Then test again to see that the module still loads with the new and old >>> DT string. >> >> In total I am not really convinced that adding 7 lines of code is better >> than one (the "tpo," alias) that is tested and works... >> >> And it looks like a lot of unplanned code testing for me which takes more >> than 5 minutes :) >> >> So I'd prefer to leave that exercise of fixing the MODULE_ALIAS/DEVICE_TABLE >> to someone else... >> > > That's fine, someday I'll probably get some script to do this for all > the drivers that still have MODULE_ALIAS and an existing table. That would be cool! On a second thought, I think there is a quick experiment for this driver not needing to monitor events. 1st attempt: remove ALIASES => if it still loads it would be fine 2nd attempt: add your id table => if it loads again, it is fine if not, let's keep ALIASES. Maybe I can try tomorrow. BR and thanks, Nikolaus > >> BR and thanks, >> Nikolaus >> >>> >>> Andrew >>> >>>> BR and thanks, >>>> Nikolaus Schaller >>>> >>>>> >>>>>> >>>>>> Should I submit a new version? >>>>>> >>>>>> BR, >>>>>> Nikolaus >>>>>> >>>>>> -- >>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>>>>> the body of a message to majordomo@vger.kernel.org >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, > Am 16.11.2017 um 19:56 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > Hi Andrew, > >> Am 16.11.2017 um 19:32 schrieb Andrew F. Davis <afd@ti.com>: >> >> On 11/16/2017 12:18 PM, H. Nikolaus Schaller wrote: >>> >>>> Am 16.11.2017 um 18:08 schrieb Andrew F. Davis <afd@ti.com>: >>>> >>>> On 11/16/2017 10:10 AM, H. Nikolaus Schaller wrote: >>>>> Hi Andrew, >>>>> >>>>>> Am 16.11.2017 um 16:53 schrieb Andrew F. Davis <afd@ti.com>: >>>>>> >>>>>> On 11/16/2017 07:43 AM, H. Nikolaus Schaller wrote: >>>>>>> >>>>>>>> Am 16.11.2017 um 13:32 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>: >>>>>>>> >>>>>>>> On 16/11/17 10:50, H. Nikolaus Schaller wrote: >>>>>>>>> The vendor name was "toppoly" but other panels and the vendor list >>>>>>>>> have defined it as "tpo". So let's fix it in driver and bindings. >>>>>>>>> >>>>>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>>>>>> --- >>>>>>>> >>>>>>>> >>>>>>>>> -MODULE_ALIAS("spi:toppoly,td028ttec1"); >>>>>>>>> +MODULE_ALIAS("spi:tpo,td028ttec1"); >>>>>>>> >>>>>>>> Doesn't this mean that the module won't load if you have old bindings? >>>>>>> >>>>>>> Hm. >>>>>>> >>>>>>> Well, I think it can load but doesn't automatically from DT strings which might >>>>>>> be unexpected. >>>>>>> >>>>>>>> Can't we have two module aliases? >>>>>>> >>>>>>> I think we can. Just a random example: >>>>>>> https://elixir.free-electrons.com/linux/latest/source/drivers/w1/slaves/w1_therm.c#L754 >>>>>>> >>>>>>> So we should keep both. >>>>>> >>>>>> Even better would be to drop both MODULE_ALIAS and let the >>>>>> MODULE_DEVICE_TABLE macro define them for your from the SPI id table. >>>>> >>>>> Why would that be better? >>>>> >>>> >>>> MODULE_ALIAS is ugly, you already have a table (usually) of device names >>>> that are supported by the driver, the module aliases should be generated >>>> from that table. This also keeps supported device list in one place. >>>> >>>>> As far as I see it will need more code and changes than adding one line of >>>>> MODULE_ALIAS. >>>>> >>>>>> Although it doesn't look like this driver has an SPI id table, you >>>>>> should probably add one, I be interested to see if this module is always >>>>>> being matched through the "spi" or the "of" alias.. >>>>> >>>>> Could you please propose how that code should look like, so that I can test? >>>>> >>>> >>>> Sure, >>>> >>>> start with >>>> $ udevadm monitor >>>> and see what string the kernel is looking for when trying to find a >>>> module for this device. >>> >>> Well, the module is loaded automatically from DT at boot time well before >>> I can start udevadm. So that is the most tricky part to setup the system >>> to suppress this... >>> >>>> >>>> If it is only ever looking for "of:toppoly,td028ttec1", then you can >>>> drop the MODULE_ALIAS and be done as it was never getting used anyway. >>> >>> Since it is an SPI client, I am sure it looks for "spi:something. >>> >>>> >>>> What I expect though is "spi:toppoly,td028ttec1", in which case you >>>> should add >>>> >>>> static const struct spi_device_id td028ttec1_ids[] = { >>>> { "toppoly,td028ttec1", 0 }, >>>> { "tpo,td028ttec1", 0}, >>>> { /* sentinel */ } >>>> }; >>>> MODULE_DEVICE_TABLE(spi, td028ttec1_ids); >>> >>> We already have a static const struct of_device_id td028ttec1_of_match[] >>> table with the same information. >>> >>> So we still have two places to keep in sync. >>> >>> Or can we remove the td028ttec1_of_match[]? AFAIK not. >>> >>>> >>>> link to it in the td028ttec1_spi_driver struct: >>>> .id_table = td028ttec1_ids, >>>> >>>> Then test again to see that the module still loads with the new and old >>>> DT string. >>> >>> In total I am not really convinced that adding 7 lines of code is better >>> than one (the "tpo," alias) that is tested and works... >>> >>> And it looks like a lot of unplanned code testing for me which takes more >>> than 5 minutes :) >>> >>> So I'd prefer to leave that exercise of fixing the MODULE_ALIAS/DEVICE_TABLE >>> to someone else... >>> >> >> That's fine, someday I'll probably get some script to do this for all >> the drivers that still have MODULE_ALIAS and an existing table. > > That would be cool! > > On a second thought, I think there is a quick experiment for this driver > not needing to monitor events. > > 1st attempt: remove ALIASES => if it still loads it would be fine > 2nd attempt: add your id table => if it loads again, it is fine > if not, let's keep ALIASES. > > Maybe I can try tomorrow. I found time to give it a try and indeed: 1. with module_aliases root@letux:~# lsmod | fgrep td028 panel_tpo_td028ttec1 16384 1 omapdss_base 16384 5 connector_analog_tv,encoder_opa362,panel_tpo_td028ttec1,omapdrm,omapdss root@letux:~# modprobe -c | fgrep td028 alias of:N*T*Comapdss,toppoly,td028ttec1 panel_tpo_td028ttec1 alias of:N*T*Comapdss,toppoly,td028ttec1C* panel_tpo_td028ttec1 alias of:N*T*Comapdss,tpo,td028ttec1 panel_tpo_td028ttec1 alias of:N*T*Comapdss,tpo,td028ttec1C* panel_tpo_td028ttec1 alias spi:toppoly,td028ttec1 panel_tpo_td028ttec1 alias spi:tpo,td028ttec1 panel_tpo_td028ttec1 root@letux:~# => two MODULE_ALIASEs are possible. 2. without module_aliases No display and: root@letux:~# lsmod | fgrep td028 root@letux:~# modprobe -c | fgrep td028 alias of:N*T*Comapdss,toppoly,td028ttec1 panel_tpo_td028ttec1 alias of:N*T*Comapdss,toppoly,td028ttec1C* panel_tpo_td028ttec1 alias of:N*T*Comapdss,tpo,td028ttec1 panel_tpo_td028ttec1 alias of:N*T*Comapdss,tpo,td028ttec1C* panel_tpo_td028ttec1 root@letux:~# => spi: entries are needed. 3. with MODULE_DEVICE_TABLE Display is back and: root@letux:~# lsmod | fgrep td028 panel_tpo_td028ttec1 16384 1 omapdss_base 16384 5 connector_analog_tv,encoder_opa362,panel_tpo_td028ttec1,omapdrm,omapdss root@letux:~# modprobe -c | fgrep td028 alias of:N*T*Comapdss,toppoly,td028ttec1 panel_tpo_td028ttec1 alias of:N*T*Comapdss,toppoly,td028ttec1C* panel_tpo_td028ttec1 alias of:N*T*Comapdss,tpo,td028ttec1 panel_tpo_td028ttec1 alias of:N*T*Comapdss,tpo,td028ttec1C* panel_tpo_td028ttec1 alias spi:toppoly,td028ttec1 panel_tpo_td028ttec1 alias spi:tpo,td028ttec1 panel_tpo_td028ttec1 root@letux:~# => MODULE_DEVICE_TABLE works equally well. I have prepared a separate patch so that a first one adds another MODULE_ALIAS and the new one replaces both MODULE_ALIASes with MODULE_DEVICE_TABLE. I will submit a [PATCH v3] asap. BR and thanks, Nikolaus -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/display/panel/toppoly,td028ttec1.txt b/Documentation/devicetree/bindings/display/panel/tpo,td028ttec1.txt similarity index 84% rename from Documentation/devicetree/bindings/display/panel/toppoly,td028ttec1.txt rename to Documentation/devicetree/bindings/display/panel/tpo,td028ttec1.txt index 7175dc3740ac..ed34253d9fb1 100644 --- a/Documentation/devicetree/bindings/display/panel/toppoly,td028ttec1.txt +++ b/Documentation/devicetree/bindings/display/panel/tpo,td028ttec1.txt @@ -2,7 +2,7 @@ Toppoly TD028TTEC1 Panel ======================== Required properties: -- compatible: "toppoly,td028ttec1" +- compatible: "tpo,td028ttec1" Optional properties: - label: a symbolic name for the panel @@ -14,7 +14,7 @@ Example ------- lcd-panel: td028ttec1@0 { - compatible = "toppoly,td028ttec1"; + compatible = "tpo,td028ttec1"; reg = <0>; spi-max-frequency = <100000>; spi-cpol; diff --git a/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c b/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c index 0a38a0e8c925..2dab491478c2 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c @@ -452,7 +452,7 @@ static int td028ttec1_panel_remove(struct spi_device *spi) } static const struct of_device_id td028ttec1_of_match[] = { - { .compatible = "omapdss,toppoly,td028ttec1", }, + { .compatible = "omapdss,tpo,td028ttec1", }, {}, }; @@ -471,7 +471,7 @@ static struct spi_driver td028ttec1_spi_driver = { module_spi_driver(td028ttec1_spi_driver); -MODULE_ALIAS("spi:toppoly,td028ttec1"); +MODULE_ALIAS("spi:tpo,td028ttec1"); MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>"); MODULE_DESCRIPTION("Toppoly TD028TTEC1 panel driver"); MODULE_LICENSE("GPL"); diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c index 57e9e146ff74..b67b324e9919 100644 --- a/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c +++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c @@ -455,6 +455,8 @@ static int td028ttec1_panel_remove(struct spi_device *spi) } static const struct of_device_id td028ttec1_of_match[] = { + { .compatible = "omapdss,tpo,td028ttec1", }, + /* keep to not break older DTB */ { .compatible = "omapdss,toppoly,td028ttec1", }, {}, }; @@ -474,7 +476,7 @@ static struct spi_driver td028ttec1_spi_driver = { module_spi_driver(td028ttec1_spi_driver); -MODULE_ALIAS("spi:toppoly,td028ttec1"); +MODULE_ALIAS("spi:tpo,td028ttec1"); MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>"); MODULE_DESCRIPTION("Toppoly TD028TTEC1 panel driver"); MODULE_LICENSE("GPL");
The vendor name was "toppoly" but other panels and the vendor list have defined it as "tpo". So let's fix it in driver and bindings. Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- .../display/panel/{toppoly,td028ttec1.txt => tpo,td028ttec1.txt} | 4 ++-- drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 4 ++-- drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c | 4 +++- 3 files changed, 7 insertions(+), 5 deletions(-) rename Documentation/devicetree/bindings/display/panel/{toppoly,td028ttec1.txt => tpo,td028ttec1.txt} (84%)