[PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Michael Ellerman]

Because we build kernels with drivers built in for many platforms, it's
normal for the ibmpowernv driver to be loaded on systems that don't have
the appropriate hardware.

Currently the driver spams the log with:

  ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
  ibmpowernv: Platfrom driver probe failed

But there is no error, this machine is not a powernv and doesn't have
the hardware. So change the sensors message to dev_dbg(), and only print
an error about the probe failing if it's not ENODEV.

Also fix the spelling of "Platfrom" and print the actual error value.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 drivers/hwmon/ibmpowernv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index d2bf2c97ae70..6a30eeea94be 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct platform_device *pdev)
 
 	opal = of_find_node_by_path("/ibm,opal/sensors");
 	if (!opal) {
-		dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
+		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
 		return -ENODEV;
 	}
 
@@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
 
 	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
 	if (err) {
-		pr_err("Platfrom driver probe failed\n");
+		if (err != -ENODEV)
+			pr_err("Platform driver probe failed (%d)\n", err);
+
 		goto exit_device_del;
 	}
 
-- 
1.9.1

[PATCH 2/2] hwmon: (ibmpowernv) Make the driver name more recognisable

[Michael Ellerman]

Seeing "ibmpowernv" in dmesg is not very useful, that is just the name
of the platform and doesn't identify the message as coming from the
hwmon driver.

Change DRVNAME to "powernv-hwmon" to make it clearer.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 drivers/hwmon/ibmpowernv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index 6a30eeea94be..3ee00928cda9 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -16,7 +16,7 @@
  * along with this program.
  */
 
-#define DRVNAME		"ibmpowernv"
+#define DRVNAME		"powernv-hwmon"
 #define pr_fmt(fmt)	DRVNAME ": " fmt
 
 #include <linux/init.h>
-- 
1.9.1

Re: [PATCH 2/2] hwmon: (ibmpowernv) Make the driver name more recognisable

[Jean Delvare]

Hi Michael,

On Fri, 31 Oct 2014 17:45:23 +1100, Michael Ellerman wrote:
> Seeing "ibmpowernv" in dmesg is not very useful, that is just the name
> of the platform and doesn't identify the message as coming from the
> hwmon driver.
> 
> Change DRVNAME to "powernv-hwmon" to make it clearer.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  drivers/hwmon/ibmpowernv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index 6a30eeea94be..3ee00928cda9 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -16,7 +16,7 @@
>   * along with this program.
>   */
>  
> -#define DRVNAME		"ibmpowernv"
> +#define DRVNAME		"powernv-hwmon"
>  #define pr_fmt(fmt)	DRVNAME ": " fmt
>  
>  #include <linux/init.h>

Nack. For one thing, DRVNAME is used for
devm_hwmon_device_register_with_groups and dashes are not allowed in
the name attribute. For another, making the driver name different from
the module name makes things more confusing, not clearer.

Like it or not, almost no hwmon driver has hwmon in its name anyway.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Jean Delvare]

Hi Michael,

On Fri, 31 Oct 2014 17:45:22 +1100, Michael Ellerman wrote:
> Because we build kernels with drivers built in for many platforms, it's
> normal for the ibmpowernv driver to be loaded on systems that don't have
> the appropriate hardware.
> 
> Currently the driver spams the log with:
> 
>   ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
>   ibmpowernv: Platfrom driver probe failed
> 
> But there is no error, this machine is not a powernv and doesn't have
> the hardware. So change the sensors message to dev_dbg(), and only print
> an error about the probe failing if it's not ENODEV.
> 
> Also fix the spelling of "Platfrom" and print the actual error value.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  drivers/hwmon/ibmpowernv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index d2bf2c97ae70..6a30eeea94be 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct platform_device *pdev)
>  
>  	opal = of_find_node_by_path("/ibm,opal/sensors");
>  	if (!opal) {
> -		dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
> +		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
>  		return -ENODEV;
>  	}
>  
> @@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
>  
>  	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
>  	if (err) {
> -		pr_err("Platfrom driver probe failed\n");
> +		if (err != -ENODEV)
> +			pr_err("Platform driver probe failed (%d)\n", err);
> +
>  		goto exit_device_del;
>  	}
>  

