Re: [PATCH v3 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Andy Shevchenko]

On Wed, Sep 09, 2026 at 11:38:13PM -0700, Chang Yu wrote:
> This patch adds a driver for the AMS AS7343 14-channel multi-spectral
> sensor with I2C interface.
> 
> The driver exposes 12 spectral channels (11 visible + 1 near-infrared)
> via the IIO sysfs interface. Each channel's raw data is provided as a
> 16-bit little-endian unsigned integer.
> 
> Basic power management (suspend/resume) is supported. Note that power
> management is designed in such a way that we only stop spectrual
> measurements when susepended and do not power off.
> 
> More complex features such as auto-suspend, interrupts, and
> configurable gain/integration time will be added in future patches.

It's v3 already. Can you browse the linux-iio@ mailing list archive and see
what are the common comments on the new contributions? I think you may ask
AI to help with the summary. This patch has tons of what has been repeated
over and over...

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>

> +#include <linux/pm.h>

This is implied by pm_runtime.h IIRC.

> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>

...

> +/* AS7343 registers */
> +#define AS7343_ID	    0x5a
> +
> +#define AS7343_ENABLE	    0x80

Make sure the indentation of the definition of the same kind are the same.

...

> +/*
> + * Integration time is calculated as (ATIME + 1) * ((ASTEP + 1) * 2.78us).
> + * Setting a 30 * 1.67ms = 50.1ms integration test as the default for now.
> + */
> +#define AS7343_ATIME			   0x81

I believe this one is related to the register offsets? (see above why)

...

> +static int as7343_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)

Please, split logically.

			   struct iio_chan_spec const *chan,
			   int *val, int *val2, long mask)

> +{
> +	struct as7343_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +	unsigned int unused;
> +	__le16 result;

Preserve reversed xmas tree order.

> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;

Use PM_RUNTIME_ACQUIRE*().

> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		/* Wait until integration time passes for all 3 cycles. */
> +		msleep(160);
> +
> +		/*
> +		 * Reading ASTATUS latches all data registers to this read.
> +		 * We don't care about the returned saturation/gain status for
> +		 * now.
> +		 */
> +		guard(mutex)(&data->mutex);
> +		ret = regmap_read(data->regmap, AS7343_ASTATUS, &unused);
> +		if (ret)
> +			break;
> +
> +		ret = regmap_bulk_read(data->regmap, chan->address, &result, 2);

sizeof()

> +		if (ret)
> +			break;
> +
> +		*val = le16_to_cpu(result);
> +		ret = IIO_VAL_INT;
> +		break;
> +	}
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put(dev);
> +	return ret;
> +}

(All comments for the above function is what has been repeated in many
 contributions for sure.)

 ...

> +static const char *as7343_channel_label(struct iio_chan_spec const *chan)
> +{
> +	switch (chan->channel) {
> +	case AS7343_CHAN_IDX_FZ:
> +		return "FZ";
> +	case AS7343_CHAN_IDX_FY:
> +		return "FY";
> +	case AS7343_CHAN_IDX_FXL:
> +		return "FXL";
> +	case AS7343_CHAN_IDX_NIR:
> +		return "NIR";
> +	case AS7343_CHAN_IDX_F2:
> +		return "F2";
> +	case AS7343_CHAN_IDX_F3:
> +		return "F3";
> +	case AS7343_CHAN_IDX_F4:
> +		return "F4";
> +	case AS7343_CHAN_IDX_F6:
> +		return "F6";
> +	case AS7343_CHAN_IDX_F1:
> +		return "F1";
> +	case AS7343_CHAN_IDX_F7:
> +		return "F7";
> +	case AS7343_CHAN_IDX_F8:
> +		return "F8";
> +	case AS7343_CHAN_IDX_F5:
> +		return "F5";
> +	default:
> +		return NULL;
> +	}

Why not keeping this in a static array?

> +}
> +
> +static int as7343_read_label(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, char *label)
> +{
> +	const char *name;
> +
> +	name = as7343_channel_label(chan);
> +	if (!name)
> +		return -EINVAL;

Why? Can't it be taken from DT?

> +	return sysfs_emit(label, "%s\n", name);
> +}

...

> +static const struct regmap_config as7343_regmap_config = {
> +	.name = "as7343",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AS7343_MAX,
> +	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,

> +	.cache_type = REGCACHE_NONE,

Why?! This needs a very good justification.

> +};

...

> +static int as7343_setup_device(struct device *dev, struct as7343_data *data)
> +{

	struct regmap *map = data->regmap;

will help to reduce verbosity of the below, and might even save some LoC...

> +	unsigned int val;
> +	__le16 step;
> +	int ret;
> +
> +	/* Power on */
> +	ret = regmap_set_bits(data->regmap, AS7343_ENABLE, AS7343_ENABLE_PON);
> +	if (ret)
> +		return ret;
> +
> +	/* Need to set REG_BANK to 1 before we can access ID */
> +	ret = regmap_set_bits(data->regmap, AS7343_CFG0, AS7343_CFG0_REG_BANK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, AS7343_ID, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != 0x81)
> +		dev_info(dev, "Unknown device ID: %x\n", val);
> +
> +	ret = regmap_clear_bits(data->regmap, AS7343_CFG0,
> +				AS7343_CFG0_REG_BANK);

...for example, here:

	ret = regmap_clear_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);

> +	if (ret)
> +		return ret;
> +
> +	/* Configure the SMUX to readout all channels */

> +	ret = regmap_update_bits(

Huh?! Please, check the formatting and indentation style.

> +		data->regmap, AS7343_CFG20, AS7343_CFG20_AUTO_SMUX,
> +		FIELD_PREP(AS7343_CFG20_AUTO_SMUX,
> +			   AS7343_CFG20_AUTO_SMUX_READOUT_ALL));
> +	if (ret)
> +		return ret;
> +
> +	/* Set 50.1ms integration time and x256 gain for now */
> +	step = cpu_to_le16(AS7343_ASTEP_VAL);
> +	ret = regmap_bulk_write(data->regmap, AS7343_ASTEP, &step, 2);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AS7343_ATIME, AS7343_ATIME_VAL);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, AS7343_CFG1, AS7343_CFG1_AGAIN,
> +				  FIELD_PREP(AS7343_CFG1_AGAIN,
> +					     AS7343_CFG1_AGAIN_X256));
> +}

...

> +static int as7343_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct as7343_data *data = iio_priv(indio_dev);
> +
> +	return regmap_clear_bits(data->regmap, AS7343_ENABLE,
> +				 AS7343_ENABLE_SP_EN);
> +}
> +
> +static int as7343_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct as7343_data *data = iio_priv(indio_dev);
> +
> +	return regmap_set_bits(data->regmap, AS7343_ENABLE,
> +			       AS7343_ENABLE_SP_EN);
> +}

Same, use temporary for struct regmap.

...

> +static int as7343_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct as7343_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init_i2c(client, &as7343_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->regmap = regmap;

> +	mutex_init(&data->mutex);

devm_mutex_init().

> +
> +	indio_dev->name = "as7343";
> +	indio_dev->info = &as7343_info;
> +	indio_dev->channels = as7343_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(as7343_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return ret;
> +
> +	ret = as7343_setup_device(dev, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, as7343_suspend_action, dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add suspend action\n");
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to activate PM runtime\n");
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> +
> +	/* Start measurements */
> +	ret = regmap_set_bits(data->regmap, AS7343_ENABLE, AS7343_ENABLE_SP_EN);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume,
> +				 NULL);

Again, split logically. Options are:

static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume, NULL);

static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
				 as7343_suspend, as7343_resume, NULL);

static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
				 as7343_suspend,
				 as7343_resume,
				 NULL);

(I personally prefer compromise as depicted in the second example).

...

> +static const struct of_device_id as7343_of_match[] = {
> +	{ .compatible = "ams,as7343" },
> +	{ },

No comma in the terminator entry.

> +};
> +MODULE_DEVICE_TABLE(of, as7343_of_match);
> +
> +static const struct i2c_device_id as7343_id[] = {
> +	{ .name = "as7343" },
> +	{ },

Ditto.

> +};
> +MODULE_DEVICE_TABLE(i2c, as7343_id);

...

> +static struct i2c_driver as7343_driver = {
> +	.driver = {
> +		.name           = "as7343",
> +		.of_match_table = as7343_of_match,
> +		.pm             = pm_ptr(&as7343_pm_ops),
> +	},
> +	.probe      = as7343_probe,
> +	.id_table   = as7343_id,

Indentation of the assignees with tabs makes it harder to maintain (in case
more lines got added it might require to reindent all of them).

> +};
> +module_i2c_driver(as7343_driver);

-- 
With Best Regards,
Andy Shevchenko