Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[AngeloGioacchino Del Regno]

Il 04/06/24 13:05, Andy Shevchenko ha scritto:
> On Tue, Jun 4, 2024 at 1:38 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> > Il 30/05/24 15:34, Andy Shevchenko ha scritto:
> > > On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
>
> ...
>
> > > > +#define PMIC_RG_RESET_VAL              (BIT(0) | BIT(3))
> > >
> > > In this form it requires a comment explaining each mentioned bit.
> >
> > I don't have an explanation for this, I know it's two different bits from some
> > reveng, but the downstream driver declares that simply as 0x9.
> >
> > Should I just "mask" this as 0x9 instead?
>
> In this case for all of the questionable forms, please add a oneline
> comment suggesting that "these are different bits without known
> purpose of each." or something like that.

Perfect. Comment added.

> ...
>
> > > > +#define MT6358_IMP0_CLEAR              (BIT(14) | BIT(7))
> > >
> > > As per above.
> >
> > Same, I don't have any explanation for that.
> >
> > If you prefer, I can define this as 0x4080, but honestly I prefer keeping
> > it as-is since I am sure it's not a magic number but really two bits to flip
> > in a register.
>
> As per above.
>
> ...
>
> > > > +       u8 r_numerator;
> > > > +       u8 r_denominator;
> > >
> > > Can you add struct u8_fract to the math.h and use it? I will Ack/R the
> > > respective patch.
> >
> > Yeah, I did that exactly because u8_fract wasn't there and I didn't want
> > to waste more bits, but since you just asked for it... well, I'm happier :-)
>
> Note, it's enough to have my Rb tag and route that change via IIO
> tree. We have done similar way for other changes in math.h (or aline)
> in the past.

Sure.

> ...
>
> > > > +       /* Assert ADC reset */
> > > > +       regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
> > >
> > > No required delay in between?
> >
> > No, as strange as it may look, there is no delay required in between: this is
> > because the register R/W is behind the PMIC Wrapper as much as all of the other
> > MediaTek PMIC (sub)devices, so, missing delays was intentional here, yes.
>
> Maybe a comment?

Done :-)

/* De-assert ADC reset. No wait required, as pwrap takes care of that for us. */

> ...
>
> > > > +       mutex_lock(&adc_dev->lock);
> > >
> > > Why not use cleanup.h?
> >
> > I want to unlock the mutex immediately right after executing read_imp() or
> > mt6359_auxadc_read_adc(), and I don't want the reset to be done while a mutex
> > is being held, as that makes no sense for this driver.
>
> That's why we have scoped_guard(). Exactly for such cases.

Thanks for the hint, looking at other usages that was straightforward.

> > Besides, I find the macros in cleanup.h to be cryptic - in my opinion, they
> > require better documentation as, for example, I don't understand when the
> > guard(mutex)(my_mutex) is supposed to acquire the lock and when it's supposed
> > to release it.
>
> They are cryptic due to limitations in C language. But for the end
> user it doesn't matter. The behaviour is well understandable and makes
> code cleaner and less prone for errors such as missing unlocks. So,
> please use cleanup.h.

Indeed, but my point was that the documentation can (and probably should)
be improved.


Cheers,
Angelo