Date:   Mon, 10 Dec 2018 14:14:55 -0600
From:   Dan Murphy <dmurphy@...com>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <robh+dt@...nel.org>
Subject: Re: [PATCH 2/2] iio: adc: Add the TI ads124s08 ADC code

Jonathan

Thanks for the review

On 12/08/2018 05:56 AM, Jonathan Cameron wrote:
> On Tue, 4 Dec 2018 11:59:55 -0600
> Dan Murphy <dmurphy@...com> wrote:
> 
>> Introduce the TI ADS124S08 and the ADS124S06 ADC
>> devices from TI.  The ADS124S08 is the 12 channel ADC
>> and the ADS124S06 is the 6 channel ADC device
>>
>> These devices share a common datasheet:
>> http://www.ti.com/lit/gpn/ads124s08
>>
>> Signed-off-by: Dan Murphy <dmurphy@...com>
> Various minor things inline.
> 

No worries.  I believe I used the ADS8688 driver as a reference so that driver
may need to be updated as well

> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/Kconfig        |  10 +
>>  drivers/iio/adc/Makefile       |   1 +
>>  drivers/iio/adc/ti-ads124s08.c | 437 +++++++++++++++++++++++++++++++++
>>  3 files changed, 448 insertions(+)
>>  create mode 100644 drivers/iio/adc/ti-ads124s08.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index a52fea8749a9..e8c5686e6716 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -887,6 +887,16 @@ config TI_ADS8688
>>  	  This driver can also be built as a module. If so, the module will be
>>  	  called ti-ads8688.
>>  
>> +config TI_ADS124S08
>> +	tristate "Texas Instruments ADS124S08"
>> +	depends on SPI && OF
>> +	help
>> +	  If you say yes here you get support for Texas Instruments ADS124S08
>> +	  and ADS124S06 ADC chips
>> +
>> +	  This driver can also be built as a module. If so, the module will be
>> +	  called ti-ads124s08.
>> +
>>  config TI_AM335X_ADC
>>  	tristate "TI's AM335X ADC driver"
>>  	depends on MFD_TI_AM335X_TSCADC && HAS_DMA
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index a6e6a0b659e2..d70293abfdba 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -79,6 +79,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
>>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>> +obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
>>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>>  obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
>>  obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
>> diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c
>> new file mode 100644
>> index 000000000000..b6fc93f37355
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads124s08.c
>> @@ -0,0 +1,437 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// TI ADS124S0X chip family driver
>> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
> An oddity of current kernel style is that typically only the spdx
> line is // 
> 
> The rest are a normal kernel multiline comment.

Ack.  Finding it is up to the maintainer here on the preference so I will change it.

> 
>> +
>> +#include <linux/err.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define ADS124S08_ID		0x00
>> +#define ADS124S06_ID		0x01
> 
> Use an enum for these as the actual values don't matter, only that
> they are unique.
> 

Ack.  Is there a preference on using the sentinnal or not?

>> +
>> +/* Commands */
>> +#define ADS124S_CMD_NOP		0x00
> Why this shorter prefix? Are all ADS124S parts
> compatible with this list? 
> 

Ack.  Yes the 124S are all compatible since there are only 2 ATM.


>> +#define ADS124S_CMD_WAKEUP	0x02
>> +#define ADS124S_CMD_PWRDWN	0x04
>> +#define ADS124S_CMD_RESET	0x06
>> +#define ADS124S_CMD_START	0x08
>> +#define ADS124S_CMD_STOP	0x0a
>> +#define ADS124S_CMD_SYOCAL	0x16
>> +#define ADS124S_CMD_SYGCAL	0x17
>> +#define ADS124S_CMD_SFOCAL	0x19
>> +#define ADS124S_CMD_RDATA	0x12
>> +#define ADS124S_CMD_RREG	0x20
>> +#define ADS124S_CMD_WREG	0x40
>> +
>> +/* Registers */
>> +#define ADS124S0X_ID		0x00
> Avoid wild cards in naming.  It always goes wrong when
> along comes another part that is similar but not quite the
> same yet fits in your naming scheme.  Just pick a part
> and name after that.

