Message-ID: <20230805185900.2441a20e@jic23-huawei>
Date:   Sat, 5 Aug 2023 18:59:00 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     <marius.cristea@...rochip.com>
Cc:     <lars@...afoo.de>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] iio: adc: adding support for MCP3564 ADC

On Fri, 4 Aug 2023 17:28:20 +0300
<marius.cristea@...rochip.com> wrote:

> From: Marius Cristea <marius.cristea@...rochip.com>
> 
> This is the iio driver for Microchip family of 153.6 ksps,
> Low-Noise 16/24-Bit Delta-Sigma ADCs with an SPI interface
> (Microchip's  MCP3461, MCP3462, MCP3464, MCP3461R, MCP3462R,
> MCP3464R, MCP3561, MCP3562, MCP3564, MCP3561R,
> MCP3562R and MCP3564R analog to digital converters).
> 
> Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>
Hi Marius

Various things inline.  Driver is coming together nicely so mostly
suggestions on how to tidy things up a little + give an easier to
review / maintain driver.

Thanks,

Jonathan

> ---
>  .../ABI/testing/sysfs-bus-iio-adc-mcp3564     |   53 +
>  MAINTAINERS                                   |    7 +
>  drivers/iio/adc/Kconfig                       |   13 +
>  drivers/iio/adc/Makefile                      |    1 +
>  drivers/iio/adc/mcp3564.c                     | 1541 +++++++++++++++++
>  5 files changed, 1615 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-mcp3564
>  create mode 100644 drivers/iio/adc/mcp3564.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-mcp3564 b/Documentation/ABI/testing/sysfs-bus-iio-adc-mcp3564
> new file mode 100644
> index 000000000000..feed2f596537
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-mcp3564
> @@ -0,0 +1,53 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/boost_current
> +KernelVersion:	6.4
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		This attribute is used to set the biasing circuit of the
> +		Delta-Sigma modulator. The different BOOST settings are applied
> +		to the entire modulator circuit, including the voltage reference
> +		buffers.

Units?  Should be  mA given that is what we use for other current attributes
in IIO.

This one may cause us problems with generality of the description if we reuse
if for other devices (and hence move it to the more general files), but we
can figure this out at the time.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/boost_current_available
> +KernelVersion:	6.4
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		Reading returns a list with the possible values for
> +		the current biasing circuit of the Delta-Sigma modulator.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/auto_zeroing_mux_enable
> +KernelVersion:	6.4
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		This attribute is used to enable the analog input multiplexer
> +		auto-zeroing algorithm (the input multiplexer and the ADC
> +		include an offset cancellation algorithm that cancels the offset
> +		contribution of the ADC). When the offset cancellation algorithm
> +		is enabled, ADC takes two conversions, one with the differential
> +		input as VIN+/VIN-, one with VIN+/VIN- inverted. In this case the
> +		conversion time is multiplied by two compared to the default
> +		case where the algorithm is disabled. This technique allows the
> +		cancellation of the ADC offset error and the achievement of
> +		ultra-low offset without any digital calibration. The resulting
> +		offset is the residue of the difference between the two
> +		conversions, which is on the order of magnitude of the noise
> +		floor. This offset is effectively canceled at every conversion,
> +		so the residual offset error temperature drift is extremely low.
> +		Write '1' to enable it, write '0' to disable it.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/auto_zeroing_ref_enable
> +KernelVersion:	6.4
> +Contact:	linux-iio@...r.kernel.org
> +Description:
> +		This attribute is used to enable the chopping algorithm for the 
> +		internal voltage reference buffer. This setting has no effect
> +		when external voltage reference is selected.
> +		Internal voltage reference buffer injects a certain quantity of
> +		1/f noise into the system that can be modulated with the
> +		incoming input signals and can limit the SNR performance at
> +		higher Oversampling Ratio values (over 256). To overcome this
> +		limitation, the buffer includes an auto-zeroing algorithm that
> +		greatly reduces (cancels out) the 1/f noise and cancels the
> +		offset value of the reference buffer. As a result, the SNR of
> +		the system is not affected by this 1/f noise component of the
> +		reference buffer, even at maximum oversampling ratio values.
> +		Write '1' to enable it, write '0' to disable it.

