LKML Archive mirror
 help / color / mirror / Atom feed
From: Mike Looijmans <mike.looijmans@topic.nl>
To: marius.cristea@microchip.com, jic23@kernel.org, lars@metafoo.de,
	robh+dt@kernel.org
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC
Date: Wed, 24 May 2023 13:05:12 +0200	[thread overview]
Message-ID: <915a5d31-5eb4-54b9-3fc9-ff69836f3d76@topic.nl> (raw)
In-Reply-To: <20230519160145.44208-3-marius.cristea@microchip.com>

On 19-05-2023 18:01, marius.cristea@microchip.com wrote:
> From: Marius Cristea <marius.cristea@microchip.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.

Oh nice.. I tried to submit a similar driver just yesterday, I'll comment on 
yours instead...

> 
> Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> ---
...

> +
> +static inline u8 mcp356x_reg_write(u8 chip_addr, u8 reg)
> +{
> +	return ((chip_addr << 6) | (reg << 2) | MCP356X_WRT_CTRL);
> +}
> +
> +static inline u8 mcp356x_reg_read(u8 chip_addr, u8 reg)
> +{
> +	return ((chip_addr << 6) | (reg << 2) | MCP356X_RD_CTRL);
> +}

How about using FIELD_PREP() here?

> +
> +static inline u8 mcp356x_reg_fast_cmd(u8 chip_addr, u8 cmd)
> +{
> +	return ((chip_addr << 6) | (cmd << 2));
> +}
> +
> +static int mcp356x_read(struct mcp356x_state *adc, u8 reg, u32 *val, u8 len)
> +{
> +	int ret;
> +	u8 tmp_reg;
> +
> +	tmp_reg = mcp356x_reg_read(adc->dev_addr, reg);
> +
> +	ret = spi_write_then_read(adc->spi, &tmp_reg, 1, val, len);

I had issues with spi_write_then_read not communicating properly with the 
device. This may be due to the SPI controller (Xilinx IP in FPGA) though.

I had to use spi_sync() to get reliable answers. Also has the added benefit of 
giving access to the interrupt flags.

> +
> +	be32_to_cpus(val);
> +	*val >>= ((4 - len) * 8);
> +
> +	return ret;
> +}
> +
> +static int mcp356x_write(struct mcp356x_state *adc, u8 reg, u32 val, u8 len)
> +{
> +	val |= (mcp356x_reg_write(adc->dev_addr, reg) << (len * 8));
> +	val <<= (3 - len) * 8;
> +	cpu_to_be32s(&val);
> +
> +	return spi_write(adc->spi, &val, len + 1);
> +}
> +
> +static int mcp356x_fast_cmd(struct mcp356x_state *adc, u8 fast_cmd)
> +{
> +	u8 val;
> +
> +	val = mcp356x_reg_fast_cmd(adc->dev_addr, fast_cmd);
> +
> +	return spi_write(adc->spi, &val, 1);
> +}
> +
> +static int mcp356x_update(struct mcp356x_state *adc, u8 reg, u32 mask, u32 val,
> +			  u8 len)
> +{
> +	u32 tmp;
> +	int ret;
> +
> +	ret = mcp356x_read(adc, reg, &tmp, len);
> +
> +	if (ret == 0) {
> +		val &= mask;
> +		val |= tmp & ~mask;
> +		ret = mcp356x_write(adc, reg, val, len);
> +	}
> +
> +	return ret;
> +}


This update function is used a lot, and is just a re-implementation of 
"regmap" functionality. Since this driver only reads/writes single registers, 
I would recommend using regmap with the read/write functions above. That would 
also reduce the SPI traffic as regmap can cache the values for you.

...

