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. > >Mike, would that break your application, or can you specify the mode in > >devicetree ? > > I'm fine with specifying this in the devicetree. It will break things for > me, but I've been warned and willing to bow for the greater good :) > I should have asked if your system uses devicetree. If it does, the problem should be easy to fix for you. If not, we'll need to find a solution for your use case. 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