linux-kernel - Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141210183146.GA22847@roeck-us.net>
Date:	Wed, 10 Dec 2014 10:31:46 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Benoit Cousson <bcousson@...libre.com>,
	Patrick Titiano <ptitiano@...libre.com>,
	LM Sensors <lm-sensors@...sensors.org>
Subject: Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable
 at run-time

On Wed, Dec 10, 2014 at 05:46:43PM +0100, Bartosz Golaszewski wrote:
> 2014-12-10 15:20 GMT+01:00 Guenter Roeck <linux@...ck-us.net>:
> >> +       case INA2XX_CALIBRATION:
> >> +               if (data->regs[reg] == 0)
> >> +                       val = 0;
> >> +               else
> >> +                       val = data->config->calibration_factor
> >> +                                               / data->regs[reg];
> >> +               break;
> >
> >
> > This doesn't really make sense. What you want to show is rshunt, not the
> > above.
> > I think it would be better to write a separate show function to display it.
> 
> Hi Guenter,
> 
> this is the only way to display values read from the chip. It also
> tells the user what the actual programmed value is. In fact it was
> your suggestion (https://lkml.org/lkml/2014/11/30/233) and I agree
> that it's a better alternative. Are you sure you don't want it done
> this way?
> 
I don't really want it at all ;-). Seems to me all those options are broken
one way or another. The only real solution would be to re-instantiate the
driver if the chip was removed and re-inserted, and to provide parameters
either with platform data or with devicetree data.

On a side note, data->rshunt is not really needed anymore with your current
code. The only reason for storing it in data is to use it in
ina2xx_calibration_val(), but you could as well pass it in as parameter
to that function. Even better would be to have a function such as
ina2xx_calibrate() and let it handle the write to the calibration register.
Also, I would suggest to move the above code into its own show function.
It is probably not a good idea to have a single function for all those
conversions in the first place, and converting from 'calibration' to
rshunt goes a bit beyond the original intent to convert from one representation
to the other.  

That still doesn't really solve the structural problem of having to deal with
an uninitialized chip which doesn't like to be uninitialized. But then on the
other side I guess that is really a problem with your platform, not a driver
problem.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/