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