[2/2] net: dsa: register hwmon for any provided function

A switch driver may only provide one of the temperature limit accessors,
or the temperature alarm getter. So register the hwmon subsystem if any
of the related functions is provided.

Thus, check get_temp to set the visibility of temp1_input.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

On Thu, Apr 16, 2015 at 02:38:19PM -0400, Vivien Didelot wrote:
> A switch driver may only provide one of the temperature limit accessors,
> or the temperature alarm getter. So register the hwmon subsystem if any
> of the related functions is provided.
> 
> Thus, check get_temp to set the visibility of temp1_input.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  net/dsa/dsa.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 67d2983..6b68994 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -157,6 +157,10 @@ static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
>  	umode_t mode = 0;
>  
>  	switch (index) {
> +	case 0: /* temp1_input */
> +		if (drv->get_temp)
> +			mode |= S_IRUGO;

This should be mandatory. Sorry, I don't really understand what you are
trying to accomplish here.

Can you give me a real world example where a chip would support setting
a limit but not reading it ?

Thanks,
Guenter

> +		break;
>  	case 1: /* temp1_max */
>  		if (drv->get_temp_limit)
>  			mode |= S_IRUGO;
> @@ -310,7 +314,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  	 * register with hardware monitoring subsystem.
>  	 * Treat registration error as non-fatal and ignore it.
>  	 */
> -	if (drv->get_temp) {
> +	if (drv->get_temp || drv->get_temp_limit || drv->set_temp_limit ||
> +	    drv->get_temp_alarm) {
>  		const char *netname = netdev_name(dst->master_netdev);
>  		char hname[IFNAMSIZ + 1];
>  		int i, j;
> -- 
> 2.3.5
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Guenter,

> >          switch (index) {
> > +        case 0: /* temp1_input */
> > +                if (drv->get_temp)
> > +                        mode |= S_IRUGO;
> 
> This should be mandatory. Sorry, I don't really understand what you are
> trying to accomplish here.
> 
> Can you give me a real world example where a chip would support setting
> a limit but not reading it ?

I have no such example. I just did not see why this couldn't be allowed
(e.g. setting only set_temp_limit and get_temp_alarm looks fine to me).
But if you say that get_temp should be mandatory, I'm OK with that.

The primary goal of this patchset was to use DEVICE_ATTR_RW to declare
temp1_max, instead of reflecting the minimal permissions needed.

Best,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

hi Vivien,

On 04/16/2015 03:05 PM, Vivien Didelot wrote:
> Hi Guenter,
>
>>>           switch (index) {
>>> +        case 0: /* temp1_input */
>>> +                if (drv->get_temp)
>>> +                        mode |= S_IRUGO;
>>
>> This should be mandatory. Sorry, I don't really understand what you are
>> trying to accomplish here.
>>
>> Can you give me a real world example where a chip would support setting
>> a limit but not reading it ?
>
> I have no such example. I just did not see why this couldn't be allowed
> (e.g. setting only set_temp_limit and get_temp_alarm looks fine to me).
> But if you say that get_temp should be mandatory, I'm OK with that.
>
write-only attributes are not defined in the hwmon ABI. If the 'sensors'
command encounters such an attribute, it will create an error message
each time it executes. That doesn't sound very useful to me.

If a chip - for whatever reason - does not have a limit register
but an alarm register or flag, its temperature limit is usually hard-coded
and can be reported this way (the AMD temperature sensor driver does this,
for example). If there is ever a need to support the alarm-register-only
situation for some odd reason, we can add the code at the time.
For now, it just seems to me that you are adding complexity to solve
some theoretic problem which is very unlikely to occur in the real world.

> The primary goal of this patchset was to use DEVICE_ATTR_RW to declare
> temp1_max, instead of reflecting the minimal permissions needed.
>
Then why don't you just do that and nothing else ? The goal should be
to simplify code, not to make it more complicated. If the result isn't
less code, I don't think it is worth it.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Guenter,

> >>>           switch (index) {
> >>> +        case 0: /* temp1_input */
> >>> +                if (drv->get_temp)
> >>> +                        mode |= S_IRUGO;
> >>
> >> This should be mandatory. Sorry, I don't really understand what you
> >> are trying to accomplish here.
> >>
> >> Can you give me a real world example where a chip would support
> >> setting a limit but not reading it ?
> >
> > I have no such example. I just did not see why this couldn't be
> > allowed (e.g. setting only set_temp_limit and get_temp_alarm looks
> > fine to me).  But if you say that get_temp should be mandatory, I'm
> > OK with that.
> >
> write-only attributes are not defined in the hwmon ABI. If the
> 'sensors' command encounters such an attribute, it will create an
> error message each time it executes. That doesn't sound very useful to
> me.

Ok, good to know.

> If a chip - for whatever reason - does not have a limit register but
> an alarm register or flag, its temperature limit is usually hard-coded
> and can be reported this way (the AMD temperature sensor driver does
> this, for example). If there is ever a need to support the
> alarm-register-only situation for some odd reason, we can add the code
> at the time.  For now, it just seems to me that you are adding
> complexity to solve some theoretic problem which is very unlikely to
> occur in the real world.

You're right, this change is not necessary.

> > The primary goal of this patchset was to use DEVICE_ATTR_RW to
> > declare temp1_max, instead of reflecting the minimal permissions
> > needed.
> >
> Then why don't you just do that and nothing else ? The goal should be
> to simplify code, not to make it more complicated. If the result isn't
> less code, I don't think it is worth it.

Indeed, I'll rework this patch then.

Thanks for the review,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html