Re: [PATCH v2 2/3] iio: light: Add support for AL3000a illuminance sensor

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 15 Feb 2025 12:31:58 +0200
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> AL3000a is a simple I2C-based ambient light sensor, which is
> closely related to AL3010 and AL3320a, but has significantly
> different way of processing data generated by the sensor.
> 
> Tested-by: Robert Eckelmann <longnoserob@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

Hi,

A few really minor comments inline.  A nice small driver :)

Jonathan


> diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
> new file mode 100644
> index 000000000000..58d4336dd081
> --- /dev/null
> +++ b/drivers/iio/light/al3000a.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
The iio/sysfs.h header is a bit of a legacy thing.  Ideally we will
eventually get rid of it.  In this driver, you are correctly not using
anything it provides so drop this include.

> +
> +static const struct iio_chan_spec al3000a_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
As below. I'm a little confused on units, but this may want to just
be BIT(IIO_CHAN_INFO_PROCESSED)
> +	},
> +};
> +
> +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE;
> +	int ret;
> +
> +	if (pwr) {

The two flows are different enough I'd split this into power on and power
off functions.  Will be a few lines longer but slightly easier to read.

> +		ret = regulator_enable(data->vdd_supply);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable vdd power supply\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to write system register\n");
> +		return ret;
> +	}
> +
> +	if (!pwr) {
> +		ret = regulator_disable(data->vdd_supply);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to disable vdd power supply\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void al3000a_set_pwr_off(void *_data)
> +{
> +	struct al3000a_data *data = _data;
> +
> +	al3000a_set_pwr(data, false);
> +}
> +
> +static int al3000a_init(struct al3000a_data *data)
> +{
> +	int ret;
> +
> +	ret = al3000a_set_pwr(data, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_ENABLE);

regmap functions return <= 0 in all cases. So you can just do if (ret)
or in cases like this save a few lines with

	return regmap_write();

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int al3000a_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct al3000a_data *data = iio_priv(indio_dev);
> +	int ret, gain;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(data->regmap, AL3000A_REG_DATA, &gain);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = lux_table[gain & AL3000A_GAIN_MASK];

I may have misinterpreted the other thread.  IS this value in lux?
If it is make this channel IIO_CHAN_INFO_PROCESSED instead.

> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1;

Either way, default scale is 1 so this should not be expressed at all.

> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info al3000a_info = {
> +	.read_raw	= al3000a_read_raw,

I'd not put a tab in the middle.  Single space is fine.
This sort of alignment is a pain for long term maintenance anyway
as we get very noisy patches realigning things.
It makes even less sense with only one item!

> +};
> +
> +static int al3000a_probe(struct i2c_client *client)
> +{
> +	struct al3000a_data *data;
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &al3000a_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "cannot allocate regmap\n");
> +
> +	data->vdd_supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
> +				     "failed to get vdd regulator\n");
> +
> +	indio_dev->info = &al3000a_info;
> +	indio_dev->name = AL3000A_DRV_NAME;

As there is no actual reason why this name should necessarily match the
name of the driver I'd prefer to see the string expressed directly in both
places.  That avoids giving the wrong impression as the driver gains support
for additional devices and generally makes things slightly easier to review.

> +	indio_dev->channels = al3000a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = al3000a_init(data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to init ALS\n");
> +
> +	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to add action\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(al3000a_pm_ops, al3000a_suspend, al3000a_resume);
> +
> +static const struct of_device_id al3000a_of_match[] = {
> +	{ .compatible = "dynaimage,al3000a" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, al3000a_of_match);

Also provide the i2c_device_id table as it's used for autoprobing of the
driver (long story for why we still need that!)

> +
> +static struct i2c_driver al3000a_driver = {
> +	.driver = {
> +		.name = AL3000A_DRV_NAME,
> +		.of_match_table = al3000a_of_match,
> +		.pm = pm_sleep_ptr(&al3000a_pm_ops),
> +	},
> +	.probe = al3000a_probe,
> +};
> +module_i2c_driver(al3000a_driver);
> +
> +MODULE_AUTHOR("Svyatolsav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL");