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
