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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 18 Jul 2024 23:02:06 +0100
Mudit Sharma <muditsharma.info@gmail.com> 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. 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@gmail.com>
> Reviewed-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Hi Mudit.

Rather than go around again, I've applied a few things I noticed
and changes Javier suggests whilst picking this up.

I also tweaked a few long lines.

Anyhow, applied to the testing branch of iio.git which will be rebased
on rc1 once available and pushed out as togreg for linux-next to
pick up.

Thanks,

Jonathan


> +
> +static irqreturn_t bh1745_trigger_handler(int interrupt, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	struct {
> +		u16 chans[4];
> +		s64 timestamp __aligned(8);
> +	} scan;
> +	u16 value;
> +	int ret;
> +	int i;
> +	int j = 0;
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {

Nuno's new helper iio_for_each_active_channel()
cleans this up. I'll switch to that whilst applying rather than adding another driver
for him to convert over.


> +		ret = regmap_bulk_read(data->regmap, BH1745_RED_LSB + 2 * i, &value, 2);
> +		if (ret)
> +			goto err;
> +
> +		scan.chans[j++] = value;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
> +
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

> +
> +static int bh1745_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	int value;
> +	int part_id;
> +	struct bh1745_data *data;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &client->dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, indio_dev);
Trivial, but is this ever used?  I couldn't figure out where it if is.
So I've dropped it. Shout if it needs to be here.

Jonathan