Message-ID: <20181210203931.2975ef15@archlinux>
Date:   Mon, 10 Dec 2018 20:39:31 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Dan Murphy <dmurphy@...com>
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

Hi Dan,

Replies to queries inline.

On Mon, 10 Dec 2018 14:14:55 -0600
Dan Murphy <dmurphy@...com> wrote:

> 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

Great if you don't mind fixing the buffer size issue in there 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?

If the sentinel is useful for a loop or range check then yes, if not
then don't bother.

> 
> >> +
> >> +/* 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

Use 08 seems like a good solution as they are definitely valid for that.

...

> >> +
> >> +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.

There definitely are though I couldn't name one right now.
Some devices use the CS to reset a simple state machine.  If you drop
it then the state machine resets, even mid read.

...
> 
> >   
> >> +
> >> +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.

Yes that looks wrong.  Thanks!

> 
> >   
> >> +	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

doh.  I'm not sure how I misread that.  However, not true for the raw
read above I think?
   
...
> > 
> > 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.

Given that reason for the order is hidden by the defines
I'd reorder to avoid someone else coming and tidying it up later!

> 
> Dan
...

Jonathan

Powered by blists - more mailing lists