From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <4DDF6A2F.70102@analog.com> Date: Fri, 27 May 2011 11:09:03 +0200 From: Michael Hennerich Reply-To: MIME-Version: 1.0 To: "Hennerich, Michael" CC: Jonathan Cameron , "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> In-Reply-To: <4DDF5C32.6010905@analog.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed List-ID: 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