On Sun, Sep 18, 2016 at 4:20 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On 16/09/16 06:32, Matt Ranostay wrote:
>> Allow IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_OFFSET attributes for
>> processing by checking voltage from a regulator.
>>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> Comments inline.
>> ---
>> .../devicetree/bindings/iio/adc/ti-adc161s626.txt | 2 +
>> drivers/iio/adc/ti-adc161s626.c | 60 ++++++++++++++++++----
>> 2 files changed, 51 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
>> index 2bdad2d13d6f..0a50ee9541c8 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
>> @@ -3,6 +3,7 @@
>> Required properties:
>> - compatible: Should be "ti,adc141s626" or "ti,adc161s626"
>> - reg: spi chip select number for the device
>> + - vdda-supply: supply voltage to VDDA pin
>>
>> Recommended properties:
>> - spi-max-frequency: Definition as per
>> @@ -11,6 +12,7 @@ Recommended properties:
>> Example:
>> adc@0 {
>> compatible = "ti,adc161s626";
>> + vdda-supply = <&vdda_fixed>;
>> reg = <0>;
>> spi-max-frequency = <4300000>;
>> };
>> diff --git a/drivers/iio/adc/ti-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c
>> index f94b69f9c288..d192951056cf 100644
>> --- a/drivers/iio/adc/ti-adc161s626.c
>> +++ b/drivers/iio/adc/ti-adc161s626.c
>> @@ -27,6 +27,7 @@
>> #include <linux/iio/buffer.h>
>> #include <linux/iio/trigger_consumer.h>
>> #include <linux/iio/triggered_buffer.h>
>> +#include <linux/regulator/consumer.h>
>>
>> #define TI_ADC_DRV_NAME "ti-adc161s626"
>>
>> @@ -39,7 +40,9 @@ static const struct iio_chan_spec ti_adc141s626_channels[] = {
>> {
>> .type = IIO_VOLTAGE,
>> .channel = 0,
>> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_OFFSET),
>> .scan_index = 0,
>> .scan_type = {
>> .sign = 's',
>> @@ -54,7 +57,9 @@ static const struct iio_chan_spec ti_adc161s626_channels[] = {
>> {
>> .type = IIO_VOLTAGE,
>> .channel = 0,
>> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_OFFSET),
>> .scan_index = 0,
>> .scan_type = {
>> .sign = 's',
>> @@ -68,6 +73,8 @@ static const struct iio_chan_spec ti_adc161s626_channels[] = {
>> struct ti_adc_data {
>> struct iio_dev *indio_dev;
>> struct spi_device *spi;
>> + struct regulator *ref;
>> +
>> u8 read_size;
>> u8 shift;
>>
>> @@ -135,18 +142,32 @@ static int ti_adc_read_raw(struct iio_dev *indio_dev,
>> struct ti_adc_data *data = iio_priv(indio_dev);
>> int ret;
>>
>> - if (mask != IIO_CHAN_INFO_RAW)
>> - return -EINVAL;
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = iio_device_claim_direct_mode(indio_dev);
>> + if (ret)
>> + return ret;
>>
>> - ret = iio_device_claim_direct_mode(indio_dev);
>> - if (ret)
>> - return ret;
>> + ret = ti_adc_read_measurement(data, chan, val);
>> + iio_device_release_direct_mode(indio_dev);
>>
>> - ret = ti_adc_read_measurement(data, chan, val);
>> - iio_device_release_direct_mode(indio_dev);
>> + if (ret)
>> + return ret;
>>
>> - if (!ret)
>> return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SCALE:
>> + ret = regulator_get_voltage(data->ref);
>> + if (ret < 0)
>> + return ret;
>> +
>> + *val = ret / 1000;
>> + *val2 = chan->scan_type.realbits;
>> +
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> + case IIO_CHAN_INFO_OFFSET:
>> + *val = 1 << (chan->scan_type.realbits - 1);
>> + return IIO_VAL_INT;
>> + }
>>
>> return 0;
>> }
>> @@ -191,10 +212,22 @@ static int ti_adc_probe(struct spi_device *spi)
>> break;
>> }
>>
>> + data->ref = devm_regulator_get(&spi->dev, "vdda");
>> + if (!IS_ERR(data->ref)) {
>> + ret = regulator_enable(data->ref);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = regulator_get_voltage(data->ref);
>> + if (ret < 0)
>> + goto error_regulator_disable;
>> + } else
>> + return IS_ERR(data->ref);
> This is an overly complex way of organizing things.
> Check the error and return first.
>
> Then the good path just becomes normal flow.
>
> There is also the irritating case of the regulator not being specified.
> Then we get a dummy which is fine until we ask it what voltage it
> is supplying..
>
> In that case I think we get a return -EINVAL.
> I'd have no trouble with us returning -EINVAL on an attempt to read
> the scale attribute if the regulator isn't specified, but refusing to
> probe seems harsh. Particularly as before this support was added it
> would have probed fine.
Ok will fix in v2
>> +
>> ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> ti_adc_trigger_handler, NULL);
>> if (ret)
>> - return ret;
>> + goto error_regulator_disable;
>>
>> ret = iio_device_register(indio_dev);
>> if (ret)
>> @@ -205,15 +238,20 @@ static int ti_adc_probe(struct spi_device *spi)
>> error_unreg_buffer:
>> iio_triggered_buffer_cleanup(indio_dev);
>>
>> +error_regulator_disable:
>> + regulator_disable(data->ref);
>> +
>> return ret;
>> }
>>
>> static int ti_adc_remove(struct spi_device *spi)
>> {
>> struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> + struct ti_adc_data *data = iio_priv(indio_dev);
>>
>> iio_device_unregister(indio_dev);
>> iio_triggered_buffer_cleanup(indio_dev);
>> + regulator_disable(data->ref);
>>
>> return 0;
>> }
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
