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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html