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