Message-ID: <56112D76.90206@kernel.org>
Date:	Sun, 4 Oct 2015 14:45:26 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Paul Cercueil <paul.cercueil@...log.com>
Cc:	Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Peter Meerwald <pmeerw@...erw.net>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Antonio Fiol <antonio@...l.es>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: dac: Add support for the AD5592R/AD5593R
 ADCs/DACs

On 02/10/15 13:45, Paul Cercueil wrote:
> This patch adds support for the AD5592R (spi) and AD5593R (i2c)
> ADC/DAC devices.
> 
> Signed-off-by: Paul Cercueil <paul.cercueil@...log.com>
SMBUS makes the assumption that the wire order is little endian. As such
all the i2c drivers should be doing an necessary endian conversions
for word write / read internally.  Hence if the wire order is be16 you'll
want unconditional swaps.  Note we have i2c_smbus_read_word_swapped
and i2c_smbus_write_word_swapped for this.

I've mentioned this in a few places inline but there are probably others.
See Documentation/i2c/smbus-protocol.txt for more details.

Various other bits inline.

I would in particular like you to use a common module to handle the base
part rather than linking this object file into two separate modules..

Jonathan
> ---
>  drivers/iio/dac/Kconfig               |  22 +++
>  drivers/iio/dac/Makefile              |   2 +
>  drivers/iio/dac/ad5592r-base.c        | 290 ++++++++++++++++++++++++++++++++++
>  drivers/iio/dac/ad5592r-base.h        |  59 +++++++
>  drivers/iio/dac/ad5592r.c             | 121 ++++++++++++++
>  drivers/iio/dac/ad5593r.c             | 121 ++++++++++++++
>  include/dt-bindings/iio/adi,ad5592r.h |  13 ++
>  7 files changed, 628 insertions(+)
>  create mode 100644 drivers/iio/dac/ad5592r-base.c
>  create mode 100644 drivers/iio/dac/ad5592r-base.h
>  create mode 100644 drivers/iio/dac/ad5592r.c
>  create mode 100644 drivers/iio/dac/ad5593r.c
>  create mode 100644 include/dt-bindings/iio/adi,ad5592r.h
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index e701e28..e520059 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -72,6 +72,28 @@ config AD5449
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad5449.
>  
> +config AD5592R
> +	tristate "Analog Devices AD5592R ADC/DAC driver"
> +	depends on SPI_MASTER
> +	depends on OF
> +	help
> +	  Say yes here to build support for Analog Devices AD5592R
> +	  Digital to Analog / Analog to Digital Converter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5592r.
> +
> +config AD5593R
> +	tristate "Analog Devices AD5593R ADC/DAC driver"
> +	depends on I2C
> +	depends on OF
> +	help
> +	  Say yes here to build support for Analog Devices AD5593R
> +	  Digital to Analog / Analog to Digital Converter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5593r.
> +
>  config AD5504
>  	tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>  	depends on SPI
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 63ae056..181fc28 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -11,6 +11,8 @@ obj-$(CONFIG_AD5064) += ad5064.o
>  obj-$(CONFIG_AD5504) += ad5504.o
>  obj-$(CONFIG_AD5446) += ad5446.o
>  obj-$(CONFIG_AD5449) += ad5449.o
> +obj-$(CONFIG_AD5592R) += ad5592r.o ad5592r-base.o
> +obj-$(CONFIG_AD5593R) += ad5593r.o ad5592r-base.o
Note the majority of distros will just enable anything and everything.  Hence
most of the time they'll both be built.  As such I'd prefer you to make
the minor mods needed to have a common base module rather than dublicating
the *-base.o object in both modules.