...


> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index e07e4a3e6237..91f713af574f 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MAX9611) += max9611.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> +obj-$(CONFIG_MCP3564) += mcp3564.o
>  obj-$(CONFIG_MCP3911) += mcp3911.o
>  obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
>  obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
> diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c
> new file mode 100644
> index 000000000000..60d6a803c0c2
> --- /dev/null
> +++ b/drivers/iio/adc/mcp3564.c
> @@ -0,0 +1,1541 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for MCP356X/MCP356XR and MCP346X/MCP346XR series ADC chip family
> + *
> + * Copyright (C) 2022-2023 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Marius Cristea <marius.cristea@...rochip.com>
> + *
> + * Datasheet for MCP3561, MCP3562, MCP3564 can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP3561-2-4-Family-Data-Sheet-DS20006181C.pdf
> + * Datasheet for MCP3561R, MCP3562R, MCP3564R can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3561_2_4R-Data-Sheet-DS200006391C.pdf
> + * Datasheet for MCP3461, MCP3462, MCP3464 can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf
> + * Datasheet for MCP3461R, MCP3462R, MCP3464R can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4R-Family-Data-Sheet-DS20006404C.pdf
> + *
Last blank line here adds nothing so I'd drop it.

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/iopoll.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define MCP3564_ADCDATA		0x00

Always good to name register addresses so it is clear what they are.
_REG or _ADDR or similar.

> +#define MCP3564_CONFIG0		0x01
> +#define MCP3564_CONFIG1		0x02
> +#define MCP3564_CONFIG2		0x03
> +#define MCP3564_CONFIG3		0x04
> +#define MCP3564_IRQ		0x05
> +#define MCP3564_MUX		0x06
> +#define MCP3564_SCAN		0x07
> +#define MCP3564_TIMER		0x08
> +#define MCP3564_OFFSETCAL	0x09
> +#define MCP3564_GAINCAL		0x0A
> +#define MCP3564_RESERVED_B	0x0B
> +#define MCP3564_RESERVED_C	0x0C
> +#define MCP3564_LOCK		0x0D
> +#define MCP3564_RESERVED_E	0x0E
> +#define MCP3564_CRCCFG		0x0F
> +
Lots of the comments that follow apply across all the
register field definitions.  Main theme is I'm looking for
explicit naming of everything and minimal number of defines.

> +/*
> + * CONFIG 3 Register - Conversion Mode Selection
> + * 0b11 = Continuous Conversion mode or continuous conversion cycle in SCAN mode.
> + * 0b10 = One-shot conversion or one-shot cycle in SCAN mode. It sets ADC_MODE[1:0] to ‘10’
> + *        (standby) at the end of the conversion or at the end of the conversion cycle in SCAN mode.
> + * 0b0x = One-shot conversion or one-shot cycle in SCAN mode. It sets ADC_MODE[1:0] to ‘0x’ (ADC
> + *        Shutdown) at the end of the conversion or at the end of the conversion cycle in SCAN
> + *        mode (default).
> + */
> +#define MCP3464_CONV_MODE_CONTINUOUS		3
> +#define MCP3464_CONV_MODE_ONE_SHOT_STANDBY	2
> +#define MCP3464_CONV_MODE_ONE_SHOT_SHUTDOWN	0
> +#define MCP3464_CONV_MODE_MASK			GENMASK(7, 6)
> +#define MCP3564_CONV_MODE_SET(x)		((u8)FIELD_PREP(MCP3464_CONV_MODE_MASK, (x)))
> +
> +/*
> + * CONFIG 3 Register - ADC Output Data Format Selection
> + * 0b11 = 32-bit (25-bit right justified data + Channel ID):
> + *        CHID[3:0] + SGN extension (4 bits) + 24-bit ADC data.
> + *        It allows overrange with the SGN extension.

I'd put these immediately before each define rather than in a block up here.

> + * 0b10 = 32-bit (25-bit right justified data):
> + *        SGN extension (8-bit) + 24-bit ADC data.
> + *        It allows overrange with the SGN extension.
> + * 0b01 = 32-bit (24-bit left justified data):
> + *        24-bit ADC data + 0x00 (8-bit).
> + *        It does not allow overrange (ADC code locked to 0xFFFFFF or 0x800000).
> + * 0b00 = 24-bit (default ADC coding): 24-bit ADC data.
> + *        It does not allow overrange (ADC code locked to 0xFFFFFF or 0x800000).
> + */
> +#define MCP3456_DATA_FORMAT_32B_WITH_CH_ID	3
> +#define MCP3456_DATA_FORMAT_32B_SGN_EXT		2
> +#define MCP3456_DATA_FORMAT_32B_LEFT_JUSTIFIED	1
> +#define MCP3456_DATA_FORMAT_24B			0
> +#define MCP3464_DATA_FORMAT_MASK		GENMASK(5, 4)
I'd generally like to see the register name in these field names as well.
Perhaps

