Messages in this thread | | | Subject | Re: [PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups | From | Guenter Roeck <> | Date | Wed, 4 Jul 2018 14:42:56 -0700 |
| |
On 07/04/2018 09:53 AM, Shilpasri G Bhat wrote: > Hi Guenter, > > Thanks for reviewing the patch. > On 07/04/2018 08:16 PM, Guenter Roeck wrote: >>> + /* Disable if last sensor in the group */ >>> + send_command = true; >>> + for (i = 0; i < sg->nr_sensor; i++) { >>> + struct sensor_data *sd = sg->sensors[i]; >>> + >>> + if (sd->enable) { >>> + send_command = false; >>> + break; >>> + } >> >> This is weird. So there are situations where a request to disable >> a sensor is accepted, but effectively ignored ? Shouldn't that >> return, say, -EBUSY ? > > This is because we do not support per-sensor enable/disable. We can only > enable/disable at a sensor-group level. > > This patch follows the semantic to disable a sensor group iff all the sensors > belonging to that group have been disabled. Otherwise the sensor alone is marked > to be disabled and returns -ENODATA on reading it. > > And a sensor group will be enabled if any of the sensor in that group is enabled. >
In similar situations, where setting one attribute affects others, a common solution is to make only the first attribute writable and have it affect all the others. I think that would make sense here as well, and it would be much simpler to implement.
Guenter
> I will make changes to the remaining code according to your suggestion. > > Thanks and Regards, > Shilpa > > -- > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
| |