Message-ID: <20230424093011.1301c739@bootlin.com>
Date:   Mon, 24 Apr 2023 09:30:11 +0200
From:   Herve Codina <herve.codina@...tlin.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 2/3] iio: potentiometer: Add support for the Renesas
 X9250 potentiometers

Hi Jonathan,

On Sat, 22 Apr 2023 17:34:53 +0100
Jonathan Cameron <jic23@...nel.org> wrote:

> On Fri, 21 Apr 2023 10:52:44 +0200
> Herve Codina <herve.codina@...tlin.com> wrote:
> 
> > The Renesas X9250 integrates four digitally controlled potentiometers.
> > On each potentiometer, the X9250T has a 100 kOhms total resistance and
> > the X9250U has a 50 kOhms total resistance.
> > 
> > Signed-off-by: Herve Codina <herve.codina@...tlin.com>  
> 
> Hi Herve,
> 
> Nice clean driver.  Comments are mostly about ways to avoid boilerplate
> and simplify the code a bit.
> 
> Note that the binding comment on regulators should be matched by
> appropriate devm_get_regulator_enabled() calls in here to ensure they 
> are turned on.

Yes, I will use devm_get_regulator_enabled().

> 
> Jonathan
> 
> > ---
> >  drivers/iio/potentiometer/Kconfig  |  10 ++
> >  drivers/iio/potentiometer/Makefile |   1 +
> >  drivers/iio/potentiometer/x9250.c  | 234 +++++++++++++++++++++++++++++
> >  3 files changed, 245 insertions(+)
> >  create mode 100644 drivers/iio/potentiometer/x9250.c
> > 
> > diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> > index 01dd3f858d99..e6a9a3c67845 100644
> > --- a/drivers/iio/potentiometer/Kconfig
> > +++ b/drivers/iio/potentiometer/Kconfig
> > @@ -136,4 +136,14 @@ config TPL0102
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called tpl0102.
> >  
> > +config X9250
> > +	tristate "Renesas X9250 quad controlled potentiometers"
> > +	depends on SPI
> > +	help
> > +	  Enable support for the Renesas X9250 quad controlled
> > +	  potentiometers.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called x9250.
> > +
> >  endmenu
> > diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> > index 5ebb8e3bbd76..d11fb739176c 100644
> > --- a/drivers/iio/potentiometer/Makefile
> > +++ b/drivers/iio/potentiometer/Makefile
> > @@ -15,3 +15,4 @@ obj-$(CONFIG_MCP4131) += mcp4131.o
> >  obj-$(CONFIG_MCP4531) += mcp4531.o
> >  obj-$(CONFIG_MCP41010) += mcp41010.o
> >  obj-$(CONFIG_TPL0102) += tpl0102.o
> > +obj-$(CONFIG_X9250) += x9250.o
> > diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> > new file mode 100644
> > index 000000000000..c6bc959205a3
> > --- /dev/null
> > +++ b/drivers/iio/potentiometer/x9250.c
> > @@ -0,0 +1,234 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *
> > + * x9250.c  --  Renesas X9250 potentiometers IIO driver
> > + *
> > + * Copyright 2023 CS GROUP France
> > + *
> > + * Author: Herve Codina <herve.codina@...tlin.com>
> > + */
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/limits.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/spi/spi.h>
> > +
> > +struct x9250_cfg {
> > +	int kohms;
> > +};
> > +
> > +struct x9250 {
> > +	struct spi_device *spi;
> > +	const struct x9250_cfg *cfg;
> > +	struct mutex lock; /* Protect tx and rx buffers */
> > +	/* Cannot use stack area for SPI (dma-safe memory) */
> > +	u8 spi_tx_buf[3] __aligned(IIO_DMA_MINALIGN);
> > +	u8 spi_rx_buf[3] __aligned(IIO_DMA_MINALIGN);  
> 
> Technically you don't need to align the second one.  A particular peripheral
> is always assumed to not step on it self.  So as long as only that
> peripheral is accessing data in a cacheline then it's fine.
> 
> Note that below I suggest you just use spi_write_then_read() for all these
> cases allowing  you to drop these and the lock.

Indeed, I will give a try to spi_write_then_read()

> 
> > +};
> > +
> > +#define X9250_ID		0x50
> > +#define X9250_CMD_RD_WCR(_p)    (0x90 | (_p))
> > +#define X9250_CMD_WR_WCR(_p)    (0xa0 | (_p))
> > +
> > +static int x9250_write8(struct x9250 *x9250, u8 cmd, u8 val)
> > +{
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = &x9250->spi_tx_buf,
> > +		.len = 3,
> > +	};
> > +	int ret;
> > +
> > +	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);  
> 
> I don't see an advantage in this check as the buffer
> is allocated only a few lines above here and will have size
> of 3.
> 
> If there were lots of different uses of that buffer
> then it would be fair enough, but there aren't.
> Anyhow, follow advice below and the buffers go away anyway
> making this point irrelevant :)
> 
> > +
> > +	mutex_lock(&x9250->lock);
> > +
> > +	x9250->spi_tx_buf[0] = X9250_ID;
> > +	x9250->spi_tx_buf[1] = cmd;
> > +	x9250->spi_tx_buf[2] = val;
> > +
> > +	ret = spi_sync_transfer(x9250->spi, &xfer, 1);  
> 
> Given buffer is small, I'd be tempted to use the 'cheat' approach
> of spi_write_then_read() with a 0 sized read.  Avoids the need
> for ensuring dma safe buffers etc and having to handle the buffer
> locking in your driver.
> 
> > +
> > +	mutex_unlock(&x9250->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int x9250_read8(struct x9250 *x9250, u8 cmd, u8 *val)
> > +{
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = &x9250->spi_tx_buf,
> > +		.rx_buf = &x9250->spi_rx_buf,
> > +		.len = 3,
> > +	};
> > +	int ret;
> > +
> > +	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);
> > +	BUILD_BUG_ON(sizeof(x9250->spi_rx_buf) < 3);
> > +
> > +	mutex_lock(&x9250->lock);
> > +
> > +	x9250->spi_tx_buf[0] = X9250_ID;
> > +	x9250->spi_tx_buf[1] = cmd;
> > +
> > +	ret = spi_sync_transfer(x9250->spi, &xfer, 1);
> > +	if (ret)
> > +		goto end;
> > +
> > +	*val = x9250->spi_rx_buf[2];  
> 
> Cleaner as an spi_write_then_read() as transfer is not actually
> duplex.  + then you don't need to bother with your own locks
> for the buffer. The spi core can do that for you (and provide
> DMA safe buffers as needed).
> 
> 
> > +	ret = 0;
> > +end:
> > +	mutex_unlock(&x9250->lock);
> > +	return ret;
> > +}
> > +
> > +#define X9250_CHANNEL(ch) {						\
> > +	.type = IIO_RESISTANCE,						\
> > +	.indexed = 1,							\
> > +	.output = 1,							\
> > +	.channel = (ch),						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> > +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW),	\
> > +}
> > +
> > +static const struct iio_chan_spec x9250_channels[] = {
> > +	X9250_CHANNEL(0),
> > +	X9250_CHANNEL(1),
> > +	X9250_CHANNEL(2),
> > +	X9250_CHANNEL(3),
> > +};
> > +
> > +static int x9250_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > +			  int *val, int *val2, long mask)
> > +{
> > +	struct x9250 *x9250 = iio_priv(indio_dev);
> > +	int ch = chan->channel;
> > +	int ret;
> > +	u8 v;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = x9250_read8(x9250, X9250_CMD_RD_WCR(ch), &v);
> > +		if (ret)
> > +			return ret;
> > +		*val = v;
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 1000 * x9250->cfg->kohms;
> > +		*val2 = U8_MAX;
> > +		return IIO_VAL_FRACTIONAL;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int x9250_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > +			    const int **vals, int *type, int *length, long mask)
> > +{
> > +	static const int range[] = {0, 1, 255}; /* min, step, max */
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		*length = ARRAY_SIZE(range);
> > +		*vals = range;
> > +		*type = IIO_VAL_INT;
> > +		return IIO_AVAIL_RANGE;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int x9250_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > +			   int val, int val2, long mask)
> > +{
> > +	struct x9250 *x9250 = iio_priv(indio_dev);
> > +	int ch = chan->channel;
> > +
> > +	if (mask != IIO_CHAN_INFO_RAW)
> > +		return -EINVAL;
> > +
> > +	if (val > U8_MAX || val < 0)
> > +		return -EINVAL;
> > +
> > +	return x9250_write8(x9250, X9250_CMD_WR_WCR(ch), val);
> > +}
> > +
> > +static const struct iio_info x9250_info = {
> > +	.read_raw = x9250_read_raw,
> > +	.read_avail = x9250_read_avail,
> > +	.write_raw = x9250_write_raw,
> > +};
> > +
> > +enum x9250_type {
> > +	X9250T,
> > +	X9250U,
> > +};
> > +
> > +static const struct x9250_cfg x9250_cfg[] = {
> > +	[X9250T] = { .kohms =  100, },
> > +	[X9250U] = { .kohms =  50, },
> > +};
> > +
> > +static int x9250_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct x9250 *x9250;
> > +	int ret;
> > +
> > +	spi->bits_per_word = 8;  
> 
> This is configuring it to the default value.  You shouldn't need to
> call spi_setup() explicitly. It will already have been called by
> the spi subsystem as part of adding the device.  If you feel this
> has important documentation value, then I don't mind it. However
> we rarely do this unless we need to choose a non default value.