Looks reasonable.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] hwmon: (ibmpowernv) Make the driver name more recognisable

[Guenter Roeck]

On 10/30/2014 11:45 PM, Michael Ellerman wrote:
> Seeing "ibmpowernv" in dmesg is not very useful, that is just the name
> of the platform and doesn't identify the message as coming from the
> hwmon driver.
>
> Change DRVNAME to "powernv-hwmon" to make it clearer.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   drivers/hwmon/ibmpowernv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index 6a30eeea94be..3ee00928cda9 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -16,7 +16,7 @@
>    * along with this program.
>    */
>
> -#define DRVNAME		"ibmpowernv"
> +#define DRVNAME		"powernv-hwmon"
>   #define pr_fmt(fmt)	DRVNAME ": " fmt
>
>   #include <linux/init.h>
>
That results in a sensor name that includes "hwmon" and a dash.
The first doesn't make sense, the second is illegal and will be
rejected by the hwmon subsystem.

Nack.

Guenter

Re: [PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Guenter Roeck]

On 10/30/2014 11:45 PM, Michael Ellerman wrote:
> Because we build kernels with drivers built in for many platforms, it's
> normal for the ibmpowernv driver to be loaded on systems that don't have
> the appropriate hardware.
>
> Currently the driver spams the log with:
>
>    ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
>    ibmpowernv: Platfrom driver probe failed
>
> But there is no error, this machine is not a powernv and doesn't have
> the hardware. So change the sensors message to dev_dbg(), and only print
> an error about the probe failing if it's not ENODEV.
>

Then it should be built as module and not be loaded. Why is it loaded
on your platform ?

Guenter

> Also fix the spelling of "Platfrom" and print the actual error value.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   drivers/hwmon/ibmpowernv.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index d2bf2c97ae70..6a30eeea94be 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct platform_device *pdev)
>
>   	opal = of_find_node_by_path("/ibm,opal/sensors");
>   	if (!opal) {
> -		dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
> +		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
>   		return -ENODEV;
>   	}
>
> @@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
>
>   	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
>   	if (err) {
> -		pr_err("Platfrom driver probe failed\n");
> +		if (err != -ENODEV)
> +			pr_err("Platform driver probe failed (%d)\n", err);
> +
>   		goto exit_device_del;
>   	}
>
>

Re: [PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Guenter Roeck]

On 10/31/2014 02:41 AM, Jean Delvare wrote:
> Hi Michael,
>
> On Fri, 31 Oct 2014 17:45:22 +1100, Michael Ellerman wrote:
>> Because we build kernels with drivers built in for many platforms, it's
>> normal for the ibmpowernv driver to be loaded on systems that don't have
>> the appropriate hardware.
>>
>> Currently the driver spams the log with:
>>
>>    ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
>>    ibmpowernv: Platfrom driver probe failed
>>
>> But there is no error, this machine is not a powernv and doesn't have
>> the hardware. So change the sensors message to dev_dbg(), and only print
>> an error about the probe failing if it's not ENODEV.
>>
>> Also fix the spelling of "Platfrom" and print the actual error value.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>   drivers/hwmon/ibmpowernv.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>> index d2bf2c97ae70..6a30eeea94be 100644
>> --- a/drivers/hwmon/ibmpowernv.c
>> +++ b/drivers/hwmon/ibmpowernv.c
>> @@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct platform_device *pdev)
>>
>>   	opal = of_find_node_by_path("/ibm,opal/sensors");
>>   	if (!opal) {
>> -		dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
>> +		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
>>   		return -ENODEV;
>>   	}
>>
>> @@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
>>
>>   	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
>>   	if (err) {
>> -		pr_err("Platfrom driver probe failed\n");
>> +		if (err != -ENODEV)
>> +			pr_err("Platform driver probe failed (%d)\n", err);
>>
>>   		goto exit_device_del;
>>   	}
>>
>
> Looks reasonable.
>
The reduced noise, ok, but a better fix would be to not attempt to load
the driver in the first place. This should be doable by introducing an
of_match_table.

Guenter

> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>

Re: [PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Guenter Roeck]

