...
> >
> >>>>>> + if (en)
> >>>>>> + return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> >>>>>> + KX022A_MASK_DRDY);
> >>>>>
> >>>>> I would put redundant 'else' here to have them on the same column, but
> >>>>> it's pity we don't have regmap_assign_bits() API (or whatever name you
> >>>>> can come up with) to hide this kind of code.
> >>>>
> >>>> Eh, you mean you would put else here even though we return from the if? And
> >>>> then put another return in else, and no return outside the if/else?
> >>>>
> >>>> I definitely don't like the idea.
> >>>>
> >>>> We could probably use regmap_update_bits and ternary but in my opinion that
> >>>> would be just much less obvious. I do like the use of set/clear bits which
> >>>> also makes the meaning / "polarity" of bits really clear.
> >>>
> >>> The idea behind is simple (and note that I'm usually on the side of _removing_
> >>> redundancy)
> >>>
> >>> if (en)
> >>> return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> >>> KX022A_MASK_DRDY);
> >>> else
> >>> return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> >>> ...
> >>>
> >>> Because the branches are semantically tighten to each other. But it's not
> >>> a big deal to me.
> >>
> >> What I do not really like in above example is that we never reach the
> >> end of function body.
> >
> > What do you mean by that? Compiler does warn or what?
>
> I don't know if compiler warns about it as I didn't test it. Now that
> you mentioned it, I think I have seen such warnings from a compiler or
> some static analyzer (klocwork?) in the past though. What I mean is that:
>
> int foo()
> {
> if () {
> ...
> return 1;
> } else {
> ...
> return 2;
> }
> }
>
For reference, this is the one I'd write if both options are good
(or both are bad) and we don't need to worry about reducing indent
for readability.
However, I long since decided this was trivial enough not to
comment on it in the code of others!
> construct makes mistakes like:
>
> int foo()
> {
> if () {
> ...
> return 1;
> } else {
> ...
> return 2;
> }
>
> ...
>
> return 0;
> }
That should given unreachable code warning unless you've really managed
to confuse the compiler / static analysis tooling.
>
> easy to make. When one uses:
>
> int foo()
> {
> if () {
> ...
> return 1;
> }
>
> ...
> return 2;
> }
>
> to begin with there is zero room for such confusion.
>
> >
> >> It is against the expectation - and adding the
> >> else has no real meaning when if returns. I actually think that
> >> checkpatch could even warn about that construct.
> >
> > No way we ever accept such a thing for checkpatch because it's subjective
>
> Eh. Are you telling me that there is no subjective stuff in checkpatch? ;)
>
> > (very dependant on the code piece). OTOH the proposed pattern is used in
> > many places and left like that in places where I cleaned up the 'else',
> > to leave the semantic tights with the above lines).
> >
> >>>>>> + return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> >>>>>> + KX022A_MASK_DRDY);
> >
> > I see that we have a strong disagreement here. I leave it to maintainers.
>
>
> Okay, let's hear what others have to say here.
Non answer above ;)
Time for the old "Don't let perfect be the enemy of good!"
>
> Thanks for all the input this far.
>
> Best Regards
> -- Matti
>
