On 24/06/2024 23:55, Mudit Sharma wrote:
> Add support for BH1745, which is an I2C colour sensor with red, green,
> blue and clear channels. It has a programmable active low interrupt
> pin. Interrupt occurs when the signal from the selected interrupt
> source channel crosses set interrupt threshold high or low level.
>
> Interrupt source for the device can be configured by enabling the
> corresponding event and interrupt latch is always enabled when setting
> up interrupt.
>
> Add myself as the maintainer for this driver in MAINTAINERS.
>
> Signed-off-by: Mudit Sharma <muditsharma.info@xxxxxxxxx>
> Reviewed-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx>
> Reviewed-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
> ---
Hi Mudit, some minor comments inline form my side.
...
> +
> +struct bh1745_data {
> + /*
> + * Lock to prevent device setting update or read before related
typo: calculations. I would recommend you to use checkpatch.pl
--codespell to cach such typos.
> + * caluculations or event push are completed
> + */
> + struct mutex lock;
> + struct regmap *regmap;
> + struct i2c_client *client;
> + struct iio_trigger *trig;
> + struct iio_gts gts;
> + u8 int_src;
> +};
> +
...
> +static int bh1745_int_time_to_us(int val, int val2)
> +{
> + for (u8 i = 0; i < ARRAY_SIZE(bh1745_int_time); i++) {
> + if (val == bh1745_int_time[i][0] && val2 == bh1745_int_time[i][1])
> + return bh1745_int_time_us[i];
Nit: unnecessary blank line before a close brace.
> +
> + }
> +
> + return -EINVAL;
> +}
...
> +static int bh1745_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bh1745_data *data = iio_priv(indio_dev);
> + int ret;
> + int value;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
Missing scope indentation.
> + ret = regmap_bulk_read(data->regmap, chan->address, &value, 2);
> + if (ret)
> + return ret;
> + *val = value;
> +
> + return IIO_VAL_INT;
> + }
> + unreachable();
> + }
> +
...
> +static int bh1745_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> + int ret;
Why is value initialized here? If regmap returns an error, you will not
use value anyway. I caught my eye because it is initialized here, and
not in the other functions where you use the same pattern.
> + int value = 0;
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct bh1745_data *data = iio_priv(indio_dev);
> +
> + guard(mutex)(&data->lock);
> + if (state) {
> + ret = regmap_read(data->regmap, BH1745_INTR, &value);
> + if (ret)
> + return ret;
> + // Latch is always set when enabling interrupt
> + value |= BH1745_INT_ENABLE |
> + FIELD_PREP(BH1745_INT_SIGNAL_LATCHED, 1) |
> + FIELD_PREP(BH1745_INT_SOURCE_MASK, data->int_src);
> + return regmap_write(data->regmap, BH1745_INTR, value);
> + }
> +
> + return regmap_write(data->regmap, BH1745_INTR, value);
> +}
Best regards,
Javier Carrasco
