Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip

["Nuno Sá" <noname.nuno@gmail.com>]

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á