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

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

On 05/27/2011 10:09 AM, Michael Hennerich wrote:
> 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?

I think everything atof() or scanf() eats, should be fine.
Let's introduce an exponent?

     int (*read_raw)(struct iio_dev *indio_dev,
             struct iio_chan_spec const *chan,
             int *val,
             int *val2,
             int exp,
             long mask);

> 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
>
>
> _______________________________________________
> Device-drivers-devel mailing list
> Device-drivers-devel@blackfin.uclinux.org
> https://blackfin.uclinux.org/mailman/listinfo/device-drivers-devel
>


-- 
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