[PATCH v2 1/4] omapdrm: fix compatible string for td028ttec1
H. Nikolaus Schaller
hns at goldelico.com
Sun Nov 19 21:36:48 PST 2017
Hi Andrew,
> Am 16.11.2017 um 19:56 schrieb H. Nikolaus Schaller <hns at goldelico.com>:
>
> Hi Andrew,
>
>> Am 16.11.2017 um 19:32 schrieb Andrew F. Davis <afd at 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 at 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 at 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 at 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 at 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 at letux:~# lsmod | fgrep td028
panel_tpo_td028ttec1 16384 1
omapdss_base 16384 5 connector_analog_tv,encoder_opa362,panel_tpo_td028ttec1,omapdrm,omapdss
root at 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 at letux:~#
=> two MODULE_ALIASEs are possible.
2. without module_aliases
No display and:
root at letux:~# lsmod | fgrep td028
root at 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 at letux:~#
=> spi: entries are needed.
3. with MODULE_DEVICE_TABLE
Display is back and:
root at letux:~# lsmod | fgrep td028
panel_tpo_td028ttec1 16384 1
omapdss_base 16384 5 connector_analog_tv,encoder_opa362,panel_tpo_td028ttec1,omapdrm,omapdss
root at 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 at 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
More information about the linux-arm-kernel
mailing list