#define MCP3464_CONFIG3_DATA_FMT_...
as it makes it easy to see if thy are being written to the write registers.

Same for all other register fields.  Sometimes the code is a bit longer doing
this but it makes it easier to tell if there is a mismatch.

> +#define MCP3564_DATA_FORMAT_SET(x)		((u8)FIELD_PREP(MCP3464_DATA_FORMAT_MASK, (x)))
> +
> +#define MCP3464_CRC_FORMAT		BIT(3)
> +#define MCP3464_EN_CRCCOM		BIT(2)
> +#define MCP3464_EN_OFFCAL		BIT(1)
> +#define MCP3464_EN_GAINCAL		BIT(0)
> +
> +/* IRQ REGISTER */
> +#define MCP3564_DATA_READY_MASK		BIT(6)
> +#define MCP3564_CRCCFG_STATUS		BIT(5)

For consistency name these all _MASK as they are all fields that can have values 0 or 1 in them.

> +#define MCP3564_POR_STATUS		BIT(4)
> +#define MCP3464_IRQ_MODE_1		BIT(3)
> +#define MCP3464_IRQ_MODE_0		BIT(2)
> +#define MCP3464_EN_FASTCMD		BIT(1)
> +#define MCP3464_EN_STP			BIT(0)
> +
> +/* MULTIPLEXER (MUX) REGISTER */
> +#define MCP3564_MUX_VIN_P_MASK		GENMASK(7, 4)
> +#define MCP3564_MUX_VIN_N_MASK		GENMASK(3, 0)
> +#define MCP3564_MUX_SET(x, y)		((u8)FIELD_PREP(MCP3564_MUX_VIN_P_MASK, (x)) |	\
> +					 (u8)FIELD_PREP(MCP3564_MUX_VIN_N_MASK, (y)))

This one (unlike below) is probably worth having, but why are the casts needed?


> +/* OFFSETCAL REGISTER */
> +#define MCP3564_DEFAULT_OFFSETCAL	0
> +#define MCP3564_OFFSETCAL_MASK		GENMASK(23, 0)
> +#define MCP3564_OFFSETCAL_SET(x)	((u32)FIELD_PREP(MCP3564_OFFSETCAL_MASK, (x)))
> +
> +/* GAINCAL REGISTER */
You could move these up to next to the register definition.
A bit of playing with indenting can then be used to separate the fields from
the register itself. That tends to make things more self documenting so you can
get rid of the comments and end up with tidier code.

e.g.

#define MCP3564_GAINCAL_REG		0x0A
#define   MCP3564_DEFAULT_GAINCAL		0x00800000
#define   MCP3564_GAINCAL_MASK			GENMASK(23, 0)


