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

[Jonathan Cameron]

> 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.
> 
> Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> Signed-off-by: Chang Yu <marcus.yu.56@xxxxxxxxx>

FWIW I believe the sashiko feedback autosuspend is incorrect, but
probably good to sanity check that by proving device and just checking
the calls in dd.c are sufficient to put it to sleep after the auto
suspend time.

Sashiko generally seems not to spot that probe is special in this
regard.  Probably doesn't help that there are quite a few drivers
that do have the handling it suggests, presumably because humans
didn't know it was special either ;)

A couple of follow up comments inline.  This is coming together
nicely so I think we are down to tiny details now.

Jonathan

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f0c08e634bf4..9496280903d9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1460,6 +1460,7 @@ M:	Chang Yu <marcus.yu.56@xxxxxxxxx>
>  L:	linux-iio@xxxxxxxxxxxxxxx
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> +F:	drivers/iio/light/as7343.c
>  
>  AMT (Automatic Multicast Tunneling)
>  M:	Taehee Yoo <ap420073@xxxxxxxxx>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 462fff1ddef2..d0dea4f3113b 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -152,6 +152,17 @@ config AS73211
>  	 This driver can also be built as a module.  If so, the module
>  	 will be called as73211.
>  
> +config AS7343
> +	tristate "AMS AS7343 14-Channel Multi-Spectral Sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	 Say Y here to build support for the AMS AS7343 14-channel
> +	 multi-spectral sensor.
> +
> +	 To compile this driver as a module, choose M here: the module will
> +	 be called as7343.
> +
>  config BH1745
>  	tristate "ROHM BH1745 colour sensor"
>  	depends on I2C

> diff --git a/drivers/iio/light/as7343.c b/drivers/iio/light/as7343.c
> new file mode 100644
> index 000000000000..c079e0c6aa70
> --- /dev/null
> +++ b/drivers/iio/light/as7343.c


> +
> +/*
> + * Integration time is calculated as (ATIME + 1) * ((ASTEP + 1) * 2.78us).
> + * ATIME must be between 0 and 255, inclusive
> + * ASTEP must be between 0 and 65534, inclusive
> + */
> +static inline u8 as7343_atime_steps(unsigned int step)
> +{
> +	BUILD_BUG_ON(step < 1 || step > 256);

Whilst for now this is fine as you aren't making these controllable,
I would make them runtime checks just to make a future change to
make these userspce adjustable easier to make.

That will complicate the code a little but give less churn in the
longer term.

> +	return step - 1;
> +}
> +
> +static inline u16 as7343_astep_x2780ns(unsigned int n)
> +{
> +	BUILD_BUG_ON(n < 1 || n > 65535);

Same here.

> +	return n - 1;
> +}
> +


...

> +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_REG, AS7343_ENABLE_PON);

If the regulators are fixed supplies and as such we haven't just
turned the power on here and device hasn't just taken default values,
could the previously driver / firmware / whatever have left us
using bank 1.  If so would that power on call do anything?

It's common to have a few control registers on a device accessible
from every bank, but that doesn't seem to be the case here. Though
given it won't work otherwise, I'm assuming it is the case for
CFG0 despite it being at an address of greater than 0x80!
As such expect to see first call being one to get the device using bank 0.

> +	if (ret)
> +		return ret;
> +
> +	/* Need to set REG_BANK to 1 before we can access ID */
> +	ret = regmap_update_bits(map, AS7343_CFG0_REG, AS7343_CFG0_REG_BANK,
> +				 FIELD_PREP(AS7343_CFG0_REG_BANK, 1));
> +	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);
> +
> +	ret = regmap_update_bits(map, AS7343_CFG0_REG, AS7343_CFG0_REG_BANK,
> +				 FIELD_PREP(AS7343_CFG0_REG_BANK, 0));
> +	if (ret)
> +		return ret;
> +
> +	/* Configure the SMUX to readout all channels */
> +	ret = regmap_update_bits(map, AS7343_CFG20_REG, AS7343_CFG20_AUTO_SMUX,
> +				 FIELD_PREP(AS7343_CFG20_AUTO_SMUX,
> +					    AS7343_CFG20_AUTO_SMUX_READOUT_ALL));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Setting a 600 * 2.78us * 30 = 50ms integration time as the default
> +	 * for now.
> +	 */
> +	step = cpu_to_le16(as7343_astep_x2780ns(600));
> +	ret = regmap_bulk_write(map, AS7343_ASTEP_REG, &step, sizeof(step));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(map, AS7343_ATIME_REG, as7343_atime_steps(30));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(map, AS7343_CFG1_REG, AS7343_CFG1_AGAIN,
> +				  FIELD_PREP(AS7343_CFG1_AGAIN,
> +					     AS7343_CFG1_AGAIN_X256));
> +}

...

> +static int as7343_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct as7343_data *data;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	regmap = devm_regmap_init_i2c(client, &as7343_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	data = iio_priv(indio_dev);
> +	data->regmap = regmap;
> +
> +	ret = devm_mutex_init(dev, &data->mutex);
> +	if (ret)
> +		return ret;
> +
> +	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;
> +
> +	/* Start measurements */
> +	ret = regmap_set_bits(regmap, AS7343_ENABLE_REG, AS7343_ENABLE_SP_EN);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 3000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = devm_add_action_or_reset(dev, as7343_suspend_action, dev);

Why here?  Neither of the two calls immediately above have anything to
do with what this is unwinding?  I think this should logically be
before the pm_runtime_set_autosuspend_delay() call so that is
is clearly matched with the regmap one above that.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>