Re: [PATCH 2/2] iio: amplifiers: add ADA4255 driver

[Jonathan Cameron <jic23@xxxxxxxxxx>]

On Mon, 20 Jan 2025 12:54:25 +0200
Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:

> The ADA4255 is a  precision programmable gain instrumentation amplifier
> (PGIA) with integrated bipolar charge pumps.

Good to shout a bit about this including a gpio chip.

> 
> With its integrated charge pumps, the ADA4255 internally produces the
> high voltage bipolar supplies needed to achieve a wide input voltage
> range (38V typical with VDDCP = 5V) without lowering input impedance.
> 
> The charge pump topology of the ADA4255 allows channels to be isolated
> with only low voltage components, reducing complexity, size, and
> implementation time in industrial and process control systems.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@xxxxxxxxx>

A few comments inline, but all superficial stuff. Looks like
a nice driver to me

Jonathan

> diff --git a/drivers/iio/amplifiers/ada4255.c b/drivers/iio/amplifiers/ada4255.c
> new file mode 100644
> index 0000000000000..8d25ffa7baa6c
> --- /dev/null
> +++ b/drivers/iio/amplifiers/ada4255.c
> @@ -0,0 +1,937 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2022 Analog Devices, Inc.

That date wants a -2025 given you will be changing this
in response to review anyway.

> + * Author: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>

> +
> +#define ADA4255_NAME				"ada4255"

Just use the string inline.  These generic 'driver name' defines
are unhelpful to reading the code in comparison with the string
in the place it's used.

> +
> +#define ada4255_from_clk_hw(hw) \
> +	container_of(hw, struct ada4255_state, int_clk_hw)
> +
> +struct ada4255_chip_info {
> +	const char			*name;
> +	bool				has_charge_pump;
> +};
> +
> +struct ada4255_state {
> +	struct spi_device		*spi;
only used to get to the dev.  So either have a struct device pointer
here or just get that from the regmap where needed.

> +	struct regmap			*regmap;
> +	struct clk			*mclk;
> +	const struct ada4255_chip_info	*chip_info;
> +
> +	/*
> +	 * Synchronize consecutive regmap operations.

Be more specific as regmap has it's own locks.
Perhaps a read modify write section that needs to
be locked around?

> +	 */
> +	struct mutex			lock;
> +	struct gpio_chip		gc;
> +	struct notifier_block		ext_clk_nb;
> +	struct clk_hw			int_clk_hw;
> +
> +	bool				gpio4_clk_en;
> +	bool				int_clk_out_en;
> +};

> +
> +static const unsigned int ada4255_excitation_current_ua_tbl[] = {
> +	0, 100, 200, 300, 400, 500, 600, 700, 800,
> +	900, 1000, 1100, 1200, 1300, 1400, 1500
9 on 1st line is a bit random.  Go with a power of 2.  8 is per line
and 1 on the last line probably best choiceh ere.


> +};
> +
> +static const unsigned int ada4255_clk_cp_sel_hz_tbl[] = {
> +	16000000, 8000000
> +};
> +
> +static const unsigned int ada4255_int_clk_rate_hz_tbl[] = {
> +	1000000, 125000,
> +};
> +
> +static int _ada4255_find_tbl_index(const unsigned int *tbl, unsigned int tbl_len,
> +				   unsigned int val, unsigned int *index)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < tbl_len; i++)

The kernel style docs are a bit ambiguous IIRC on this, but I'd add {}
on basis next bit is multiline.

> +		if (val == tbl[i]) {
> +			*index = i;
> +			return 0;
> +		}
> +
> +	return -EINVAL;
> +}

> +static int ada4255_set_gain(struct ada4255_state *st, int val, int val2)
> +{
> +	ssize_t tbl_len = ARRAY_SIZE(ada4255_gain_tbl);
> +	unsigned int i, g5, g4, g3_0;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(ada4255_gain_tbl); i++)
> +		if (val == ada4255_gain_tbl[i][0] &&
> +		    val2 == ada4255_gain_tbl[i][1])
> +			break;
> +
> +	if (i == tbl_len)
> +		return -EINVAL;
> +
> +	g5 = ada4255_gain_reg_tbl[i][0];
> +	g4 = ada4255_gain_reg_tbl[i][1];
> +	g3_0 = ada4255_gain_reg_tbl[i][2];
> +
> +	mutex_lock(&st->lock);
As below. guard() helps in this sort of case


