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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