>  obj-$(CONFIG_AD5755) += ad5755.o
>  obj-$(CONFIG_AD5764) += ad5764.o
>  obj-$(CONFIG_AD5791) += ad5791.o
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> new file mode 100644
> index 0000000..347f209
> --- /dev/null
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -0,0 +1,290 @@
> +/*
> + * AD5592R Digital <-> Analog converters driver
> + *
> + * Copyright 2014 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil@...log.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +/*
> + * TODO:
> + *     - Add support for using channels as GPIOs
> + */
> +
> +#include "ad5592r-base.h"
> +
> +#include <dt-bindings/iio/adi,ad5592r.h>
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +
> +static int ad5592r_set_channel_modes(struct ad5592r_state *st)
> +{
> +	const struct ad5592r_rw_ops *ops = st->ops;
> +	int ret;
> +	unsigned i;
> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
> +	u8 pulldown = 0, open_drain = 0, tristate = 0,
> +	   dac = 0, adc = 0, gpio_in = 0, gpio_out = 0;
> +	u16 read_back;
> +
> +	for (i = 0; i < st->chip_info->num_channels; i++) {
> +		switch (st->channel_modes[i]) {
> +		case CH_MODE_DAC:
> +			dac |= BIT(i);
> +
> +		/* fall-through */
> +		case CH_MODE_ADC:
> +			adc |= BIT(i);
> +			break;
> +
> +		case CH_MODE_GPIO_OUT:
> +			gpio_out |= BIT(i);
> +
> +		/* fall-through */
> +		case CH_MODE_GPIO_IN:
> +			gpio_in |= BIT(i);
> +			break;
> +
> +		case CH_MODE_GPIO_OPEN_DRAIN:
> +			open_drain |= BIT(i);
> +			break;
> +
> +		case CH_MODE_GPIO_TRISTATE:
> +			tristate |= BIT(i);
> +			break;
> +
> +		default:
> +			pulldown |= BIT(i);
> +			break;
> +		}
> +	}
> +
> +	mutex_lock(&iio_dev->mlock);
> +
> +	/* Configure pins that we use */
> +	ret = ops->reg_write(st, AD5592R_REG_DAC_EN, dac);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_ADC_EN, adc);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, gpio_out);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, gpio_in);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_OPEN_DRAIN, open_drain);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_TRISTATE, tristate);
> +	if (ret)
> +		goto err_unlock;
> +
> +	/* Pull down unused pins to GND */
> +	ret = ops->reg_write(st, AD5592R_REG_PULLDOWN, pulldown);
> +	if (ret)
> +		goto err_unlock;
> +
> +	/* Verify that we can read back at least one register */
> +	ret = ops->reg_read(st, AD5592R_REG_ADC_EN, &read_back);
> +	if (!ret && (read_back & 0xff) != adc)
> +		ret = -EIO;
> +
> +err_unlock:
> +	mutex_unlock(&iio_dev->mlock);
> +	return ret;
> +}
> +
> +static int ad5592r_write_raw(struct iio_dev *iio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long mask)
> +{
> +	struct ad5592r_state *st = iio_priv(iio_dev);
> +	int ret = -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val >= (1 << chan->scan_type.realbits) || val < 0)
> +			return -EINVAL;
> +
> +		mutex_lock(&iio_dev->mlock);
> +		ret = st->ops->dac_write(st, chan->address, val);
> +		mutex_unlock(&iio_dev->mlock);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int ad5592r_read_raw(struct iio_dev *iio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long m)
> +{
> +	struct ad5592r_state *st = iio_priv(iio_dev);
> +	int ret = -EINVAL;
The only path this is relevant in is the default in the switch
statement. Just return -EINVAL there and drop the assignment here.

> +	u16 read_val;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&iio_dev->mlock);
> +		ret = st->ops->adc_read(st, chan->channel, &read_val);
> +		mutex_unlock(&iio_dev->mlock);
> +		if (ret)
> +			return ret;
> +
> +		if ((read_val >> 12 & 0x7) != chan->channel) {
> +			dev_err(st->dev, "Error while reading channel %u\n",
> +					chan->channel);
> +			return -EIO;
> +		}
> +
> +		read_val &= GENMASK(11, 0);
> +
> +		dev_dbg(st->dev, "ADC read: 0x%04hX\n", read_val);
> +		*val = (int) read_val;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static void ad5592r_reset(struct ad5592r_state *st)
> +{
> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
> +
> +	mutex_lock(&iio_dev->mlock);
> +	st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
> +	udelay(250);
> +	mutex_unlock(&iio_dev->mlock);
> +}
> +
> +static const struct iio_info ad5592r_info = {
> +	.read_raw = ad5592r_read_raw,
> +	.write_raw = ad5592r_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static void ad5592r_setup_channel(struct iio_dev *iio_dev,
> +		struct iio_chan_spec *chan, bool output, unsigned id)
> +{
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->output = output;
> +	chan->channel = id;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->address = id;
> +	chan->scan_type.sign = 'u';
> +	chan->scan_type.realbits = 12;
> +	chan->scan_type.storagebits = 16;
> +}
> +
> +static int ad5592r_alloc_channels(struct ad5592r_state *st)
> +{
> +	unsigned i, curr_channel = 0,
> +		 num_channels = st->chip_info->num_channels;
> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
> +	struct iio_chan_spec *channels;
> +	int ret;
> +
> +	ret = of_property_read_u8_array(st->dev->of_node, "channel-modes",
> +			st->channel_modes, num_channels);
> +	if (ret)
> +		return ret;
> +
> +	channels = devm_kzalloc(st->dev,
> +			2 * num_channels * sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_channels; i++) {
> +		switch (st->channel_modes[i]) {
> +		case CH_MODE_DAC:
> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
> +					true, curr_channel);
> +			curr_channel++;
> +			break;
> +
> +		case CH_MODE_ADC:
> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
> +					false, curr_channel);
> +			curr_channel++;
> +
> +		/* fall-through */
> +		default:
> +			continue;
> +		}
> +	}
> +
> +	iio_dev->num_channels = curr_channel;
> +	iio_dev->channels = channels;
Blank line here.
> +	return 0;
> +}
> +
> +int ad5592r_probe(struct device *dev, const char *name,
> +		const struct ad5592r_rw_ops *ops,
> +		const struct ad5592r_chip_info *chip_info)
> +{
> +	struct iio_dev *iio_dev;
> +	struct ad5592r_state *st;
> +	int ret;
> +
> +	iio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(iio_dev);
> +	st->dev = dev;
> +	st->ops = ops;
> +	st->chip_info = chip_info;
> +	dev_set_drvdata(dev, iio_dev);
> +
> +	BUG_ON(st->chip_info->num_channels > 8);
I'd just fail to probe with an appropriate error message rather than
a BUG_ON call...
> +
> +	iio_dev->dev.parent = dev;
> +	iio_dev->name = name;
> +	iio_dev->info = &ad5592r_info;
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ad5592r_reset(st);
> +
> +	ret = ad5592r_alloc_channels(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5592r_set_channel_modes(st);
> +	if (ret)
> +		return ret;
> +
> +	return iio_device_register(iio_dev);
> +}
> +EXPORT_SYMBOL_GPL(ad5592r_probe);
> +
> +int ad5592r_remove(struct device *dev)
> +{
> +	struct iio_dev *iio_dev = dev_get_drvdata(dev);
> +
> +	iio_device_unregister(iio_dev);
Currently you have no unwinding of state of the device
in the remove functions.  If it stays like that you can use
the devm_iio_device_register route and kill off all the
remove functions.  I would have expected there to be an
optimal state to leave I/Os in though from a power point of
view so you  probaby do want to do something in the remove...
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ad5592r_remove);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/dac/ad5592r-base.h b/drivers/iio/dac/ad5592r-base.h
> new file mode 100644
> index 0000000..40ef550
> --- /dev/null
> +++ b/drivers/iio/dac/ad5592r-base.h
> @@ -0,0 +1,59 @@
> +/*
> + * AD5592R / AD5593R Digital <-> Analog converters driver
> + *
> + * Copyright 2015 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil@...log.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#ifndef __DRIVERS_IIO_DAC_AD5592R_BASE_H__
> +#define __DRIVERS_IIO_DAC_AD5592R_BASE_H__
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct ad5592r_state;
> +
> +enum ad5592r_registers {
> +	AD5592R_REG_NOOP		= 0x0,
> +	AD5592R_REG_DAC_READBACK	= 0x1,
> +	AD5592R_REG_ADC_SEQ		= 0x2,
> +	AD5592R_REG_CTRL		= 0x3,
> +	AD5592R_REG_ADC_EN		= 0x4,
> +	AD5592R_REG_DAC_EN		= 0x5,
> +	AD5592R_REG_PULLDOWN		= 0x6,
> +	AD5592R_REG_LDAC		= 0x7,
> +	AD5592R_REG_GPIO_OUT_EN		= 0x8,
> +	AD5592R_REG_GPIO_SET		= 0x9,
> +	AD5592R_REG_GPIO_IN_EN		= 0xA,
> +	AD5592R_REG_PD			= 0xB,
> +	AD5592R_REG_OPEN_DRAIN		= 0xC,
> +	AD5592R_REG_TRISTATE		= 0xD,
> +	AD5592R_REG_RESET		= 0xF,
> +};
> +
> +struct ad5592r_chip_info {
> +	unsigned num_channels;
> +};
> +
> +struct ad5592r_rw_ops {
> +	int (*dac_write)(struct ad5592r_state *st, unsigned chan, u16 value);
> +	int (*adc_read)(struct ad5592r_state *st, unsigned chan, u16 *value);
> +	int (*reg_write)(struct ad5592r_state *st, u8 reg, u16 value);
> +	int (*reg_read)(struct ad5592r_state *st, u8 reg, u16 *value);
> +};
> +
> +struct ad5592r_state {
> +	struct device *dev;
> +	const struct ad5592r_chip_info *chip_info;
> +	const struct ad5592r_rw_ops *ops;
> +	u8 channel_modes[8];
> +};
> +
> +int ad5592r_probe(struct device *dev, const char *name,
> +		const struct ad5592r_rw_ops *ops,
> +		const struct ad5592r_chip_info *chip_info);
> +int ad5592r_remove(struct device *dev);
> +
> +#endif /* __DRIVERS_IIO_DAC_AD5592R_BASE_H__ */
> diff --git a/drivers/iio/dac/ad5592r.c b/drivers/iio/dac/ad5592r.c
> new file mode 100644
> index 0000000..663dfd9
> --- /dev/null
> +++ b/drivers/iio/dac/ad5592r.c
> @@ -0,0 +1,121 @@
> +/*
> + * AD5592R Digital <-> Analog converters driver
> + *
> + * Copyright 2015 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil@...log.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include "ad5592r-base.h"
> +
> +#include <linux/bitops.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +static int ad5592r_dac_write(struct ad5592r_state *st, unsigned chan, u16 value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +	u16 msg = cpu_to_be16(BIT(15) | (chan << 12) | value);
> +
> +	return spi_write(spi, &msg, sizeof(msg));
> +}
> +
> +static int ad5592r_adc_read(struct ad5592r_state *st, unsigned chan, u16 *value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +	u16 msg = cpu_to_be16((AD5592R_REG_ADC_SEQ << 11) | BIT(chan));
> +	int ret;
> +
> +	ret = spi_write(spi, &msg, sizeof(msg));
> +	if (ret)
> +		return ret;
> +
> +	spi_read(spi, &msg, sizeof(msg)); /* Invalid data */
Should still be checking for errors.
> +
> +	ret = spi_read(spi, &msg, sizeof(msg));
> +	if (ret)
> +		return ret;
> +
> +	*value = be16_to_cpu(msg);
blank line here.
> +	return 0;
> +}
> +
> +static int ad5592r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +	u16 msg = cpu_to_be16((reg << 11) | value);
Unlike i2c, there are not guarantees on wire ordering for spi so you
are correct in using cpu_to_be16 here.
> +
> +	return spi_write(spi, &msg, sizeof(msg));
> +}
> +
> +static int ad5592r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +	u16 msg = cpu_to_be16((AD5592R_REG_LDAC << 11) | BIT(6) | (reg << 2));
> +	int ret;
> +
> +	ret = spi_write(spi, &msg, sizeof(msg));

