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

[Andy Shevchenko <andy@kernel.org>]

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?

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

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

...

> > > +	msleep(3200);
> > 
> > Not a single letter to comment such a huge delay :-(
> 
> Well, it's after doing a reset so it should be pretty obvious is the number
> given in the DS. But I'll put a comment on it.

Please do.

-- 
With Best Regards,
Andy Shevchenko