Re: [PATCH 1/3] lm90: separate register accessors from sysfs

["Stéphan Kochen" <stephan-j6uo2F6POYhmR6Xm/wNWPw@public.gmane.org>]

Guenter,

Thanks for the taking the time to review!

You are absolutely right. I will split this up, more carefully check
coding style and simply do away with unnecessary changes.

Regarding:

On Fri, Jan 22, 2016 at 06:42:26AM -0800, Guenter Roeck wrote:
> On 01/21/2016 11:34 AM, Stéphan Kochen wrote:
> >+#define SELECT_REGS(_reg, _channel) { \
> >+	high = LM90_REG_W_##_reg##H; \
> >+	low = LM90_REG_W_##_reg##L; \
> >+	channel = _channel; \
> >+}
> 
> We have been trying to get rid of function defines such as this one.
> Most of the time it increases code size, and reduces readability.
> Plus, it increases the amount of time we have to spend reviewing
> the code to ensure it is correct.
> 
> >+	switch (index) {
> >+	case REMOTE_LOW:    SELECT_REGS(REMOTE_LOW,  0); break;
> >+	case REMOTE_HIGH:   SELECT_REGS(REMOTE_HIGH, 0); break;
> >+	case REMOTE_OFFSET: SELECT_REGS(REMOTE_OFFS, 0); break;
> >+	case REMOTE2_LOW:   SELECT_REGS(REMOTE_LOW,  1); break;
> >+	case REMOTE2_HIGH:  SELECT_REGS(REMOTE_HIGH, 1); break;
> >+	default: return;
> 
> ... and, in this case, introduces checkpatch errors.

Should I simply unfold the macro here? Another option would be an array
of structs like in set_temp8, but it'd have some gaps here. Or is some
other construct preferred?

I guess this is where I snuck in another change (which I will separate),
to replace SENSOR_DEVICE_ATTR_2 with regular ATTR. The setter args of
_ATTR_2 were using literal numbers to point into an array defined in the
setter function. (Maybe to avoid exactly the kind of thing I built
here.)

Would it be an idea to combine temp8 and temp11 variants, and let the
getters and setters figure out what kind of register access to do based
on register index?

Thanks again,

-- 
Stéphan Kochen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html