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
