Re: [PATCH v5 2/2] iio: light: ROHM BH1745 colour sensor

[Javier Carrasco]

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