diff mbox

[v2,1/4] omapdrm: fix compatible string for td028ttec1

Message ID 474bfc72b67e1d0e2a223084a69f04d2baa1dda8.1510822218.git.hns@goldelico.com (mailing list archive)
State New, archived
Headers show

Commit Message

H. Nikolaus Schaller Nov. 16, 2017, 8:50 a.m. UTC
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%)

Comments

Andrew Davis Nov. 16, 2017, 3:53 p.m. UTC | #1
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
>
H. Nikolaus Schaller Nov. 16, 2017, 4:10 p.m. UTC | #2
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
>>
Andrew Davis Nov. 16, 2017, 5:08 p.m. UTC | #3
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
>
H. Nikolaus Schaller Nov. 16, 2017, 6:18 p.m. UTC | #4
> 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
>>
Andrew Davis Nov. 16, 2017, 6:32 p.m. UTC | #5
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
>
H. Nikolaus Schaller Nov. 16, 2017, 6:56 p.m. UTC | #6
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
>>
H. Nikolaus Schaller Nov. 20, 2017, 5:36 a.m. UTC | #7
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
diff mbox

Patch

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");