Re: [PATCH] IIO: ADC: New driver for AD7792/AD7793 3 Channel SPI ADC

[Michael Hennerich <michael.hennerich@analog.com>]

On 05/26/2011 04:58 PM, Jonathan Cameron wrote:
> ...
>>> What would happen if this driver used any other trigger?  Would everything work?
>> No. But other drivers can you the trigger. It's not really an trigger it's a data ready.
> Most are.  As you say, it is useful to trigger other reads from this, but not to trigger
> this to read from other sources...
>>> I think it would do an immediate read which is going to be a problem.  Perhaps
>>> we need a way of restricting triggers.  This one can be used by anyone, but the
>>> part can only use it's own trigger (I think).
>> Having the ability to reject alien triggers are nice to have.
> True enough.  I guess the easiest is some sort of 'filter' callback on trigger connect.
> Then drivers that care, can reject devices that don't match what they need.  Would
> probably want one in each direction.  Trigggers can reject devices and devices can
> reject triggers.
>
>>> Looked is a bool really, might as well make it explicit. Reg can only be a couple
>>> of bytes, so maybe a u8?  Doesn't really matter though.
>>>> +static int __ad7793_write_reg(struct ad7793_state *st, unsigned locked,
>>>> +                           unsigned cs_change, unsigned reg,
>>>> +                           unsigned size, unsigned val)
>>>> +{
>>>> +     u8 data[4];
>>> Worth putting in board state?
>> I'll add data to the state structure.
>>
>>>> +     struct spi_transfer t = {
>>>> +             .tx_buf         = data,
>>>> +             .len            = size + 1,
>>>> +             .cs_change      = cs_change,
>>>> +     };
>>>> +     struct spi_message m;
>>>> +
>>>> +     data[0] = AD7793_COMM_WRITE | AD7793_COMM_ADDR(reg);
>>>> +
>>>> +     switch (size) {
>>>> +     case 3:
>>>> +             data[1] = val>>   16;
>>>> +             data[2] = val>>   8;
>>>> +             data[3] = val;
>>>> +             break;
>>>> +     case 2:
>>>> +             data[1] = val>>   8;
>>>> +             data[2] = val;
>>>> +             break;
>>>> +     case 1:
>>>> +             data[1] = val;
>>>> +             break;
>>> This is a bit nasty, but I can see why you did it.  Though it would give
>>> longer code, I'd be inclined to move the data[0] assignment for all of the
>>> above cases into the switch statement.  Then this last element fits
>>> in better with the rest.
>> Actually I don't use this function with size=0, so I remove this part completely.
>> Originally it was intended to allow access to the COMM register in order to enable CSREAD.
>>
> Good, that's even better.
>
> ...
>
>>>> +     ret = ad7793_write_reg(st, AD7793_REG_MODE, sizeof(st->mode), st->mode);
>>>> +     if (ret)
>>>> +             goto out;
>>>> +     /* write/read test for device presence */
>>> Hmm.. this sort of test is always pretty hit and miss.  I'd just assume the
>>> board config is correct and not bother with the test when there isn't a who_am_I
>>> register available...
>>>
>> Actually there is an id register that we can query.
>>
> Yeah, I noticed that when looking at the datasheet later in the review.
> Much better idea ;)
>>>> +static int ad7793_ring_postdisable(struct iio_dev *indio_dev)
>>>> +{
>>>> +     struct ad7793_state *st = iio_priv(indio_dev);
>>>> +
>>>> +     st->mode  = (st->mode&   ~AD7793_MODE_SEL(-1)) |
>>>> +                 AD7793_MODE_SEL(AD7793_MODE_IDLE);
>>>> +
>>>> +     st->done = false;
>>>> +     wait_event_interruptible(st->wq_data_avail, st->done);
>>> So basically this is waiting for one last wakeup to occur before
>>> disabling the irq?
>> Yes - for CREAD mode is is mandatory that the device is RDY, when
>> exiting the continuous conversion mode. For continuous conversion mode
>> not using CREAD we can write to the mode register anytime.
>>>> +
>>>> +     if (!st->irq_dis)
>>>> +             disable_irq_nosync(st->spi->irq);
>>>> +
>>>> +     __ad7793_write_reg(st, 1, 0, AD7793_REG_MODE,
>>>> +                        sizeof(st->mode), st->mode);
>>>> +
>>>> +
>>>> +     return spi_bus_unlock(st->spi->master);
>>>> +}
>>>> +
>>>> +/**
>>>> + * ad7793_trigger_handler() bh of trigger launched polling to ring buffer
>>>> + **/
>>>> +
>>>> +static irqreturn_t ad7793_trigger_handler(int irq, void *p)
>>>> +{
>>>> +     struct iio_poll_func *pf = p;
>>>> +     struct iio_dev *indio_dev = pf->private_data;
>>>> +     struct iio_ring_buffer *ring = indio_dev->ring;
>>>> +     struct ad7793_state *st = iio_priv(indio_dev);
>>> I like this approach to alignment, nice and tidy ;)
>>>> +     s64 dat64[2];
>>>> +     s32 *dat32 = (s32 *)dat64;
>>>> +
>>> On this front, is it not worth using CREAD bit of the communications register?
>>> Then if I understand correctly, there is no need to do the write element
>>> of this read?
>> Originally - I thought to use the CREAD, but exiting this mode is not 100% error prone.
>> See my comment above.
> Hmm.. That is somewhat awkward, so I guess what you have is pretty much the only option.
> ...
>>>> +&iio_dev_attr_sampling_frequency.dev_attr.attr,
>>>> +&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>>> +&iio_dev_attr_range.dev_attr.attr,
>>>> +&iio_dev_attr_range_available.dev_attr.attr,
>>> hmm. I've always been keener on controlling range via 'scale' parameters.
>>> Or does this mean something else for this part?
>> Well - range implies the maximum input voltage that can be applied.
>> Scale means something different for me - but I might be wrong.
> They tend to be closely connected.  So many bits exist, and they apply over
> a certain range.  That means the scale factor to be applied also changes
> as one changes the range. Often it's just a matter of picking one of
> scale and range to make controllable, and having the other change
> explicitly.  We have to have scale available for raw attributes, whereas
> range is optional, so I'd generally advocate changing scale to change
> the range rather than the other way around..
>
>

Hi Jonathan,


root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> ls
device0:buffer0               power
in-in_scale                   range
in0-in0_raw                   range_available
in1-in1_raw                   sampling_frequency
in2-in2_raw                   sampling_frequency_available
in3_raw                       subsystem
in4_supply_raw                temp0_raw
in4_supply_scale              temp_scale
in_scale                      trigger
name                          uevent

root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat in_scale
0.000140

root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat range_available
2500 1250 625 312 156 78 39 19

root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> echo 312 > range
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat in_scale
0.000010

root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> echo 78 > range
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat in_scale
0.000000
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0>

with these 24-bit converters and input AMPs we are already exhausted
the number of available digits we have for scale.

What shall we do?

Also how would you name AIN1(-) - AIN1(-)?

#define AD7793_CH_AIN1P_AIN1M        0 /* AIN1(+) - AIN1(-) */
#define AD7793_CH_AIN2P_AIN2M        1 /* AIN2(+) - AIN2(-) */
#define AD7793_CH_AIN3P_AIN3M        2 /* AIN3(+) - AIN3(-) */
#define AD7793_CH_AIN1M_AIN1M        3 /* AIN1(-) - AIN1(-) */

in0-in0_zerooffset_raw ?


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif