Re: [PATCH v2 3/3] hwmon: ltc2990: support all measurement modes

[Guenter Roeck]

On 11/18/2016 04:23 AM, Tom Levens wrote:
> On Fri, 18 Nov 2016, Guenter Roeck wrote:
>
> > On Thu, Nov 17, 2016 at 11:25:30PM +0000, Tom Levens wrote:
> > > On 17 Nov 2016, at 22:54, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > > > On Thu, Nov 17, 2016 at 08:52:12PM +0100, Mike Looijmans wrote:
> > > > > ïOn 17-11-2016 19:56, Guenter Roeck wrote:
> > > > > > On Thu, Nov 17, 2016 at 06:40:17PM +0100, Mike Looijmans wrote:
> > > > > > > ïOn 17-11-16 17:56, Guenter Roeck wrote:
> > > > > > > > On 11/17/2016 04:10 AM, Tom Levens wrote:
> > > > > > > > > Updated version of the ltc2990 driver which supports all measurement
> > > > > > > > > modes available in the chip. The mode can be set through a devicetree
> > > > > > > > > attribute.
> > > > > > [ ... ]
> > > > > >
> > > > > > > > > static int ltc2990_i2c_probe(struct i2c_client *i2c,
> > > > > > > > >                 const struct i2c_device_id *id)
> > > > > > > > > {
> > > > > > > > >    int ret;
> > > > > > > > >    struct device *hwmon_dev;
> > > > > > > > > +    struct ltc2990_data *data;
> > > > > > > > > +    struct device_node *of_node = i2c->dev.of_node;
> > > > > > > > >
> > > > > > > > >    if (!i2c_check_functionality(i2c->adapter,
> > > > > > > > > I2C_FUNC_SMBUS_BYTE_DATA |
> > > > > > > > >                     I2C_FUNC_SMBUS_WORD_DATA))
> > > > > > > > >        return -ENODEV;
> > > > > > > > >
> > > > > > > > > -    /* Setup continuous mode, current monitor */
> > > > > > > > > +    data = devm_kzalloc(&i2c->dev, sizeof(struct ltc2990_data),
> > > > > > > > > GFP_KERNEL);
> > > > > > > > > +    if (unlikely(!data))
> > > > > > > > > +        return -ENOMEM;
> > > > > > > > > +    data->i2c = i2c;
> > > > > > > > > +
> > > > > > > > > +    if (!of_node || of_property_read_u32(of_node, "lltc,mode",
> > > > > > > > > &data->mode))
> > > > > > > > > +        data->mode = LTC2990_CONTROL_MODE_DEFAULT;
> > > > > > > >
> > > > > > > > Iam arguing with myself if we should still do this or if we should read
> > > > > > > > the mode
> > > > > > > > from the chip instead if it isn't provided (after all, it may have been
> > > > > > > > initialized
> > > > > > > > by the BIOS/ROMMON).
> > > > > > >
> > > > > > > I think the mode should be explicitly set, without default. There's no way
> > > > > > > to tell whether the BIOS or bootloader has really set it up or whether the
> > > > > > > chip is just reporting whatever it happened to default to. And given the
> > > > > > > chip's function, it's unlikely a bootloader would want to initialize it.
> > > > > > Unlikely but possible. Even if we all agree that the chip should be configured
> > > > > > by the driver, I don't like imposing that view on everyone else.
> > > > > >
> > > > > > > My advice would be to make it a required property. If not set, display an
> > > > > > > error and bail out.
> > > > > > It is not that easy, unfortunately. It also has to work on a non-devicetree
> > > > > > system. I would not object to making the property mandatory, but we would
> > > > > > still need to provide non-DT support.
> > > > > >
> > > > > > My "use case" for taking the current mode from the chip if not specified
> > > > > > is that it would enable me to run a module test with all modes. I consider
> > > > > > this extremely valuable.
> > > > >
> > > > > Good point.
> > > > >
> > > > > The chip defaults to measuring internal temperature only, and the mode
> > > > > defaults to "0".
> > > > >
> > > > > Choosing a mode that doesn't match the actual circuitry could be bad for the
> > > > > chip or the board (though unlikely, it'll probably just be useless) since it
> > > > > will actively drive some of the inputs in the temperature modes (which is
> > > > > default for V3/V4 pins).
> > > > >
> > > > > Instead of failing, one could choose to set the default mode to "7", which
> > > > > just measures the 4 voltages, which would be a harmless mode in all cases.
> > > > >
> > > > > As a way to let a bootloader set things up, I think it would be a good check
> > > > > to see if CONTROL register bits 4:3 are set. If "00", the chip is not
> > > > > acquiring data at all, and probably needs configuration still. In that case,
> > > > > the mode must be provided by the devicetree (or the default "7").
> > > > > If bits 4:3 are "11", it has already been set up to measure its inputs, and
> > > > > it's okay to continue doing just that and use the current value of 2:0
> > > > > register as default mode (if the devicetree didn't specify any mode at all).
> > > >
> > > > At first glance, agreed, though by default b[3:4] are 00, and only the
> > > > internal temperature is measured. Actually, the 5 mode bits are all
> > > > relevant to determine what is measured. Maybe it would be better to take
> > > > all 5 bits into account instead of blindly setting b[34]:=11 and a specific
> > > > setting of b[0:2]. Sure, that would make the mode table a bit larger,
> > > > but then ltc2990_attrs_ena[] could be made an u16 array, and a table size
> > > > of 64 bytes would not be that bad.
> > >
> > > I would tend to agree that it should be possible to configure all 5 mode
> > > bits through the devicetree. What I would propose is as follows.
> > >
> > > If a devicetree node exists, the mode parameter(s?) are required and the
> > > chip is initialised.
> > >
> > > If a devicetree node doesn't exist, it is assumed that the chip has
> > > already been configured (by the BIOS, etc). The mode is read from the
> > > chip to set the visibility of the sysfs attributes. In the worst case, where the
> > > chip has not been configured by another source, it would only be possible
> > > to measure the internal temperature -- but I think this is an acceptable
> > > limitation.
> > SGTM.
> >
> > > The only case that this does not cover is if the device tree node
> > > exists but the chip is expected to be configured by some other source.
> > > Maybe I am wrong, but I would not expect this to be a terribly common
> > > situation.
> > >
> > > What do you think?
> > I would not bother about this case. Just make the mode property mandatory.
>
> What do you think about making the devicetree property a list of two integers? Something like
>
> lltc,mode = <7 3>;
>
> which would set mode[2..0]=7 and mode[4..3]=3.
I would personally just use a single value for b[4..0]. But that is really up for bikeshedding
(eg should it be <7 3> or <3 7>. I'll leave that up to Rob to decide - he knows better than me
of what makes more sense from a DT perspective.

Guenter