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

[Guenter Roeck]

On Wed, Dec 10, 2014 at 05:46:43PM +0100, Bartosz Golaszewski wrote:
> 2014-12-10 15:20 GMT+01:00 Guenter Roeck <linux@xxxxxxxxxxxx>:
> >> +       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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/