> +
> +	ret = regmap_update_bits(st->regmap, ADA4255_TEST_MUX_REG,
> +				 ADA4255_GAIN_G5_MASK,
> +				 FIELD_PREP(ADA4255_GAIN_G5_MASK, g5));
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_update_bits(st->regmap, ADA4255_GAIN_MUX_REG,
> +				 ADA4255_GAIN_G4_MASK | ADA4255_GAIN_G3_0_MASK,
> +				 FIELD_PREP(ADA4255_GAIN_G4_MASK, g4) |
> +				 FIELD_PREP(ADA4255_GAIN_G3_0_MASK, g3_0));
> +
> +out:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int ada4255_get_gain(struct ada4255_state *st, int *val, int *val2)
> +{
> +	ssize_t tbl_len = ARRAY_SIZE(ada4255_gain_reg_tbl);
> +	unsigned int i, gain, test_gain, g5, g4, g3_0;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = regmap_read(st->regmap, ADA4255_GAIN_MUX_REG, &gain);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_read(st->regmap, ADA4255_TEST_MUX_REG, &test_gain);
> +
> +out:
> +	mutex_unlock(&st->lock);
scoped_guard(mutex, &st->lock) 
is useful in cases like this as can return without having to worry
about unlocking by hand.

> +
> +	if (ret)
> +		return ret;
> +
> +	g5 = FIELD_GET(ADA4255_GAIN_G5_MASK, test_gain);
> +	g4 = FIELD_GET(ADA4255_GAIN_G4_MASK, gain);
> +	g3_0 = FIELD_GET(ADA4255_GAIN_G3_0_MASK, gain);
> +
> +	for (i = 0; i < ARRAY_SIZE(ada4255_gain_reg_tbl); i++)
> +		if (g5 == ada4255_gain_reg_tbl[i][0] &&
> +		    g4 == ada4255_gain_reg_tbl[i][1] &&
> +		    g3_0 == ada4255_gain_reg_tbl[i][2])
> +			break;
> +
> +	if (i == tbl_len)
> +		return -EINVAL;
> +
> +	*val = ada4255_gain_tbl[i][0];
> +	*val2 = ada4255_gain_tbl[i][1];
> +
> +	return IIO_VAL_INT_PLUS_NANO;
> +}

> +static int ada4255_set_ch_en(struct ada4255_state *st, unsigned int ch, int val)
> +{
> +	unsigned int ch_val = val ? ada4255_ch_input_mux_tbl[ch] : 0;
> +	int current_val;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
guard(mutex)(&st->lock);
+ include cleanup.h
so we can have simpler direct returns instead of goto.

> +
> +	ret = ada4255_get_ch_en(st, ch, &current_val);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (current_val == val) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ret = regmap_write(st->regmap, ADA4255_INPUT_MUX_REG, ch_val);
> +
> +out:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}

> +static unsigned long ada4255_int_clk_recalc_rate(struct clk_hw *hw,
> +						 unsigned long parent_rate)
> +{
> +	struct ada4255_state *st = ada4255_from_clk_hw(hw);
> +	struct device *dev = &st->spi->dev;
> +	unsigned int i = 0;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, ADA4255_SYNC_CFG_REG, &i);
> +	if (ret)
> +		dev_err(dev, "Failed to read internal clock rate: %d\n", ret);
> +
> +	i = FIELD_GET(ADA4255_INT_CLK_OUT_MASK, i);
> +
> +	return ada4255_int_clk_rate_hz_tbl[i];
Might as well combine as

	return ada4244_int_clk_rate_hz_tbl[FIELD_GET(ADA4255_INT_CLK_OUT_MASK)];
to avoid confusing reuse of i.  Rename i as reg_val or something as well.

> +}
> +

> +static int ada4255_setup_ext_clk(struct ada4255_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	unsigned int divider;
> +	unsigned long rate;
> +	int ret;
> +
> +	ret = clk_prepare_enable(st->mclk);
Use get_optional_enabled() below.  If we have it want to turn it on and
then you don't need to handle tear down yourself.

> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, ada4255_clk_disable_unprepare,
> +				       st->mclk);

...

> +}
> +

