Hi, On 01/19/2013 04:05 PM, christophe leroy wrote: > Hi Lars, > > Sorry to respond so late, I've been very busy lately. > Please see answers/questions below > > Main point is that our driver is mainly copied and adapted from AD7298 > driver, and your comments are on things that we have not modified from > AD7298, so what should we do really ? The AD7288 driver has recently seen some updates. The issues which your driver inherited from the AD7288 driver have been fixed in the original driver. See: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=drivers/iio/adc/ad7298.c;h=b34d754994d50d53f60fee694440658ba0b137dd;hb=HEAD > > We are a bit new comers in Kernel Development, so thanks for your help. > > Christophe > > Le 08/01/2013 11:27, Lars-Peter Clausen a écrit : >> On 01/08/2013 09:42 AM, Christophe Leroy wrote: >>> This patch adds support for Analog Devices AD7923 ADC in the IIO >>> Subsystem. >>> >>> Signed-off-by: Patrick Vasseur <patrick.vasseur@xxxxxx> >>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx> >> Hi, >> >> Thanks for the driver, looks pretty good. Some comments inline. >> >> - Lars >> >>> diff -uN linux-3.7.1/drivers/staging/iio/adc/Kconfig >>> linux/drivers/staging/iio/adc/Kconfig >>> --- linux-3.7.1/drivers/staging/iio/adc/Kconfig 2012-12-17 >>> 20:14:54.000000000 +0100 >>> +++ linux/drivers/staging/iio/adc/Kconfig 2012-12-13 >>> 19:52:10.000000000 +0100 >> New IIO drivers should not be added to staging, unless there is a very >> good >> reason. Since this driver does not have any major issues it should >> directly go >> into drivers/iio/ > Our driver is based on AD7298 driver, which is today in staging. Should > we put ours in drivers/iio/ directly anyway ? >> >> [...] >> [...] >>> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c >>> linux/drivers/staging/iio/adc/ad7923_core.c >>> --- linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c 1970-01-01 >>> 01:00:00.000000000 +0100 >>> +++ linux/drivers/staging/iio/adc/ad7923_core.c 2013-01-05 >> [...] >>> + >>> +static int ad7923_read_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *chan, >>> + int *val, >>> + int *val2, >>> + long m) >>> +{ >>> + int ret; >>> + struct ad7923_state *st = iio_priv(indio_dev); >>> + >>> + switch (m) { >>> + case IIO_CHAN_INFO_RAW: >>> + mutex_lock(&indio_dev->mlock); >>> + if (iio_buffer_enabled(indio_dev)) >>> + ret = -EBUSY; >>> + else >>> + ret = ad7923_scan_direct(st, chan->address); >>> + mutex_unlock(&indio_dev->mlock); >>> + >>> + if (ret < 0) >>> + return ret; >>> + if (chan->address == EXTRACT(ret, 12, 4)) { >>> + *val = EXTRACT(ret, 0, 12); >>> + *val2 = EXTRACT_PERCENT(*val, 12); >> val2 is never used in the upper layers in this case. > I don't understand what you mean. Just drop the *val2 = ... line. It doesn't make any sense in this context. Also make sure to remove the EXTRACT_PERCENT macro since it wont be used anymore. > Again, this is copied from AD7298 >> [...] >>> +/** >>> + * ad7923_update_scan_mode() setup the spi transfer buffer for the >>> new scan mask >>> + **/ >>> +int ad7923_update_scan_mode(struct iio_dev *indio_dev, >>> + const unsigned long *active_scan_mask) >>> +{ >>> + struct ad7923_state *st = iio_priv(indio_dev); >>> + int i, cmd, channel; >>> + int scan_count; >>> + >>> + /* Now compute overall size */ >>> + for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++) >>> + if (test_bit(i, active_scan_mask)) >>> + channel = i; >>> + >>> + cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE | >>> + AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) | >>> + AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_ON) | >>> + AD7923_CHANNEL_WRITE(channel); >> Hm, ok this looks a bit strange. You lookup the first channel which is >> selected >> and then also sample all channels which come before it. E.g. if only >> channel 2 >> is selected you sample channel 0, 1 and 2. In the trigger handler you >> push the >> buffer to the IIO core. But in your buffer you have the result of >> channel 0 in >> the position where IIO would expect channel 2. >> I think it is better if you send a cmd for each channel that needs to be >> samples. E.g.: >> >> if (for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN)) { >> cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE | >> AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) | >> AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) | >> AD7923_CHANNEL_WRITE(i); >> cmd <<= AD7923_SHIFT_REGISTER; >> st->tx_buf[j++] = cpu_to_be16(cmd); >> } > Ok, here we are trying to use the sequence mode. But unlike the AD7298, > here sequence mode is only able to go from channel 0 to a given channel. > Hence the reason why we try to identify the highest requested channel, > then we do a sequential read of all from 0 to this one. > The issue is that this doesn't work for all configurations. E.g. imagine channel 0 is not selected and channel 1 is selected. You are now going to sample both channel 0 and channel 1. Although you should only sample channel 1. > Wouldn't the use on non sequential mode be less performant ? I'm not sure whether the sequential mode actually gives you better performance or whether it's just for convenience. But you can add a special case to the sample function where it will use the sequential mode when possible. - Lars -- 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
