нд, 16 лют. 2025 р. о 16:54 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
>
> On Sat, 15 Feb 2025 12:31:58 +0200
> Svyatoslav Ryhel <clamor95@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
>
> 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.
>
This is actually a really good hint, I will check if this works out
and if yes, then definitely will use it. Thank you.
> > +
> > + 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@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("al3000a Ambient Light Sensor driver");
> > +MODULE_LICENSE("GPL");
>
Everything else, acknowledged and accepted. Thank you.