> +#define MCP3564_DEFAULT_GAINCAL		0x00800000
> +#define MCP3564_GAINCAL_MASK		GENMASK(23, 0)
> +#define MCP3564_GAINCAL_SET(x)		((u32)FIELD_PREP(MCP3564_GAINCAL_MASK, (x)))

Are these _SET macros worth having?
FIELD_PREP(MCP3565_GAINCAL_MASK, x) is pretty clear without needing a macro to name it.

> +
> +
> +#define MCP3564_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)

For 3 uses?  I'd be tempted to just have them long hand



> +
> +static const struct iio_chan_spec_ext_info mcp3564_ext_info[] = {
> +	IIO_ENUM("boost_current", IIO_SHARED_BY_ALL, &mcp3564_current_boost_mode_enum),
> +	{
> +		.name = "boost_current_available",
> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = iio_enum_available_read,
> +		.private = (uintptr_t)&mcp3564_current_boost_mode_enum,
> +	},
> +	{}
	{ }

for consistency with some cases below.

> +};

> +static int mcp3564_read_single_value(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *channel,
> +				     int *val)
> +{
> +	struct mcp3564_state *adc = iio_priv(indio_dev);
> +	int ret;
> +	u8 tmp;
> +	int ret_read = 0;
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_MUX, channel->address);
> +	if (ret)
> +		return ret;
> +
> +	/* Start ADC Conversion using fast command (overwrites ADC_MODE[1:0] = 11) */
> +	ret = mcp3564_fast_cmd(adc, MCP3564_FASTCMD_START);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Check if the conversion is ready. If not, wait a little bit, and
> +	 * in case of timeout exit with an error.
> +	 */
> +	ret = read_poll_timeout(mcp3564_read_8bits, ret_read,
> +				ret_read || !(tmp & MCP3564_DATA_READY_MASK),
> +				20000, MCP3564_DATA_READY_TIMEOUT_MS * 1000, true,
> +				adc, MCP3564_IRQ, &tmp);
> +
> +	/* failed to read status register */
> +	if (ret_read)
> +		return ret;

Probably want to to return ret_read in this case.

> +
> +	if (ret)
> +		return -ETIMEDOUT;

ret should be -ETIMEDOUT so just return ret and save anyone reading the docs :)

> +
> +	if (tmp & MCP3564_DATA_READY_MASK)
> +		/* failing to finish conversion */
> +		return -EBUSY;
> +
> +	return mcp3564_read_32bits(adc, MCP3564_ADCDATA, val);
> +}

