Messages in this thread | | | Date | Fri, 17 Apr 2015 10:45:04 -0400 (EDT) | From | Vivien Didelot <> | Subject | Re: [PATCH 2/2] net: dsa: register hwmon for any provided function |
| |
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
| |