ср, 12 лют. 2025 р. о 16:28 Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> пише:
>
> 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.
>
Patch checking script did not catch this as warning or style issue. If
such commenting is discouraged than please add this to patch checking
script. Comments are stripped on compilation anyway, they do not
affect size.
> ...
>
> > +#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.
>
Is there a check which determines the amount of headers I must include
and which headers are mandatory to be included and which are forbidden
to inclusion. Maybe at least a list? Thanks
>
> > +#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).
No, this list will not grow since the bit field seems to be 0x3f
(datasheet is not available, code is adaptation of downstream driver).
> > +};
>
> ...
>
> > +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涎 APIs?
>
This adaptation was written quite a long time ago, patch check did not
complained about using of i2c smbus. Is use of regmap mandatory now?
Is it somewhere specified? Thanks
I am not a full time linux contributor and may not be familiar with
the recent rules.
> > + 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
>
>
Everything else is valid, thank you. I will add fixes and try to
switch to regmap.