The issue is that there are some registers below that are S08 only
and do not apply to the S06.

This is why I choose the wild card.  I can use the 08 since that has a master set.
Unless I can keep the 0X

> 
>> +#define ADS124S0X_STATUS	0x01
>> +#define ADS124S0X_INPUT_MUX	0x02
>> +#define ADS124S0X_PGA		0x03
>> +#define ADS124S0X_DATA_RATE	0x04
>> +#define ADS124S0X_REF		0x05
>> +#define ADS124S0X_IDACMAG	0x06
>> +#define ADS124S0X_IDACMUX	0x07
>> +#define ADS124S0X_VBIAS		0x08
>> +#define ADS124S0X_SYS		0x09
>> +#define ADS124S0X_OFCAL0	0x0a
>> +#define ADS124S0X_OFCAL1	0x0b
>> +#define ADS124S0X_OFCAL2	0x0c
>> +#define ADS124S0X_FSCAL0	0x0d
>> +#define ADS124S0X_FSCAL1	0x0e
>> +#define ADS124S0X_FSCAL2	0x0f
>> +#define ADS124S0X_GPIODAT	0x10
>> +#define ADS124S0X_GPIOCON	0x11
>> +
>> +/* ADS124S0x common channels */
>> +#define ADS124S0X_AIN0		0x00
>> +#define ADS124S0X_AIN1		0x01
>> +#define ADS124S0X_AIN2		0x02
>> +#define ADS124S0X_AIN3		0x03
>> +#define ADS124S0X_AIN4		0x04
>> +#define ADS124S0X_AIN5		0x05
>> +#define ADS124S08_AINCOM	0x0c
>> +/* ADS124S08 only channels */
>> +#define ADS124S08_AIN6		0x06
>> +#define ADS124S08_AIN7		0x07
>> +#define ADS124S08_AIN8		0x08
>> +#define ADS124S08_AIN9		0x09
>> +#define ADS124S08_AIN10		0x0a
>> +#define ADS124S08_AIN11		0x0b
>> +#define ADS124S0X_MAX_CHANNELS	12
>> +
>> +#define ADS124S0X_POS_MUX_SHIFT	0x04
>> +#define ADS124S_INT_REF		0x09
>> +
>> +#define ADS124S_START_REG_MASK	0x1f
>> +#define ADS124S_NUM_BYTES_MASK	0x1f
>> +
>> +#define ADS124S_START_CONV	0x01
>> +#define ADS124S_STOP_CONV	0x00
>> +
>> +struct ads124s_chip_info {
>> +	const struct iio_chan_spec *channels;
>> +	unsigned int num_channels;
>> +};
>> +
>> +struct ads124s_private {
>> +	const struct ads124s_chip_info	*chip_info;
>> +	struct gpio_desc *reset_gpio;
>> +	struct spi_device *spi;
>> +	struct regulator *reg;
>> +	struct mutex lock;
>> +	unsigned int vref_mv;
>> +	u8 data[ADS124S0X_MAX_CHANNELS];
> This will break in horrible ways on spi controllers using DMA.
> The data buffer needs to be in it's own cacheline to avoid
> possibly overwriting data in the rest of this structure.
> 
> ___cache_line_aligned is your friend.   Wolfram did a very
> good talk on this issue at elce. 
> https://events.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
> 
> There's a video on youtube as well.
> 
> He was addressing i2c, but the arguments still hold and unlike
> i2c, spi has always had dma controllers who assume they have
> dma safe buffers.
> 

ACK.  I will look into this.  Probably will do something similar to the 8688 buffer

>> +};
>> +
>> +#define ADS124S_CHAN(index)					\
>> +{								\
>> +	.type = IIO_VOLTAGE,					\
>> +	.indexed = 1,						\
>> +	.channel = index,					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> 
> It looks like you started adding scale but didn't finish doing so.
> It's definitely a good thing to have so please see if that can be
> added for V2.

Yes I did but could not figure out how to apply it to this part. I will investigate this in v2

> 
>> +	.scan_index = index,					\
>> +	.scan_type = {						\
>> +		.sign = 'u',					\
>> +		.realbits = 24,					\
>> +		.storagebits = 24,				\
>> +		.endianness = IIO_BE,				\
>> +	},							\
>> +}
>> +
>> +static const struct iio_chan_spec ads124s06_channels[] = {
>> +	ADS124S_CHAN(0),
>> +	ADS124S_CHAN(1),
>> +	ADS124S_CHAN(2),
>> +	ADS124S_CHAN(3),
>> +	ADS124S_CHAN(4),
>> +	ADS124S_CHAN(5),
>> +};
>> +
>> +static const struct iio_chan_spec ads124s08_channels[] = {
>> +	ADS124S_CHAN(0),
>> +	ADS124S_CHAN(1),
>> +	ADS124S_CHAN(2),
>> +	ADS124S_CHAN(3),
>> +	ADS124S_CHAN(4),
>> +	ADS124S_CHAN(5),
>> +	ADS124S_CHAN(6),
>> +	ADS124S_CHAN(7),
>> +	ADS124S_CHAN(8),
>> +	ADS124S_CHAN(9),
>> +	ADS124S_CHAN(10),
>> +	ADS124S_CHAN(11),
>> +};
>> +
>> +static const struct ads124s_chip_info ads124s_chip_info_tbl[] = {
>> +	[ADS124S08_ID] = {
>> +		.channels = ads124s08_channels,
>> +		.num_channels = ARRAY_SIZE(ads124s08_channels),
>> +	},
>> +	[ADS124S06_ID] = {
>> +		.channels = ads124s06_channels,
>> +		.num_channels = ARRAY_SIZE(ads124s06_channels),
>> +	},
>> +};
>> +
>> +static void ads124s_fill_nop(u8 *data, int start, int count)
>> +{
>> +	int i;
>> +
>> +	for (i = start; i <= count; i++)
>> +		data[i] = ADS124S_CMD_NOP;
> 
> memset
> 

ACK

>> +
>> +};
>> +
>> +static int ads124s_write_cmd(struct iio_dev *indio_dev, u8 command)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	struct spi_transfer t[] = {
>> +		{
>> +			.tx_buf = &priv->data[0],
>> +			.len = 1,
>> +		}
>> +	};
>> +
>> +	priv->data[0] = command;
>> +
>> +	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
> 
> spi_write?

