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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Sep 11, 2026 at 06:39:12PM -0700, Chang Yu wrote:
> Add a driver for the AMS AS7343 14-channel multi-spectral sensor.
> 
> The AS7343 is a 14-channel spectral sensor featuring 11 visible
> channels, 1 near-infrared channel, 1 clear channel (VIS), and 1
> flicker detection channel.
> 
> The driver exposes 12 spectral channels (11 visible light and 1
> near-infrared) via sysfs. Runtime PM is implemented to stop
> measurements when the device is suspended or torn down. Power is
> never cut (PON=1 always) to preserve register values.
> 
> Future patches will add configurable gain and integration time,
> interrupt support, buffered reads, VIS channel, and flicker
> detection.

...

> +/*
> + * Support for AMS AS7343 14-channel multi-spectral sensor.
> + * (7-bit I2C slave address 0x39)
> + *
> + * Based on the work of:
> + *   Christian Eggers <ceggers@arri.de> (AS73211 driver)
> + *
> + * Copyright (c) 2026 Chang Yu <marcus.yu.56@gmail.com>
> + *
> + * Datasheets:

Why plural?

> + *   https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> + *
> + * TODO:
> + *   - Autosuspend
> + *   - Support for configurable gain and integration time
> + *   - Interrupt support
> + *   - Add support for reading the VIS channel
> + *   - Flicker detection
> + */

...

> +#define AS7343_ID				0x5a

> +/* AS7343 config registers */
> +#define AS7343_ENABLE				0x80

> +#define AS7343_ATIME				0x81

> +#define AS7343_ASTEP				0xd4


> +#define AS7343_CFG0				0xbf
> +
> +#define AS7343_CFG1				0xc6

> +#define AS7343_CFG20				0xd6

> +#define AS7343_CONTROL				0xfa
> +
> +/* AS7343 status registers */
> +#define AS7343_STATUS2				0x90
> +#define AS7343_STATUS3				0x91
> +#define AS7343_STATUS				0x93
> +#define AS7343_ASTATUS				0x94
> +#define AS7343_STATUS5				0xbb
> +#define AS7343_STATUS4				0xbc
> +#define AS7343_FD_STATUS			0xe3
> +
> +/* AS7343 spectral data registers */
> +#define AS7343_DATA_FZ		0x95
> +#define AS7343_DATA_FY		0x97
> +#define AS7343_DATA_FXL		0x99
> +#define AS7343_DATA_NIR		0x9b
> +#define AS7343_DATA_F2		0xa1
> +#define AS7343_DATA_F3		0xa3
> +#define AS7343_DATA_F4		0xa5
> +#define AS7343_DATA_F6		0xa7
> +#define AS7343_DATA_F1		0xad
> +#define AS7343_DATA_F7		0xaf
> +#define AS7343_DATA_F8		0xb1
> +#define AS7343_DATA_F5		0xb3
> +#define AS7343_DATA_FD_L	0xb7
> +#define AS7343_DATA_FD_H	0xb8

> +/* AS7343 FIFO buffer data registers */
> +#define AS7343_FIFO_LVL		0xfd
> +#define AS7343_FDATA_L		0xfe
> +#define AS7343_FDATA_H		0xff

Please, keep indentation for the registers the same. Also would be nice to have
them sorted by offset.

...

> +static int as7343_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct as7343_data *data = iio_priv(indio_dev);
> +	unsigned int unused;
> +	struct regmap *map;
> +	struct device *dev;
> +	__le16 result;
> +	int ret;

> +	map = data->regmap;
> +	dev = regmap_get_device(map);

Assign them directly in the definition block above.

> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	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);

Blank line. Always make guard()() to be visible (by grouping it separately
from the other pieces of code).

> +		ret = regmap_read(map, AS7343_ASTATUS, &unused);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_bulk_read(map, chan->address,
> +				       &result, sizeof(result));
> +		if (ret)
> +			return ret;
> +
> +		*val = le16_to_cpu(result);
> +		return IIO_VAL_INT;
> +	}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int as7343_setup_device(struct device *dev, struct as7343_data *data)
> +{
> +	struct regmap *map = data->regmap;
> +	unsigned int val;
> +	__le16 step;
> +	int ret;
> +
> +	/* Power on */
> +	ret = regmap_set_bits(map, 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(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, AS7343_ID, &val);
> +	if (ret)
> +		return ret;

> +	if (val != 0x81)
> +		dev_info(dev, "Unknown device ID: %x\n", val);

Wouldn't be better to define 0x81 with meaningful name?

> +	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(map, 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(map, AS7343_ASTEP, &step, sizeof(step));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(map, AS7343_ATIME, AS7343_ATIME_VAL);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(map, 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);
> +	struct regmap *map = data->regmap;

> +	return regmap_clear_bits(map, AS7343_ENABLE, AS7343_ENABLE_SP_EN);

Can this mess up the raw read? If so, also needs a mutex to be held.

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

Same Q.

...

> +static void as7343_suspend_action(void *data)
> +{
> +	struct device *dev = data;

Unneeded casting, can name parameter 'dev' above.

> +	as7343_suspend(dev);
> +}

-- 
With Best Regards,
Andy Shevchenko