Message-ID: <20250920125144.41f70a1f@jic23-huawei>
Date: Sat, 20 Sep 2025 12:51:44 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marius Cristea <marius.cristea@...rochip.com>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, "Andy Shevchenko" <andy@...nel.org>, Rob Herring
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] iio: temperature: add support for EMC1812

On Wed, 17 Sep 2025 15:21:58 +0300
Marius Cristea <marius.cristea@...rochip.com> wrote:

> This is the iio driver for Microchip EMC1812/13/14/15/33
> Multichannel Low-Voltage Remote Diode Sensor Family.
> 
> Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>

A few minor comments inline,

Thanks,

Jonathan

> diff --git a/drivers/iio/temperature/emc1812.c b/drivers/iio/temperature/emc1812.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b1f567b222026a7138e9c29d739d0240f4dc726f
> --- /dev/null
> +++ b/drivers/iio/temperature/emc1812.c
> @@ -0,0 +1,792 @@


> +
> +static int emc1812_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct emc1812_priv *priv = iio_priv(indio_dev);
> +	unsigned int i;
> +	int ret;
> +
> +	guard(mutex)(&priv->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(emc1812_freq); i++)
> +			if (val == emc1812_freq[i][0] && val2 == emc1812_freq[i][1])
> +				break;
> +
> +		if (i == ARRAY_SIZE(emc1812_freq))
> +			return -EINVAL;
> +
> +		ret = regmap_write(priv->regmap, EMC1812_CONV_ADDR, i);
> +		if (ret)
> +			return ret;
> +
> +		priv->freq_idx = i;
> +		break;
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		for (i = 0; i < ARRAY_SIZE(emc1812_3db_values_map_tbl[priv->freq_idx]); i++)
> +			if (val == emc1812_3db_values_map_tbl[priv->freq_idx][i][0] &&
> +			    val2 == emc1812_3db_values_map_tbl[priv->freq_idx][i][1])
> +				break;
> +
> +		if (i == ARRAY_SIZE(emc1812_3db_values_map_tbl[priv->freq_idx]))
> +			return -EINVAL;
> +
> +		/*
> +		 * In emc1812_3db_values_map_tbl the second index maps:
> +		 * 0 for filter off
> +		 * 1 for filter at level 1
> +		 * 2 for filter at level 2
> +		 */
> +		if (i == 2)
> +			i = 3;
> +
> +		ret = regmap_write(priv->regmap, EMC1812_FILTER_SEL_ADDR, i);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
Prefer early returns above so that a reader can quickly see where a particular
code path goes without needing to scroll down here to see there is nothing else
to do.

> +}