On Fri, Oct 31, 2014 at 05:45:22PM +1100, Michael Ellerman wrote:
> Because we build kernels with drivers built in for many platforms, it's
> normal for the ibmpowernv driver to be loaded on systems that don't have
> the appropriate hardware.
> 
> Currently the driver spams the log with:
> 
>   ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
>   ibmpowernv: Platfrom driver probe failed
> 
> But there is no error, this machine is not a powernv and doesn't have
> the hardware. So change the sensors message to dev_dbg(), and only print
> an error about the probe failing if it's not ENODEV.
> 
> Also fix the spelling of "Platfrom" and print the actual error value.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Neelesh,

is there a compatible property in the powernv devicetree which can be
used to identify the system ?

I am ok with applying this patch for the time being, but it would be better
if we can change the driver to only instantiate when actually needed.

Thanks,
Guenter

> ---
>  drivers/hwmon/ibmpowernv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index d2bf2c97ae70..6a30eeea94be 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct platform_device *pdev)
>  
>  	opal = of_find_node_by_path("/ibm,opal/sensors");
>  	if (!opal) {
> -		dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
> +		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
>  		return -ENODEV;
>  	}
>  
> @@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
>  
>  	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
>  	if (err) {
> -		pr_err("Platfrom driver probe failed\n");
> +		if (err != -ENODEV)
> +			pr_err("Platform driver probe failed (%d)\n", err);
> +
>  		goto exit_device_del;
>  	}
>  
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Neelesh Gupta]

On 10/31/2014 06:51 PM, Guenter Roeck wrote:
> On 10/31/2014 02:41 AM, Jean Delvare wrote:
>> Hi Michael,
>>
>> On Fri, 31 Oct 2014 17:45:22 +1100, Michael Ellerman wrote:
>>> Because we build kernels with drivers built in for many platforms, it's
>>> normal for the ibmpowernv driver to be loaded on systems that don't 
>>> have
>>> the appropriate hardware.
>>>
>>> Currently the driver spams the log with:
>>>
>>>    ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
>>>    ibmpowernv: Platfrom driver probe failed
>>>
>>> But there is no error, this machine is not a powernv and doesn't have
>>> the hardware. So change the sensors message to dev_dbg(), and only 
>>> print
>>> an error about the probe failing if it's not ENODEV.
>>>
>>> Also fix the spelling of "Platfrom" and print the actual error value.
>>>
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>>   drivers/hwmon/ibmpowernv.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>> index d2bf2c97ae70..6a30eeea94be 100644
>>> --- a/drivers/hwmon/ibmpowernv.c
>>> +++ b/drivers/hwmon/ibmpowernv.c
>>> @@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct 
>>> platform_device *pdev)
>>>
>>>       opal = of_find_node_by_path("/ibm,opal/sensors");
>>>       if (!opal) {
>>> -        dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
>>> +        dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
>>>           return -ENODEV;
>>>       }
>>>
>>> @@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
>>>
>>>       err = platform_driver_probe(&ibmpowernv_driver, 
>>> ibmpowernv_probe);
>>>       if (err) {
>>> -        pr_err("Platfrom driver probe failed\n");
>>> +        if (err != -ENODEV)
>>> +            pr_err("Platform driver probe failed (%d)\n", err);
>>>
>>>           goto exit_device_del;
>>>       }
>>>
>>
>> Looks reasonable.
>>
> The reduced noise, ok, but a better fix would be to not attempt to load
> the driver in the first place. This should be doable by introducing an
> of_match_table.

The parent 'sensors' node doesn't have a compatible property, rather 
individual
child sensors have. So, instead we can introduce 
'platform_driver.id_table' for
the probe.

Thanks,
Neelesh

>
> Guenter
>
>> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>>
>

Re: [PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Guenter Roeck]