> +static int mcp3564_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel,
> +			    int *val, int *val2, long mask)
> +{
> +	struct mcp3564_state *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (channel->output) {
> +			mutex_lock(&adc->lock);
> +			*val = mcp3564_burnout_avail[adc->burnout_mode][0];
> +			*val2 = mcp3564_burnout_avail[adc->burnout_mode][1];
> +			mutex_unlock(&adc->lock);
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		}
> +
> +		ret = mcp3564_read_single_value(indio_dev, channel, val);
> +		if (ret)
> +			return -EINVAL;
> +		else

Drop the else.

> +			return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		mutex_lock(&adc->lock);
> +		*val = adc->scale_tbls[adc->hwgain][0];
> +		*val2 = adc->scale_tbls[adc->hwgain][1];
> +		mutex_unlock(&adc->lock);
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = mcp3564_oversampling_avail[adc->oversampling];
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*val = adc->calib_bias;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		*val = adc->calib_scale;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int mcp3564_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *channel, int val,
> +			     int val2, long mask)
> +{
> +	struct mcp3564_state *adc = iio_priv(indio_dev);
> +	int tmp;
> +	unsigned int hwgain;
> +	enum mcp3564_burnout burnout;
> +	int ret = 0;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (channel->output) {

Flip logic to cut down on indent
		if (!channel->output)
			return -EINVAL;

		for (..

> +			for (burnout = 0; burnout < MCP3564_MAX_BURNOUT_IDX; burnout++) {
> +				if (val == mcp3564_burnout_avail[burnout][0] &&
> +				    val2 == mcp3564_burnout_avail[burnout][1])
> +					break;
> +			}
> +
> +			if (burnout == MCP3564_MAX_BURNOUT_IDX)
> +				return -EINVAL;
> +
> +			if (burnout == adc->burnout_mode)
> +				return ret;
> +
> +			mutex_lock(&adc->lock);
> +			ret = mcp3564_update_8bits(adc, MCP3564_CONFIG0,
> +						   MCP3564_CS_SEL_MASK,
> +						   MCP3564_CS_SEL_SET(burnout));
> +
> +			if (ret)
> +				dev_err(&indio_dev->dev, "Failed to configure burnout current\n");
> +			else
> +				adc->burnout_mode = burnout;
> +			mutex_unlock(&adc->lock);
> +			return ret;
> +		}
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (val < mcp3564_calib_bias[0] && val > mcp3564_calib_bias[2])
> +			return -EINVAL;
> +
> +		mutex_lock(&adc->lock);
> +		ret = mcp3564_write_24bits(adc, MCP3564_OFFSETCAL, val);
> +		if (!ret)
> +			adc->calib_bias = val;
> +		mutex_unlock(&adc->lock);
> +		return ret;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (val < mcp3564_calib_bias[0] && val > mcp3564_calib_bias[2])

calib_scale for scale?

> +			return -EINVAL;
> +
> +		if (adc->calib_scale == val)
> +			return ret;
> +
> +		mutex_lock(&adc->lock);
> +		ret = mcp3564_write_24bits(adc, MCP3564_GAINCAL, val);
> +		if (!ret)
> +			adc->calib_scale = val;
> +		mutex_unlock(&adc->lock);
> +		return ret;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		if (val < 0)
> +			return -EINVAL;
> +
> +		tmp = find_closest(val, mcp3564_oversampling_avail,
> +				   ARRAY_SIZE(mcp3564_oversampling_avail));
> +
> +		if (adc->oversampling == tmp)
> +			return ret;
> +
> +		mutex_lock(&adc->lock);
> +		ret = mcp3564_update_8bits(adc, MCP3564_CONFIG1, MCP3564_OVERSPL_RATIO_MASK,
> +					   MCP3564_OVERSAMPLING_RATIO_SET(adc->oversampling));
> +		if (!ret)
> +			adc->oversampling = tmp;
> +		mutex_unlock(&adc->lock);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		for (hwgain = 0; hwgain < MCP3564_MAX_PGA; hwgain++) {
> +			if (val == adc->scale_tbls[hwgain][0] &&
> +			    val2 == adc->scale_tbls[hwgain][1])
> +				break;
> +		}
> +
> +		if (hwgain == MCP3564_MAX_PGA)
> +			return -EINVAL;
> +
> +		if (hwgain == adc->hwgain)
> +			return ret;
> +
> +		mutex_lock(&adc->lock);
> +		/* Update GAIN in CONFIG2[5:3] -> GAIN[2:0]*/
> +		ret = mcp3564_update_8bits(adc, MCP3564_CONFIG2, MCP3564_HARDWARE_GAIN_MASK,
> +					   MCP3564_HARDWARE_GAIN_SET(hwgain));
> +		if (!ret)
> +			adc->hwgain = hwgain;
> +
> +		mutex_unlock(&adc->lock);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int mcp3564_config(struct iio_dev *indio_dev)
> +{
> +	struct mcp3564_state *adc = iio_priv(indio_dev);
> +	struct device *dev = &adc->spi->dev;
> +	const struct spi_device_id *dev_id;
> +	u8 tmp_reg;
> +	u16 tmp_u16;
> +	enum mcp3564_ids ids;
> +	int ret = 0;
> +	unsigned int tmp = 0x01;
> +	bool err = true;
> +
> +	/*
> +	 * The address is set on a per-device basis by fuses in the factory,
> +	 * configured on request. If not requested, the fuses are set for 0x1.
> +	 * The device address is part of the device markings to avoid
> +	 * potential confusion. This address is coded on two bits, so four possible
> +	 * addresses are available when multiple devices are present on the same
> +	 * SPI bus with only one Chip Select line for all devices.
> +	 */
> +	device_property_read_u32(dev, "microchip,hw-device-address", &tmp);
> +
> +	if (tmp > 3) {
> +		dev_err_probe(dev, tmp,
> +			      "invalid device address. Must be in range 0-3.\n");
> +		return -EINVAL;
> +	}
> +
> +	adc->dev_addr = MCP3564_HW_ADDR_MASK & tmp;
> +
> +	dev_dbg(dev, "use device address %i\n", adc->dev_addr);
> +
> +	ret = mcp3564_read_8bits(adc, MCP3564_RESERVED_C, &tmp_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (tmp_reg) {
> +	case MCP3564_REG_C_DEFAULT:
> +		adc->has_vref = false;
> +		break;
> +	case MCP3564R_REG_C_DEFAULT:
> +		adc->has_vref = true;
> +		break;
> +	default:
> +		dev_info(dev, "Unknown chip found: %d\n", tmp_reg);
> +		err = true;
> +	}
> +
> +	if (!err) {
> +		ret = mcp3564_read_16bits(adc, MCP3564_RESERVED_E, &tmp_u16);
> +		if (ret < 0)
> +			return ret;
> +
> +		switch (tmp_u16 & MCP3564_HW_ID_MASK) {
> +		case MCP3461_HW_ID:
> +			if (adc->has_vref)
> +				ids = mcp3461r;
> +			else
> +				ids = mcp3461;
> +			break;
> +		case MCP3462_HW_ID:
> +			if (adc->has_vref)
> +				ids = mcp3462r;
> +			else
> +				ids = mcp3462;
> +			break;
> +		case MCP3464_HW_ID:
> +			if (adc->has_vref)
> +				ids = mcp3464r;
> +			else
> +				ids = mcp3464;
> +			break;
> +		case MCP3561_HW_ID:
> +			if (adc->has_vref)
> +				ids = mcp3561r;
> +			else
> +				ids = mcp3561;
> +			break;
> +		case MCP3562_HW_ID:
> +			if (adc->has_vref)
> +				ids = mcp3562r;
> +			else
> +				ids = mcp3562;
> +			break;
> +		case MCP3564_HW_ID:
> +			if (adc->has_vref)
> +				ids = mcp3564r;
> +			else
> +				ids = mcp3564;
> +			break;
> +		default:
> +			dev_info(dev, "Unknown chip found: %d\n", tmp_u16);
> +			err = true;
> +		}
> +	}
> +
> +	if (err) {
> +		/*
> +		 * If failed to identify the hardware based on internal registers,
> +		 * try using fallback compatible in device tree to deal with some newer part number.
> +		 */

Can we use spi_get_device_match_data() here?

> +		adc->chip_info = device_get_match_data(&adc->spi->dev);
> +		if (!adc->chip_info) {
> +			dev_id = spi_get_device_id(adc->spi);
> +			adc->chip_info = (const struct mcp3564_chip_info *)dev_id->driver_data;
> +		}
> +
> +		adc->has_vref = adc->chip_info->has_vref;
> +	} else {
> +		adc->chip_info = &mcp3564_chip_infos_tbl[ids];
> +	}
> +
> +	dev_dbg(dev, "Found %s chip\n", adc->chip_info->name);
> +
> +	adc->vref = devm_regulator_get_optional(dev, "vref");

> +	if (IS_ERR(adc->vref)) {
> +		if (PTR_ERR(adc->vref) == -ENODEV) {
> +			/* Check if chip has internal vref */
> +			if (!adc->has_vref)
> +				return dev_err_probe(dev, -EINVAL,

ERR_PTR(adc->vref) would be better as might tell us something more useful.

> +					     "Unknown Vref\n");
> +			adc->vref = NULL;
> +			dev_dbg(dev, "%s: Using internal Vref\n", __func__);
> +		} else {
> +			return dev_err_probe(dev, PTR_ERR(adc->vref),
> +					     "failed to get regulator\n");
> +		}
> +	} else {
> +		ret = regulator_enable(adc->vref);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(dev, mcp3564_disable_reg,
> +					       adc->vref);
> +		if (ret)
> +			return ret;
> +
> +		dev_dbg(dev, "%s: Using External Vref\n", __func__);
> +
> +		ret = regulator_get_voltage(adc->vref);
> +		if (ret < 0) {
> +			return dev_err_probe(dev, ret,
> +					     "Failed to read vref regulator\n");
> +		}
> +
> +		adc->vref_mv = ret / MILLI;
> +	}
> +
> +	ret = mcp3564_parse_fw_children(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Command sequence that ensures a recovery with the desired settings
> +	 * in any cases of loss-of-power scenario (Full Chip Reset):
> +	 *  - Write LOCK register to 0xA5
> +	 *  - Write IRQ register to 0x03
> +	 *  - Send "Device Full Reset" fast command
> +	 *  - Wait 1ms for "Full Reset" to complete
> +	 */
> +	ret = mcp3564_write_8bits(adc, MCP3564_LOCK, MCP3564_LOCK_WRITE_ACCESS_PASSWORD);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_IRQ, 0x03);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp3564_fast_cmd(adc, MCP3564_FASTCMD_RESET);
> +	if (ret)
> +		return ret;
> +
> +	mdelay(1);
> +
> +	/* set a gain of 1x for GAINCAL */
> +	ret = mcp3564_write_24bits(adc, MCP3564_GAINCAL,
> +				   MCP3564_GAINCAL_SET(MCP3564_DEFAULT_GAINCAL));

It's the default register value - I wouldn't expect it to be masked via the
set function (it's not the default of just that field).

> +	if (ret)
> +		return ret;
> +
> +	adc->calib_scale = MCP3564_GAINCAL_SET(MCP3564_DEFAULT_GAINCAL);
> +
> +	ret = mcp3564_write_24bits(adc, MCP3564_OFFSETCAL,
> +				   MCP3564_OFFSETCAL_SET(MCP3564_DEFAULT_OFFSETCAL));
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp3564_write_24bits(adc, MCP3564_TIMER,
> +				   MCP3564_TIMER_SET(MCP3564_TIMER_DEFAULT_VALUE));
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp3564_write_24bits(adc, MCP3564_SCAN,
> +				   MCP3564_DELAY_TIME_SET(MCP3564_NO_DELAY) |
> +				   MCP3564_SCAN_CH_SEL_SET(MCP3564_SCAN_DEFAULT_VALUE));
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_MUX, MCP3564_MUX_SET(MCP3564_CH0, MCP3564_CH1));
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_IRQ, MCP3464_EN_FASTCMD | MCP3464_EN_STP);
> +	if (ret)
> +		return ret;
> +
> +	tmp_reg = MCP3564_CONV_MODE_SET(MCP3464_CONV_MODE_ONE_SHOT_STANDBY);
> +	tmp_reg |= MCP3564_DATA_FORMAT_SET(MCP3456_DATA_FORMAT_32B_SGN_EXT);
> +	tmp_reg |= MCP3464_EN_OFFCAL;
> +	tmp_reg |= MCP3464_EN_GAINCAL;
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_CONFIG3, tmp_reg);
> +	if (ret)
> +		return ret;
> +
> +	tmp_reg = MCP3564_BOOST_CURRENT_SET(MCP3564_BOOST_CURRENT_x1_00);
> +	tmp_reg |= MCP3564_HARDWARE_GAIN_SET(0x01);
> +	tmp_reg |= MCP3564_AZ_MUX_SET(1);
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_CONFIG2, tmp_reg);
> +	if (ret)
> +		return ret;
> +
> +	adc->hwgain = 0x01;
> +	adc->auto_zeroing_mux = true;
> +	adc->auto_zeroing_ref = false;
> +	adc->current_boost_mode = MCP3564_BOOST_CURRENT_x1_00;
> +
> +	tmp_reg = MCP3564_OVERSAMPLING_RATIO_SET(MCP3564_OVERSAMPLING_RATIO_98304);
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_CONFIG1, tmp_reg);
> +	if (ret)
> +		return ret;
> +
> +	adc->oversampling = MCP3564_OVERSAMPLING_RATIO_98304;
> +
> +	tmp_reg = MCP3564_ADC_MODE_MASK_SET(MCP3564_ADC_MODE_STANDBY);
> +	tmp_reg |= MCP3564_CS_SEL_SET(MCP3564_CS_SEL_0_0_uA);
> +	tmp_reg |= MCP3564_CLK_SEL_SET(MCP3564_USE_INTERNAL_CLK);
> +	tmp_reg |= MCP3456_CONFIG0_BIT6_DEFAULT;
> +
> +	if (!adc->vref) {
> +		tmp_reg |= MCP3564_VREF_SET(1);
> +		adc->vref_mv = MCP3564R_INT_VREF_MV;
> +	}
> +
> +	ret = mcp3564_write_8bits(adc, MCP3564_CONFIG0, tmp_reg);
> +
> +	adc->burnout_mode = MCP3564_CS_SEL_0_0_uA;
> +
> +	return ret;
> +}