> +static int emc1812_init(struct emc1812_priv *priv)
> +{
> +	unsigned int i;
> +	int ret;
> +	u8 val;
> +
> +	/*
> +	 * Depending on the chip, lock channel beta 1 and/or 2 to Diode Mode
> +	 * when APDD is enabled.
> +	 */
> +	if (priv->chip->lock_beta1 && priv->apdd_en)
> +		priv->beta_values[0] = 0x0F;
> +	if (priv->chip->lock_beta2 && priv->apdd_en)
> +		priv->beta_values[1] = 0x0F;
> +
> +	/*
> +	 * Set default values in registers. APDD, RECD12 and RECD34 are active
> +	 * on 0.
> +	 * Set the device to be in Run (Active) state and converting on all
> +	 * channels.
> +	 * The temperature measurement range is -64°C to +191.875°C.
> +	 */
> +	val = FIELD_PREP(EMC1812_CFG_MSKAL, 1) |
> +	      FIELD_PREP(EMC1812_CFG_RS, 0) |
> +	      FIELD_PREP(EMC1812_CFG_ATTHM, 1) |
> +	      FIELD_PREP(EMC1812_CFG_RECD12, !priv->recd12_en) |
> +	      FIELD_PREP(EMC1812_CFG_RECD34, !priv->recd34_en) |
> +	      FIELD_PREP(EMC1812_CFG_RANGE, 1) |
> +	      FIELD_PREP(EMC1812_CFG_DA_ENA, 0) |
> +	      FIELD_PREP(EMC1812_CFG_APDD, !priv->apdd_en);
> +
> +	ret = regmap_write(priv->regmap, EMC1812_CFG_ADDR, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Default is 4 conversions/seconds */
/second

Odd that is the value 6.  Maybe this needs a define?
> +	ret = regmap_write(priv->regmap, EMC1812_CONV_ADDR, 6);
> +	if (ret)
> +		return ret;
> +	priv->freq_idx = 6;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_THRM_HYS_ADDR, 0x0A);

These values are all rather non obvious.  So add a comment or where possible
defines to explain the values being set.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_CONSEC_ALERT_ADDR, 0x70);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_FILTER_SEL_ADDR, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_HOTTEST_CFG_ADDR, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Set beta1 and beta2 compensation parameters */
> +	for (i = 0; i < ARRAY_SIZE(priv->beta_values); i++) {
> +		ret = regmap_write(priv->regmap, EMC1812_BETA_CFG_ADDR(i),
> +				   priv->beta_values[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Set ideality factor for all external channels */
> +	ret = regmap_write(priv->regmap, EMC1812_EXT1_IDEALITY_FACTOR_ADDR,

Perhaps a look up table for the registers and a loop?


> +			   priv->ideality_value[0]);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_EXT2_IDEALITY_FACTOR_ADDR,
> +			   priv->ideality_value[1]);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_EXT3_IDEALITY_FACTOR_ADDR,
> +			   priv->ideality_value[2]);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_EXT4_IDEALITY_FACTOR_ADDR,
> +			   priv->ideality_value[3]);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
return regmap_write()

