On Thu, 14 Nov 2024 14:43:11 +0100 Matteo Martelli <matteomartelli3@xxxxxxxxx> wrote: > On Thu, 14 Nov 2024 12:52:12 +0000, <Victor.Duicu@xxxxxxxxxxxxx> wrote: > > On Thu, 2024-11-14 at 12:00 +0100, Matteo Martelli wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > > Hi Matteo, > > > > > On Thu, 14 Nov 2024 10:47:02 +0200, <victor.duicu@xxxxxxxxxxxxx> > > > wrote: > > > > From: Victor Duicu <victor.duicu@xxxxxxxxxxxxx> > > > > > > > > This patch implements ACPI support to Microchip pac1921. > > > > The driver can read the shunt resistor value and label from the > > > > ACPI table. > > > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx> > > > > Signed-off-by: Victor Duicu <victor.duicu@xxxxxxxxxxxxx> > > > > --- > > > > > > > > .... > > > > > > > > > > > > > > +#define PAC1921_ACPI_GET_uOHMS_VALS 0 > > > > +#define PAC1921_ACPI_GET_LABEL 1 > > > > +/* > > > > + * The maximum acceptable shunt value is 2146.999999 OHM. > > > > + * This value, which is below INT_MAX, was chosen in order to > > > > + * allow the readings from dt and ACPI to share the same range > > > > + * and to simplify the checks. > > > > + * With this value the maximum current that can be read is > > > > + * 0.1V / 2146.999999OHM = 46.576 uA > > > > + * If we use INT_MAX the maximum current that can be read is > > > > + * 0.1V / 2147.483647OHM = 46.566 uA > > > > + * The relative error between the two values is > > > > + * |(46.566 - 46.576) / 46.566| * 100 = 0.0214 > > > > + */ > > > > +#define PAC1921_MAX_SHUNT_VALUE_uOHMS 2146999999UL > > > > + > > > > > > Just a minor point about this: if I understand correctly that value > > > comes from (INT_MAX / MICRO - 1) * MICRO + MAX_MICRO. This was to > > > simplify the check in a single statement in > > > pac1921_write_shunt_resistor() > > > which is called when the shunt resistor is set from *sysfs* (neither > > > from DT nor ACPI). I'm fine with this value and the new check but I > > > find > > > the explanation comment a bit confusing. If you could come up with a > > > bit > > > more clear explanation about the reason of such value I think it > > > would be > > > better otherwise I am fine with it as it is. Also, maybe use the full > > > room > > > for 80 characters per line and UOHMS instead of uOHMS to avoid mixed > > > case if > > > you are going with a new version. > > > > We could completely remove the need to use a constant below INT_MAX > > with this check in pac1921_write_shunt_resistor: > > > > if ((!val && !val_fract) || > > ((val >= INT_MAX / MICRO) && (val_fract > INT_MAX % MICRO))) > > return -EINVAL; > > > > Do you agree with this approach? > > Yes, something like this would be clearer to me. > > Anyway, I think you also need to check for val > INT_MAX / MICRO when > val_fract is < INT_MAX % MICRO, right? > > Also, I think you can remove a couple of parenthesis. > > So something like the following maybe (but please double check it): > > if ((!val && !val_fract) || val > INT_MAX / MICRO || > (val == INT_MAX / MICRO && val_fract > INT_MAX % MICRO)) > > I think that usually it would be better to use pre-computed constants > instead of run-time divisions for efficiency but since the shunt > resistor is likely going to be set rarely, I would go for this code for > better clarity. FWIW The compiler should be able to squash those into compile time constants anyway. > > > > Also, the use of mixed case was suggested by Andy to increase > > readability. > > Ah, sorry for missing Andy's comment. I am fine with it if you also find > it more readable. > > > > > ... > > > > > > > > > > > Best regards, > > > Matteo Martelli > > > > With Best Regards, > > Duicu Victor > > Thanks, > Matteo Martelli
