On 10/18/11 15:56, Lars-Peter Clausen wrote:
> On 10/18/2011 04:34 PM, Jonathan Cameron wrote:
>> Couple of nitpicks, but nothing that actually matters so
>> make your own mind up.
>>
>> At some point we need to have a think about cleaner ways of handling that
>> powerdown mode stuff through chan_spec. It might be something people want
>> inkernel access to? One for another day though.
> Yes, definitely. I already though about this as well and also already have
> an experimental patch which adds a powerdown info attribute to chan spec.
> But I'm not quite sure yet whether this is actually the right thing to do in
> respect to that we want to add an in kernel interface. If we want to allow
> individual channels to be requested and the device only has a global power-
> down attribute, we might want to do reference counting for it. So we might
> need a extra interface for power management.
True. Not easy to do. Should also make this play nicely with runtime pm which
is going to be interesting.
I have been wondering from the practical point of view of doing this whether
we need equivalents of our chan_into mask and read / write _raw functions
for boolean values and for things best represented as strings. The strings
one is particularly tricky as we need a way of specifying which ones make sense.
Feel free to have a go at any of this. Right now I'm afraid most of my time
is going to be on the attempt to move some of this out of staging. These
corner cases can come later (even if like this one they are pretty common).
It's just possible this powerdown stuff ought to map onto the pinctrl
infrastructure that has been proposed. I haven't kept up with it enough to
be sure though.
>
>> Thanks for your work on this. Now I think all our DAC drivers can
>> be moved into my list of clean drivers :) New drivers are always nice
>> but I really really like people who clean up existing ones!
>
> I have some additional cleanups and fixes for the ad5624r driver, which had
> some problems.
Excellent :)
>
>> On 10/18/11 11:54, Lars-Peter Clausen wrote:
>>> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
>> Acked-by: Jonathan Cameron <jic23@xxxxxxxxx>
>
> Thanks,
> - Lars
>>> ---
>>> drivers/staging/iio/dac/ad5624r.h | 4 +-
>>> drivers/staging/iio/dac/ad5624r_spi.c | 123 +++++++++++++++++++++------------
>>> 2 files changed, 82 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/dac/ad5624r.h b/drivers/staging/iio/dac/ad5624r.h
>>> index b71c6a0..5dca302 100644
>>> --- a/drivers/staging/iio/dac/ad5624r.h
>>> +++ b/drivers/staging/iio/dac/ad5624r.h
>>> @@ -32,12 +32,12 @@
>>>
>>> /**
>>> * struct ad5624r_chip_info - chip specific information
>>> - * @bits: accuracy of the DAC in bits
>>> + * @channels: channel spec for the DAC
>>> * @int_vref_mv: AD5620/40/60: the internal reference voltage
>>> */
>>>
>>> struct ad5624r_chip_info {
>>> - u8 bits;
>>> + const struct iio_chan_spec *channels;
>>> u16 int_vref_mv;
>>> };
>>>
>>> diff --git a/drivers/staging/iio/dac/ad5624r_spi.c b/drivers/staging/iio/dac/ad5624r_spi.c
>>> index 284d879..d054f27 100644
>>> --- a/drivers/staging/iio/dac/ad5624r_spi.c
>>> +++ b/drivers/staging/iio/dac/ad5624r_spi.c
>>> @@ -21,29 +21,60 @@
>>> #include "dac.h"
>>> #include "ad5624r.h"
>>>
>>> +#define AD5624R_CHANNEL(_chan, _bits) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .indexed = 1, \
>>> + .output = 1, \
>>> + .channel = (_chan), \
>>> + .info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED), \
>>> + .address = (_chan), \
>>> + .scan_type = IIO_ST('u', (_bits), 16, 16 - (_bits)), \
>>> +}
>>> +
>> Could save a few lines via a trivial
>> #define AD5624_CHANNELS(bits) macro given there are always 4.
>> Perhaps not worth bothering.
>>> +static const struct iio_chan_spec ad5624r_channels_12[] = {
>>> + AD5624R_CHANNEL(0, 12),
>>> + AD5624R_CHANNEL(1, 12),
>>> + AD5624R_CHANNEL(2, 12),
>>> + AD5624R_CHANNEL(3, 12),
>>> +};
>>> +
>>> +static const struct iio_chan_spec ad5624r_channels_14[] = {
>>> + AD5624R_CHANNEL(0, 14),
>>> + AD5624R_CHANNEL(1, 14),
>>> + AD5624R_CHANNEL(2, 14),
>>> + AD5624R_CHANNEL(3, 14),
>>> +};
>>> +
>>> +static const struct iio_chan_spec ad5624r_channels_16[] = {
>>> + AD5624R_CHANNEL(0, 16),
>>> + AD5624R_CHANNEL(1, 16),
>>> + AD5624R_CHANNEL(2, 16),
>>> + AD5624R_CHANNEL(3, 16),
>>> +};
>>> +
>>> static const struct ad5624r_chip_info ad5624r_chip_info_tbl[] = {
>> I'd reorder this into alphabetical order. Took me a few secs to work
>> out what was going on (obviously not your fault but might as well tidy
>> up whilst you are here!)
>>> [ID_AD5624R3] = {
>>> - .bits = 12,
>>> + .channels = ad5624r_channels_12,
>>> .int_vref_mv = 1250,
>>> },
>>> [ID_AD5644R3] = {
>>> - .bits = 14,
>>> + .channels = ad5624r_channels_14,
>>> .int_vref_mv = 1250,
>>> },
>>> [ID_AD5664R3] = {
>>> - .bits = 16,
>>> + .channels = ad5624r_channels_16,
>>> .int_vref_mv = 1250,
>>> },
>>> [ID_AD5624R5] = {
>>> - .bits = 12,
>>> + .channels = ad5624r_channels_12,
>>> .int_vref_mv = 2500,
>>> },
>>> [ID_AD5644R5] = {
>>> - .bits = 14,
>>> + .channels = ad5624r_channels_14,
>>> .int_vref_mv = 2500,
>>> },
>>> [ID_AD5664R5] = {
>>> - .bits = 16,
>>> + .channels = ad5624r_channels_16,
>>> .int_vref_mv = 2500,
>>> },
>>> };
>>> @@ -70,24 +101,49 @@ static int ad5624r_spi_write(struct spi_device *spi,
>>> return spi_write(spi, msg, 3);
>>> }
>>>
>>> -static ssize_t ad5624r_write_dac(struct device *dev,
>>> - struct device_attribute *attr,
>>> - const char *buf, size_t len)
>>> +static int ad5624r_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val,
>>> + int *val2,
>>> + long m)
>>> {
>>> - long readin;
>>> - int ret;
>>> - struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>> struct ad5624r_state *st = iio_priv(indio_dev);
>>> - struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> + unsigned long scale_uv;
>>>
>>> - ret = strict_strtol(buf, 10, &readin);
>>> - if (ret)
>>> - return ret;
>>> + switch (m) {
>>> + case (1 << IIO_CHAN_INFO_SCALE_SHARED):
>>> + scale_uv = (st->vref_mv * 1000) >> chan->scan_type.realbits;
>>> + *val = scale_uv / 1000;
>>> + *val2 = (scale_uv % 1000) * 1000;
>>> + return IIO_VAL_INT_PLUS_MICRO;
>>>
>>> - ret = ad5624r_spi_write(st->us, AD5624R_CMD_WRITE_INPUT_N_UPDATE_N,
>>> - this_attr->address, readin,
>>> - st->chip_info->bits);
>>> - return ret ? ret : len;
>>> + }
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int ad5624r_write_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int val,
>>> + int val2,
>>> + long mask)
>>> +{
>>> + struct ad5624r_state *st = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case 0:
>>> + if (val >= (1 << chan->scan_type.realbits) || val < 0)
>>> + return -EINVAL;
>>> +
>>> + return ad5624r_spi_write(st->us,
>>> + AD5624R_CMD_WRITE_INPUT_N_UPDATE_N,
>>> + chan->address, val,
>>> + chan->scan_type.shift);
>>> + default:
>>> + ret = -EINVAL;
>>> + }
>>> +
>>> + return -EINVAL;
>>> }
>>>
>>> static ssize_t ad5624r_read_powerdown_mode(struct device *dev,
>>> @@ -161,24 +217,6 @@ static ssize_t ad5624r_write_dac_powerdown(struct device *dev,
>>> return ret ? ret : len;
>>> }
>>>
>>> -static ssize_t ad5624r_show_scale(struct device *dev,
>>> - struct device_attribute *attr,
>>> - char *buf)
>>> -{
>>> - struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>> - struct ad5624r_state *st = iio_priv(indio_dev);
>>> - /* Corresponds to Vref / 2^(bits) */
>>> - unsigned int scale_uv = (st->vref_mv * 1000) >> st->chip_info->bits;
>>> -
>>> - return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
>>> -}
>>> -static IIO_DEVICE_ATTR(out_voltage_scale, S_IRUGO, ad5624r_show_scale, NULL, 0);
>>> -
>>> -static IIO_DEV_ATTR_OUT_RAW(0, ad5624r_write_dac, AD5624R_ADDR_DAC0);
>>> -static IIO_DEV_ATTR_OUT_RAW(1, ad5624r_write_dac, AD5624R_ADDR_DAC1);
>>> -static IIO_DEV_ATTR_OUT_RAW(2, ad5624r_write_dac, AD5624R_ADDR_DAC2);
>>> -static IIO_DEV_ATTR_OUT_RAW(3, ad5624r_write_dac, AD5624R_ADDR_DAC3);
>>> -
>>> static IIO_DEVICE_ATTR(out_voltage_powerdown_mode, S_IRUGO |
>>> S_IWUSR, ad5624r_read_powerdown_mode,
>>> ad5624r_write_powerdown_mode, 0);
>>> @@ -200,17 +238,12 @@ static IIO_DEV_ATTR_DAC_POWERDOWN(3, ad5624r_read_dac_powerdown,
>>> ad5624r_write_dac_powerdown, 3);
>>>
>>> static struct attribute *ad5624r_attributes[] = {
>>> - &iio_dev_attr_out_voltage0_raw.dev_attr.attr,
>>> - &iio_dev_attr_out_voltage1_raw.dev_attr.attr,
>>> - &iio_dev_attr_out_voltage2_raw.dev_attr.attr,
>>> - &iio_dev_attr_out_voltage3_raw.dev_attr.attr,
>>> &iio_dev_attr_out_voltage0_powerdown.dev_attr.attr,
>>> &iio_dev_attr_out_voltage1_powerdown.dev_attr.attr,
>>> &iio_dev_attr_out_voltage2_powerdown.dev_attr.attr,
>>> &iio_dev_attr_out_voltage3_powerdown.dev_attr.attr,
>>> &iio_dev_attr_out_voltage_powerdown_mode.dev_attr.attr,
>>> &iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
>>> - &iio_dev_attr_out_voltage_scale.dev_attr.attr,
>>> NULL,
>>> };
>>>
>>> @@ -219,6 +252,8 @@ static const struct attribute_group ad5624r_attribute_group = {
>>> };
>>>
>>> static const struct iio_info ad5624r_info = {
>>> + .write_raw = ad5624r_write_raw,
>>> + .read_raw = ad5624r_read_raw,
>>> .attrs = &ad5624r_attribute_group,
>>> .driver_module = THIS_MODULE,
>>> };
>>> @@ -259,6 +294,8 @@ static int __devinit ad5624r_probe(struct spi_device *spi)
>>> indio_dev->name = spi_get_device_id(spi)->name;
>>> indio_dev->info = &ad5624r_info;
>>> indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->channels = st->chip_info->channels;
>>> + indio_dev->num_channels = AD5624R_DAC_CHANNELS;
>>>
>>> ret = ad5624r_spi_write(spi, AD5624R_CMD_INTERNAL_REFER_SETUP, 0,
>>> !!voltage_uv, 16);
>>
>
> --
> 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
>
--
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