> +}
> +
> +static int emc1812_parse_fw_config(struct emc1812_priv *priv, struct device *dev,
> +				   int device_nr_channels)
> +{
> +	unsigned int reg_nr, iio_idx, tmp;
> +	int ret;
> +
> +	priv->apdd_en = device_property_read_bool(dev, "microchip,enable-anti-parallel");
> +	priv->recd12_en = device_property_read_bool(dev, "microchip,parasitic-res-on-channel1-2");
> +	priv->recd34_en = device_property_read_bool(dev, "microchip,parasitic-res-on-channel3-4");
> +
> +	memset32(priv->beta_values, 16, ARRAY_SIZE(priv->beta_values));

I would just set the two separately. This optimisation would make sense if there were a lot
more entries, but not worth making things harder to read when it is only two values,
particularly when the type isn't a specific size int.

> +	device_property_read_u32(dev, "microchip,beta1", &priv->beta_values[0]);
> +	device_property_read_u32(dev, "microchip,beta2", &priv->beta_values[1]);
> +	if (priv->beta_values[0] > 16 || priv->beta_values[1] > 16)
> +		return dev_err_probe(dev, -EINVAL, "Invalid beta value\n");
> +
> +	priv->num_channels = device_get_child_node_count(dev) + 1;
> +
> +	if (priv->num_channels > priv->chip->phys_channels)
> +		return dev_err_probe(dev, -E2BIG, "More channels than the chip supports\n");
> +
> +	priv->iio_chan[0] = EMC1812_CHAN(0, 0, EMC1812_CH_ADDR(0));
> +

I would remove this blank line to keep the association between the channel and it's label
tighter. That avoids the need for an explanatory comment on what this first channel is.

I'll note this code is very similar to what I commented on earlier in Victors driver so
take a look at that and see if I failed to notice anything here I picked up on in that review.

> +	priv->labels[0] = "internal_diode";
> +	iio_idx = 1;
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg_nr);
> +		if (ret || reg_nr >= priv->chip->phys_channels)
> +			return dev_err_probe(dev, -EINVAL,
> +				     "The index of the channels does not match the chip\n");
> +
> +		ret = fwnode_property_read_u32(child, "microchip,ideality-factor", &tmp);
> +		if (ret == 0) {
> +			if (tmp < 8  || tmp > 63)
> +				return dev_err_probe(dev, ret, "Invalid ideality value\n");
> +			priv->ideality_value[reg_nr - 1] = tmp;
> +		} else {
> +			priv->ideality_value[reg_nr - 1] = 18;
> +		}
> +
> +		fwnode_property_read_string(child, "label", &priv->labels[reg_nr]);
> +
> +		priv->iio_chan[iio_idx++] = EMC1812_CHAN(reg_nr, reg_nr, EMC1812_CH_ADDR(reg_nr));
> +	}
> +
> +	return 0;
> +}
> +
> +static int emc1812_chip_identify(struct emc1812_priv *priv, struct i2c_client *client)
> +{
> +	int ret, tmp;
> +
> +	ret = regmap_read(priv->regmap, EMC1812_PRODUCT_ID_ADDR, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	switch (tmp) {
> +	case EMC1812_PID:
> +		priv->chip = &emc1812_chip_config;
> +		break;

When there is nothing else to do, I'm a big fan of early returns that
immediately let the reader know we are done.

> +	case EMC1813_PID:
> +		priv->chip = &emc1813_chip_config;
> +		break;
> +	case EMC1814_PID:
> +		priv->chip = &emc1814_chip_config;
> +		break;
> +	case EMC1815_PID:
> +		priv->chip = &emc1815_chip_config;
> +		break;
> +	case EMC1833_PID:
> +		priv->chip = &emc1833_chip_config;
> +		break;
> +	default:
> +		/*
> +		 * If failed to identify the hardware based on internal registers,
> +		 * try using fallback compatible in device tree to deal with some
> +		 * newer part number.
> +		 */
> +		priv->chip = i2c_get_match_data(client);
> +		if (!priv->chip)
> +			return -EINVAL;
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int emc1812_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct emc1812_priv *priv;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(indio_dev);
> +	priv->regmap = devm_regmap_init_i2c(client, &emc1812_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "Cannot initialize register map\n");
> +
> +	ret = devm_mutex_init(dev, &priv->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = emc1812_chip_identify(priv, client);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Chip identification fails\n");
> +
> +	dev_info(dev, "Device name: %s\n", priv->chip->name);

Too noisy. dev_dbg() at most, probably just drop it entirely as these are easy
to query if the driver probes successfully. 

> +
> +	ret = emc1812_parse_fw_config(priv, dev, priv->chip->phys_channels);
> +	if (ret)
> +		return ret;
> +
> +	ret = emc1812_init(priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot initialize device\n");
> +
> +	indio_dev->name = priv->chip->name;
> +	indio_dev->info = &emc1812_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &priv->iio_chan[0];

Given it's a pointer to an array to me using priv->iio_chan seems more
logical.

> +	indio_dev->num_channels = priv->num_channels;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot register IIO device\n");
> +
> +	return 0;
> +}

> +static const struct of_device_id emc1812_of_match[] = {
> +	{
> +		.compatible = "microchip,emc1812",
> +		.data = &emc1812_chip_config
> +	},
> +	{
> +		.compatible = "microchip,emc1813",
> +		.data = &emc1813_chip_config
> +	},
> +	{
> +		.compatible = "microchip,emc1814",
> +		.data = &emc1814_chip_config
> +	},
> +	{
> +		.compatible = "microchip,emc1815",
> +		.data = &emc1815_chip_config
> +	},
> +	{
> +		.compatible = "microchip,emc1833",
> +		.data = &emc1833_chip_config

Trailing commas needed for these.  Reduces churn if we want
to set other fields in future.

> +	},
> +	{ }
> +};

Powered by blists - more mailing lists