...

> +
> +static const struct of_device_id mcp3564_dt_ids[] = {
> +	{ .compatible = "microchip,mcp3461", .data = &mcp3564_chip_infos_tbl[mcp3461] },
> +	{ .compatible = "microchip,mcp3462", .data = &mcp3564_chip_infos_tbl[mcp3462] },
> +	{ .compatible = "microchip,mcp3464", .data = &mcp3564_chip_infos_tbl[mcp3464] },
> +	{ .compatible = "microchip,mcp3561", .data = &mcp3564_chip_infos_tbl[mcp3561] },
> +	{ .compatible = "microchip,mcp3562", .data = &mcp3564_chip_infos_tbl[mcp3562] },
> +	{ .compatible = "microchip,mcp3564", .data = &mcp3564_chip_infos_tbl[mcp3564] },
> +	{ .compatible = "microchip,mcp3461r", .data = &mcp3564_chip_infos_tbl[mcp3461r] },
> +	{ .compatible = "microchip,mcp3462r", .data = &mcp3564_chip_infos_tbl[mcp3462r] },
> +	{ .compatible = "microchip,mcp3464r", .data = &mcp3564_chip_infos_tbl[mcp3464r] },
> +	{ .compatible = "microchip,mcp3561r", .data = &mcp3564_chip_infos_tbl[mcp3561r] },
> +	{ .compatible = "microchip,mcp3562r", .data = &mcp3564_chip_infos_tbl[mcp3562r] },
> +	{ .compatible = "microchip,mcp3564r", .data = &mcp3564_chip_infos_tbl[mcp3564r] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mcp3564_dt_ids);
> +
> +static const struct spi_device_id mcp3564_id[] = {
> +	{ "mcp3461", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3461] },
> +	{ "mcp3462", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3462] },
> +	{ "mcp3464", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3464] },
> +	{ "mcp3561", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3561] },
> +	{ "mcp3562", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3562] },
> +	{ "mcp3564", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3564] },
> +	{ "mcp3461r", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3461r] },
> +	{ "mcp3462r", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3462r] },
> +	{ "mcp3464r", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3464r] },
> +	{ "mcp3561r", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3561r] },
> +	{ "mcp3562r", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3562r] },
> +	{ "mcp3564r", (kernel_ulong_t)&mcp3564_chip_infos_tbl[mcp3564r] },
> +	{ }
> +};
> +

No blank line here (for consistency with above)

> +MODULE_DEVICE_TABLE(spi, mcp3564_id);

Powered by blists - more mailing lists