> +
> +static int ada4255_setup(struct ada4255_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	unsigned int i;
> +	int ret = 0;

Always set, so no need to initialise here.

> +	u32 val;
> +
> +	st->mclk = devm_clk_get_optional(dev, "mclk");
> +	if (IS_ERR(st->mclk))
> +		return dev_err_probe(dev, PTR_ERR(st->mclk),
> +				     "Failed to get mclk\n");
> +
> +	if (st->mclk)
> +		ret = ada4255_setup_ext_clk(st);
> +	else
> +		ret = ada4255_setup_int_clk(st);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_read_u32(dev, "adi,excitation-current-microamp", &val);
> +	if (!ret) {
> +		ret = ada4255_find_tbl_index(ada4255_excitation_current_ua_tbl,
> +					     val, &i);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(st->regmap, AD4255_EX_CURRENT_CFG_REG,
> +					 AD4255_EX_CURRENT_MASK,
> +					 FIELD_PREP(AD4255_EX_CURRENT_MASK, i));
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(st->regmap, AD4255_EX_CURRENT_CFG_REG,
> +					 AD4255_EX_CURRENT_SEL_MASK,
> +					 FIELD_PREP(AD4255_EX_CURRENT_SEL_MASK, 1));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "adi,charge-pump-freq-hz", &val);
> +	if (!ret) {
> +		if (!st->chip_info->has_charge_pump)

This seems backwards.  Not up to us to check for extra properties in drivers.
So I'd check if we expect one before trying to read the property.

	if (!st->chip_info->has_charge_pump)
		return 0;

	ret = device_property_read_u32(dev, "adi,charge-pump-freq-hz", &val);
	if (ret)
		return ret; // 0 if it is optional.

	etc


> +			return dev_err_probe(dev, -EINVAL,
> +					     "Unsupported charge pump function\n");
> +
> +		ret = ada4255_find_tbl_index(ada4255_clk_cp_sel_hz_tbl, val, &i);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(st->regmap, ADA4255_SYNC_CFG_REG,
> +					 ADA4255_SYNC_CLK_CP_SEL_MASK,
> +					 FIELD_PREP(ADA4255_SYNC_CLK_CP_SEL_MASK, i));
> +		if (ret)
> +			return ret;

After reorder as above, can return regmap_update_bits() for this last one.

> +	}
> +
> +	return 0;
> +}

> +static int ada4255_reset(struct ada4255_state *st)
> +{
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, ADA4255_RESET_REG,
> +			   FIELD_PREP(ADA4255_RESET_REG, 1));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * A full calibration occurs after a soft reset and it takes
> +	 * approximately 85ms.
> +	 * See datasheet page 29, Section OUTPUT RIPPLE CALIBRATION
> +	 * CONFIGURATION.
I'd wrap this as one paragraph.  Looks a bit odd as it is now.
	 * A full calibration occurs after a soft reset and it takes
	 * approximately 85ms. See datasheet page 29, Section OUTPUT
	 * RIPPLE CALIBRATION CONFIGURATION.

> +	 */
> +	fsleep(85000);
> +
> +	return 0;
> +}
> +
> +static int ada4255_probe(struct spi_device *spi)
> +{
> +	static const char * const regulator_names[] = {
> +		"avdd",
> +		"dvdd",
> +		"vddcp",
> +		"vocm",

Fine to put these 4 on one line. We aren't likely to see
more added later and it will be a fairly short line.

> +	};
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ada4255_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +	st->chip_info = spi_get_device_match_data(spi);
> +	if (!st->chip_info)
> +		return -EINVAL;
> +
> +	indio_dev->info = &ada4255_info;
> +	indio_dev->name = st->chip_info->name;
> +	indio_dev->channels = ada4255_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ada4255_channels);
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
> +					     regulator_names);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> +
> +	st->regmap = devm_regmap_init_spi(spi, &ada4255_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
> +
> +	ret = ada4255_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ada4255_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ada4255_setup_gpio_chip(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

> +static struct spi_driver ada4255_driver = {
> +	.driver = {
> +			.name = ADA4255_NAME,
> +			.of_match_table = ada4255_of_match,

Indent is 1 tab more than standard

> +		},

Also for this.

> +	.probe = ada4255_probe,
> +	.id_table = ada4255_id,
> +};
> +
A nice pattern that people use is to group the
module_spi_driver() with it's one parameter by not
putting a blank line inbetween. Hence I would delete this
one.
> +module_spi_driver(ada4255_driver);

THanks,

Jonathan