ACK

> 
>> +}
>> +
>> +static int ads124s_write_reg(struct iio_dev *indio_dev, u8 reg, u8 data)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	struct spi_transfer t[] = {
>> +		{
>> +			.tx_buf = &priv->data[0],
>> +			.len = 3,
>> +		}
>> +	};
>> +
>> +	priv->data[0] = ADS124S_CMD_WREG | reg;
>> +	priv->data[1] = 0x0;
>> +	priv->data[2] = data;
>> +
>> +	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
> 
> spi_write?
> 

ACK

>> +}
>> +
>> +static int ads124s_reset(struct iio_dev *indio_dev)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +
>> +	if (priv->reset_gpio) {
>> +		gpiod_direction_output(priv->reset_gpio, 0);
> I'd expect that gpio to always be in the output
> direction, so just being controlled here.
> 
> So gpiod_set_value

ACK

> 
>> +		udelay(200);
>> +		gpiod_direction_output(priv->reset_gpio, 1);
>> +	} else {
>> +		return ads124s_write_cmd(indio_dev, ADS124S_CMD_RESET);
>> +	}
>> +
>> +	return 0;
>> +};
>> +
>> +static int ads124s_read(struct iio_dev *indio_dev, unsigned int chan)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	int ret;
>> +	u32 tmp;
>> +	struct spi_transfer t[] = {
>> +		{
>> +			.tx_buf = &priv->data[0],
>> +			.len = 4,
>> +			.cs_change = 1,
>> +		}, {
>> +			.tx_buf = &priv->data[1],
>> +			.rx_buf = &priv->data[1],
>> +			.len = 4,
>> +		},
>> +	};
> 
> Pity we don't have a spi_write_the_read_with_cs or
> something like that.  I wonder how common this structure is?

