[PATCH v2] hwmon: (tmp401) Add of_match_table

[PATCH v2] hwmon: (tmp401) Add of_match_table

[Camel Guo]

When tmp401 is built as kernel module, it won't be automatically loaded
even through there is a device node in the devicetree. e.g:
    i2c {
      #address-cells = <1>;
      #size-cells = <0>;

      sensor@4c {
        compatible = "ti,tmp401";
        reg = <0x4c>;
      };
    };
In order to make sure it is loaded automatically, this commit adds
of_match_table for tmp401.

Signed-off-by: Camel Guo <camel.guo@axis.com>
---

Notes:
    v2:
     - Put evidence and circumstances in commit message

 drivers/hwmon/tmp401.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
index b86d9df7105d..52c9e7d3f2ae 100644
--- a/drivers/hwmon/tmp401.c
+++ b/drivers/hwmon/tmp401.c
@@ -708,10 +708,21 @@ static int tmp401_probe(struct i2c_client *client)
 	return 0;
 }
 
+static const struct of_device_id __maybe_unused tmp4xx_of_match[] = {
+	{ .compatible = "ti,tmp401", },
+	{ .compatible = "ti,tmp411", },
+	{ .compatible = "ti,tmp431", },
+	{ .compatible = "ti,tmp432", },
+	{ .compatible = "ti,tmp435", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tmp4xx_of_match);
+
 static struct i2c_driver tmp401_driver = {
 	.class		= I2C_CLASS_HWMON,
 	.driver = {
 		.name	= "tmp401",
+		.of_match_table = of_match_ptr(tmp4xx_of_match),
 	},
 	.probe_new	= tmp401_probe,
 	.id_table	= tmp401_id,

base-commit: 38d741cb70b30741c0e802cbed7bd9cf4fd15fa4
-- 
2.30.2

Re: [PATCH v2] hwmon: (tmp401) Add of_match_table

[Guenter Roeck]

On 5/2/22 02:19, Camel Guo wrote:
> When tmp401 is built as kernel module, it won't be automatically loaded
> even through there is a device node in the devicetree. e.g:
>      i2c {
>        #address-cells = <1>;
>        #size-cells = <0>;
> 
>        sensor@4c {
>          compatible = "ti,tmp401";
>          reg = <0x4c>;
>        };
>      };
> In order to make sure it is loaded automatically, this commit adds
> of_match_table for tmp401.
> 

As mentioned before, historically i2c devices would instantiate based
on the i2c match table. You are claiming that this is no longer the case.
The above is no evidence; that would require a log output on an affected
system showing that the sensors are not or no longer longer instantiated.

I am not absolutely opposed to adding the nodes, but the explanation
needs to match reality. If you can not provide evidence from an actual
boot log, I'll have to implement a tmp401 device model in qemu and test
myself. That will take a while.

Guenter

Re: [PATCH v2] hwmon: (tmp401) Add of_match_table

[Vincent Whitchurch]

On Mon, May 02, 2022 at 03:57:50PM +0200, Guenter Roeck wrote:
> On 5/2/22 02:19, Camel Guo wrote:
> > When tmp401 is built as kernel module, it won't be automatically loaded
> > even through there is a device node in the devicetree. e.g:
> >      i2c {
> >        #address-cells = <1>;
> >        #size-cells = <0>;
> > 
> >        sensor@4c {
> >          compatible = "ti,tmp401";
> >          reg = <0x4c>;
> >        };
> >      };
> > In order to make sure it is loaded automatically, this commit adds
> > of_match_table for tmp401.
> > 
> 
> As mentioned before, historically i2c devices would instantiate based
> on the i2c match table. You are claiming that this is no longer the case.

Note that while the commit message in the first version of the patch did
wrongly claim that probe would not work without the of_match_table, this
corrected description in v2 does mention the actual problem: that the
module will not be automatically loaded without the of_match_table.  (If
the module is loaded manually or the driver is built-in to the kernel,
there is no problem.)

See commit 72fc64c68decf119466 ("hwmon: (tmp103) Add OF device ID
table") or commit 98b16a09861aa85d6 ("hwmon: (max31785) Add OF device ID
table") for similar changes to other hwmon drivers.

The potential future change mentioned in the commit messages of
72fc64c68decf119466 and 98b16a09861aa85d6 happened in commit
af503716ac1444db61d80 ("i2c: core: report OF style module alias for
devices registered via OF").  The commit message of
af503716ac1444db61d80 has a lot of details about the change being made,
and while it says that all in-tree drivers had been converted, it looks
like some of them, like tmp401, were missed.

> The above is no evidence; that would require a log output on an affected
> system showing that the sensors are not or no longer longer instantiated.

A log would simply show nothing happening so that's probably not going
to be that useful, but here is what the modaliases look like.  As you
can see, the modalias of the device in sysfs does not match any of the
alias patterns of the module without this patch:

$ cat /sys/bus/i2c/devices/4-004c/modalias
of:Ntemperature-sensorT<NULL>Cti,tmp431

modinfo without this patch:

$ modinfo ./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
filename:       /storage2/femfyra/linux-2.6/.roadtest/./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
license:        GPL
description:    Texas Instruments TMP401 temperature sensor driver
author:         Hans de Goede <hdegoede@redhat.com>
alias:          i2c:tmp435
alias:          i2c:tmp432
alias:          i2c:tmp431
alias:          i2c:tmp411
alias:          i2c:tmp401
depends:        
intree:         Y
name:           tmp401
vermagic:       5.18.0-rc1 mod_unload 

modinfo after this patch:

$ modinfo ./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
filename:       /storage2/femfyra/linux-2.6/./.roadtest/modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
license:        GPL
description:    Texas Instruments TMP401 temperature sensor driver
author:         Hans de Goede <hdegoede@redhat.com>
alias:          i2c:tmp435
alias:          i2c:tmp432
alias:          i2c:tmp431
alias:          i2c:tmp411
alias:          i2c:tmp401
alias:          of:N*T*Cti,tmp435C*
alias:          of:N*T*Cti,tmp435
alias:          of:N*T*Cti,tmp432C*
alias:          of:N*T*Cti,tmp432
alias:          of:N*T*Cti,tmp431C*
alias:          of:N*T*Cti,tmp431
alias:          of:N*T*Cti,tmp411C*
alias:          of:N*T*Cti,tmp411
alias:          of:N*T*Cti,tmp401C*
alias:          of:N*T*Cti,tmp401
depends:        
intree:         Y
name:           tmp401
vermagic:       5.18.0-rc1 mod_unload 

Re: [PATCH v2] hwmon: (tmp401) Add of_match_table

[Guenter Roeck]

On 5/2/22 07:58, Vincent Whitchurch wrote:
> On Mon, May 02, 2022 at 03:57:50PM +0200, Guenter Roeck wrote:
>> On 5/2/22 02:19, Camel Guo wrote:
>>> When tmp401 is built as kernel module, it won't be automatically loaded
>>> even through there is a device node in the devicetree. e.g:
>>>       i2c {
>>>         #address-cells = <1>;
>>>         #size-cells = <0>;
>>>
>>>         sensor@4c {
>>>           compatible = "ti,tmp401";
>>>           reg = <0x4c>;
>>>         };
>>>       };
>>> In order to make sure it is loaded automatically, this commit adds
>>> of_match_table for tmp401.
>>>
>>
>> As mentioned before, historically i2c devices would instantiate based
>> on the i2c match table. You are claiming that this is no longer the case.
> 
> Note that while the commit message in the first version of the patch did
> wrongly claim that probe would not work without the of_match_table, this
> corrected description in v2 does mention the actual problem: that the
> module will not be automatically loaded without the of_match_table.  (If
> the module is loaded manually or the driver is built-in to the kernel,
> there is no problem.)
> 

No, it doesn't. None of the information you provided below is mentioned
in the description, but is essential to understand your patch and the
reason for it.

> See commit 72fc64c68decf119466 ("hwmon: (tmp103) Add OF device ID
> table") or commit 98b16a09861aa85d6 ("hwmon: (max31785) Add OF device ID
> table") for similar changes to other hwmon drivers.
> 

Those commits provide a valid and acceptable explanation.

> The potential future change mentioned in the commit messages of
> 72fc64c68decf119466 and 98b16a09861aa85d6 happened in commit
> af503716ac1444db61d80 ("i2c: core: report OF style module alias for
> devices registered via OF").  The commit message of
> af503716ac1444db61d80 has a lot of details about the change being made,
> and while it says that all in-tree drivers had been converted, it looks
> like some of them, like tmp401, were missed.
> 

And this is the missing link. If you provide that information
in the commit log I have no problems. Please also provide a Fixes:
tag.

Fixes: af503716ac14 ("i2c: core: report OF style module alias for devices registered via OF")

Thanks,
Guenter

>> The above is no evidence; that would require a log output on an affected
>> system showing that the sensors are not or no longer longer instantiated.
> 
> A log would simply show nothing happening so that's probably not going
> to be that useful, but here is what the modaliases look like.  As you
> can see, the modalias of the device in sysfs does not match any of the
> alias patterns of the module without this patch:
> 
> $ cat /sys/bus/i2c/devices/4-004c/modalias
> of:Ntemperature-sensorT<NULL>Cti,tmp431
> 
> modinfo without this patch:
> 
> $ modinfo ./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
> filename:       /storage2/femfyra/linux-2.6/.roadtest/./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
> license:        GPL
> description:    Texas Instruments TMP401 temperature sensor driver
> author:         Hans de Goede <hdegoede@redhat.com>
> alias:          i2c:tmp435
> alias:          i2c:tmp432
> alias:          i2c:tmp431
> alias:          i2c:tmp411
> alias:          i2c:tmp401
> depends:
> intree:         Y
> name:           tmp401
> vermagic:       5.18.0-rc1 mod_unload
> 
> modinfo after this patch:
> 
> $ modinfo ./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
> filename:       /storage2/femfyra/linux-2.6/./.roadtest/modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
> license:        GPL
> description:    Texas Instruments TMP401 temperature sensor driver
> author:         Hans de Goede <hdegoede@redhat.com>
> alias:          i2c:tmp435
> alias:          i2c:tmp432
> alias:          i2c:tmp431
> alias:          i2c:tmp411
> alias:          i2c:tmp401
> alias:          of:N*T*Cti,tmp435C*
> alias:          of:N*T*Cti,tmp435
> alias:          of:N*T*Cti,tmp432C*
> alias:          of:N*T*Cti,tmp432
> alias:          of:N*T*Cti,tmp431C*
> alias:          of:N*T*Cti,tmp431
> alias:          of:N*T*Cti,tmp411C*
> alias:          of:N*T*Cti,tmp411
> alias:          of:N*T*Cti,tmp401C*
> alias:          of:N*T*Cti,tmp401
> depends:
> intree:         Y
> name:           tmp401
> vermagic:       5.18.0-rc1 mod_unload

Re: [PATCH v2] hwmon: (tmp401) Add of_match_table

[Camel Guo]

On 5/2/22 18:16, Guenter Roeck wrote:
> On 5/2/22 07:58, Vincent Whitchurch wrote:
>> On Mon, May 02, 2022 at 03:57:50PM +0200, Guenter Roeck wrote:
>>> On 5/2/22 02:19, Camel Guo wrote:
>>>> When tmp401 is built as kernel module, it won't be automatically loaded
>>>> even through there is a device node in the devicetree. e.g:
>>>>       i2c {
>>>>         #address-cells = <1>;
>>>>         #size-cells = <0>;
>>>>
>>>>         sensor@4c {
>>>>           compatible = "ti,tmp401";
>>>>           reg = <0x4c>;
>>>>         };
>>>>       };
>>>> In order to make sure it is loaded automatically, this commit adds
>>>> of_match_table for tmp401.
>>>>
>>>
>>> As mentioned before, historically i2c devices would instantiate based
>>> on the i2c match table. You are claiming that this is no longer the case.
>> 
>> Note that while the commit message in the first version of the patch did
>> wrongly claim that probe would not work without the of_match_table, this
>> corrected description in v2 does mention the actual problem: that the
>> module will not be automatically loaded without the of_match_table.  (If
>> the module is loaded manually or the driver is built-in to the kernel,
>> there is no problem.)
>> 
> 
> No, it doesn't. None of the information you provided below is mentioned
> in the description, but is essential to understand your patch and the
> reason for it.
> 
>> See commit 72fc64c68decf119466 ("hwmon: (tmp103) Add OF device ID
>> table") or commit 98b16a09861aa85d6 ("hwmon: (max31785) Add OF device ID
>> table") for similar changes to other hwmon drivers.
>> 
> 
> Those commits provide a valid and acceptable explanation.

Now, I just copied this commit message.

> 
>> The potential future change mentioned in the commit messages of
>> 72fc64c68decf119466 and 98b16a09861aa85d6 happened in commit
>> af503716ac1444db61d80 ("i2c: core: report OF style module alias for
>> devices registered via OF").  The commit message of
>> af503716ac1444db61d80 has a lot of details about the change being made,
>> and while it says that all in-tree drivers had been converted, it looks
>> like some of them, like tmp401, were missed.
>> 
> 
> And this is the missing link. If you provide that information
> in the commit log I have no problems. Please also provide a Fixes:
> tag.
> 
> Fixes: af503716ac14 ("i2c: core: report OF style module alias for 
> devices registered via OF")

It is added in v3

> 
> Thanks,
> Guenter
> 
>>> The above is no evidence; that would require a log output on an affected
>>> system showing that the sensors are not or no longer longer instantiated.
>> 
>> A log would simply show nothing happening so that's probably not going
>> to be that useful, but here is what the modaliases look like.  As you
>> can see, the modalias of the device in sysfs does not match any of the
>> alias patterns of the module without this patch:
>> 
>> $ cat /sys/bus/i2c/devices/4-004c/modalias
>> of:Ntemperature-sensorT<NULL>Cti,tmp431
>> 
>> modinfo without this patch:
>> 
>> $ modinfo ./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
>> filename:       /storage2/femfyra/linux-2.6/.roadtest/./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
>> license:        GPL
>> description:    Texas Instruments TMP401 temperature sensor driver
>> author:         Hans de Goede <hdegoede@redhat.com>
>> alias:          i2c:tmp435
>> alias:          i2c:tmp432
>> alias:          i2c:tmp431
>> alias:          i2c:tmp411
>> alias:          i2c:tmp401
>> depends:
>> intree:         Y
>> name:           tmp401
>> vermagic:       5.18.0-rc1 mod_unload
>> 
>> modinfo after this patch:
>> 
>> $ modinfo ./modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
>> filename:       /storage2/femfyra/linux-2.6/./.roadtest/modules/lib/modules/5.18.0-rc1/kernel/drivers/hwmon/tmp401.ko
>> license:        GPL
>> description:    Texas Instruments TMP401 temperature sensor driver
>> author:         Hans de Goede <hdegoede@redhat.com>
>> alias:          i2c:tmp435
>> alias:          i2c:tmp432
>> alias:          i2c:tmp431
>> alias:          i2c:tmp411
>> alias:          i2c:tmp401
>> alias:          of:N*T*Cti,tmp435C*
>> alias:          of:N*T*Cti,tmp435
>> alias:          of:N*T*Cti,tmp432C*
>> alias:          of:N*T*Cti,tmp432
>> alias:          of:N*T*Cti,tmp431C*
>> alias:          of:N*T*Cti,tmp431
>> alias:          of:N*T*Cti,tmp411C*
>> alias:          of:N*T*Cti,tmp411
>> alias:          of:N*T*Cti,tmp401C*
>> alias:          of:N*T*Cti,tmp401
>> depends:
>> intree:         Y
>> name:           tmp401
>> vermagic:       5.18.0-rc1 mod_unload
> 

All commits have been fixed in v3, please review that one instead.