On 11/01/2014 10:48 AM, Neelesh Gupta wrote:
>
> On 10/31/2014 11:42 PM, Guenter Roeck wrote:
>> On Fri, Oct 31, 2014 at 05:45:22PM +1100, Michael Ellerman wrote:
>>> Because we build kernels with drivers built in for many platforms, it's
>>> normal for the ibmpowernv driver to be loaded on systems that don't have
>>> the appropriate hardware.
>>>
>>> Currently the driver spams the log with:
>>>
>>>    ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
>>>    ibmpowernv: Platfrom driver probe failed
>>>
>>> But there is no error, this machine is not a powernv and doesn't have
>>> the hardware. So change the sensors message to dev_dbg(), and only print
>>> an error about the probe failing if it's not ENODEV.
>>>
>>> Also fix the spelling of "Platfrom" and print the actual error value.
>>>
>>> Signed-off-by: Michael Ellerman<mpe@ellerman.id.au>
>> Neelesh,
>>
>> is there a compatible property in the powernv devicetree which can be
>> used to identify the system ?
>
> I think the driver should not be loaded at first place on *non* powernv platforms.
> Still, we can improve by introducing 'platform_driver.id_table' and moving the
> platform device creation code in platform init 'opal-sensor.c' using
> of_platform_device_create() and just do driver register in module init.
>

Ok with me.

Can you write a patch doing that ?

Thanks,
Guenter

> - Neelesh
>
>>
>> I am ok with applying this patch for the time being, but it would be better
>> if we can change the driver to only instantiate when actually needed.
>>
>> Thanks,
>> Guenter
>>
>>> ---
>>>   drivers/hwmon/ibmpowernv.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>> index d2bf2c97ae70..6a30eeea94be 100644
>>> --- a/drivers/hwmon/ibmpowernv.c
>>> +++ b/drivers/hwmon/ibmpowernv.c
>>> @@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct platform_device *pdev)
>>>
>>>   	opal = of_find_node_by_path("/ibm,opal/sensors");
>>>   	if (!opal) {
>>> -		dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
>>> +		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
>>>   		return -ENODEV;
>>>   	}
>>>
>>> @@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
>>>
>>>   	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
>>>   	if (err) {
>>> -		pr_err("Platfrom driver probe failed\n");
>>> +		if (err != -ENODEV)
>>> +			pr_err("Platform driver probe failed (%d)\n", err);
>>> +
>>>   		goto exit_device_del;
>>>   	}
>>>
>>> --
>>> 1.9.1
>>>
>

Re: [PATCH 1/2] hwmon: (ibmpowernv) Quieten when probing finds no device

[Guenter Roeck]

On Fri, Oct 31, 2014 at 10:41:47AM +0100, Jean Delvare wrote:
> Hi Michael,
> 
> On Fri, 31 Oct 2014 17:45:22 +1100, Michael Ellerman wrote:
> > Because we build kernels with drivers built in for many platforms, it's
> > normal for the ibmpowernv driver to be loaded on systems that don't have
> > the appropriate hardware.
> > 
> > Currently the driver spams the log with:
> > 
> >   ibmpowernv ibmpowernv.0: Opal node 'sensors' not found
> >   ibmpowernv: Platfrom driver probe failed
> > 
> > But there is no error, this machine is not a powernv and doesn't have
> > the hardware. So change the sensors message to dev_dbg(), and only print
> > an error about the probe failing if it's not ENODEV.
> > 
> > Also fix the spelling of "Platfrom" and print the actual error value.
> > 
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> >  drivers/hwmon/ibmpowernv.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> > index d2bf2c97ae70..6a30eeea94be 100644
> > --- a/drivers/hwmon/ibmpowernv.c
> > +++ b/drivers/hwmon/ibmpowernv.c
> > @@ -181,7 +181,7 @@ static int __init populate_attr_groups(struct platform_device *pdev)
> >  
> >  	opal = of_find_node_by_path("/ibm,opal/sensors");
> >  	if (!opal) {
> > -		dev_err(&pdev->dev, "Opal node 'sensors' not found\n");
> > +		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
> >  		return -ENODEV;
> >  	}
> >  
> > @@ -335,7 +335,9 @@ static int __init ibmpowernv_init(void)
> >  
> >  	err = platform_driver_probe(&ibmpowernv_driver, ibmpowernv_probe);
> >  	if (err) {
> > -		pr_err("Platfrom driver probe failed\n");
> > +		if (err != -ENODEV)
> > +			pr_err("Platform driver probe failed (%d)\n", err);
> > +
> >  		goto exit_device_del;
> >  	}
> >  
> 
> Looks reasonable.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
Applied.

Neelesh, please base your patch on top of this one.

Thanks,
Guenter