| Message ID | 20230421085245.302169-3-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add the Renesas X9250 potentiometers IIO support | expand |
On Fri, 21 Apr 2023 10:52:44 +0200 Herve Codina <herve.codina@bootlin.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@bootlin.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. 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@bootlin.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. > +}; > + > +#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. > + 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. > + > + spi_set_drvdata(spi, indio_dev); Why? Nothing seems to use it that I can see. > + > + 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. > + .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@bootlin.com>"); > +MODULE_DESCRIPTION("X9250 ALSA SoC driver"); > +MODULE_LICENSE("GPL");
Hi Jonathan, On Sat, 22 Apr 2023 17:34:53 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 21 Apr 2023 10:52:44 +0200 > Herve Codina <herve.codina@bootlin.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@bootlin.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@bootlin.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@bootlin.com>"); > > +MODULE_DESCRIPTION("X9250 ALSA SoC driver"); > > +MODULE_LICENSE("GPL"); > Thanks for the review. Best regards, Hervé
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@bootlin.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); +}; + +#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); + + 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); + + 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]; + 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; + 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; + + spi_set_drvdata(spi, indio_dev); + + 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", + .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@bootlin.com>"); +MODULE_DESCRIPTION("X9250 ALSA SoC driver"); +MODULE_LICENSE("GPL");
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@bootlin.com> --- 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