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

[Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>]

Hi Stéphan and Guenter,

On Fri, 22 Jan 2016 23:00:57 +0100, Stéphan Kochen wrote:
> 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.

Yes, thanks Guenter for the initial review.

> 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?

Anything will be better than macro-generated functions.

> 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?

Can't answer that as long as I don't know which problem you are trying
to solve. Hopefully this will become clearer once the changes are
cleanly separated into incremental steps.

-- 
Jean Delvare
SUSE L3 Support
--
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