| Message ID | 20230228063151.17598-1-mike.looijmans@topic.nl |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v3,1/2] dt-bindings: iio: adc: Add TI ADS1100 and ADS1000 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 28/02/2023 07:31, Mike Looijmans wrote: > The ADS1100 is a 16-bit ADC (at 8 samples per second). > The ADS1000 is similar, but has a fixed data rate. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > > --- > > (no changes since v2) > > Changes in v2: > "reg" property is mandatory. > Add vdd-supply and #io-channel-cells Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote: > The ADS1100 is a 16-bit ADC (at 8 samples per second). > The ADS1000 is similar, but has a fixed data rate. ... > + /* Shift result to compensate for bit resolution vs. sample rate */ > + value <<= 16 - ads1100_data_bits(data); > + *val = sign_extend32(value, 15); Why not simply *val = sign_extend32(value, ads1100_data_bits(data) - 1); ? (Double check for off-by-one usage) ... > + /* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */ Can you use more math / plain English to describe the formula? Otherwise we can see the very same in the code and point of the comment is doubtful. > + gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2; Something from units.h? ... > + for (i = 0; i < 4; i++) { > + if (BIT(i) == gain) { ffs()/__ffs() (look at the documentation for the difference and use proper one). > + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); > + return 0; > + } > + } ... > + for (i = 0; i < size; ++i) { Why pre-increment? > + if (ads1100_data_rate[i] == rate) { > + return ads1100_set_config_bits( > + data, ADS1100_DR_MASK, Strange indentation. > + FIELD_PREP(ADS1100_DR_MASK, i)); > + } Do you need {} ? > + } ... > + int millivolts = regulator_get_voltage(data->reg_vdd) / 1000; units.h? ... > + data->scale_avail[i * 2] = millivolts; I would write ' * 2 + 0]', but it's up to you. > + data->scale_avail[i * 2 + 1] = 15 + i; ... > + *val = regulator_get_voltage(data->reg_vdd) / 1000; units.h? ... > + *val = ads1100_data_rate[ > + FIELD_GET(ADS1100_DR_MASK, data->config)]; Strange indentation, just use a single line. ... > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0) Why ' < 0'? > + return dev_err_probe(dev, ret, > + "Failed to register IIO device\n"); ... > +static int ads1100_runtime_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct ads1100_data *data = iio_priv(indio_dev); > + > + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); > + regulator_disable(data->reg_vdd); Wrong devm / non-devm ordering. > + return 0; > +} ... > +static const struct i2c_device_id ads1100_id[] = { > + { "ads1100", }, > + { "ads1000", }, Inner commas are not needed. > + {} > +}; ... > +static const struct of_device_id ads1100_of_match[] = { > + { .compatible = "ti,ads1100", }, > + { .compatible = "ti,ads1000", }, Ditto. > + {} > +}; ... > + Redundant blank line. > +module_i2c_driver(ads1100_driver);
Comments below. Mailserver has a top-post fetish and will inject a signature here somewhere... No further comment from me means "agree, will implement in v4"... Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topic.nl Please consider the environment before printing this e-mail On 01-03-2023 16:30, Andy Shevchenko wrote: > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote: >> The ADS1100 is a 16-bit ADC (at 8 samples per second). >> The ADS1000 is similar, but has a fixed data rate. > ... > >> + /* Shift result to compensate for bit resolution vs. sample rate */ >> + value <<= 16 - ads1100_data_bits(data); >> + *val = sign_extend32(value, 15); > Why not simply > > *val = sign_extend32(value, ads1100_data_bits(data) - 1); > > ? As discussed with Jonathan Cameron, the register is right-justified and the number of bits depend on the data rate. Rather than having the "scale" change when the sample rate changes, we chose to adjust the sample result so it's always left-justified. >> + /* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */ > Can you use more math / plain English to describe the formula? Otherwise we can > see the very same in the code and point of the comment is doubtful. I'll try to explain it better. > >> + gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2; > Something from units.h? Could put MILLI here, but I doubt if that improves things. Its actually MICRO/(MICRO/MILLI) given the explanation above... Not helping much... > ... > >> + for (i = 0; i < 4; i++) { >> + if (BIT(i) == gain) { > ffs()/__ffs() (look at the documentation for the difference and use proper one). Thought of it, but I'd rather have it return EINVAL for attempting to set the analog gain to "7" (0nly 1,2,4,8 allowed). > ... >> + for (i = 0; i < size; ++i) { > Why pre-increment? Spent too much time with other coding guidelines, missed this one... Will change. > > ... > >> + int millivolts = regulator_get_voltage(data->reg_vdd) / 1000; > units.h? Should I write: regulator_get_voltage(data->reg_vdd) / (MICROS / MILLIS); I doubt that improves readability. > ... > ... > >> +static int ads1100_runtime_suspend(struct device *dev) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); >> + struct ads1100_data *data = iio_priv(indio_dev); >> + >> + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); >> + regulator_disable(data->reg_vdd); > Wrong devm / non-devm ordering. Don't understand your remark, can you explain further please? devm / non-devm ordering would be related to the "probe" function. As far as I can tell, I'm not allocating resources after the devm calls. And the "remove" is empty.
On 3/1/23 23:49, Mike Looijmans wrote: > > >> ... >> ... >> >>> +static int ads1100_runtime_suspend(struct device *dev) >>> +{ >>> + struct iio_dev *indio_dev = >>> i2c_get_clientdata(to_i2c_client(dev)); >>> + struct ads1100_data *data = iio_priv(indio_dev); >>> + >>> + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); >>> + regulator_disable(data->reg_vdd); >> Wrong devm / non-devm ordering. > > Don't understand your remark, can you explain further please? > > devm / non-devm ordering would be related to the "probe" function. As > far as I can tell, I'm not allocating resources after the devm calls. > And the "remove" is empty. Strictly speaking we need to unregister the IIO device before disabling the regulator, otherwise there is a small window where the IIO device still exists, but doesn't work anymore. This is a very theoretical scenario though. You are lucky :) There is a new function `devm_regulator_get_enable()`[1], which will manage the regulator_disable() for you. Using that will also reduce the boilerplate in `probe()` a bit - Lars [1] https://lwn.net/Articles/904383/
On 3/2/23 05:16, Lars-Peter Clausen wrote: > On 3/1/23 23:49, Mike Looijmans wrote: >> >> >>> ... >>> ... >>> >>>> +static int ads1100_runtime_suspend(struct device *dev) >>>> +{ >>>> + struct iio_dev *indio_dev = >>>> i2c_get_clientdata(to_i2c_client(dev)); >>>> + struct ads1100_data *data = iio_priv(indio_dev); >>>> + >>>> + ads1100_set_config_bits(data, ADS1100_CFG_SC, >>>> ADS1100_SINGLESHOT); >>>> + regulator_disable(data->reg_vdd); >>> Wrong devm / non-devm ordering. >> >> Don't understand your remark, can you explain further please? >> >> devm / non-devm ordering would be related to the "probe" function. As >> far as I can tell, I'm not allocating resources after the devm calls. >> And the "remove" is empty. > > Strictly speaking we need to unregister the IIO device before > disabling the regulator, otherwise there is a small window where the > IIO device still exists, but doesn't work anymore. This is a very > theoretical scenario though. > > You are lucky :) There is a new function > `devm_regulator_get_enable()`[1], which will manage the > regulator_disable() for you. Using that will also reduce the > boilerplate in `probe()` a bit > > - Lars > > [1] https://lwn.net/Articles/904383/ > Sorry, just saw that Andy's comment was on the suspend() function, not remove(). In that case there is of course no need for any devm things. But still a good idea to use `devm_regulator_get_enable()` in probe for the boiler plate.
On Thu, Mar 02, 2023 at 08:49:22AM +0100, Mike Looijmans wrote: > On 01-03-2023 16:30, Andy Shevchenko wrote: > > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote: ... > > > + /* Shift result to compensate for bit resolution vs. sample rate */ > > > + value <<= 16 - ads1100_data_bits(data); > > > + *val = sign_extend32(value, 15); > > Why not simply > > > > *val = sign_extend32(value, ads1100_data_bits(data) - 1); > > > > ? > > As discussed with Jonathan Cameron, the register is right-justified and the > number of bits depend on the data rate. Rather than having the "scale" > change when the sample rate changes, we chose to adjust the sample result so > it's always left-justified. Hmm... OK, but it adds unneeded code I think. ... > > > + for (i = 0; i < 4; i++) { > > > + if (BIT(i) == gain) { > > ffs()/__ffs() (look at the documentation for the difference and use proper one). > > Thought of it, but I'd rather have it return EINVAL for attempting to set > the analog gain to "7" (0nly 1,2,4,8 allowed). I'm not sure what you are implying. You have open coded something that has already to be a function which on some architectures become a single assembly instruction. That said, drop your for-loop if-cond and use one of the proposed directly. Then you may compare the result to what ever you want to be a limit and return whatever error code you want to. ... > > > + for (i = 0; i < size; ++i) { > > Why pre-increment? > > Spent too much time with other coding guidelines, missed this one... Will > change. I don't remember that's in coding guidelines, but it's standard practice in the Linux kernel project. Yeah, we have a few hundreds of the pre-increments, but reasons may be quite different for those. ... > > > + int millivolts = regulator_get_voltage(data->reg_vdd) / 1000; > > units.h? > > Should I write: > > regulator_get_voltage(data->reg_vdd) / (MICROS / MILLIS); > > I doubt that improves readability. Yeah, it should be something like MICROVOLT_PER_MILLIVOLT. But it's not defined yet. ... > > > +static int ads1100_runtime_suspend(struct device *dev) > > > +{ > > > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > > + struct ads1100_data *data = iio_priv(indio_dev); > > > + > > > + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); > > > + regulator_disable(data->reg_vdd); > > Wrong devm / non-devm ordering. > > Don't understand your remark, can you explain further please? > > devm / non-devm ordering would be related to the "probe" function. As far as > I can tell, I'm not allocating resources after the devm calls. And the > "remove" is empty. Ah, it's my mistake, I misread it as ->remove().
On Thu, Mar 02, 2023 at 05:20:38AM -0800, Lars-Peter Clausen wrote: > On 3/2/23 05:16, Lars-Peter Clausen wrote: > > On 3/1/23 23:49, Mike Looijmans wrote: ... > > > > > +static int ads1100_runtime_suspend(struct device *dev) > > > > > +{ > > > > > + struct iio_dev *indio_dev = > > > > > i2c_get_clientdata(to_i2c_client(dev)); > > > > > + struct ads1100_data *data = iio_priv(indio_dev); > > > > > + > > > > > + ads1100_set_config_bits(data, ADS1100_CFG_SC, > > > > > ADS1100_SINGLESHOT); > > > > > + regulator_disable(data->reg_vdd); > > > > Wrong devm / non-devm ordering. > > > > > > Don't understand your remark, can you explain further please? > > > > > > devm / non-devm ordering would be related to the "probe" function. > > > As far as I can tell, I'm not allocating resources after the devm > > > calls. And the "remove" is empty. > > > > Strictly speaking we need to unregister the IIO device before disabling > > the regulator, otherwise there is a small window where the IIO device > > still exists, but doesn't work anymore. This is a very theoretical > > scenario though. > > > > You are lucky :) There is a new function > > `devm_regulator_get_enable()`[1], which will manage the > > regulator_disable() for you. Using that will also reduce the boilerplate > > in `probe()` a bit > > > > [1] https://lwn.net/Articles/904383/ > > > Sorry, just saw that Andy's comment was on the suspend() function, not > remove(). In that case there is of course no need for any devm things. But > still a good idea to use `devm_regulator_get_enable()` in probe for the > boiler plate. Yeah, sorry, I mistakenly took it as ->remove().
On Thu, 2 Mar 2023 05:20:38 -0800 Lars-Peter Clausen <lars@metafoo.de> wrote: > On 3/2/23 05:16, Lars-Peter Clausen wrote: > > On 3/1/23 23:49, Mike Looijmans wrote: > >> > >> > >>> ... > >>> ... > >>> > >>>> +static int ads1100_runtime_suspend(struct device *dev) > >>>> +{ > >>>> + struct iio_dev *indio_dev = > >>>> i2c_get_clientdata(to_i2c_client(dev)); > >>>> + struct ads1100_data *data = iio_priv(indio_dev); > >>>> + > >>>> + ads1100_set_config_bits(data, ADS1100_CFG_SC, > >>>> ADS1100_SINGLESHOT); > >>>> + regulator_disable(data->reg_vdd); > >>> Wrong devm / non-devm ordering. > >> > >> Don't understand your remark, can you explain further please? > >> > >> devm / non-devm ordering would be related to the "probe" function. As > >> far as I can tell, I'm not allocating resources after the devm calls. > >> And the "remove" is empty. > > > > Strictly speaking we need to unregister the IIO device before > > disabling the regulator, otherwise there is a small window where the > > IIO device still exists, but doesn't work anymore. This is a very > > theoretical scenario though. > > > > You are lucky :) There is a new function > > `devm_regulator_get_enable()`[1], which will manage the > > regulator_disable() for you. Using that will also reduce the > > boilerplate in `probe()` a bit > > > > - Lars > > > > [1] https://lwn.net/Articles/904383/ > > > Sorry, just saw that Andy's comment was on the suspend() function, not > remove(). In that case there is of course no need for any devm things. > But still a good idea to use `devm_regulator_get_enable()` in probe for > the boiler plate. > You can't because (annoyingly) devem_regulator_get_enable() doesn't provide you access to the struct regulator that you need to be able to turn it of for power management. That case only works for the leave the power on all the time cases. Jonathan
On Thu, 2 Mar 2023 16:23:14 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, Mar 02, 2023 at 08:49:22AM +0100, Mike Looijmans wrote: > > On 01-03-2023 16:30, Andy Shevchenko wrote: > > > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote: > > ... > > > > > + /* Shift result to compensate for bit resolution vs. sample rate */ > > > > + value <<= 16 - ads1100_data_bits(data); > > > > + *val = sign_extend32(value, 15); > > > Why not simply > > > > > > *val = sign_extend32(value, ads1100_data_bits(data) - 1); > > > > > > ? > > > > As discussed with Jonathan Cameron, the register is right-justified and the > > number of bits depend on the data rate. Rather than having the "scale" > > change when the sample rate changes, we chose to adjust the sample result so > > it's always left-justified. > > Hmm... OK, but it adds unneeded code I think. There isn't a way to do it in one go that I can think of. The first statement is multiplying the value by a power of 2, not just sign extending it. You could sign extend first then shift to do the multiply, but ends up same amount of code. It does look a bit like a weird open coded sign extension though so I can see where the confusion came from! > > ... > > > > > + for (i = 0; i < 4; i++) { > > > > + if (BIT(i) == gain) { > > > ffs()/__ffs() (look at the documentation for the difference and use proper one). > > > > Thought of it, but I'd rather have it return EINVAL for attempting to set > > the analog gain to "7" (0nly 1,2,4,8 allowed). > > I'm not sure what you are implying. > > You have open coded something that has already to be a function which on some > architectures become a single assembly instruction. > > That said, drop your for-loop if-cond and use one of the proposed directly. > Then you may compare the result to what ever you want to be a limit and return > whatever error code you want to Agreed, could do it with appropriate ffs() followed by if (BIT(i) != gain) return -EINVAL;
On Tue, 28 Feb 2023 07:31:51 +0100 Mike Looijmans <mike.looijmans@topic.nl> wrote: > The ADS1100 is a 16-bit ADC (at 8 samples per second). > The ADS1000 is similar, but has a fixed data rate. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> Hi Mike, A few minor things + one request for a test as trying to chase a possible ref count overflow around the runtime_pm was giving me a enough of a headache that it's easier to ask you just to poke it and see. If it doesn't fail as I expect I'll take a closer look! Jonathan > +static int ads1100_set_scale(struct ads1100_data *data, int val, int val2) > +{ > + int microvolts; > + int gain; > + int i; > + > + /* With Vdd between 2.7 and 5V, the scale is always below 1 */ > + if (val) > + return -EINVAL; > + > + microvolts = regulator_get_voltage(data->reg_vdd); > + /* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */ > + gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2; > + > + for (i = 0; i < 4; i++) { > + if (BIT(i) == gain) { > + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); > + return 0; > + } > + } Andy's suggestion of something like.. if (!gain) return -EINVAL; i = ffs(gain); if (i >= 4 || BIT(i) != gain) return -EINVAL; ads... Is perhaps nicer than the loop. > + > + return -EINVAL; > +} > +static void ads1100_disable_continuous(void *data) > +{ > + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); > +} > + > +static int ads1100_probe(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev; > + struct ads1100_data *data; > + struct device *dev = &client->dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); You can avoid the slightly nasty mix of i2c_set_clientdata vs dev_get_drvdata() below by taking advantage of the fact you have a local dev pointer. dev_set_drvdata(dev, indio_dev); and no confusing mix is left. Of course it's doing the same thing but to my mind slightly nicer to use the same one. > + data->client = client; > + mutex_init(&data->lock); > + > + indio_dev->name = "ads1100"; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = &ads1100_channel; > + indio_dev->num_channels = 1; > + indio_dev->info = &ads1100_info; > + > + data->reg_vdd = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(data->reg_vdd)) > + return dev_err_probe(dev, PTR_ERR(data->reg_vdd), > + "Failed to get vdd regulator\n"); > + > + ret = regulator_enable(data->reg_vdd); > + if (ret < 0) > + return dev_err_probe(dev, PTR_ERR(data->reg_vdd), > + "Failed to enable vdd regulator\n"); > + > + ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd); > + if (ret) > + return ret; Please could you check a subtle interaction of runtime pm and this devm managed flow. I think we can hit the following flow. 1) In runtime suspend (wait long enough for this to happen). 2) Unbind the driver (rmmod will do) 3) During the unbind we exit suspend then enter it again before we call remove (that's just part of the normal remove flow). 4) We then end up calling regulator disable when it's already disabled. We've traditionally avoided that by having the remove explicitly call pm_runtime_get_sync() before we then disable runtime pm. I don't think that happens with devm_pm_runtime_enable() but I could be missing a path where it does. If the sequence goes wrong you should get a warning about an unbalanced regulator disable. The fix would be an extra devm_add_action_or_reset() before the devm_iio_device_register() below that just calls pm_runtime_get_sync() to force the state to on. Gah. These subtle paths always give me a headache. We don't normally have too much problem with this because many runtime_resume / suspend functions don't change reference counts. > + > + ret = ads1100_setup(data); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to communicate with device\n"); > + > + ret = devm_add_action_or_reset(dev, ads1100_disable_continuous, data); > + if (ret) > + return ret; > + > + ads1100_calc_scale_avail(data); > + > + pm_runtime_set_autosuspend_delay(dev, ADS1100_SLEEP_DELAY_MS); > + pm_runtime_use_autosuspend(dev); > + pm_runtime_set_active(dev); > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable pm_runtime\n"); > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Failed to register IIO device\n"); > + > + return 0; > +} > + > +static int ads1100_runtime_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); Fine to just short cut this dance with struct iio_dev *indio_dev = dev_get_drvdata(dev); It's a bit nasty from a readability point of view, but the pattern is so common we've kind of gotten used to it. > + struct ads1100_data *data = iio_priv(indio_dev); As you don't need the indio_dev, can combine all this into struct ads110_data *data = iio_priv(dev_get_drvdata(dev)); > + > + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); > + regulator_disable(data->reg_vdd); > + > + return 0; > +} > + > +static int ads1100_runtime_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); As above. > + struct ads1100_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = regulator_enable(data->reg_vdd); > + if (ret) { > + dev_err(&data->client->dev, "Failed to enable Vdd\n"); > + return ret; > + } > + > + /* > + * We'll always change the mode bit in the config register, so there is > + * no need here to "force" a write to the config register. If the device > + * has been power-cycled, we'll re-write its config register now. > + */ > + return ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_CONTINUOUS); > +} > +
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topic.nl Please consider the environment before printing this e-mail On 04-03-2023 18:57, Jonathan Cameron wrote: > On Tue, 28 Feb 2023 07:31:51 +0100 > Mike Looijmans <mike.looijmans@topic.nl> wrote: > >> The ADS1100 is a 16-bit ADC (at 8 samples per second). >> The ADS1000 is similar, but has a fixed data rate. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > Hi Mike, > > A few minor things + one request for a test as trying to chase a possible > ref count overflow around the runtime_pm was giving me a enough of a headache > that it's easier to ask you just to poke it and see. If it doesn't fail as > I expect I'll take a closer look! Will do, but it may take a few days to get access to the hardware again. I'll report on that later. >> +static int ads1100_set_scale(struct ads1100_data *data, int val, int val2) >> +{ >> + int microvolts; >> + int gain; >> + int i; >> + >> + /* With Vdd between 2.7 and 5V, the scale is always below 1 */ >> + if (val) >> + return -EINVAL; >> + >> + microvolts = regulator_get_voltage(data->reg_vdd); >> + /* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */ >> + gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2; >> + >> + for (i = 0; i < 4; i++) { >> + if (BIT(i) == gain) { >> + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); >> + return 0; >> + } >> + } > Andy's suggestion of something like.. > if (!gain) > return -EINVAL; > i = ffs(gain); > if (i >= 4 || BIT(i) != gain) > return -EINVAL; > > ads... > > Is perhaps nicer than the loop. Yes, takes out a loop. > >> +static void ads1100_disable_continuous(void *data) >> +{ >> + ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT); >> +} >> + >> +static int ads1100_probe(struct i2c_client *client) >> +{ >> + struct iio_dev *indio_dev; >> + struct ads1100_data *data; >> + struct device *dev = &client->dev; >> + int ret; >> + >> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + data = iio_priv(indio_dev); >> + i2c_set_clientdata(client, indio_dev); > You can avoid the slightly nasty mix of i2c_set_clientdata vs dev_get_drvdata() > below by taking advantage of the fact you have a local dev pointer. > > dev_set_drvdata(dev, indio_dev); > and no confusing mix is left. Of course it's doing the same thing but to my > mind slightly nicer to use the same one. Since I don't need indio_dev anywhere, I might as well say this directly: dev_set_drvdata(dev, data); (Only the pm_ routines use this) > ... >
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topic.nl Please consider the environment before printing this e-mail On 04-03-2023 18:57, Jonathan Cameron wrote: > On Tue, 28 Feb 2023 07:31:51 +0100 > Mike Looijmans <mike.looijmans@topic.nl> wrote: > >> The ADS1100 is a 16-bit ADC (at 8 samples per second). >> The ADS1000 is similar, but has a fixed data rate. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > Hi Mike, > > A few minor things + one request for a test as trying to chase a possible > ref count overflow around the runtime_pm was giving me a enough of a headache > that it's easier to ask you just to poke it and see. If it doesn't fail as > I expect I'll take a closer look! > > Jonathan > > ... >> + data->client = client; >> + mutex_init(&data->lock); >> + >> + indio_dev->name = "ads1100"; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->channels = &ads1100_channel; >> + indio_dev->num_channels = 1; >> + indio_dev->info = &ads1100_info; >> + >> + data->reg_vdd = devm_regulator_get(dev, "vdd"); >> + if (IS_ERR(data->reg_vdd)) >> + return dev_err_probe(dev, PTR_ERR(data->reg_vdd), >> + "Failed to get vdd regulator\n"); >> + >> + ret = regulator_enable(data->reg_vdd); >> + if (ret < 0) >> + return dev_err_probe(dev, PTR_ERR(data->reg_vdd), >> + "Failed to enable vdd regulator\n"); >> + >> + ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd); >> + if (ret) >> + return ret; > Please could you check a subtle interaction of runtime pm and this devm managed > flow. > > I think we can hit the following flow. > 1) In runtime suspend (wait long enough for this to happen). > 2) Unbind the driver (rmmod will do) > 3) During the unbind we exit suspend then enter it again before we call remove > (that's just part of the normal remove flow). > 4) We then end up calling regulator disable when it's already disabled. > > We've traditionally avoided that by having the remove explicitly call > pm_runtime_get_sync() before we then disable runtime pm. I don't > think that happens with devm_pm_runtime_enable() but I could be missing > a path where it does. > > If the sequence goes wrong you should get a warning about an unbalanced regulator > disable. The fix would be an extra devm_add_action_or_reset() before the > devm_iio_device_register() below that just calls pm_runtime_get_sync() > to force the state to on. > > Gah. These subtle paths always give me a headache. > We don't normally have too much problem with this because many > runtime_resume / suspend functions don't change reference counts. Just did this test, waited a few seconds, checked /sys/kernel/debug/regulator... that the regulator had been disabled. Then executed: echo -n 3-004a > /sys/bus/i2c/drivers/ads1100/unbind to unload the driver, and no messages were added to the kernel log. I could see the driver going away and removing itself from iio and regulators. Tried this a couple of times (using bind/unbind), and no problem reported. Hopes this helps with your headaches...
On Sat, Mar 04, 2023 at 05:26:18PM +0000, Jonathan Cameron wrote: > On Thu, 2 Mar 2023 16:23:14 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Mar 02, 2023 at 08:49:22AM +0100, Mike Looijmans wrote: > > > On 01-03-2023 16:30, Andy Shevchenko wrote: > > > > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote: ... > > > > > + /* Shift result to compensate for bit resolution vs. sample rate */ > > > > > + value <<= 16 - ads1100_data_bits(data); > > > > > + *val = sign_extend32(value, 15); > > > > Why not simply > > > > > > > > *val = sign_extend32(value, ads1100_data_bits(data) - 1); > > > > > > > > ? > > > > > > As discussed with Jonathan Cameron, the register is right-justified and the > > > number of bits depend on the data rate. Rather than having the "scale" > > > change when the sample rate changes, we chose to adjust the sample result so > > > it's always left-justified. > > > > Hmm... OK, but it adds unneeded code I think. > > There isn't a way to do it in one go that I can think of. > The first statement is multiplying the value by a power of 2, not just sign extending it. > You could sign extend first then shift to do the multiply, but ends up same amount > of code. > > It does look a bit like a weird open coded sign extension though so I can see where > the confusion came from! I see, for the negative value both approaches will work, for the positive the original one will provide a multiplied value. Yeah, doesn't seem to be a subject to the (micro-)optimizations. ... > > > > > + for (i = 0; i < 4; i++) { > > > > > + if (BIT(i) == gain) { > > > > ffs()/__ffs() (look at the documentation for the difference and use proper one). > > > > > > Thought of it, but I'd rather have it return EINVAL for attempting to set > > > the analog gain to "7" (0nly 1,2,4,8 allowed). > > > > I'm not sure what you are implying. > > > > You have open coded something that has already to be a function which on some > > architectures become a single assembly instruction. > > > > That said, drop your for-loop if-cond and use one of the proposed directly. > > Then you may compare the result to what ever you want to be a limit and return > > whatever error code you want to > > Agreed, could do it with appropriate ffs() followed by if (BIT(i) != gain) return -EINVAL; I meant something different. i = ffs(gain); // or __ffs(gain)? if (i >= 4) return -EINVAL;
On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote: > On Tue, 28 Feb 2023 07:31:51 +0100 > Mike Looijmans <mike.looijmans@topic.nl> wrote: ... > > + for (i = 0; i < 4; i++) { > > + if (BIT(i) == gain) { > > + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); > > + return 0; > > + } > > + } > Andy's suggestion of something like.. > if (!gain) > return -EINVAL; > i = ffs(gain); > if (i >= 4 || BIT(i) != gain) > return -EINVAL; > > ads... > > Is perhaps nicer than the loop. Even better: if (!gain || !is_power_of_2(gain)) return -EINVAL; i = ffs(gain); if (i >= 4) return -EINVAL;
On Mon, Mar 06, 2023 at 02:11:48PM +0200, Andy Shevchenko wrote: > On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote: > > On Tue, 28 Feb 2023 07:31:51 +0100 > > Mike Looijmans <mike.looijmans@topic.nl> wrote: ... > > > + for (i = 0; i < 4; i++) { > > > + if (BIT(i) == gain) { > > > + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); > > > + return 0; > > > + } > > > + } > > Andy's suggestion of something like.. > > if (!gain) > > return -EINVAL; > > i = ffs(gain); > > if (i >= 4 || BIT(i) != gain) > > return -EINVAL; > > > > ads... > > > > Is perhaps nicer than the loop. > > Even better: > > if (!gain || !is_power_of_2(gain)) > return -EINVAL; Or if you want to combine all checks: if (clamp_val(gain, BIT(0), BIT(3)) != gain || !is_power_of_2(gain)) return -EINVAL; ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain)); return 0; (You can play with bloat-o-meter for the code generation and see which one is better from that aspect)
On Mon, Mar 06, 2023 at 02:16:24PM +0200, Andy Shevchenko wrote: > On Mon, Mar 06, 2023 at 02:11:48PM +0200, Andy Shevchenko wrote: > > On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote: > > > On Tue, 28 Feb 2023 07:31:51 +0100 > > > Mike Looijmans <mike.looijmans@topic.nl> wrote: ... > > > > + for (i = 0; i < 4; i++) { > > > > + if (BIT(i) == gain) { > > > > + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); > > > > + return 0; > > > > + } > > > > + } > > > Andy's suggestion of something like.. > > > if (!gain) > > > return -EINVAL; > > > i = ffs(gain); > > > if (i >= 4 || BIT(i) != gain) > > > return -EINVAL; > > > > > > ads... > > > > > > Is perhaps nicer than the loop. > > > > Even better: > > > > if (!gain || !is_power_of_2(gain)) > > return -EINVAL; > > Or if you want to combine all checks: > if (clamp_val(gain, BIT(0), BIT(3)) != gain || !is_power_of_2(gain)) > return -EINVAL; Just a side note. I have been wanting to have is_in_range() for a long time. Perhaps we can add a such to the kernel (math.h) /* Check if in the range [start .. end] */ #define is_in_range(value, start, end) (value >= start && value <= end) With it, the above will probably better look if (!is_in_range(gain, BIT(0), BIT(3) || !is_power_of_2(gain)) > ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain)); > return 0; > > (You can play with bloat-o-meter for the code generation and see which one is > better from that aspect)
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topic.nl Please consider the environment before printing this e-mail On 06-03-2023 13:11, Andy Shevchenko wrote: > On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote: >> On Tue, 28 Feb 2023 07:31:51 +0100 >> Mike Looijmans <mike.looijmans@topic.nl> wrote: > ... > >>> + for (i = 0; i < 4; i++) { >>> + if (BIT(i) == gain) { >>> + ads1100_set_config_bits(data, ADS1100_PGA_MASK, i); >>> + return 0; >>> + } >>> + } >> Andy's suggestion of something like.. >> if (!gain) >> return -EINVAL; >> i = ffs(gain); >> if (i >= 4 || BIT(i) != gain) >> return -EINVAL; >> >> ads... >> >> Is perhaps nicer than the loop. > Even better: > > if (!gain || !is_power_of_2(gain)) > return -EINVAL; > > i = ffs(gain); > if (i >= 4) > return -EINVAL; > I'd guess that "is_power_of_2" is about the same complexity as "ffs". If we want smaller code, in retrospect, I'd vote for omitting that power-of-two check altogether. The IIO device reports this for a 3v3 Vdd: # cat /sys/bus/iio/devices/iio\:device1/scale_available 0.100708007 0.050354003 0.025177001 0.012588500 # echo 0.012588500 > /sys/bus/iio/devices/iio:device1/scale That last statement results in val2=12588 in this call. The whole point of this exercise is that the value '0.012588' corresponds to a gain of '8' here. There's already quite a bit of rounding going on, since writing to the scale turns the value into "micro" scale. Also, looking at the "avail" table. the values for "7" and "8" are much closer together than "3" and "4". And the correct formula for the inverse of "gain = BIT(i);" is "i = ffs(gain) - 1;" because ffs(1) == 1 So I propose this code: if (gain <= 0 || gain > 8) return -EINVAL; regval = ffs(gain) - 1; ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);
On Mon, Mar 06, 2023 at 01:56:15PM +0100, Mike Looijmans wrote: > On 06-03-2023 13:11, Andy Shevchenko wrote: > > On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote: > > > On Tue, 28 Feb 2023 07:31:51 +0100 > > > Mike Looijmans <mike.looijmans@topic.nl> wrote: ... > So I propose this code: > > if (gain <= 0 || gain > 8) Maybe BIT(0) and BIT(3) will be more explicit. Otherwise I'm fine with it. > return -EINVAL; > > regval = ffs(gain) - 1; > ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);
On Mon, 6 Mar 2023 12:21:44 +0100 Mike Looijmans <mike.looijmans@topic.nl> wrote: > Met vriendelijke groet / kind regards, > > Mike Looijmans > System Expert > > > TOPIC Embedded Products B.V. > Materiaalweg 4, 5681 RJ Best > The Netherlands > > T: +31 (0) 499 33 69 69 > E: mike.looijmans@topicproducts.com > W: www.topic.nl > > Please consider the environment before printing this e-mail > On 04-03-2023 18:57, Jonathan Cameron wrote: > > On Tue, 28 Feb 2023 07:31:51 +0100 > > Mike Looijmans <mike.looijmans@topic.nl> wrote: > > > >> The ADS1100 is a 16-bit ADC (at 8 samples per second). > >> The ADS1000 is similar, but has a fixed data rate. > >> > >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > > Hi Mike, > > > > A few minor things + one request for a test as trying to chase a possible > > ref count overflow around the runtime_pm was giving me a enough of a headache > > that it's easier to ask you just to poke it and see. If it doesn't fail as > > I expect I'll take a closer look! > > > > Jonathan > > > > ... > >> + data->client = client; > >> + mutex_init(&data->lock); > >> + > >> + indio_dev->name = "ads1100"; > >> + indio_dev->modes = INDIO_DIRECT_MODE; > >> + indio_dev->channels = &ads1100_channel; > >> + indio_dev->num_channels = 1; > >> + indio_dev->info = &ads1100_info; > >> + > >> + data->reg_vdd = devm_regulator_get(dev, "vdd"); > >> + if (IS_ERR(data->reg_vdd)) > >> + return dev_err_probe(dev, PTR_ERR(data->reg_vdd), > >> + "Failed to get vdd regulator\n"); > >> + > >> + ret = regulator_enable(data->reg_vdd); > >> + if (ret < 0) > >> + return dev_err_probe(dev, PTR_ERR(data->reg_vdd), > >> + "Failed to enable vdd regulator\n"); > >> + > >> + ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd); > >> + if (ret) > >> + return ret; > > Please could you check a subtle interaction of runtime pm and this devm managed > > flow. > > > > I think we can hit the following flow. > > 1) In runtime suspend (wait long enough for this to happen). > > 2) Unbind the driver (rmmod will do) > > 3) During the unbind we exit suspend then enter it again before we call remove > > (that's just part of the normal remove flow). > > 4) We then end up calling regulator disable when it's already disabled. > > > > We've traditionally avoided that by having the remove explicitly call > > pm_runtime_get_sync() before we then disable runtime pm. I don't > > think that happens with devm_pm_runtime_enable() but I could be missing > > a path where it does. > > > > If the sequence goes wrong you should get a warning about an unbalanced regulator > > disable. The fix would be an extra devm_add_action_or_reset() before the > > devm_iio_device_register() below that just calls pm_runtime_get_sync() > > to force the state to on. > > > > Gah. These subtle paths always give me a headache. > > We don't normally have too much problem with this because many > > runtime_resume / suspend functions don't change reference counts. > > Just did this test, waited a few seconds, checked > /sys/kernel/debug/regulator... that the regulator had been disabled. > > Then executed: > echo -n 3-004a > /sys/bus/i2c/drivers/ads1100/unbind > > to unload the driver, and no messages were added to the kernel log. > > I could see the driver going away and removing itself from iio and > regulators. > > Tried this a couple of times (using bind/unbind), and no problem reported. > > Hopes this helps with your headaches... Thanks! >
diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml new file mode 100644 index 000000000000..970ccab15e1e --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml @@ -0,0 +1,46 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/ti,ads1100.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI ADS1100/ADS1000 single channel I2C analog to digital converter + +maintainers: + - Mike Looijmans <mike.looijmans@topic.nl> + +description: | + Datasheet at: https://www.ti.com/lit/gpn/ads1100 + +properties: + compatible: + enum: + - ti,ads1100 + - ti,ads1000 + + reg: + maxItems: 1 + + vdd-supply: true + + "#io-channel-cells": + const: 0 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + adc@49 { + compatible = "ti,ads1100"; + reg = <0x49>; + }; + }; +...
The ADS1100 is a 16-bit ADC (at 8 samples per second). The ADS1000 is similar, but has a fixed data rate. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- (no changes since v2) Changes in v2: "reg" property is mandatory. Add vdd-supply and #io-channel-cells .../bindings/iio/adc/ti,ads1100.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml