[PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[Eugene Shalygin]

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;
 }
 
 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;
 
-- 
2.35.1

Re: [PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[Guenter Roeck]

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

Re: [PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[Eugene Shalygin]

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

Re: [PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[Guenter Roeck]

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

Re: [PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[Eugene Shalygin]

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

Re: [PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[Guenter Roeck]

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

Re: [PATCH] hwmon: (asus-ec-sensors) do not print from .probe()

[Eugene Shalygin]

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