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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcwnjrsAY1qF68MpBWV-NLFSxTP_PDL+ER==KNdBAFFTA@mail.gmail.com>
Date: Tue, 4 Jun 2024 14:05:03 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, matthias.bgg@...il.com, lee@...nel.org, andy@...nel.org, 
	nuno.sa@...log.com, bigunclemax@...il.com, dlechner@...libre.com, 
	marius.cristea@...rochip.com, marcelo.schmitt@...log.com, fr0st61te@...il.com, 
	mitrutzceclan@...il.com, mike.looijmans@...ic.nl, marcus.folkesson@...il.com, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-mediatek@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9
 Auxiliary ADC

On Tue, Jun 4, 2024 at 1:38 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> 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@...labora.com> 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.

...

> >> +#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.

...

> >> +       /* 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?

...

> >> +       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.

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

-- 
With Best Regards,
Andy Shevchenko