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

[Guenter Roeck]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/