> +
> +#define MCP3564_CHANNELS(depth) {			\
> +	MCP356X_V_CHANNEL(0, 0, depth),			\
> +	MCP356X_V_CHANNEL(1, 1, depth),			\
> +	MCP356X_V_CHANNEL(2, 2, depth),			\
> +	MCP356X_V_CHANNEL(3, 3, depth),			\
> +	MCP356X_V_CHANNEL(4, 4, depth),			\
> +	MCP356X_V_CHANNEL(5, 5, depth),			\
> +	MCP356X_V_CHANNEL(6, 6, depth),			\
> +	MCP356X_V_CHANNEL(7, 7, depth),			\
> +	MCP356X_T_CHAN(depth),				\
> +	MCP356X_V_CHANNEL_DIFF(0, 1, 0x01, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(1, 0, 0x10, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(0, 2, 0x02, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(0, 3, 0x03, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(1, 2, 0x12, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(1, 3, 0x13, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(2, 3, 0x23, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(2, 0, 0x20, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(3, 0, 0x30, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(2, 1, 0x21, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(3, 1, 0x31, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(3, 2, 0x32, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(0, 4, 0x04, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(0, 5, 0x05, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(0, 6, 0x06, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(0, 7, 0x07, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(1, 4, 0x14, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(1, 5, 0x15, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(1, 6, 0x16, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(1, 7, 0x17, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(2, 4, 0x24, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(2, 5, 0x25, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(2, 6, 0x26, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(2, 7, 0x27, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(3, 4, 0x34, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(3, 5, 0x35, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(3, 6, 0x36, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(3, 7, 0x37, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(4, 5, 0x45, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(4, 6, 0x46, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(4, 7, 0x47, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(5, 6, 0x56, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(5, 7, 0x57, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(6, 7, 0x67, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(4, 0, 0x40, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(5, 0, 0x50, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(6, 0, 0x60, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(7, 0, 0x70, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(4, 1, 0x41, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(5, 1, 0x51, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(6, 1, 0x61, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(7, 1, 0x71, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(4, 2, 0x42, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(5, 2, 0x52, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(6, 2, 0x62, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(7, 2, 0x72, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(4, 3, 0x43, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(5, 3, 0x53, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(6, 3, 0x63, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(7, 3, 0x73, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(5, 4, 0x54, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(6, 4, 0x64, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(7, 4, 0x74, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(6, 5, 0x65, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(7, 5, 0x75, depth),	\
> +	MCP356X_V_CHANNEL_DIFF(7, 6, 0x76, depth)	\
> +}

Nice that the chip can do full 8x8 mux delivering 56 channels, but I don't 
think that will be useful to many people.

Also I'd want to see buffer support added to the device, which only works for 
the channels as in the table in the datasheet, so only 4 differential ones. 
It'd be annoying to have 56 channels but only be able to contiuously read 4 
specific ones.

...

> +
> +static int mcp356x_read_single_value(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *channel,
> +				     int *val)
> +{
> +	struct mcp356x_state *adc = iio_priv(indio_dev);
> +	int ret, tmp, ret_read = 0;
> +
> +	/* Configure MUX register with the requested channel */
> +	ret = mcp356x_write(adc, MCP356X_MUX, channel->address, 1);
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Failed to configure MUX register\n");
> +		return ret;
> +	}
> +
> +	/* ADC Conversion starts by writing ADC_MODE[1:0] = 11 to CONFIG0[1:0] =  */
> +	ret = mcp356x_update(adc, MCP356X_CONFIG0, MCP356X_ADC_MODE_MASK,
> +			     MCP356X_ADC_CONVERSION_MODE, 1);
> +	if (ret) {
> +		dev_err(&indio_dev->dev,
> +			"Failed to configure CONFIG0 register\n");
> +		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(mcp356x_read, ret_read,
> +				ret_read || !(tmp & MCP356X_DATA_READY_MASK),
> +				1000, MCP356X_DATA_READY_TIMEOUT_MS * 1000, true,
> +				adc, MCP356X_IRQ, &tmp, 1);

I have an implementation that (optionally) uses the interrupt signal from the 
chip, nicer than polling.

> +
> +	/* failed to read status register */
> +	if (ret_read)
> +		return ret;
> +
> +	if (ret)
> +		return -ETIMEDOUT;
> +
> +	if (tmp & MCP356X_DATA_READY_MASK)
> +		/* failing to finish conversion */
> +		return -EBUSY;
> +
> +	ret = mcp356x_read(adc, MCP356X_ADCDATA, &tmp, 4);
> +	if (ret)
> +		return ret;
> +
> +	*val = tmp;
> +
> +	return ret;
> +}
> +
> +static int mcp356x_read_avail(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *channel,
> +			      const int **vals, int *type,
> +			      int *length, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*type = IIO_VAL_INT;
> +		*vals = mcp356x_oversampling_avail;
> +		*length = ARRAY_SIZE(mcp356x_oversampling_avail);
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_HARDWAREGAIN:

Hmm, last time I tried to submit a driver with HARDWAREGAIN I got told to 
change it to selectable SCALE instead. See the driver I submitted yesterday 
for details.

> +		*type = IIO_VAL_FRACTIONAL;
> +		*length = ARRAY_SIZE(mcp356x_hwgain_frac);
> +		*vals = mcp356x_hwgain_frac;
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*vals = mcp356x_calib_bias;
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		*vals = mcp356x_calib_scale;
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +
> +static int mcp356x_config(struct mcp356x_state *adc)
> +{
> +	int ret = 0;
> +	unsigned int tmp;
> +
> +	dev_dbg(&adc->spi->dev, "%s: Start config...\n", __func__);
> +
> +	/*
> +	 * 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(&adc->spi->dev, "microchip,hw-device-address", &tmp);
> 

tmp may be used uninitialized here. Initialize to "1" to get a proper default 
value. That also makes the microchip,hw-device-address optional instead of 
mandatory (see my dt-bindings comment).

> +	if (tmp > 3) {
> +		dev_err_probe(&adc->spi->dev, tmp,
> +			      "invalid device address. Must be in range 0-3.\n");
> +		return -EINVAL;
> +	}
> +
> +	adc->dev_addr = 0xff & tmp;
> +
> +	dev_dbg(&adc->spi->dev, "use device address %i\n", adc->dev_addr);
> +
> +	ret = mcp356x_read(adc, MCP356X_RESERVED_E, &tmp, 2);
> +
> +	if (ret == 0) {
> +		switch (tmp & MCP356X_HW_ID_MASK) {
> +		case MCP3461_HW_ID:
> +			dev_dbg(&adc->spi->dev, "Found MCP3461 chip\n");

This will never get output unless the driver is compiled in debug mode. I'd 
have expected this ID to be used to initialize things?

> +			break;
> +		case MCP3462_HW_ID:
> +			dev_dbg(&adc->spi->dev, "Found MCP3462 chip\n");
> +			break;
> +		case MCP3464_HW_ID:
> +			dev_dbg(&adc->spi->dev, "Found MCP3464 chip\n");
> +			break;
> +		case MCP3561_HW_ID:
> +			dev_dbg(&adc->spi->dev, "Found MCP3561 chip\n");
> +			break;
> +		case MCP3562_HW_ID:
> +			dev_dbg(&adc->spi->dev, "Found MCP3562 chip\n");
> +			break;
> +		case MCP3564_HW_ID:
> +			dev_dbg(&adc->spi->dev, "Found MCP3564 chip\n");
> +			break;
> +		default:
> +			dev_err_probe(&adc->spi->dev, tmp,
> +				      "Unknown chip found\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		return ret;
> +	}
> +
> +	/* Command sequence that ensures a recovery with
> +	 * the desired settings in any cases of loss-of-power scenario.
> +	 */
> +
> +	/* Write LOCK register to 0xA5 (Write Access Password)
> +	 * Write access is allowed on the full register map.
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_LOCK, 0x000000A5, 1);
> +	if (ret)
> +		return ret;
> +
> +	/* Write IRQ register to 0x03 */
> +	/* IRQ --> IRQ Mode = Hi-Z IRQ Output  --> (0b00000011).
> +	 * IRQ = 0x00000003
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1);
> +	if (ret)
> +		return ret;
> +
> +	/* Device Full Reset Fast Command */
> +	ret = mcp356x_fast_cmd(adc, MCP356X_FULL_RESET_CMD);
> +
> +	/* wait 1ms for the chip to restart after a full reset */
> +	mdelay(1);
> +
> +	/* Reconfigure the ADC chip  */
> +
> +	/* GAINCAL --> Disabled.
> +	 * Default value is GAINCAL = 0x00800000; which provides a gain of 1x
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_GAINCAL, 0x00800000, 3);
> +	if (ret)
> +		return ret;
> +
> +	adc->calib_scale = 0x00800000;
> +
> +	/* OFFSETCAL --> 0 Counts of Offset Cancellation
> +	 * (Measured offset is negative).
> +	 * OFFSETCAL = 0x0
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_OFFSETCAL, 0x00000000, 3);
> +	if (ret)
> +		return ret;
> +
> +	/* TIMER --> Disabled.
> +	 * TIMER = 0x00000000
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_TIMER, 0x00000000, 3);
> +	if (ret)
> +		return ret;
> +
> +	/* SCAN --> Disabled.
> +	 * SCAN = 0x00000000
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_SCAN, 0x00000000, 3);
> +	if (ret)
> +		return ret;
> +
> +	/* MUX --> VIN+ = CH0, VIN- = CH1 --> (0b00000001).
> +	 * MUX = 0x00000001
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_MUX, 0x00000001, 1);
> +	if (ret)
> +		return ret;
> +
> +	/* IRQ --> IRQ Mode = Hi-Z IRQ Output  --> (0b00000011).
> +	 * IRQ = 0x00000003
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_IRQ, 0x00000003, 1);
> +	if (ret)
> +		return ret;
> +
> +	/* CONFIG3
> +	 * Conv. Mod = One-Shot/Standby,
> +	 * FORMAT = 32-bit (right justified data): SGN extension + ADC data,
> +	 * CRC_FORMAT = 16b, CRC-COM = Disabled,
> +	 * OFFSETCAL = Enabled, GAINCAL = Enabled --> (10100011).
> +	 * CONFIG3 = 0x000000A3
> +	 *
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_CONFIG3, 0x000000A3, 1);
> +	if (ret)
> +		return ret;
> +
> +	/* CONFIG2 --> BOOST = 1x, GAIN = 1x, AZ_MUX = 1 --> (0b10001101).
> +	 * CONFIG2 = 0x0000008D
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_CONFIG2, 0x0000008D, 1);
> +	if (ret)
> +		return ret;
> +
> +	adc->hwgain = 0x01;
> +	adc->auto_zeroing_mux = true;
> +	adc->auto_zeroing_ref = false;
> +	adc->current_boost_mode = MCP356X_BOOST_CURRENT_x1_00;
> +
> +	/* CONFIG1 --> AMCLK = MCLK, OSR = 98304 --> (0b00111100).
> +	 * CONFIG1 = 0x0000003C
> +	 */
> +	ret = mcp356x_write(adc, MCP356X_CONFIG1, 0x0000003C, 1);
> +	if (ret)
> +		return ret;
> +
> +	adc->oversampling = 0x0F;
> +
> +	if (!adc->vref) {
> +		/* CONFIG0 --> VREF_SEL = Internal Voltage Reference 2.4v
> +		 * CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias,
> +		 * ADC_MODE = Standby Mode --> (0b11100010).
> +		 * CONFIG0 = 0x000000E2
> +		 */
> +		ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x000000E2, 1);
> +
> +		dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n",
> +			__func__);
> +		adc->vref_mv = MCP356XR_INT_VREF_MV;
> +
> +	} else {
> +		/* CONFIG0 --> CLK_SEL = INTOSC w/o CLKOUT, CS_SEL = No Bias,
> +		 * ADC_MODE = Standby Mode --> (0b01100010).
> +		 * CONFIG0 = 0x000000E2
> +		 */
> +		ret = mcp356x_write(adc, MCP356X_CONFIG0, 0x00000062, 1);
> +	}
> +	adc->current_bias_mode = MCP356X_CS_SEL_0_0_uA;
> +
> +	return ret;
> +}
> +
> +static int mcp356x_probe(struct spi_device *spi)
> +{
> +	int ret, device_index;
> +	struct iio_dev *indio_dev;
> +	struct mcp356x_state *adc;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev) {
> +		dev_err_probe(&indio_dev->dev, PTR_ERR(indio_dev),
> +			      "Can't allocate iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +
> +	dev_dbg(&adc->spi->dev, "%s: probe(spi = 0x%p)\n", __func__, 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;
> +			dev_dbg(&adc->spi->dev, "%s: Using internal Vref\n",
> +				__func__);
> +		} else {
> +			dev_err_probe(&adc->spi->dev, PTR_ERR(adc->vref),
> +				      "failed to get regulator\n");
> +			return PTR_ERR(adc->vref);

You can "return dev_err_probe(...);" directly and save a line of code

> +		}
> +	} else {
> +		ret = regulator_enable(adc->vref);
> +		if (ret)
> +			return ret;
> +

Use devm_add_action_or_reset to register a disable function. That gets rid of 
the gotos and the remove() callback as well. See my version of the driver for 
implementation example.


> +		dev_dbg(&adc->spi->dev, "%s: Using External Vref\n",
> +			__func__);
> +
> +		ret = regulator_get_voltage(adc->vref);
> +		if (ret < 0) {
> +			dev_err_probe(&adc->spi->dev, ret,
> +				      "Failed to read vref regulator\n");
> +			goto error_disable_reg;
> +		}
> +
> +		adc->vref_mv = ret / 1000;
> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	device_index = spi_get_device_id(spi)->driver_data;
> +	adc->chip_info = &mcp356x_chip_infos_tbl[device_index];
> +
> +	adc->mcp356x_info.read_raw = mcp356x_read_raw;
> +	adc->mcp356x_info.write_raw = mcp356x_write_raw;
> +	adc->mcp356x_info.read_avail = mcp356x_read_avail;
> +
> +	ret = mcp356x_prep_custom_attributes(adc, indio_dev);
> +	if (ret) {
> +		dev_err_probe(&adc->spi->dev, ret,
> +			      "Can't configure custom attributes for MCP356X device\n");
> +		goto error_disable_reg;
> +	}
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &adc->mcp356x_info;
> +
> +	indio_dev->channels = adc->chip_info->channels;
> +	indio_dev->num_channels = adc->chip_info->num_channels;
> +	indio_dev->masklength = adc->chip_info->num_channels - 1;
> +
> +	/* initialize the chip access mutex */
> +	mutex_init(&adc->lock);
> +
> +	/* Do any chip specific initialization, e.g:
> +	 * read/write some registers
> +	 * enable/disable certain channels
> +	 * change the sampling rate to the requested value
> +	 */
> +	ret = mcp356x_config(adc);
> +	if (ret) {
> +		dev_err_probe(&adc->spi->dev, ret,
> +			      "Can't configure MCP356X device\n");
> +		goto error_disable_reg;
> +	}
> +
> +	dev_dbg(&adc->spi->dev, "%s: Vref (mV): %d\n", __func__, adc->vref_mv);
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret) {
> +		dev_err_probe(&adc->spi->dev, ret,
> +			      "Can't register IIO device\n");
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	if (adc->vref)
> +		regulator_disable(adc->vref);
> +
> +	return ret;
> +}
> +
> +static void mcp356x_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct mcp356x_state *adc = iio_priv(indio_dev);
> +
> +	if (adc->vref)
> +		regulator_disable(adc->vref);
> +}
> +
> +static const struct of_device_id mcp356x_dt_ids[] = {
> +	{ .compatible = "microchip,mcp3461" },
> +	{ .compatible = "microchip,mcp3462" },
> +	{ .compatible = "microchip,mcp3464" },
> +	{ .compatible = "microchip,mcp3461r" },
> +	{ .compatible = "microchip,mcp3462r" },
> +	{ .compatible = "microchip,mcp3464r" },
> +	{ .compatible = "microchip,mcp3561" },
> +	{ .compatible = "microchip,mcp3562" },
> +	{ .compatible = "microchip,mcp3564" },
> +	{ .compatible = "microchip,mcp3561r" },
> +	{ .compatible = "microchip,mcp3562r" },
> +	{ .compatible = "microchip,mcp3564r" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mcp356x_dt_ids);
> +
> +static const struct spi_device_id mcp356x_id[] = {
> +	{ "mcp3461",  mcp3461 },
> +	{ "mcp3462",  mcp3462 },
> +	{ "mcp3464",  mcp3464 },
> +	{ "mcp3461r", mcp3461r },
> +	{ "mcp3462r", mcp3462r },
> +	{ "mcp3464r", mcp3464r },
> +	{ "mcp3561",  mcp3561 },
> +	{ "mcp3562",  mcp3562 },
> +	{ "mcp3564",  mcp3564 },
> +	{ "mcp3561r", mcp3561r },
> +	{ "mcp3562r", mcp3562r },
> +	{ "mcp3564r", mcp3564r },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, mcp356x_id);
> +
> +static struct spi_driver mcp356x_driver = {
> +	.driver = {
> +		.name = "mcp3564",
> +		.of_match_table = mcp356x_dt_ids,
> +	},
> +	.probe = mcp356x_probe,
> +	.remove = mcp356x_remove,
> +	.id_table = mcp356x_id,
> +};
> +
> +module_spi_driver(mcp356x_driver);
> +
> +MODULE_AUTHOR("Marius Cristea <marius.cristea@microchip.com>");
> +MODULE_DESCRIPTION("Microchip MCP346x/MCP346xR and MCP356x/MCP346xR ADCs");
> +MODULE_LICENSE("GPL v2");

-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl


  parent reply	other threads:[~2023-05-24 11:05 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-19 16:01 [PATCH 0/2] Adding support for Microchip MCP3564 ADC family marius.cristea
2023-05-19 16:01 ` [PATCH 1/2] dt-bindings: iio: adc: adding MCP3564 ADC marius.cristea
2023-05-19 18:29   ` Conor Dooley
2023-05-20 15:17     ` Jonathan Cameron
2023-06-08 19:34       ` Rob Herring
2023-06-09 17:41         ` Jonathan Cameron
2023-05-20 13:20   ` Jonathan Cameron
2023-05-24 10:37   ` Mike Looijmans
2023-05-28 19:16     ` Jonathan Cameron
2023-05-19 16:01 ` [PATCH 2/2] iio: adc: adding support for " marius.cristea
2023-05-20 15:15   ` Jonathan Cameron
2023-06-22 11:32     ` Marius.Cristea
2023-07-02 10:28       ` Jonathan Cameron
2023-05-24 11:05   ` Mike Looijmans [this message]
2023-05-28 19:20     ` Jonathan Cameron
2023-06-20 14:34       ` Mike Looijmans

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=915a5d31-5eb4-54b9-3fc9-ff69836f3d76@topic.nl \
    --to=mike.looijmans@topic.nl \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marius.cristea@microchip.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).