On 11/10/2011 05:07 PM, Jean-François Dagenais wrote: > v1: first proposal, works with i2c AD5622 > Hi, Thanks for your patch. I've recently send a patch which converts the ad5446 to channel_spec, it would be good if you could rebase v2 of this patch ontop of it. Unfortunately the patch isn't in GregKH tree yet, but you can grab it from the mailing list archives: http://marc.info/?l=linux-iio&m=131914551420107&q=raw Some further comments inline. > The patch also contains some cleanup of defines that were not > used. Cleanups should go in a separate patch. Makes it easier to review them. > --- > drivers/staging/iio/dac/Kconfig | 27 +++- > drivers/staging/iio/dac/Makefile | 2 + > drivers/staging/iio/dac/ad5446-i2c.c | 135 ++++++++++++++++++ > drivers/staging/iio/dac/ad5446-spi.c | 259 ++++++++++++++++++++++++++++++++++ > drivers/staging/iio/dac/ad5446.c | 258 +++++++--------------------------- > drivers/staging/iio/dac/ad5446.h | 93 +++++------- > 6 files changed, 510 insertions(+), 264 deletions(-) > create mode 100644 drivers/staging/iio/dac/ad5446-i2c.c > create mode 100644 drivers/staging/iio/dac/ad5446-spi.c > > diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig > index 7ddae35..bb960b5 100644 > --- a/drivers/staging/iio/dac/Kconfig > +++ b/drivers/staging/iio/dac/Kconfig > @@ -11,16 +11,37 @@ config AD5624R_SPI > AD5664R convertors (DAC). This driver uses the common SPI interface. > > config AD5446 > - tristate "Analog Devices AD5444/6, AD5620/40/60 and AD5542A/12A DAC SPI driver" > - depends on SPI > + tristate "Analog Devices AD5444/6, AD5620/40/60/02/12/22 and AD5542A/12A DAC SPI/I2C driver" > help > Say yes here to build support for Analog Devices AD5444, AD5446, > AD5512A, AD5542A, AD5543, AD5553, AD5601, AD5611, AD5620, AD5621, > - AD5640, AD5660 DACs. > + AD5640, AD5660, AD5601, AD5612, AD5622 DACs. > > To compile this driver as a module, choose M here: the > module will be called ad5446. > > +config AD5446_SPI > + tristate "support SPI bus connection" > + depends on AD5446 && SPI > + default y > + help > + Say Y here if you have AD5444/6, AD5620/40/60 and AD5542A/12A > + DAC connected to a SPI bus. > + > + To compile this driver as a module, choose M here: the > + module will be called ad5446-spi. > + > +config AD5446_I2C > + tristate "support I2C bus connection" > + depends on AD5446 && I2C > + default y > + help > + Say Y here if you have AD5602/12/22 DAC connected to an I2C bus. > + > + To compile this driver as a module, choose M here: the > + module will be called ad5446-i2c. > + > + I don't think we need to split this into two separate modules. Having one handling both should be fine. The I2C and SPI specific sections should be rather small and can be put in a #ifdef CONFIG_... block. > config AD5504 > tristate "Analog Devices AD5504/AD5501 DAC SPI driver" > depends on SPI > diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile > index 7f4f2ed..481a1e9 100644 > --- a/drivers/staging/iio/dac/Makefile > +++ b/drivers/staging/iio/dac/Makefile > @@ -5,6 +5,8 @@ > obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o > obj-$(CONFIG_AD5504) += ad5504.o > obj-$(CONFIG_AD5446) += ad5446.o > +obj-$(CONFIG_AD5446_I2C) += ad5446-i2c.o > +obj-$(CONFIG_AD5446_SPI) += ad5446-spi.o > obj-$(CONFIG_AD5791) += ad5791.o > obj-$(CONFIG_AD5686) += ad5686.o > obj-$(CONFIG_MAX517) += max517.o > diff --git a/drivers/staging/iio/dac/ad5446-i2c.c b/drivers/staging/iio/dac/ad5446-i2c.c > new file mode 100644 > index 0000000..cee606e > --- /dev/null > +++ b/drivers/staging/iio/dac/ad5446-i2c.c > @@ -0,0 +1,135 @@ > +/* > + * AD5446 DAC driver SPI specific > + * > + * Copyright 2010 Analog Devices Inc. > + * > + * Licensed under the GPL-2 or later. > + */ > + > +#include <linux/device.h> > +#include <linux/i2c.h> > + > +#include "../iio.h" > + > +#include "ad5446.h" > + > +static int ad5446_i2c_sync_to_chip(struct ad5446_state *st) > +{ > + struct i2c_client *client = to_i2c_client(st->dev); > + int error = i2c_transfer(client->adapter, &st->proto.i2c.msg, 1); > + if (unlikely(error < 0)) { > + dev_err(st->dev, "I2C write error: %d\n", error); > + return error; > + } Just use i2c_master_send. > + > + return 0; > +} > + [...] > @@ -52,8 +37,19 @@ struct ad5446_state { > unsigned cached_val; > unsigned pwr_down_mode; > unsigned pwr_down; > - struct spi_transfer xfer; > - struct spi_message msg; > + union { > +#ifdef __enabled_CONFIG_AD5446_SPI Uhm, I think the __enabled_ defines were not intended to be used directly. You should rather use the IS_ENABLED macro instead. > + struct { > + struct spi_transfer xfer; > + struct spi_message msg; > + } spi; > +#endif > +#ifdef __enabled_CONFIG_AD5446_I2C > + struct { > + struct i2c_msg msg; > + } i2c; > +#endif > + } proto; > union { > unsigned short d16; > unsigned char d24[3]; > @@ -65,7 +61,7 @@ struct ad5446_state { > * @bits: accuracy of the DAC in bits > * @storagebits: number of bits written to the DAC > * @left_shift: number of bits the datum must be shifted > - * @int_vref_mv: AD5620/40/60: the internal reference voltage > + * @int_vref_mv: AD5620/40/60: the internal reference voltage, 0 means vcc ref > * @store_sample: chip specific helper function to store the datum > * @store_sample: chip specific helper function to store the powerpown cmd > */ > @@ -77,33 +73,22 @@ struct ad5446_chip_info { > u16 int_vref_mv; > void (*store_sample) (struct ad5446_state *st, unsigned val); > void (*store_pwr_down) (struct ad5446_state *st, unsigned mode); > + int (*sync_to_chip) (struct ad5446_state *st); I would put the sync callback into the ad5446_state struct and initialize it in the I2C and SPI probe function. No need to store this information on a per chip basis. And maybe call it "write" or something instead. [...] > #endif /* IIO_DAC_AD5446_H_ */ -- 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
