On Mon, 2023-11-13 at 18:31 +0200, Andy Shevchenko wrote: > On Mon, Nov 13, 2023 at 11:13:44AM +0100, Nuno Sá wrote: > > On Fri, 2023-11-10 at 18:50 +0200, Andy Shevchenko wrote: > > > On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote: > > ... > > > > > +/* > > > > + * relaxed version of FIELD_PREP() to be used when mask is not a > > > > compile > > > > time constant > > > > + * u32_encode_bits() can't also be used as the compiler needs to be > > > > able to > > > > evaluate > > > > + * mask at compile time. > > > > + */ > > > > +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) > > > > > > Can we name it accordingly as done in other places, and TBH it's a time to > > > move > > > it to the header. (At least I know about two more implementations of > > > this). > > > > Not sure what you mean? Is there some other drivers doing it already? I'll, > > anyways, wait on more feedback for the GPIO stuff because we might end up > > not > > needing it... > > $ git grep -n 'define field_prep' > > ... > > > > > + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to > > > > pull > > > > down the line */ > > > > + if (!gpio->active_high) > > > > > > Hmm... Why do you need a separate flag for this? Shouldn't be described or > > > autodetected somehow? > > > > Well, if a consumer as an active high gpio, it expects to call > > gpiod_set_value(..., 1) and the line to assert, right? To have that, we need > > to > > write 0 on the device register for some of the pins. > > It doesn't matter, the GPIO (not _raw) APIs are using logical levels, 1 — > activate, > 0 — deactivate. > > > And the same story is true for active low. gpiod_set_value(..., 0) will have > > the > > gpiolib to automatically invert the value and we get 1 in the callback. > > Yes, but why do you have that flag in the structure? Because one of the pins (GPIO_1) has the opposite behavior... > > > > > + val = !val; > > ... > > > > > + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, > > > > U16_MAX); > > > > > > I'm wondering if you can do some trick to "divide" actually to 2^16 so, it > > > will > > > not use division op at all? > > > > Hmm, not sure if it will be obvious but you mean something like: > > > > *val = (be16_to_cpu(in) * (u64)fs) >> 16; > > > > Is this what you mean? If so, we`ll loose the "CLOSEST" handling... Not so > > sure > > if we need to be "that" performant in such a code path. But Guenter can also > > share his opinion... > > *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs + (BIT(16) - > 1), BIT(16)); > > will give the same result without division, no? > What you need is to make sure that the multiplication won't get closer to > U64_MAX, which seems not the case here (max 48-bit number). Hmm, I must be missing something but you're still using DIV_ROUND_CLOSEST_ULL(). So, I guess you're rely on some formula optimization that removes the division (I'm honestly seeing it) but the result won't be exactly the same (off by 1). Again, this is not a fast path (AFAIK) and this is a typical formula to get a value from an ADC so I'm not sure making any super "smart" tricks to make this run faster beats readability. But, I'm still not seeing what you mean so I might change my mind... > > Ditto for all other similar cases which I already pointed out. > > ... > > > > > + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; > > > > > > > "* BIT(16)" / "* BIT_ULL(16)" ? > > > > Well, I can just place the number as in the formula. Not too keen on the > > BIT() > > macros as this is not really a mask. > > I'm not sure I got this. The << 16 neither a plain number and BIT() is equally Well, I do agree with << 16 part... > good. With power of two it's most likely that this is due to internal > implementation of the firmware or hardware, so again BIT() can be still good > enough to show that. > I'm still not convinced honestly... I see plain numbers to be a good fit and they match exactly with the DS. I just see things like BIT(), GENMASK, BITMAP and the likes to be used on masks. But I don't really care so unless Guenter has some opinion I can make as you suggest... - Nuno Sá
