| Message ID | 20220217072340.2472987-1-eugene.shalygin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (asus-ec-sensors) do not print from .probe() | expand |
On 2/16/22 23:23, Eugene Shalygin wrote: > Remove the call to dev_info() from the board detection function, which > is called from probe(), not only to be in line with hwmon driver rules, but > also because the message duplicates the error code returned from probe() > for that case (ENODEV). > > Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com> > --- > drivers/hwmon/asus-ec-sensors.c | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c > index 0701ade16227..cbe1b987144a 100644 > --- a/drivers/hwmon/asus-ec-sensors.c > +++ b/drivers/hwmon/asus-ec-sensors.c > @@ -597,18 +597,11 @@ static struct hwmon_chip_info asus_ec_chip_info = { > .ops = &asus_ec_hwmon_ops, > }; > > -static unsigned long __init > -get_board_sensors(const struct device *dev) > +static unsigned long __init get_board_sensors(void) > { > - const struct dmi_system_id *dmi_entry; > - > - dmi_entry = dmi_first_match(asus_ec_dmi_table); > - if (!dmi_entry) { > - dev_info(dev, "Unsupported board"); > - return 0; > - } > - > - return (unsigned long)dmi_entry->driver_data; > + const struct dmi_system_id *dmi_entry = > + dmi_first_match(asus_ec_dmi_table); > + return dmi_entry ? (unsigned long)dmi_entry->driver_data : 0; Looks like you did not run checkpatch. Either case, I think you should just drop this function In probe: const struct dmi_system_id *dmi_entry = dmi_first_match(asus_ec_dmi_table); ... if (!dmi_entry) return -ENODEV; ... ec_data->board_sensors = (unsigned long)dmi_entry->driver_data; Guenter > } > > static int __init asus_ec_probe(struct platform_device *pdev) > @@ -625,7 +618,7 @@ static int __init asus_ec_probe(struct platform_device *pdev) > struct device *hwdev; > unsigned int i; > > - board_sensors = get_board_sensors(dev); > + board_sensors = get_board_sensors(); > if (!board_sensors) > return -ENODEV; >
On Thu, 17 Feb 2022 at 19:26, Guenter Roeck <linux@roeck-us.net> wrote: > Looks like you did not run checkpatch. I did (0 errors/warnings/checks). What needs to be corrected? > Either case, I think you should just drop this function In probe: Yes, currently that function is tiny, but some tests with motherboards from other families are done by users and if I add other families, the information required for each board model will grow and in that case I'd switch from dmi_system_id array to a custom struct to define all the board-related data at at the same place, and to save some space in the module binary, as unused parts of the dmi_system_id array already take a quarter of the total binary size. So, the function will likely get some more code soon. Regards, Eugene
On 2/17/22 10:43, Eugene Shalygin wrote: > On Thu, 17 Feb 2022 at 19:26, Guenter Roeck <linux@roeck-us.net> wrote: >> Looks like you did not run checkpatch. > > I did (0 errors/warnings/checks). What needs to be corrected? > Interesting. It appears that the continuation line in the declaration confuses it. Otherwise you would get: WARNING: Missing a blank line after declarations >> Either case, I think you should just drop this function In probe: > > Yes, currently that function is tiny, but some tests with motherboards > from other families are done by users and if I add other families, the > information required for each board model will grow and in that case > I'd switch from dmi_system_id array to a custom struct to define all > the board-related data at at the same place, and to save some space in > the module binary, as unused parts of the dmi_system_id array already > take a quarter of the total binary size. So, the function will likely > get some more code soon. > Hmm, ok. Wouldn't you still need some kind of dmi match ? Thanks, Guenter > Regards, > Eugene
On Thu, 17 Feb 2022 at 20:33, Guenter Roeck <linux@roeck-us.net> wrote: > > On 2/17/22 10:43, Eugene Shalygin wrote: > > On Thu, 17 Feb 2022 at 19:26, Guenter Roeck <linux@roeck-us.net> wrote: > >> Looks like you did not run checkpatch. > > > > I did (0 errors/warnings/checks). What needs to be corrected? > > > > Interesting. It appears that the continuation line in the declaration > confuses it. Otherwise you would get: > > WARNING: Missing a blank line after declarations Added in v2, thank you! > >> Either case, I think you should just drop this function In probe: > > > > Yes, currently that function is tiny, but some tests with motherboards > > from other families are done by users and if I add other families, the > > information required for each board model will grow and in that case > > I'd switch from dmi_system_id array to a custom struct to define all > > the board-related data at at the same place, and to save some space in > > the module binary, as unused parts of the dmi_system_id array already > > take a quarter of the total binary size. So, the function will likely > > get some more code soon. > > > > Hmm, ok. Wouldn't you still need some kind of dmi match ? Of course, just not via dmi_first_match(): https://github.com/zeule/asus-ec-sensors/blob/feature/prime-x470-pro-no-dmi/asus-ec-sensors.c#L787 Regards, Eugene
On 2/17/22 11:49, Eugene Shalygin wrote: > On Thu, 17 Feb 2022 at 20:33, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 2/17/22 10:43, Eugene Shalygin wrote: >>> On Thu, 17 Feb 2022 at 19:26, Guenter Roeck <linux@roeck-us.net> wrote: >>>> Looks like you did not run checkpatch. >>> >>> I did (0 errors/warnings/checks). What needs to be corrected? >>> >> >> Interesting. It appears that the continuation line in the declaration >> confuses it. Otherwise you would get: >> >> WARNING: Missing a blank line after declarations > > Added in v2, thank you! > >>>> Either case, I think you should just drop this function In probe: >>> >>> Yes, currently that function is tiny, but some tests with motherboards >>> from other families are done by users and if I add other families, the >>> information required for each board model will grow and in that case >>> I'd switch from dmi_system_id array to a custom struct to define all >>> the board-related data at at the same place, and to save some space in >>> the module binary, as unused parts of the dmi_system_id array already >>> take a quarter of the total binary size. So, the function will likely >>> get some more code soon. >>> >> >> Hmm, ok. Wouldn't you still need some kind of dmi match ? > > Of course, just not via dmi_first_match(): > https://github.com/zeule/asus-ec-sensors/blob/feature/prime-x470-pro-no-dmi/asus-ec-sensors.c#L787 > !strcmp(), and, yes, dmi functions can return NULL. Guenter
On Thu, 17 Feb 2022 at 22:25, Guenter Roeck <linux@roeck-us.net> wrote:
> !strcmp(), and, yes, dmi functions can return NULL.
Thanks!
Regards,
Eugene
diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c index 0701ade16227..cbe1b987144a 100644 --- a/drivers/hwmon/asus-ec-sensors.c +++ b/drivers/hwmon/asus-ec-sensors.c @@ -597,18 +597,11 @@ static struct hwmon_chip_info asus_ec_chip_info = { .ops = &asus_ec_hwmon_ops, }; -static unsigned long __init -get_board_sensors(const struct device *dev) +static unsigned long __init get_board_sensors(void) { - const struct dmi_system_id *dmi_entry; - - dmi_entry = dmi_first_match(asus_ec_dmi_table); - if (!dmi_entry) { - dev_info(dev, "Unsupported board"); - return 0; - } - - return (unsigned long)dmi_entry->driver_data; + const struct dmi_system_id *dmi_entry = + dmi_first_match(asus_ec_dmi_table); + return dmi_entry ? (unsigned long)dmi_entry->driver_data : 0; } static int __init asus_ec_probe(struct platform_device *pdev) @@ -625,7 +618,7 @@ static int __init asus_ec_probe(struct platform_device *pdev) struct device *hwdev; unsigned int i; - board_sensors = get_board_sensors(dev); + board_sensors = get_board_sensors(); if (!board_sensors) return -ENODEV;
Remove the call to dev_info() from the board detection function, which is called from probe(), not only to be in line with hwmon driver rules, but also because the message duplicates the error code returned from probe() for that case (ENODEV). Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com> --- drivers/hwmon/asus-ec-sensors.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)