Now, msg is on the stack. So there are no guarantees on what else is in
it's cacheline.  SPI buffers passed to spi_write/spi_read need to be
DMA safe to avoid cacheline corruption.   Hence thy need to be on the heap
and appropriately protected against other elements of any containing structure
being corrupted. __cachline_aligned etc - check other drivers for how this is
done.

Or just use spi_write_then_read which uses buffers allocated by the spi
subsystem to avoid any such problem but will do additional copies.
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_read(spi, &msg, sizeof(msg));
> +	if (ret)
> +		return ret;
> +
> +	if (value)
> +		*value = be16_to_cpu(msg);
blank line here.
> +	return 0;
> +}
> +
> +static const struct ad5592r_rw_ops ad5592r_rw_ops = {
> +	.dac_write = ad5592r_dac_write,
> +	.adc_read = ad5592r_adc_read,
> +	.reg_write = ad5592r_reg_write,
> +	.reg_read = ad5592r_reg_read,
> +};
> +
> +static const struct ad5592r_chip_info ad5592r_chip_info = {
> +	.num_channels = 8,
> +};
> +
> +static int ad5592r_spi_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	return ad5592r_probe(&spi->dev, id->name,
> +			&ad5592r_rw_ops, &ad5592r_chip_info);
> +}
> +
> +static int ad5592r_spi_remove(struct spi_device *spi)
> +{
> +	return ad5592r_remove(&spi->dev);
> +}
> +
> +static const struct spi_device_id ad5592r_spi_ids[] = {
> +	{ .name = "ad5592r", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad5592r_spi_ids);
> +
> +static const struct of_device_id ad5592r_of_match[] = {
> +	{ .compatible = "adi,ad5592r", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ad5592r_of_match);
> +
> +static struct spi_driver ad5592r_spi_driver = {
> +	.driver = {
> +		.name = "ad5592r",
> +		.of_match_table = of_match_ptr(ad5592r_of_match),
> +	},
> +	.probe = ad5592r_spi_probe,
> +	.remove = ad5592r_spi_remove,
> +	.id_table = ad5592r_spi_ids,
> +};
> +module_spi_driver(ad5592r_spi_driver);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
> new file mode 100644
> index 0000000..94ebba1
> --- /dev/null
> +++ b/drivers/iio/dac/ad5593r.c
> @@ -0,0 +1,121 @@
> +/*
> + * AD5593R Digital <-> Analog converters driver
> + *
> + * Copyright 2015 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil@...log.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include "ad5592r-base.h"
> +
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
These should all be prefixed with the driver name.
AD5593R_MODE_CONF
> +#define MODE_CONF		(0 << 4)
> +#define MODE_DAC_WRITE		(1 << 4)
> +#define MODE_ADC_READBACK	(4 << 4)
> +#define MODE_DAC_READBACK	(5 << 4)
> +#define MODE_GPIO_READBACK	(6 << 4)
> +#define MODE_REG_READBACK	(7 << 4)
> +
> +static int ad5593r_dac_write(struct ad5592r_state *st, unsigned chan, u16 value)
> +{
> +	struct i2c_client *i2c = container_of(st->dev, struct i2c_client, dev);
> +
> +	value = cpu_to_be16(value);
> +	return i2c_smbus_write_word_data(i2c, MODE_DAC_WRITE | chan, value);
Again, use the swapped variant.
> +}
> +
> +static int ad5593r_adc_read(struct ad5592r_state *st, unsigned chan, u16 *value)
> +{
> +	struct i2c_client *i2c = container_of(st->dev, struct i2c_client, dev);
> +	s32 val;
> +
> +	val = i2c_smbus_write_word_data(i2c, MODE_CONF | AD5592R_REG_ADC_SEQ,
> +			cpu_to_be16(BIT(chan)));
> +	if (val < 0)
> +		return (int) val;
> +
> +	i2c_smbus_read_word_data(i2c, MODE_ADC_READBACK); /* Invalid data */
Should still check for an error condition even if you are really just
forcing the i2c clock to run.
> +
> +	val = i2c_smbus_read_word_data(i2c, MODE_ADC_READBACK);
> +	if (val < 0)
> +		return (int) val;
> +
> +	*value = be16_to_cpu((u16) val);
Same comments apply here as the similar funcitons below.
> +	return 0;
> +}
> +
> +static int ad5593r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
> +{
> +	struct i2c_client *i2c = container_of(st->dev, struct i2c_client, dev);
Use to_i2c_client(st->dev);

> +
> +	value = cpu_to_be16(value);
> +	return i2c_smbus_write_word_data(i2c, MODE_CONF | reg, value);
This is odd.  The I2C functions assume that the wire order is le16 but
the functions will be provided with cpu endian data.

If you want to have be16 wire order then you want to swap unconditionally
I think.
> +}
> +
> +static int ad5593r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
> +{
> +	struct i2c_client *i2c = container_of(st->dev, struct i2c_client, dev);
> +	s32 val;
> +
> +	val = i2c_smbus_read_word_data(i2c, MODE_REG_READBACK | reg);
> +	if (val < 0)
> +		return (int) val;
> +
> +	*value = be16_to_cpu((u16) val);
Would assume casting to __be16 would be more appropriate.
Also I believe smbus transfers are assumed to be le16 so this should be
an unconditional swap whatever the endianness of the host cpu.

blank line here preferred.
> +	return 0;
> +}
> +
> +static const struct ad5592r_rw_ops ad5593r_rw_ops = {
> +	.dac_write = ad5593r_dac_write,
> +	.adc_read = ad5593r_adc_read,
> +	.reg_write = ad5593r_reg_write,
> +	.reg_read = ad5593r_reg_read,
> +};
> +
This is rather minimalist! I would drop this structure for now and
reintroduced it when presumably you need it to add more functionality
in the future?
> +static const struct ad5592r_chip_info ad5592r_chip_info = {
> +	.num_channels = 8,
> +};
> +
> +static int ad5593r_i2c_probe(struct i2c_client *i2c,
> +		const struct i2c_device_id *id)
> +{
> +	return ad5592r_probe(&i2c->dev, id->name,
> +			&ad5593r_rw_ops, &ad5592r_chip_info);
> +}
> +
> +static int ad5593r_i2c_remove(struct i2c_client *i2c)
> +{
> +	return ad5592r_remove(&i2c->dev);
> +}
> +
> +static const struct i2c_device_id ad5593r_i2c_ids[] = {
> +	{ .name = "ad5593r", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, ad5593r_i2c_ids);
> +
> +static const struct of_device_id ad5593r_of_match[] = {
> +	{ .compatible = "adi,ad5593r", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ad5593r_of_match);
> +
> +static struct i2c_driver ad5593r_driver = {
> +	.driver = {
> +		.name = "ad5593r",
> +		.of_match_table = of_match_ptr(ad5593r_of_match),
> +	},
> +	.probe = ad5593r_i2c_probe,
> +	.remove = ad5593r_i2c_remove,
> +	.id_table = ad5593r_i2c_ids,
> +};
> +module_i2c_driver(ad5593r_driver);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/dt-bindings/iio/adi,ad5592r.h b/include/dt-bindings/iio/adi,ad5592r.h
> new file mode 100644
> index 0000000..fcb6293
> --- /dev/null
> +++ b/include/dt-bindings/iio/adi,ad5592r.h
> @@ -0,0 +1,13 @@
> +
> +#ifndef _DT_BINDINGS_ADI_AD5592R_H
> +#define _DT_BINDINGS_ADI_AD5592R_H
> +
> +#define CH_MODE_UNUSED		0
> +#define CH_MODE_DAC		1
> +#define CH_MODE_ADC		2
> +#define CH_MODE_GPIO_OUT	3
> +#define CH_MODE_GPIO_IN		4
> +#define CH_MODE_GPIO_OPEN_DRAIN	5
> +#define CH_MODE_GPIO_TRISTATE	6
> +
> +#endif /* _DT_BINDINGS_ADI_AD5592R_H */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists