From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ppsw-50.csi.cam.ac.uk ([131.111.8.150]:42606 "EHLO ppsw-50.csi.cam.ac.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752657Ab1E0JlZ (ORCPT ); Fri, 27 May 2011 05:41:25 -0400 Message-ID: <4DDF73A3.6080306@cam.ac.uk> Date: Fri, 27 May 2011 10:49:23 +0100 From: Jonathan Cameron MIME-Version: 1.0 To: michael.hennerich@analog.com CC: "linux-iio@vger.kernel.org" , Drivers , "device-drivers-devel@blackfin.uclinux.org" Subject: Re: [Device-drivers-devel] [PATCH] IIO: ADC: New driver for AD7792/AD7793 3 Channel SPI ADC References: <1306335750-428-1-git-send-email-michael.hennerich@analog.com> <4DDE28F4.5040705@cam.ac.uk> <4DDE63B1.2060301@analog.com> <4DDE6A8D.5000007@cam.ac.uk> <4DDF5C32.6010905@analog.com> <4DDF6A2F.70102@analog.com> In-Reply-To: <4DDF6A2F.70102@analog.com> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 05/27/11 10:09, Michael Hennerich wrote: > 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); That's another option. I think I favour the clean nature of our current approach (extended appropriately). Maybe this is more general though...