This is common with this part and the ads8688 and another spi part I am working on for CAN.

I just don't know if there are any parts that hold CS for more then one data xfer.

> 
>> +
>> +	priv->data[0] = ADS124S_CMD_RDATA;
>> +	ads124s_fill_nop(priv->data, 1, 3);
>> +
>> +	ret = spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	tmp = priv->data[2] << 16 | priv->data[3] << 8 | priv->data[4];
>> +
>> +	return tmp;
>> +}
>> +
>> +static int ads124s_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long m)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_lock(&priv->lock);
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX,
>> +					chan->channel);
>> +		if (ret) {
>> +			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
> 
> You are eating a bus error in favour of -EINVAL.  Please don't!

ACK.  I was debating on setting ret here so I will set ret and return that instead.
Rinse and repeat for the ones below too.

> 
>> +			goto out;
>> +		}
>> +
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
>> +		if (ret) {
>> +			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
>> +			goto out;
>> +		}
>> +
>> +		ret = ads124s_read(indio_dev, chan->channel);
>> +		if (ret < 0) {
>> +			dev_err(&priv->spi->dev, "Read ADC failed\n");
>> +			goto out;
>> +		} else
>> +			*val = ret;
> 
> This is the same code block as found in the buffered version below.  Factor
> it out to a utility function and use it both paths.

It is not exactly the same but I will see where I can consolidate the code.
The buffered version loops through and reads every channel this reads just one.

> 
>> +
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
>> +		if (ret) {
>> +			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
>> +			goto out;
>> +		}
>> +
>> +		mutex_unlock(&priv->lock);
>> +		if (ret < 0)
>> +			return ret;
> Given you need to unlock in both this path and the error path, cleaner
> to just set ret here and break.  Then return ret below having set
> it appropriately in the error paths.

ACK.  Similar to the above.

>> +
>> +		return IIO_VAL_INT;
>> +	default:
>> +		break;
>> +	}
>> +out:
>> +	mutex_unlock(&priv->lock);
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info ads124s_info = {
>> +	.read_raw = &ads124s_read_raw,
>> +};
>> +
>> +static irqreturn_t ads124s_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	u16 buffer[ADS124S0X_MAX_CHANNELS];
> 
> There is an unfortunate oddity in how push_to_buffers_with_timestamp
> works in that it builds the data for the kfifo inline in the buffer
> provided.  Thus that buffer has to have room for the channels and
> the 64 bit timestamp.

Hmmm.  This is like the 8688.  I am starting to think the 8688 needs to be updated.

> 
>> +	int i, j = 0;
>> +	int ret;
>> +
>> +	for (i = 0; i < indio_dev->masklength; i++) {
>> +		if (!test_bit(i, indio_dev->active_scan_mask))
> 
> for_each_bit_set 

Same as above.  But I can change it.

> 
>> +			continue;
>> +
>> +		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX, i);
> 
> Does this need to be rewritten if the channel is already correct?

This is an iterative loop so the channel should be different each time 0 - 12

> 
>> +		if (ret)
>> +			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
>> +
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
>> +		if (ret)
>> +			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
>> +
>> +		buffer[j] = ads124s_read(indio_dev, i);
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
>> +		if (ret)
>> +			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
>> +
>> +		j++;
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>> +			pf->timestamp);
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int ads124s_probe(struct spi_device *spi)
>> +{
>> +	struct ads124s_private *ads124s_priv;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*ads124s_priv));
>> +	if (indio_dev == NULL)
>> +		return -ENOMEM;
>> +
>> +	ads124s_priv = iio_priv(indio_dev);
>> +
>> +	ads124s_priv->reg = devm_regulator_get_optional(&spi->dev, "vref");
>> +	if (!IS_ERR(ads124s_priv->reg)) {
>> +		ret = regulator_enable(ads124s_priv->reg);
>> +		if (ret)
>> +			return ret;
> 
> 	As mentioned below, use devm_add_action_or_reset to provide
> 	automatic disabling of this regulator letting you used managed
> 	cleanup throughout. 

ACK.  will look into these APIs

> 
>> +
>> +		ads124s_priv->vref_mv = regulator_get_voltage(ads124s_priv->reg);
> 
> This doesn't actually seem to be used...  As a general rule the only time you 
> want to read this is to provide a scale value.  As that isn't a fast path
> it's better to read it where needed in case the value is changing (not unusual
> during boot up if the voltage is being used for several things).

