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

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

On Wed, Feb 12, 2025 at 08:46:56AM +0200, Svyatoslav Ryhel wrote:
> AL3000a is a simple I2C-based ambient light sensor, which is
> closely related to AL3010 and AL3320a, but has significantly
> different hardware configuration.

(Note, the part of the below comments are applicable to your other series)

...

> +/*
> + * AL3000a - Dyna Image Ambient Light Sensor
> + */

Can be on a single line.

...

> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

No of*.h in the new code, please.

> +#include <linux/regulator/consumer.h>

Too small headers to be included. You use much more.


> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

...

> +/*
> + * This are pre-calculated lux values based on possible output
> + * of sensor (range 0x00 - 0x3F)
> + */

types.h

> +static const u32 lux_table[64] = {

I think you don't need 64 to be there, but okay, I understand the intention.

> +	1, 1, 1, 2, 2, 2, 3, 4, 4, 5, 6, 7, 9, 11, 13, 16,

For the better readability and maintenance put pow-of-2 amount of values per
line, like 8, and add the respective comment:

	1, 1, 1, 2, 2, 2, 3, 4,					/*  0 -  7 */
	4, 5, 6, 7, 9, 11, 13, 16,				/*  8 - 15 */

> +	19, 22, 27, 32, 39, 46, 56, 67, 80, 96, 116, 139,
> +	167, 200, 240, 289, 347, 416, 499, 600, 720, 864,
> +	1037, 1245, 1495, 1795, 2155, 2587, 3105, 3728,	4475,
> +	5373, 6450, 7743, 9296, 11160, 13397, 16084, 19309,
> +	23180, 27828, 33408, 40107, 48148, 57803, 69393,
> +	83306, 100000

Leave trailing comma, it's not a terminated list generally speaking
(in the future it might grow).

> +};

...

> +struct al3000a_data {
> +	struct i2c_client *client;

	struct regmap *map;

will suffice, I believe, see below.


> +	struct regulator *vdd_supply;
> +};

...

> +static const struct iio_chan_spec al3000a_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),

> +	}

Leave trailing comma

> +};

...

> +static int al3000a_set_pwr(struct al3000a_data *data, bool pwr)
> +{
> +	struct device *dev = &data->client->dev;
> +	u8 val = pwr ? AL3000A_CONFIG_ENABLE : AL3000A_CONFIG_DISABLE;
> +	int ret;
> +
> +	if (pwr) {
> +		ret = regulator_enable(data->vdd_supply);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable vdd power supply\n");
> +			return ret;

With struct regmap *map in mind, the struct device *dev can be derived using
the respective API.

> +		}
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, AL3000A_REG_SYSTEM, val);

Why not using regmap I²C APIs?

> +	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 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;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

> +		ret = i2c_smbus_read_byte_data(data->client,
> +					       AL3000A_REG_DATA);

It may be a single line. There is a lot of room.

> +		if (ret < 0)
> +			return ret;
> +
> +		*val = lux_table[ret & 0x3F];

I believe you want to define the size of that table and use it here.
Also this needs a comment to explain the meaning of the ret >= 64 and
when it may happen.

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

> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;

Return directly from the default case.

> +}

...

> +static int al3000a_probe(struct i2c_client *client)
> +{
> +	struct al3000a_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;

	struct device *dev = &client->dev;

will make the below lines shorter and easier to read.


> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
> +				     "failed to get vdd regulator\n");

err.h

> +	indio_dev->info = &al3000a_info;
> +	indio_dev->name = AL3000A_DRV_NAME;
> +	indio_dev->channels = al3000a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);

array_size.h

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = al3000a_init(data);
> +	if (ret < 0)

> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to init ALS\n");

Single line.

> +	ret = devm_add_action_or_reset(&client->dev, al3000a_set_pwr_off,
> +				       data);

Ditto.

device.h

> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to add action\n");
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}

...

> +static const struct of_device_id al3000a_of_match[] = {

mod_devicetable.h

> +	{ .compatible = "dynaimage,al3000a" },
> +	{ /* sentinel */ }
> +};

...

> +static struct i2c_driver al3000a_driver = {
> +	.driver = {
> +		.name = AL3000A_DRV_NAME,
> +		.of_match_table = al3000a_of_match,

> +		.pm = pm_sleep_ptr(&al3000a_pm_ops),

pm.h

> +	},
> +	.probe = al3000a_probe,
> +};

-- 
With Best Regards,
Andy Shevchenko