Agree.
I will remove in next iteration.

> 
> 
> > +	ret = spi_setup(spi);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*x9250));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	x9250 = iio_priv(indio_dev);
> > +	x9250->spi = spi;
> > +	x9250->cfg = device_get_match_data(&spi->dev);
> > +	if (!x9250->cfg)
> > +		x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
> > +
> > +	mutex_init(&x9250->lock);
> > +
> > +	indio_dev->info = &x9250_info;
> > +	indio_dev->channels = x9250_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(x9250_channels);
> > +	indio_dev->name = spi_get_device_id(spi)->name;  
> 
> Not relying on that preferred because the two tables may get out
> of sync and spi_get_device_id() fail to match as a result.
> 
> Put a const char *name  in the x9250 structure and repeat
> the names there.  Let the compiler do the magic of fusing the
> identical strings if it can.

Ok,
I will add a name field in the x9250_cfg structure and use this new field
here.

> 
> 
> > +
> > +	spi_set_drvdata(spi, indio_dev);  
> 
> Why?  Nothing seems to use it that I can see.

Indeed, nothing use the drv data. I will remove in the next iteration.

> 
> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id x9250_of_match[] = {
> > +	{ .compatible = "renesas,x9250t", &x9250_cfg[X9250T]},
> > +	{ .compatible = "renesas,x9250u", &x9250_cfg[X9250U]},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, x9250_of_match);
> > +
> > +static const struct spi_device_id x9250_id_table[] = {
> > +	{ "x9250t", X9250T },
> > +	{ "x9250u", X9250U },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, x9250_id_table);
> > +
> > +static struct spi_driver x9250_spi_driver = {
> > +	.driver  = {
> > +		.name   = "x9250",  
> 
> I'd stick to a single space before the =
> It just ends up messy if you attempt to align things with
> extra spaces.

Will be fixed.

> 
> > +		.of_match_table = x9250_of_match,
> > +	},
> > +	.id_table = x9250_id_table,
> > +	.probe  = x9250_probe,
> > +};
> > +
> > +module_spi_driver(x9250_spi_driver);
> > +
> > +MODULE_AUTHOR("Herve Codina <herve.codina@...tlin.com>");
> > +MODULE_DESCRIPTION("X9250 ALSA SoC driver");
> > +MODULE_LICENSE("GPL");  
> 

Thanks for the review.

Best regards,
Hervé

Powered by blists - more mailing lists