I will look into this if I add the scale otherwise I will remove it.

> 
>> +		if (ads124s_priv->vref_mv <= 0)
>> +			goto err_regulator_disable;
>> +	} else {
>> +		/* Use internal reference */
>> +		ads124s_priv->vref_mv = 0;
>> +	}
>> +
>> +	ads124s_priv->reset_gpio = devm_gpiod_get_optional(&spi->dev,
>> +						   "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ads124s_priv->reset_gpio))
>> +		dev_info(&spi->dev, "Reset GPIO not defined\n");
>> +
>> +	ads124s_priv->chip_info = &ads124s_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +
>> +	ads124s_priv->spi = spi;
>> +
>> +	indio_dev->name = spi_get_device_id(spi)->name;
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->dev.of_node = spi->dev.of_node;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = ads124s_priv->chip_info->channels;
>> +	indio_dev->num_channels = ads124s_priv->chip_info->num_channels;
>> +	indio_dev->info = &ads124s_info;
>> +
>> +	mutex_init(&ads124s_priv->lock);
>> +
>> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +					 ads124s_trigger_handler, NULL);
>> +	if (ret < 0) {
>> +		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
>> +		goto err_regulator_disable;
>> +	}
>> +
>> +	ads124s_reset(indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		goto err_buffer_cleanup;
>> +
>> +	return 0;
>> +
>> +err_buffer_cleanup:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +err_regulator_disable:
>> +	if (!IS_ERR(ads124s_priv->reg))
>> +		regulator_disable(ads124s_priv->reg);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ads124s_remove(struct spi_device *spi)
>> +{
>> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	iio_triggered_buffer_cleanup(indio_dev);
> Both of these functions can be automatically cleaned
> up if you use the devm_ variants of the registration functions
> which makes me suspicious..  Ah. The regulator disable
> is missing.

Ack.  Regulator is based on above

> 
> You can move to fully managed if you use devm_add_action_or_reset
> to turn the regulator off.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id ads124s_id[] = {
>> +	{ "ads124s08", ADS124S08_ID },
>> +	{ "ads124s06", ADS124S06_ID },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, ads124s_id);
>> +
>> +static const struct of_device_id ads124s_of_table[] = {
>> +	{ .compatible = "ti,ads124s08" },
>> +	{ .compatible = "ti,ads124s06" },
> 
> It's trivial but numerical order preferred as it makes it
> obvious where to add new devices later.

Actually I based these off the ID read from the device.
The S08 ID is 0x0 and the S06 is 0x1.  Bit I can reorder this since it just the compatible.

Dan

> 
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ads124s_of_table);
>> +
>> +static struct spi_driver ads124s_driver = {
>> +	.driver = {
>> +		.name	= "ads124s",
>> +		.of_match_table = ads124s_of_table,
>> +	},
>> +	.probe		= ads124s_probe,
>> +	.remove		= ads124s_remove,
>> +	.id_table	= ads124s_id,
>> +};
>> +module_spi_driver(ads124s_driver);
>> +
>> +MODULE_AUTHOR("Dan Murphy <dmuprhy@...com>");
>> +MODULE_DESCRIPTION("TI TI_ADS12S0X ADC");
>> +MODULE_LICENSE("GPL v2");
> 


-- 
------------------
Dan Murphy

Powered by blists - more mailing lists