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