On Sun, Nov 19, 2023 at 10:24:35PM +0100, Marek Vasut wrote:
> The ISL76682 is very basic ALS which only supports ALS or IR mode
> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any
> other fancy functionality.
...
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
This is incomplete.
At least array_size.h, bits.h, cleanup.h, types.h are missing.
...
> +#define ISL76682_ADC_MAX 0xffff
Wouldn't the (BIT(16) - 1) be better in order to show that this is limited by
a bit field capacity. Also you can use FIELD_MAX().
...
> +static int isl76682_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct isl76682_chip *chip = iio_priv(indio_dev);
> + int ret = -EINVAL;
This is less maintainable, use it directly (see below).
Ah, the value is even not used!
> + int i;
> +
> + if (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY)
> + return -EINVAL;
> +
> + guard(mutex)(&chip->lock);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_LIGHT:
> + ret = isl76682_get(chip, false, val);
> + return (ret < 0) ? ret : IIO_VAL_INT;
> + case IIO_INTENSITY:
> + ret = isl76682_get(chip, true, val);
> + return (ret < 0) ? ret : IIO_VAL_INT;
> + default:
> + break;
return -EINVAL;
> + }
> +
> + return -EINVAL;
> + case IIO_CHAN_INFO_SCALE:
> + for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) {
> + if (chip->range != isl76682_range_table[i].range)
> + continue;
> +
> + *val = 0;
...
> + if (chan->type == IIO_LIGHT)
> + *val2 = isl76682_range_table[i].als;
> + else if (chan->type == IIO_INTENSITY)
> + *val2 = isl76682_range_table[i].ir;
> + else
> + return -EINVAL;
Why not switch-case for consistency's sake?
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + *val2 = ISL76682_INT_TIME_US;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
Move it to default.
> +}
...
> +static int illuminance_scale_available[] = {
> + 0, 15000,
> + 0, 60000,
> + 0, 240000,
> + 0, 960000
Leave trailing comma as it's not a terminator and will be an additional burden
if this list is going to be expanded in the future.
> +};
> +
> +static int intensity_scale_available[] = {
> + 0, 10500,
> + 0, 42000,
> + 0, 168000,
> + 0, 673000
Ditto.
> +};
...
> +static int isl76682_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type,
> + int *length, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->type == IIO_LIGHT) {
> + *vals = illuminance_scale_available;
> + *length = ARRAY_SIZE(illuminance_scale_available);
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_LIST;
> + } else if (chan->type == IIO_INTENSITY) {
> + *vals = intensity_scale_available;
> + *length = ARRAY_SIZE(intensity_scale_available);
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_LIST;
> + }
> + return -EINVAL;
Same Q: why not switch-case?
> + case IIO_CHAN_INFO_INT_TIME:
> + *vals = integration_time_available;
> + *length = ARRAY_SIZE(integration_time_available);
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_LIST;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
Move it to default.
> +}
...
> +static int isl76682_clear_configure_reg(struct isl76682_chip *chip)
> +{
> + struct device *dev = regmap_get_device(chip->regmap);
> + int ret;
> +
> + ret = regmap_write(chip->regmap, ISL76682_REG_COMMAND, 0x0);
> + if (ret < 0)
> + dev_err(dev, "Error %d clearing the CONFIGURE register\n", ret);
> + chip->command = 0;
Even in an error case? Is it a problem?
> + return ret;
> +}
...
> +static void isl76682_reset_action(void *data)
> +{
> + isl76682_clear_configure_reg((struct isl76682_chip *)data);
Redundant casting, just name the field as you line, seems "chip" is what being
used elsewhere.
> +}
...
> + i2c_set_clientdata(client, indio_dev);
How is this being used?
...
> + chip->regmap = devm_regmap_init_i2c(client, &isl76682_regmap_config);
> + if (IS_ERR(chip->regmap)) {
> + return dev_err_probe(dev, PTR_ERR(chip->regmap),
> + "Error initializing regmap\n");
> + }
Redundant {}
Also can be rewritten as
chip->regmap = devm_regmap_init_i2c(client, &isl76682_regmap_config);
ret = PTR_ERR_OR_ZERO(chip->regmap);
if (ret)
return dev_err_probe(dev, ret, "Error initializing regmap\n");
...
> +static const struct i2c_device_id isl76682_id[] = {
> + {"isl76682", 0},
Unneeded 0.
> + {}
> +};
...
> +static const struct of_device_id isl76682_of_match[] = {
> + { .compatible = "isil,isl76682", },
> + { },
Remove comma from the terminator.
> +};
...
> +
Redundant blank line.
> +module_i2c_driver(isl76682_driver);
--
With Best Regards,
Andy Shevchenko
