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

[Guenter Roeck <linux@roeck-us.net>]

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.

What do you think ?

Thanks,
Guenter