> > it's
> > hard to understand which one relates to which register. Also, why not using
> > bitfield.h?
>
> I saw no need for it?
Hmm... Okay, I think Jonathan will ask that if really needed.
...
> > > + * These macros return the address of the 1.st reg for the given channel
> >
> > first
>
> Huh?
Replace "1.st" (which seems not a standard representation of it) by "first".
> > (and missing period at the end).
>
> Ok.
...
> > > +#define BD79124_GET_HIGH_LIMIT_REG(ch) (BD79124_REG_HYSTERESIS_CH0 + (ch) * 4)
> > > +#define BD79124_GET_LOW_LIMIT_REG(ch) (BD79124_REG_EVENTCOUNT_CH0 + (ch) * 4)
> > > +#define BD79124_GET_LIMIT_REG(ch, dir) ((dir) == IIO_EV_DIR_RISING ? \
> > > + BD79124_GET_HIGH_LIMIT_REG(ch) : BD79124_GET_LOW_LIMIT_REG(ch))
> > > +#define BD79124_GET_RECENT_RES_REG(ch) (BD79124_REG_RECENT_CH0_LSB + (ch) * 2)
> >
> > Don't we want to have something in bitfield.h for arrays in the register, i.e.
> > when register(s) is(are) split to 2+ bits per an element in an array of the
> > same semantics. Would be nice to eliminate such a boilerplate here and in many
> > other drivers.
>
> Not necessarily a bad idea. Still, I am not willing to expand this series
> any more - currently there is 10 patches, 2 of which are directly related to
> the BD79124. I don't want to delay this driver for another cycle due to
> added helpers.
It was just a side note. Consider as a poll for opinions.
...
> > > +static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)
> >
> > You should use .set_rv()
...
> > > +static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> > > + unsigned long *bits)
> >
> > Ditto, .set_multiple_rv().
>
> As you know, this started at v6.14 cycle which is still ongoing. I don't
> think .set_rv() and .set_multiple_rv() are available?
You mean that you are still hope to have this series to be squeezed for
v6.15-rc1? That's not me who decides, but if not, those APIs will be part of
the v6.15-rc1 (at least they are pending in GPIO subsystem).
...
> > > +struct bd79124_raw {
> > > + u8 bit0_3; /* Is set in high bits of the byte */
> > > + u8 bit4_11;
> > > +};
> > > +#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))
> >
> > And how this is different from treating it as __le16? Needs a good comment
> > about bit layout.
>
> You don't think:
> >> +struct bd79124_raw {
> >> + u8 bit0_3; /* Is set in high bits of the byte */
> >> + u8 bit4_11;
> >> +};
> suffices? It's hard for me to think how to explain bit layout more
> explicitly.
I do not think it suffices. It's hard to decode what you meant by the naming
and the comment. Actually it even confuses me.
> This was discussed during the RFC review. I explained the rationale why I
> rather represent this as two 8 bit variables than le16 with (mysterious to
> me) shift. As a result, Jonathan told me he's not feeling (too) strong about
> this (but also warned we may see follow-up patches converting this to le and
> shift - which, by the way, is harder for me to understand).
If you want to leave them, the good comment needs to be added for both
1) explaining bit layouts;
2) why __le16 is not being used.
...
> > > +static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
> > > + unsigned int val)
> > > +{
> > > + struct bd79124_raw raw;
> > > + int ret, tmp;
> >
> > > + raw.bit4_11 = (u8)(val >> 4);
> > > + raw.bit0_3 = (u8)(val << 4);
> >
> > Why casting?
>
> To make it clear storing unsigned int to u8 is considered to be Ok. I can
> drop it though if you feel strong about it.
In C (not C++) explicit casting is a point to stumble over and check if there
is any problems or call for asking the question "Whu?!"
...
> > > + ret = regmap_read(data->map, reg, &tmp);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + raw.bit0_3 |= (0xf & tmp);
> >
> > GENMASK() ?
>
> For me 0xf is both shorter and clearer. (For me 0xf - or, 0x0f if you like,
> 0xf0 and 0xff are clear by a glance).
>
> I can go for GENMASK() if it is absolutely required, but for me it is in
> this case just making this harder to read. I like GENMASK() for something
> like 0xe or 0x60 though.
It makes code inconsistent and one letter is not so visible. Ideally it should
be a definition with self-explanatory name.
> > > + return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
> > > +}
...
> > > + case IIO_EV_INFO_HYSTERESIS:
> > > + reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
> > > + val >>= 3;
> >
> > Second time I see this. Perhaps you need a macro to explain this right shift?
>
> Not really sure about this. I think it's kind of obvious why this is shifted
> - and in case it's not, there is a comment explaining it.
>
> I could have a macro like REGVAL2HYSTERESIS() - but I don't think it is a
> great idea, because then anyone interested in understanding the value read
> from register would need to dig out the macro to find how value needs to be
> converted. Doing the shift here shows the register format to a reader right
> away - and honestly, it should be pretty obvious to the reader that this
> shift converts register value to proper format.
These shifts are not a big issue, so whatever you choose / chose.
> > > + return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
> > > + val);
> >
> > Can be one line.
>
> I still prefer to have lines < 80 to make them fit my terminal.
> I think we have discussed this before. I don't think we will agree on this
> as I have a very real reason for short lines. It does directly impact on my
> daily work. I don't think you're going to like it no matter how I explain
> this.
IIRC this takes 82 characters.
Yes, we discussed and I see no issues to avoid uglifying / adding unnecessary
lines to the code. We can continue arguing, but this what I think and I don't
know what argument may change my opinion. Apparently this is on maintainer's
field now to decide.
--
With Best Regards,
Andy Shevchenko
