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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > 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.