On Friday, February 10, 2017 9:21:06 AM CET Guenter Roeck wrote: > On Fri, Feb 10, 2017 at 05:12:30PM +0100, Christian Lamparter wrote: > > This patch integrates the LOCAL, REMOTE and REMOTE2 > > channel definitions into the lm90.c driver. > > > > Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx> > > --- > > This is an optional patch to showcase how the channel definition > > in the dt-bindings are mapped into the driver. > > In theory, this makes it possible to easily remap the channel > > indices. However, it does make the driver harder to read. > > It also makes the driver dependent on external defines which are not controlled > by the driver. If anyone changes those defines to be non-sequential or to not > start with 0, we would be in trouble. Sure, that might and likely would result > in compile errors, but still ... Yes, gcc will complain with "array out of bounds errors" if any of the LM90_SENSORS_ defines are less than 0 or higher than 2. This is because of: static const u8 lm90_temp_index[3] and lm90_temp_min_index, ... The BUILD_BUG_ON(LOCAL == REMOTE || ... || REMOTE2 == LOCAL) will prevent duplicated values so LOCAL, REMOTE, REMOTE2 have to be different. > Besides, it is not complete. Anyone changing channel index values would > (at least) mess up alarm bit association. Yes, that's true. I missed lm90_is_tripped. But... > If we want to do that kind of thing, it might make more sense to add some code > to provide the desired mapping to the hwmon core, and to let the hwmon core > handle it. No idea if that is even possible, though. > > Is that really worth it ? No, it's not worth it ;-). But thank you for your in-depth analysis. So, let's leave it with just the first patch for 4.12-ish. Regards, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
