Re: [PATCH v4 2/2] iio: light: isl76682: Add ISL76682 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 21, 2023 at 04:10:40AM +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.

...

> +	for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) {
> +		if (chan->type == IIO_LIGHT) {
> +			if (val2 != isl76682_range_table[i].als)
> +				continue;
> +		} else if (chan->type == IIO_INTENSITY) {
> +			if (val2 != isl76682_range_table[i].ir)
> +				continue;
> +		}

Redundant 'else' and you can combine if:s.

		if (chan->type == IIO_LIGHT && val2 != isl76682_range_table[i].als)
			continue;
		if (chan->type == IIO_INTENSITY && val2 != isl76682_range_table[i].ir)
			continue;

But it's up to you and Jonathan.

> +		scoped_guard(mutex, &chip->lock)
> +			chip->range = isl76682_range_table[i].range;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}

...

> +	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;

		default:
			return -EINVAL;

...

> +static const struct of_device_id isl76682_of_match[] = {
> +	{ .compatible = "isil,isl76682", },

Inner comma is not needed.

> +	{ }
> +};

...

> +
> +module_i2c_driver(isl76682_driver);
> +MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver");

...other way around:

};
module_i2c_driver(isl76682_driver);

MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver");

...

Assuming you address all these,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>


-- 
With Best Regards,
Andy Shevchenko






[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux