Hi Jonathan, Sorry for the delay, I've been away from keyboard for a few days.
On Sun, Jul 29, 2018 at 12:44:22PM +0100, Jonathan Cameron wrote: > On Tue, 24 Jul 2018 20:30:02 +0200 > Marcus Folkesson <[email protected]> wrote: > > > MCP3911 is a dual channel Analog Front End (AFE) containing two > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC). > > > > Signed-off-by: Marcus Folkesson <[email protected]> > > Signed-off-by: Kent Gustavsson <[email protected]> > Hi > > I think you missed cleaning up the clock enable in the remove. > If it is safe to not do so it should also be safe in the error path > of probe. I've actually noticed this and had it prepared in v4 :-) > > A couple of trivial other things jumped out at me whilst reading. Thank you so much, I will fix these. > > Thanks, > > Jonathan > > --- > > > > Notes: > > v2: > > - cleanups and bugfixes (thanks Peter Meerwald-Stadler) > > - drop hardware gain > > - use the presence or lack of regulator to indicate if we go > > for internal or external voltage reference > > - do not store device node in private struct > > - drop support to set width in devicetree > > - use the presence or lack of clock to indicate if we go for > > internal or external clock > > > > drivers/iio/adc/Kconfig | 10 ++ > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/mcp3911.c | 366 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 377 insertions(+) > > create mode 100644 drivers/iio/adc/mcp3911.c > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > index 15606f237480..f9a41fa96fcc 100644 > > --- a/drivers/iio/adc/Kconfig > > +++ b/drivers/iio/adc/Kconfig > > @@ -501,6 +501,16 @@ config MCP3422 > > This driver can also be built as a module. If so, the module will be > > called mcp3422. > > > > +config MCP3911 > > + tristate "Microchip Technology MCP3911 driver" > > + depends on SPI > > + help > > + Say yes here to build support for Microchip Technology's MCP3911 > > + analog to digital converter. > > + > > + This driver can also be built as a module. If so, the module will be > > + called mcp3911. > > + > > config MEDIATEK_MT6577_AUXADC > > tristate "MediaTek AUXADC driver" > > depends on ARCH_MEDIATEK || COMPILE_TEST > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > index 28a9423997f3..3cfebfff7d26 100644 > > --- a/drivers/iio/adc/Makefile > > +++ b/drivers/iio/adc/Makefile > > @@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o > > obj-$(CONFIG_MAX9611) += max9611.o > > obj-$(CONFIG_MCP320X) += mcp320x.o > > obj-$(CONFIG_MCP3422) += mcp3422.o > > +obj-$(CONFIG_MCP3911) += mcp3911.o > > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o > > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o > > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o > > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c > > new file mode 100644 > > index 000000000000..29aa39930ead > > --- /dev/null > > +++ b/drivers/iio/adc/mcp3911.c > > @@ -0,0 +1,366 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Driver for Microchip MCP3911, Two-channel Analog Front End > > + * > > + * Copyright (C) 2018 Marcus Folkesson <[email protected]> > > + * Copyright (C) 2018 Kent Gustavsson <[email protected]> > > + * > > + */ > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/err.h> > > +#include <linux/iio/iio.h> > > +#include <linux/module.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/spi/spi.h> > > + > > +#define MCP3911_REG_CHANNEL0 0x00 > > +#define MCP3911_REG_CHANNEL1 0x03 > > +#define MCP3911_REG_MOD 0x06 > > +#define MCP3911_REG_PHASE 0x07 > > +#define MCP3911_REG_GAIN 0x09 > > + > > +#define MCP3911_REG_STATUSCOM 0x0a > > +#define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4) > > +#define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3) > > +#define MCP3911_STATUSCOM_EN_OFFCAL BIT(2) > > +#define MCP3911_STATUSCOM_EN_GAINCAL BIT(1) > > + > > +#define MCP3911_REG_CONFIG 0x0c > > +#define MCP3911_CONFIG_CLKEXT BIT(1) > > +#define MCP3911_CONFIG_VREFEXT BIT(2) > > + > > +#define MCP3911_REG_OFFCAL_CH0 0x0e > > +#define MCP3911_REG_GAINCAL_CH0 0x11 > > +#define MCP3911_REG_OFFCAL_CH1 0x14 > > +#define MCP3911_REG_GAINCAL_CH1 0x17 > > +#define MCP3911_REG_VREFCAL 0x1a > > + > > +#define MCP3911_CHANNEL(x) (MCP3911_REG_CHANNEL0 + x * 3) > > +#define MCP3911_OFFCAL(x) (MCP3911_REG_OFFCAL_CH0 + x * 6) > > + > > +/* Internal voltage reference in uV */ > > +#define MCP3911_INT_VREF_UV 1200000 > > + > > +#define MCP3911_REG_READ(reg, id) ((((reg) << 1) | ((id) << 5) | (1 << > > 0)) & 0xff) > > +#define MCP3911_REG_WRITE(reg, id) ((((reg) << 1) | ((id) << 5) | (0 << > > 0)) & 0xff) > > + > > +#define MCP3911_NUM_CHANNELS 2 > > + > > +struct mcp3911 { > > + struct spi_device *spi; > > + struct mutex lock; > > + struct regulator *vref; > > + struct clk *adc_clk; > > + u32 dev_addr; > > +}; > > + > > +static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len) > > +{ > > + int ret; > > + > > + reg = MCP3911_REG_READ(reg, adc->dev_addr); > > + ret = spi_write_then_read(adc->spi, ®, 1, val, len); > > + if (ret < 0) > > + return ret; > > + > > + be32_to_cpus(val); > > + *val >>= ((4 - len) * 8); > > + dev_dbg(&adc->spi->dev, "reading 0x%x from register 0x%x\n", *val, > > + reg>>1); > > + return ret; > > +} > > + > > +static int mcp3911_write(struct mcp3911 *adc, u8 reg, u32 val, u8 len) > > +{ > > + dev_dbg(&adc->spi->dev, "writing 0x%x to register 0x%x\n", val, reg); > > + > > + val <<= (3 - len) * 8; > > + cpu_to_be32s(&val); > > + val |= MCP3911_REG_WRITE(reg, adc->dev_addr); > > + > > + return spi_write(adc->spi, &val, len+1); > > Spaces around operators needed everywhere. Please check the > kernel coding style docs. > > > +} > > + > > +static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, > > + u32 val, u8 len) > > +{ > > + u32 tmp; > > + int ret; > > + > > + ret = mcp3911_read(adc, reg, &tmp, len); > > + if (ret) > > + return ret; > > + > > + val &= mask; > > + val |= tmp & ~mask; > > + return mcp3911_write(adc, reg, val, len); > > +} > > + > > +static int mcp3911_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *channel, int *val, > > + int *val2, long mask) > > +{ > > + struct mcp3911 *adc = iio_priv(indio_dev); > > + int ret = -EINVAL; > > + > > + mutex_lock(&adc->lock); > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + ret = mcp3911_read(adc, > > + MCP3911_CHANNEL(channel->channel), val, 3); > > + if (ret) > > + goto out; > > + > > + ret = IIO_VAL_INT; > > + break; > > + > > + case IIO_CHAN_INFO_OFFSET: > > + ret = mcp3911_read(adc, > > + MCP3911_OFFCAL(channel->channel), val, 3); > > + if (ret) > > + goto out; > > + > > + ret = IIO_VAL_INT; > > + break; > > + > > + case IIO_CHAN_INFO_SCALE: > > + if (adc->vref) { > > + ret = regulator_get_voltage(adc->vref); > > + if (ret < 0) { > > + dev_err(indio_dev->dev.parent, > > + "failed to get vref voltage: %d\n", > > + ret); > > + goto out; > > + } > > + > > + *val = ret / 1000; > > + } else { > > + *val = MCP3911_INT_VREF_UV; > > + } > > + > > + *val2 = 24; > > + ret = IIO_VAL_FRACTIONAL_LOG2; > > + break; > > + } > > + > > +out: > > + mutex_unlock(&adc->lock); > > + return ret; > > +} > > + > > +static int mcp3911_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *channel, int val, > > + int val2, long mask) > > +{ > > + struct mcp3911 *adc = iio_priv(indio_dev); > > + int ret = -EINVAL; > > + > > + mutex_lock(&adc->lock); > > + switch (mask) { > > + case IIO_CHAN_INFO_OFFSET: > > + if (val2 != 0) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + /* Write offset */ > > + ret = mcp3911_write(adc, MCP3911_OFFCAL(channel->channel), val, > > + 3); > > + if (ret) > > + goto out; > > + > > + /* Enable offset*/ > > + ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, > > + MCP3911_STATUSCOM_EN_OFFCAL, > > + MCP3911_STATUSCOM_EN_OFFCAL, 2); > > + break; > > + } > > + > > +out: > > + mutex_unlock(&adc->lock); > > + return ret; > > +} > > + > > +#define MCP3911_CHAN(idx) { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .channel = idx, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > > + BIT(IIO_CHAN_INFO_OFFSET) | \ > > + BIT(IIO_CHAN_INFO_SCALE), \ > > +} > > + > > +static const struct iio_chan_spec mcp3911_channels[] = { > > + MCP3911_CHAN(0), > > + MCP3911_CHAN(1), > > +}; > > + > > +static const struct iio_info mcp3911_info = { > > + .read_raw = mcp3911_read_raw, > > + .write_raw = mcp3911_write_raw, > > +}; > > + > > +static int mcp3911_config(struct mcp3911 *adc, struct device_node *of_node) > > +{ > > + u32 configreg; > > + int ret; > > + > > + of_property_read_u32(of_node, "device-addr", &adc->dev_addr); > > + if (adc->dev_addr > 3) { > > + dev_err(&adc->spi->dev, > > + "invalid device address (%i). Must be in range > > 0-3.\n", > > + adc->dev_addr); > > + return -EINVAL; > > + } > > + dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr); > > + > > + ret = mcp3911_read(adc, MCP3911_REG_CONFIG, &configreg, 2); > > + if (ret) > > + return ret; > > + > > + if (adc->vref) { > > + dev_dbg(&adc->spi->dev, "use external voltage reference\n"); > > + configreg |= MCP3911_CONFIG_VREFEXT; > > + > The blank line here is inconsistent with other near by blocks (nitpick ;) > > + } else { > > + dev_dbg(&adc->spi->dev, "use internal voltage reference > > (1.2V)\n"); > > + configreg &= ~MCP3911_CONFIG_VREFEXT; > > + } > > + > > + if (adc->adc_clk) { > > + dev_dbg(&adc->spi->dev, "use external clock as clocksource\n"); > > + configreg |= MCP3911_CONFIG_CLKEXT; > > + } else { > > + dev_dbg(&adc->spi->dev, "use crystal oscillator as > > clocksource\n"); > > + configreg &= ~MCP3911_CONFIG_CLKEXT; > > + } > > + > > + return mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2); > > +} > > + > > +static int mcp3911_probe(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev; > > + struct mcp3911 *adc; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + adc = iio_priv(indio_dev); > > + adc->spi = spi; > > + > > + adc->vref = devm_regulator_get_optional(&adc->spi->dev, "vref"); > > + if (IS_ERR(adc->vref)) { > > + > > + if (PTR_ERR(adc->vref) == -ENODEV) { > > + adc->vref = NULL; > > + } else { > > + dev_err(&adc->spi->dev, > > + "failed to get regulator (%ld)\n", > > + PTR_ERR(adc->vref)); > > + return PTR_ERR(adc->vref); > > + } > > + > > + } else { > > + ret = regulator_enable(adc->vref); > > + if (ret) > > + return ret; > > + } > > + > > + adc->adc_clk = devm_clk_get(&adc->spi->dev, "adc_clk"); > > + if (IS_ERR(adc->adc_clk)) { > > + if (PTR_ERR(adc->adc_clk) == -ENOENT) { > > + adc->adc_clk = NULL; > > + } else { > > + dev_err(&adc->spi->dev, > > + "failed to get adc clk (%ld)\n", > > + PTR_ERR(adc->adc_clk)); > > + ret = PTR_ERR(adc->adc_clk); > > + goto reg_disable; > > + } > > + } else { > > + ret = clk_prepare_enable(adc->adc_clk); > > + if (ret < 0) { > > + dev_err(&adc->spi->dev, > > + "Failed to enable adc_clk: %d\n", ret); > > + goto reg_disable; > > + } > > + } > > + > > + ret = mcp3911_config(adc, spi->dev.of_node); > > + if (ret) > > + goto clk_disable; > > + > > + indio_dev->dev.parent = &spi->dev; > > + indio_dev->dev.of_node = spi->dev.of_node; > > + indio_dev->name = spi_get_device_id(spi)->name; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->info = &mcp3911_info; > > + spi_set_drvdata(spi, indio_dev); > > + > > + indio_dev->channels = mcp3911_channels; > > + indio_dev->num_channels = ARRAY_SIZE(mcp3911_channels); > > + > > + mutex_init(&adc->lock); > > + > > + ret = iio_device_register(indio_dev); > > + if (ret) > > + goto clk_disable; > > + > > + return ret; > > + > > +clk_disable: > > + if (adc->adc_clk) > > + clk_disable_unprepare(adc->adc_clk); > > +reg_disable: > > + if (adc->vref) > > + regulator_disable(adc->vref); > > + > > + return ret; > > +} > > + > > +static int mcp3911_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct mcp3911 *adc = iio_priv(indio_dev); > > + > > + iio_device_unregister(indio_dev); > > + > > You need to also disable_unprepare the clock in remove as well. > > > + if (adc->vref) > > + regulator_disable(adc->vref); > > + > > + return 0; > > +} > > + > > +#if defined(CONFIG_OF) > > +static const struct of_device_id mcp3911_dt_ids[] = { > > + { .compatible = "microchip,mcp3911" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, mcp3911_dt_ids); > > +#endif > > + > > +static const struct spi_device_id mcp3911_id[] = { > > + { "mcp3911", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(spi, mcp3911_id); > > + > > +static struct spi_driver mcp3911_driver = { > > + .driver = { > > + .name = "mcp3911", > > + .of_match_table = of_match_ptr(mcp3911_dt_ids), > > + }, > > + .probe = mcp3911_probe, > > + .remove = mcp3911_remove, > > + .id_table = mcp3911_id, > > +}; > > +module_spi_driver(mcp3911_driver); > > + > > +MODULE_AUTHOR("Marcus Folkesson <[email protected]>"); > > +MODULE_AUTHOR("Kent Gustavsson <[email protected]>"); > > +MODULE_DESCRIPTION("Microchip Technology MCP3911"); > > +MODULE_LICENSE("GPL v2"); > Best regards Marcus Folkesson
signature.asc
Description: PGP signature
