| Message ID | 20240625220328.558809-2-muditsharma.info@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v6,1/2] dt-bindings: iio: light: ROHM BH1745 | expand |
On Tue, 25 Jun 2024 23:03:26 +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, I'd failed on previous reviews to notice the odd trigger in here. What is it, because it doesn't seem to be a dataready trigger as the device doesn't seem to provide such an interrupt? Various other comments inline. Jonathan > --- > v5->v6: > - Fix typo > - Fix Indentation > - Drop read in bh1745_set_trigger_state() as configuring all the value > v4->v5: > - Provide scale instead of HW gain > - Use GTS helpers > - Code style fixes > - Add check for part ID during probe > - Always enable latch when enabling interrupt > - Use devm_add_action_or_reset() and drop bh1745_remove() function > - Drop custom DEVICE_ATTR and provide related read_avail and setup > interrupt source with event_config > - Make buffer support independent of IRQ > - Add power regulator handing with devm_regulator_get_enable() > - Drop read and write wrappers and use regmap functions directly > - Add MODULE_DEVICE_TABLE for of_device_id > v3->v4: > - Fix formatting: > - Remove redundant new line > - Use '0x' rather than '0X' > v2->v3: > - Squash commit for addition to MAINTAINERS > - Fix code style for consistency: > - New line before last 'return' > - Use variable name 'value' instead of 'val' in > 'bh1745_set_trigger_state()' > - Align function parameters to match parenthesis > - Avoid use of magic number > - Use named enum instead of anonymous enum > - Use 'guard(mutex)(&data->lock)' instead of 'mutex_lock()' > 'mutex_unlock()' > - Only initialize 'ret' and 'value' when necessary > - Fix and optimize logic for `in_interrupt_latch_store()` > - Fix error handling in irq , trigger handlers and dev attribute for > latch > v1->v2: > - No changes > > diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c > new file mode 100644 > index 000000000000..8412d5da3019 > --- /dev/null > +++ b/drivers/iio/light/bh1745.c > @@ -0,0 +1,931 @@ > +/* BH1745 config regs */ > +#define BH1745_SYS_CTRL 0x40 > + > +#define BH1745_MODE_CTRL_1 0x41 > +#define BH1745_MODE_CTRL_2 0x42 > +#define BH1745_MODE_CTRL_3 0x44 > + > +#define BH1745_INTR 0x60 > +#define BH1745_INTR_STATUS BIT(7) > + > +#define BH1745_PERSISTENCE 0x61 > + > +#define BH1745_TH_LSB 0x62 > +#define BH1745_TH_MSB 0x63 > + > +#define BH1745_TL_LSB 0x64 > +#define BH1745_TL_MSB 0x65 > + > +#define BH1745_MANU_ID 0x92 > + > +/* BH1745 output regs */ > +#define BH1745_RED_LSB 0x50 > +#define BH1745_RED_MSB 0x51 > +#define BH1745_GREEN_LSB 0x52 > +#define BH1745_GREEN_MSB 0x53 > +#define BH1745_BLUE_LSB 0x54 > +#define BH1745_BLUE_MSB 0x55 > +#define BH1745_CLEAR_LSB 0x56 > +#define BH1745_CLEAR_MSB 0x57 > + > +#define BH1745_SW_RESET BIT(7) > +#define BH1745_INT_RESET BIT(6) > + > +#define BH1745_MEASUREMENT_TIME_MASK GENMASK(2, 0) These files should all be clearly associated with the register they are in. The current naming scheme doesn't make that connection clear. I have no idea from these names for which register this will be ine. > + > +#define BH1745_RGBC_EN BIT(4) > + > +#define BH1745_ADC_GAIN_MASK GENMASK(1, 0) > + > +#define BH1745_INT_ENABLE BIT(0) > +#define BH1745_INT_SIGNAL_ACTIVE BIT(7) Same as INTR_STATUS above I think. > + > +#define BH1745_INT_SIGNAL_LATCHED BIT(4) > + > +#define BH1745_INT_SOURCE_MASK GENMASK(3, 2) These are related to the INTR register defined above Move them up there adn rename them to match that naming scheme. > + > +#define BH1745_PART_ID 0x0B > +#define BH1745_PART_ID_MASK GENMASK(5, 0) > + > +// From 16x max HW gain and 32x max integration time /* */ > +#define BH1745_MAX_GAIN 512 > + > +struct bh1745_data { > + /* > + * Lock to prevent device setting update or read before related > + * calculations 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; I'd just read this back from the device (even though the regcache won't help as it's a volatile register). > +}; > + > +static int bh1745_reset(struct bh1745_data *data) > +{ > + int ret; > + int value; > + > + ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &value); > + if (ret) > + return ret; > + > + value |= (BH1745_SW_RESET | BH1745_INT_RESET); regmap_set_bits() check for other cases like this. Regmap has a rich set of read modify write utility functions. > + > + return regmap_write(data->regmap, BH1745_SYS_CTRL, value); > +} > + > +static int bh1745_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct bh1745_data *data = iio_priv(indio_dev); > + > + guard(mutex)(&data->lock); > + > + return data->int_src; As below. I would just read it back from the device to avoid storing the state and anyone having to check that it is in sync when reviewing the driver. > +} > + > +static int bh1745_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, int state) > +{ > + struct bh1745_data *data = iio_priv(indio_dev); > + int ret; > + > + /* > + * We do not update the interrupt source if 'state' value is '0' as > + * there isn't a provision to not have an interrupt source at all. > + */ There is an INT ENABLE field in the interrupt register on the datasheet google found for me. So why not use that? > + if (state == 1) { > + guard(mutex)(&data->lock); > + switch (chan->channel2) { > + case IIO_MOD_LIGHT_RED: > + ret = regmap_write_bits(data->regmap, BH1745_INTR, > + BH1745_INT_SOURCE_MASK, > + BH1745_INT_SOURCE_RED); > + if (ret) > + return ret; > + > + data->int_src = BH1745_INT_SOURCE_RED; Why cache it. If anyone cares can you not read it back from the device? That way you don't need to keep driver and device in sync by hand. > + break; return 0; No need to make the reader scroll down to find out nothing else happens. Same in other cases. > + > + case IIO_MOD_LIGHT_GREEN: > + ret = regmap_write_bits(data->regmap, BH1745_INTR, > + BH1745_INT_SOURCE_MASK, > + BH1745_INT_SOURCE_GREEN); > + if (ret) > + return ret; > + > + data->int_src = BH1745_INT_SOURCE_GREEN; > + break; > + > + case IIO_MOD_LIGHT_BLUE: > + ret = regmap_write_bits(data->regmap, BH1745_INTR, > + BH1745_INT_SOURCE_MASK, > + BH1745_INT_SOURCE_BLUE); > + if (ret) > + return ret; > + > + data->int_src = BH1745_INT_SOURCE_BLUE; > + break; > + > + case IIO_MOD_LIGHT_CLEAR: > + ret = regmap_write_bits(data->regmap, BH1745_INTR, > + BH1745_INT_SOURCE_MASK, > + BH1745_INT_SOURCE_CLEAR); > + if (ret) > + return ret; > + > + data->int_src = BH1745_INT_SOURCE_CLEAR; > + break; > + > + default: > + return -EINVAL; > + } > + } > + > + return 0; > +} > +static int bh1745_set_trigger_state(struct iio_trigger *trig, bool state) > +{ > + int value = 0; > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > + struct bh1745_data *data = iio_priv(indio_dev); > + > + if (state) { > + guard(mutex)(&data->lock); > + // Latch is always set when enabling interrupt /* */ syntax for all comments in IIO other than the corner case of spdx markings that sometimes need to c++ style > + 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); > +} > + > +static const struct iio_trigger_ops bh1745_trigger_ops = { > + .set_trigger_state = bh1745_set_trigger_state, > +}; > + > +static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p) > +{ > + struct iio_dev *indio_dev = p; > + struct bh1745_data *data = iio_priv(indio_dev); > + int ret; > + int value; > + > + ret = regmap_read(data->regmap, BH1745_INTR, &value); > + if (ret) > + return IRQ_NONE; > + > + if (value & BH1745_INTR_STATUS) { > + guard(mutex)(&data->lock); > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, data->int_src, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_EITHER), > + iio_get_time_ns(indio_dev)); What is happening here. You always push out the event and use that as a trigger? This is an unusual trigger if it's appropriate to use it for one at all. You've called it a dataready trigger but it is not obvious that this device provides any such signal. > + > + iio_trigger_poll_nested(data->trig); > + > + return IRQ_HANDLED; > + } > + > + return IRQ_NONE; > +} > + > + > +static int bh1745_setup_trigger(struct iio_dev *indio_dev, struct device *parent) > +{ > + struct bh1745_data *data = iio_priv(indio_dev); > + struct device *dev = &data->client->dev; > + int ret; > + > + data->trig = devm_iio_trigger_alloc(parent, "%sdata-rdy-dev%d", > + indio_dev->name, > + iio_device_id(indio_dev)); > + if (!data->trig) > + return -ENOMEM; > + > + data->trig->ops = &bh1745_trigger_ops; > + iio_trigger_set_drvdata(data->trig, indio_dev); > + > + ret = devm_iio_trigger_register(dev, data->trig); > + if (ret) > + return dev_err_probe(dev, ret, "Trigger registration failed\n"); > + > + ret = devm_iio_triggered_buffer_setup(parent, indio_dev, NULL, > + bh1745_trigger_handler, NULL); > + if (ret) > + return dev_err_probe(dev, ret, "Triggered buffer setup failed\n"); > + > + if (data->client->irq) { If you don't have an irq, how does the trigger registered above work? That registration should be under this check as well so if you have no irq, you don't register the trigger. As it stands you have a trigger that will appear to work but never fire. > + ret = devm_request_threaded_irq(dev, data->client->irq, NULL, > + bh1745_interrupt_handler, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "bh1745_interrupt", indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "Request for IRQ failed\n"); > + } > + > + return 0; > +} > + > +static int bh1745_init(struct bh1745_data *data) > +{ > + int ret; > + struct device *dev = &data->client->dev; > + > + mutex_init(&data->lock); > + data->int_src = BH1745_INT_SOURCE_RED; > + > + ret = devm_iio_init_iio_gts(dev, BH1745_MAX_GAIN, 0, bh1745_gain, > + ARRAY_SIZE(bh1745_gain), bh1745_itimes, > + ARRAY_SIZE(bh1745_itimes), &data->gts); > + if (ret) > + return ret; > + > + ret = bh1745_reset(data); > + if (ret) { > + dev_err_probe(dev, ret, "Failed to reset sensor\n"); > + return ret; return dev_err_probe() and no {} as it will be oen line. > + } > + > + ret = bh1745_power_on(data); > + if (ret) { > + dev_err_probe(dev, ret, "Failed to turn on sensor\n"); > + return ret; return dev_err_probe() > + } > + > + return 0; > +} > + > +static int bh1745_probe(struct i2c_client *client) > +{ > + int ret; > + 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; > + > + ret = devm_regulator_get_enable(dev, "vdd"); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to get and enable regulator\n"); > + > + data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); I don't think you ever use this. If not no need to set it. > + data->client = client; > + indio_dev->info = &bh1745_info; > + indio_dev->name = "bh1745"; > + indio_dev->channels = bh1745_channels; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->num_channels = ARRAY_SIZE(bh1745_channels); > + > + data->regmap = devm_regmap_init_i2c(client, &bh1745_regmap); > + if (IS_ERR(data->regmap)) > + return dev_err_probe(dev, PTR_ERR(data->regmap), > + "Failed to initialize Regmap\n"); > + > + ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &part_id); > + if (ret) > + return ret; > + > + part_id = FIELD_GET(BH1745_PART_ID_MASK, part_id); > + if (part_id != BH1745_PART_ID) > + dev_warn(dev, "Unknown part ID 0x%x\n", part_id); > + > + ret = bh1745_init(data); > + if (ret) > + return ret; > + > + ret = devm_add_action_or_reset(dev, bh1745_power_off, data); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > + > + ret = bh1745_setup_trigger(indio_dev, indio_dev->dev.parent); > + if (ret < 0) > + return ret; > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0) { > + dev_err(dev, "Failed to register device\n"); > + return ret; return dev_err_probe(). > + } > + > + return 0; > +}
On 28/06/2024 20:37, Jonathan Cameron wrote: > Hi Mudit, > > I'd failed on previous reviews to notice the odd trigger in here. > What is it, because it doesn't seem to be a dataready trigger as the device > doesn't seem to provide such an interrupt? Hi Jonathan, Thank you for your review on this. I've incorrect called it as a dataready trigger, I missed this as part of my initial cleanup - apologies for the confusion caused by this. I should potentially call it 'threshold' or 'dev'. Please suggest what you think would be appropriate here. The sensor has an active low interrupt pin which is connected to a GPIO (input, pullup). When the sensor reading crosses value set in threshold high or threshold low resisters, interrupt signal is generated and the interrupt gets handled in 'bh1745_interrupt_handler()' (interrupt also depends on number of consecutive judgements set in BH1745_PERSISTENCE register) > > Various other comments inline. Will address all for v7 > ... >> +static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p) >> +{ >> + struct iio_dev *indio_dev = p; >> + struct bh1745_data *data = iio_priv(indio_dev); >> + int ret; >> + int value; >> + >> + ret = regmap_read(data->regmap, BH1745_INTR, &value); >> + if (ret) >> + return IRQ_NONE; >> + >> + if (value & BH1745_INTR_STATUS) { >> + guard(mutex)(&data->lock); >> + iio_push_event(indio_dev, >> + IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, data->int_src, >> + IIO_EV_TYPE_THRESH, >> + IIO_EV_DIR_EITHER), >> + iio_get_time_ns(indio_dev)); > > What is happening here. You always push out the event and use that as > a trigger? This is an unusual trigger if it's appropriate to use it for > one at all. You've called it a dataready trigger but it is not obvious > that this device provides any such signal. When an interrupt occurs, BH1745_INTR_STATUS bit is set in the BH1745_INTR register. Event is only pushed out when the BH1745_INTR_STATUS bit is set. >> + >> + iio_trigger_poll_nested(data->trig); >> + >> + return IRQ_HANDLED; >> + } >> + >> + return IRQ_NONE; >> +} Best regards, Mudit Sharma
On Sat, 29 Jun 2024 10:10:19 +0100 Mudit Sharma <muditsharma.info@gmail.com> wrote: > On 28/06/2024 20:37, Jonathan Cameron wrote: > > Hi Mudit, > > > > I'd failed on previous reviews to notice the odd trigger in here. > > What is it, because it doesn't seem to be a dataready trigger as the device > > doesn't seem to provide such an interrupt? > > Hi Jonathan, > > Thank you for your review on this. > > I've incorrect called it as a dataready trigger, I missed this as part > of my initial cleanup - apologies for the confusion caused by this. I > should potentially call it 'threshold' or 'dev'. Please suggest what you > think would be appropriate here. > > The sensor has an active low interrupt pin which is connected to a GPIO > (input, pullup). When the sensor reading crosses value set in threshold > high or threshold low resisters, interrupt signal is generated and the > interrupt gets handled in 'bh1745_interrupt_handler()' (interrupt also > depends on number of consecutive judgements set in BH1745_PERSISTENCE > register) This isn't really a trigger. Just report the event and don't register a trigger at all. In theory we could have a trigger with these properties (and we did discuss many years ago how to do this in a generic fashion) but today it isn't something any standard userspace will understand how to use. > > > > > Various other comments inline. > > Will address all for v7 > > > ... > >> +static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p) > >> +{ > >> + struct iio_dev *indio_dev = p; > >> + struct bh1745_data *data = iio_priv(indio_dev); > >> + int ret; > >> + int value; > >> + > >> + ret = regmap_read(data->regmap, BH1745_INTR, &value); > >> + if (ret) > >> + return IRQ_NONE; > >> + > >> + if (value & BH1745_INTR_STATUS) { > >> + guard(mutex)(&data->lock); > >> + iio_push_event(indio_dev, > >> + IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, data->int_src, > >> + IIO_EV_TYPE_THRESH, > >> + IIO_EV_DIR_EITHER), > >> + iio_get_time_ns(indio_dev)); > > > > What is happening here. You always push out the event and use that as > > a trigger? This is an unusual trigger if it's appropriate to use it for > > one at all. You've called it a dataready trigger but it is not obvious > > that this device provides any such signal. > > When an interrupt occurs, BH1745_INTR_STATUS bit is set in the > BH1745_INTR register. Event is only pushed out when the > BH1745_INTR_STATUS bit is set. That bit is fine. My confusion is more about the trigger. I think the short answer is drop the next line and indeed all the code registering a trigger as this device doesn't provide appropriate hardware. > >> + > >> + iio_trigger_poll_nested(data->trig); > >> + > >> + return IRQ_HANDLED; > >> + } > >> + > >> + return IRQ_NONE; > >> +} > > Best regards, > Mudit Sharma >
On 6/26/24 01:03, 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. 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> > --- > v5->v6: > - Fix typo > - Fix Indentation > - Drop read in bh1745_set_trigger_state() as configuring all the value > v4->v5: > - Provide scale instead of HW gain > - Use GTS helpers > - Code style fixes > - Add check for part ID during probe > - Always enable latch when enabling interrupt > - Use devm_add_action_or_reset() and drop bh1745_remove() function > - Drop custom DEVICE_ATTR and provide related read_avail and setup > interrupt source with event_config > - Make buffer support independent of IRQ > - Add power regulator handing with devm_regulator_get_enable() > - Drop read and write wrappers and use regmap functions directly > - Add MODULE_DEVICE_TABLE for of_device_id > v3->v4: > - Fix formatting: > - Remove redundant new line > - Use '0x' rather than '0X' > v2->v3: > - Squash commit for addition to MAINTAINERS > - Fix code style for consistency: > - New line before last 'return' > - Use variable name 'value' instead of 'val' in > 'bh1745_set_trigger_state()' > - Align function parameters to match parenthesis > - Avoid use of magic number > - Use named enum instead of anonymous enum > - Use 'guard(mutex)(&data->lock)' instead of 'mutex_lock()' > 'mutex_unlock()' > - Only initialize 'ret' and 'value' when necessary > - Fix and optimize logic for `in_interrupt_latch_store()` > - Fix error handling in irq , trigger handlers and dev attribute for > latch > v1->v2: > - No changes > > MAINTAINERS | 6 + > drivers/iio/light/Kconfig | 13 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/bh1745.c | 931 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 951 insertions(+) > create mode 100644 drivers/iio/light/bh1745.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2ca8f35dfe03..e9ff6f465e7f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19404,6 +19404,12 @@ S: Supported > F: drivers/power/supply/bd99954-charger.c > F: drivers/power/supply/bd99954-charger.h > > +ROHM BH1745 COLOUR SENSOR > +M: Mudit Sharma <muditsharma.info@gmail.com> > +L: linux-iio@vger.kernel.org > +S: Maintained > +F: drivers/iio/light/bh1745.c > + > ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER > M: Tomasz Duszynski <tduszyns@gmail.com> > S: Maintained > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index 9a587d403118..6cde702fa78d 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -114,6 +114,19 @@ config AS73211 > This driver can also be built as a module. If so, the module > will be called as73211. > > +config BH1745 > + tristate "ROHM BH1745 colour sensor" > + depends on I2C > + select REGMAP_I2C > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + select IIO_GTS_HELPER > + help > + Say Y here to build support for the ROHM bh1745 colour sensor. > + > + To compile this driver as a module, choose M here: the module will > + be called bh1745. > + > config BH1750 > tristate "ROHM BH1750 ambient light sensor" > depends on I2C > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > index a30f906e91ba..939a701a06ac 100644 > --- a/drivers/iio/light/Makefile > +++ b/drivers/iio/light/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_APDS9300) += apds9300.o > obj-$(CONFIG_APDS9306) += apds9306.o > obj-$(CONFIG_APDS9960) += apds9960.o > obj-$(CONFIG_AS73211) += as73211.o > +obj-$(CONFIG_BH1745) += bh1745.o > obj-$(CONFIG_BH1750) += bh1750.o > obj-$(CONFIG_BH1780) += bh1780.o > obj-$(CONFIG_CM32181) += cm32181.o > diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c > new file mode 100644 > index 000000000000..8412d5da3019 > --- /dev/null > +++ b/drivers/iio/light/bh1745.c > @@ -0,0 +1,931 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * ROHM BH1745 digital colour sensor driver > + * > + * Copyright (C) Mudit Sharma <muditsharma.info@gmail.com> > + * > + * 7-bit I2C slave addresses: > + * 0x38 (ADDR pin low) > + * 0x39 (ADDR pin high) > + */ > + > +#include <linux/i2c.h> > +#include <linux/mutex.h> > +#include <linux/util_macros.h> > +#include <linux/iio/events.h> > +#include <linux/regmap.h> > +#include <linux/bits.h> > +#include <linux/bitfield.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/iio/iio-gts-helper.h> > + > +/* BH1745 config regs */ > +#define BH1745_SYS_CTRL 0x40 > + > +#define BH1745_MODE_CTRL_1 0x41 > +#define BH1745_MODE_CTRL_2 0x42 > +#define BH1745_MODE_CTRL_3 0x44 > + > +#define BH1745_INTR 0x60 > +#define BH1745_INTR_STATUS BIT(7) > + > +#define BH1745_PERSISTENCE 0x61 > + > +#define BH1745_TH_LSB 0x62 > +#define BH1745_TH_MSB 0x63 > + > +#define BH1745_TL_LSB 0x64 > +#define BH1745_TL_MSB 0x65 > + > +#define BH1745_MANU_ID 0x92 > + > +/* BH1745 output regs */ > +#define BH1745_RED_LSB 0x50 > +#define BH1745_RED_MSB 0x51 > +#define BH1745_GREEN_LSB 0x52 > +#define BH1745_GREEN_MSB 0x53 > +#define BH1745_BLUE_LSB 0x54 > +#define BH1745_BLUE_MSB 0x55 > +#define BH1745_CLEAR_LSB 0x56 > +#define BH1745_CLEAR_MSB 0x57 > + > +#define BH1745_SW_RESET BIT(7) > +#define BH1745_INT_RESET BIT(6) > + > +#define BH1745_MEASUREMENT_TIME_MASK GENMASK(2, 0) > + > +#define BH1745_RGBC_EN BIT(4) > + > +#define BH1745_ADC_GAIN_MASK GENMASK(1, 0) > + > +#define BH1745_INT_ENABLE BIT(0) > +#define BH1745_INT_SIGNAL_ACTIVE BIT(7) > + > +#define BH1745_INT_SIGNAL_LATCHED BIT(4) > + > +#define BH1745_INT_SOURCE_MASK GENMASK(3, 2) > + > +#define BH1745_PART_ID 0x0B > +#define BH1745_PART_ID_MASK GENMASK(5, 0) > + > +// From 16x max HW gain and 32x max integration time > +#define BH1745_MAX_GAIN 512 > + > +static const int bh1745_int_time[][2] = { > + { 0, 160000 }, /* 160 ms */ > + { 0, 320000 }, /* 320 ms */ > + { 0, 640000 }, /* 640 ms */ > + { 1, 280000 }, /* 1280 ms */ > + { 2, 560000 }, /* 2560 ms */ > + { 5, 120000 }, /* 5120 ms */ > +}; > + > +static const u8 bh1745_gain_factor[] = { 1, 2, 16 }; > + > +static const int bh1745_int_time_us[] = { 160000, 320000, 640000, > + 1280000, 2560000, 5120000 }; I am not sure why you need these tables above? Can't the iio_gts do all the conversions from register-value to int time/gain and int-time/gain to register value, as well as the checks for supported values? Ideally, you would not need anything else but the bh1745_itimes and the bh1745_gain tables below - they should contain all the same information. > + > +enum bh1745_int_source { > + BH1745_INT_SOURCE_RED, > + BH1745_INT_SOURCE_GREEN, > + BH1745_INT_SOURCE_BLUE, > + BH1745_INT_SOURCE_CLEAR, > +}; > + > +enum bh1745_gain { > + BH1745_ADC_GAIN_1X, > + BH1745_ADC_GAIN_2X, > + BH1745_ADC_GAIN_16X, > +}; > + > +enum bh1745_measurement_time { > + BH1745_MEASUREMENT_TIME_160MS, > + BH1745_MEASUREMENT_TIME_320MS, > + BH1745_MEASUREMENT_TIME_640MS, > + BH1745_MEASUREMENT_TIME_1280MS, > + BH1745_MEASUREMENT_TIME_2560MS, > + BH1745_MEASUREMENT_TIME_5120MS, > +}; > + > +enum bh1745_presistence_value { > + BH1745_PRESISTENCE_UPDATE_TOGGLE, > + BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT, > + BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT, > + BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT, > +}; > + > +static const struct iio_gain_sel_pair bh1745_gain[] = { > + GAIN_SCALE_GAIN(1, BH1745_ADC_GAIN_1X), > + GAIN_SCALE_GAIN(2, BH1745_ADC_GAIN_2X), > + GAIN_SCALE_GAIN(16, BH1745_ADC_GAIN_16X), > +}; > + > +static const struct iio_itime_sel_mul bh1745_itimes[] = { > + GAIN_SCALE_ITIME_US(5120000, BH1745_MEASUREMENT_TIME_5120MS, 32), > + GAIN_SCALE_ITIME_US(2560000, BH1745_MEASUREMENT_TIME_2560MS, 16), > + GAIN_SCALE_ITIME_US(1280000, BH1745_MEASUREMENT_TIME_1280MS, 8), > + GAIN_SCALE_ITIME_US(640000, BH1745_MEASUREMENT_TIME_640MS, 4), > + GAIN_SCALE_ITIME_US(320000, BH1745_MEASUREMENT_TIME_320MS, 2), > + GAIN_SCALE_ITIME_US(160000, BH1745_MEASUREMENT_TIME_160MS, 1), > +}; > + > +struct bh1745_data { > + /* > + * Lock to prevent device setting update or read before related > + * calculations 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 const struct regmap_range bh1745_volatile_ranges[] = { > + regmap_reg_range(BH1745_MODE_CTRL_2, BH1745_MODE_CTRL_2), /* VALID */ > + regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB), /* Data */ > + regmap_reg_range(BH1745_INTR, BH1745_INTR), /* Interrupt */ > +}; > + > +static const struct regmap_access_table bh1745_volatile_regs = { > + .yes_ranges = bh1745_volatile_ranges, > + .n_yes_ranges = ARRAY_SIZE(bh1745_volatile_ranges), > +}; > + > +static const struct regmap_range bh1745_read_ranges[] = { > + regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2), > + regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB), > + regmap_reg_range(BH1745_INTR, BH1745_INTR), > + regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB), > + regmap_reg_range(BH1745_MANU_ID, BH1745_MANU_ID), > +}; > + > +static const struct regmap_access_table bh1745_ro_regs = { > + .yes_ranges = bh1745_read_ranges, > + .n_yes_ranges = ARRAY_SIZE(bh1745_read_ranges), > +}; > + > +static const struct regmap_range bh1745_writable_ranges[] = { > + regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2), > + regmap_reg_range(BH1745_INTR, BH1745_INTR), > + regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB), > +}; > + > +static const struct regmap_access_table bh1745_wr_regs = { > + .yes_ranges = bh1745_writable_ranges, > + .n_yes_ranges = ARRAY_SIZE(bh1745_writable_ranges), > +}; > + > +static const struct regmap_config bh1745_regmap = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = BH1745_MANU_ID, > + .cache_type = REGCACHE_RBTREE, > + .volatile_table = &bh1745_volatile_regs, > + .wr_table = &bh1745_wr_regs, > + .rd_table = &bh1745_ro_regs, I am not 100% sure what this does. (Let's say it is just my ignorance :)). Does the 'ro' in 'bh1745_ro_regs' stand for read-only? If so, shouldn't the read-inly registers be marked as "not writable", which would be adding them in .wr_table in 'no_ranges'? Also, what is the idea of the 'wr_regs'? > +}; > + > +static const struct iio_event_spec bh1745_event_spec[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE), > + }, > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE), > + }, > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_EITHER, > + .mask_shared_by_type = BIT(IIO_EV_INFO_PERIOD), > + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > + }, > +}; > + > +#define BH1745_CHANNEL(_colour, _si, _addr) \ > + { \ > + .type = IIO_INTENSITY, .modified = 1, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_INT_TIME), \ > + .info_mask_shared_by_all_available = \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_INT_TIME), \ > + .event_spec = bh1745_event_spec, \ > + .num_event_specs = ARRAY_SIZE(bh1745_event_spec), \ > + .channel2 = IIO_MOD_LIGHT_##_colour, .address = _addr, \ > + .scan_index = _si, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_CPU, \ > + }, \ > + } > + > +static const struct iio_chan_spec bh1745_channels[] = { > + BH1745_CHANNEL(RED, 0, BH1745_RED_LSB), > + BH1745_CHANNEL(GREEN, 1, BH1745_GREEN_LSB), > + BH1745_CHANNEL(BLUE, 2, BH1745_BLUE_LSB), > + BH1745_CHANNEL(CLEAR, 3, BH1745_CLEAR_LSB), > + IIO_CHAN_SOFT_TIMESTAMP(4), > +}; > + > +static int bh1745_reset(struct bh1745_data *data) > +{ > + int ret; > + int value; > + > + ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &value); > + if (ret) > + return ret; > + > + value |= (BH1745_SW_RESET | BH1745_INT_RESET); > + > + return regmap_write(data->regmap, BH1745_SYS_CTRL, value); Would it work if you used regmap_write_bits() instead? ... Sorry, my reviewing time is out :/ I may continue later but no need to wait for my comments if I am not responding. I've too much stuff piling on :( Yours, -- Matti
On 29/06/2024 18:50, Jonathan Cameron wrote: > On Sat, 29 Jun 2024 10:10:19 +0100 > Mudit Sharma <muditsharma.info@gmail.com> wrote: > >> On 28/06/2024 20:37, Jonathan Cameron wrote: >>> Hi Mudit, >>> >>> I'd failed on previous reviews to notice the odd trigger in here. >>> What is it, because it doesn't seem to be a dataready trigger as the device >>> doesn't seem to provide such an interrupt? >> >> Hi Jonathan, >> >> Thank you for your review on this. >> >> I've incorrect called it as a dataready trigger, I missed this as part >> of my initial cleanup - apologies for the confusion caused by this. I >> should potentially call it 'threshold' or 'dev'. Please suggest what you >> think would be appropriate here. >> >> The sensor has an active low interrupt pin which is connected to a GPIO >> (input, pullup). When the sensor reading crosses value set in threshold >> high or threshold low resisters, interrupt signal is generated and the >> interrupt gets handled in 'bh1745_interrupt_handler()' (interrupt also >> depends on number of consecutive judgements set in BH1745_PERSISTENCE >> register) > > This isn't really a trigger. Just report the event and don't register a trigger at > all. > > In theory we could have a trigger with these properties (and we did discuss > many years ago how to do this in a generic fashion) but today it isn't > something any standard userspace will understand how to use. > >> >>> >>> Various other comments inline. >> >> Will address all for v7 >>> >> ... >>>> +static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p) >>>> +{ >>>> + struct iio_dev *indio_dev = p; >>>> + struct bh1745_data *data = iio_priv(indio_dev); >>>> + int ret; >>>> + int value; >>>> + >>>> + ret = regmap_read(data->regmap, BH1745_INTR, &value); >>>> + if (ret) >>>> + return IRQ_NONE; >>>> + >>>> + if (value & BH1745_INTR_STATUS) { >>>> + guard(mutex)(&data->lock); >>>> + iio_push_event(indio_dev, >>>> + IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, data->int_src, >>>> + IIO_EV_TYPE_THRESH, >>>> + IIO_EV_DIR_EITHER), >>>> + iio_get_time_ns(indio_dev)); >>> >>> What is happening here. You always push out the event and use that as >>> a trigger? This is an unusual trigger if it's appropriate to use it for >>> one at all. You've called it a dataready trigger but it is not obvious >>> that this device provides any such signal. >> >> When an interrupt occurs, BH1745_INTR_STATUS bit is set in the >> BH1745_INTR register. Event is only pushed out when the >> BH1745_INTR_STATUS bit is set. > That bit is fine. My confusion is more about the trigger. I think > the short answer is drop the next line and indeed all the code registering > a trigger as this device doesn't provide appropriate hardware. > Noted - thank you for your guidance. Will include the changes for v7. Best regards, Mudit Sharma
On 01/07/2024 12:32, Matti Vaittinen wrote: >> + >> +// From 16x max HW gain and 32x max integration time >> +#define BH1745_MAX_GAIN 512 >> + >> +static const int bh1745_int_time[][2] = { >> + { 0, 160000 }, /* 160 ms */ >> + { 0, 320000 }, /* 320 ms */ >> + { 0, 640000 }, /* 640 ms */ >> + { 1, 280000 }, /* 1280 ms */ >> + { 2, 560000 }, /* 2560 ms */ >> + { 5, 120000 }, /* 5120 ms */ >> +}; >> + >> +static const u8 bh1745_gain_factor[] = { 1, 2, 16 }; >> + >> +static const int bh1745_int_time_us[] = { 160000, 320000, 640000, >> + 1280000, 2560000, 5120000 }; > > I am not sure why you need these tables above? Can't the iio_gts do all > the conversions from register-value to int time/gain and int-time/gain > to register value, as well as the checks for supported values? Ideally, > you would not need anything else but the bh1745_itimes and the > bh1745_gain tables below - they should contain all the same information. > Hi Matti, Thank you for reviewing this. Just had a look again at GTS helpers and found the appropriate functions. Will drop these for v7. >> +}; >> + >> +static const struct regmap_range bh1745_volatile_ranges[] = { >> + regmap_reg_range(BH1745_MODE_CTRL_2, BH1745_MODE_CTRL_2), /* >> VALID */ >> + regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB), /* Data */ >> + regmap_reg_range(BH1745_INTR, BH1745_INTR), /* Interrupt */ >> +}; >> + >> +static const struct regmap_access_table bh1745_volatile_regs = { >> + .yes_ranges = bh1745_volatile_ranges, >> + .n_yes_ranges = ARRAY_SIZE(bh1745_volatile_ranges), >> +}; >> + >> +static const struct regmap_range bh1745_read_ranges[] = { >> + regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2), >> + regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB), >> + regmap_reg_range(BH1745_INTR, BH1745_INTR), >> + regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB), >> + regmap_reg_range(BH1745_MANU_ID, BH1745_MANU_ID), >> +}; >> + >> +static const struct regmap_access_table bh1745_ro_regs = { >> + .yes_ranges = bh1745_read_ranges, >> + .n_yes_ranges = ARRAY_SIZE(bh1745_read_ranges), >> +}; >> + >> +static const struct regmap_range bh1745_writable_ranges[] = { >> + regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2), >> + regmap_reg_range(BH1745_INTR, BH1745_INTR), >> + regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB), >> +}; >> + >> +static const struct regmap_access_table bh1745_wr_regs = { >> + .yes_ranges = bh1745_writable_ranges, >> + .n_yes_ranges = ARRAY_SIZE(bh1745_writable_ranges), >> +}; >> + >> +static const struct regmap_config bh1745_regmap = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + .max_register = BH1745_MANU_ID, >> + .cache_type = REGCACHE_RBTREE, >> + .volatile_table = &bh1745_volatile_regs, >> + .wr_table = &bh1745_wr_regs, >> + .rd_table = &bh1745_ro_regs, > > I am not 100% sure what this does. (Let's say it is just my ignorance > :)). Does the 'ro' in 'bh1745_ro_regs' stand for read-only? > > If so, shouldn't the read-inly registers be marked as "not writable", > which would be adding them in .wr_table in 'no_ranges'? Also, what is > the idea of the 'wr_regs'? > There are some read-only (Manufacturer ID and Data registers) and some readable and writable registers. I will rename these to 'bh1745_readable_regs' and 'bh1745_writable_regs' in v7 to avoid confusion. >> +}; >> + >> +static const struct iio_event_spec bh1745_event_spec[] = { >> + { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_RISING, >> + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE), >> + }, >> + { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_FALLING, >> + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE), >> + }, >> + { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_EITHER, >> + .mask_shared_by_type = BIT(IIO_EV_INFO_PERIOD), >> + .mask_separate = BIT(IIO_EV_INFO_ENABLE), >> + }, >> +}; >> + >> +#define BH1745_CHANNEL(_colour, _si, >> _addr) \ >> + { \ >> + .type = IIO_INTENSITY, .modified = 1, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) | \ >> + BIT(IIO_CHAN_INFO_INT_TIME), \ >> + .info_mask_shared_by_all_available = \ >> + BIT(IIO_CHAN_INFO_SCALE) | \ >> + BIT(IIO_CHAN_INFO_INT_TIME), \ >> + .event_spec = bh1745_event_spec, \ >> + .num_event_specs = ARRAY_SIZE(bh1745_event_spec), \ >> + .channel2 = IIO_MOD_LIGHT_##_colour, .address = _addr, \ >> + .scan_index = _si, \ >> + .scan_type = { \ >> + .sign = 'u', \ >> + .realbits = 16, \ >> + .storagebits = 16, \ >> + .endianness = IIO_CPU, \ >> + }, \ >> + } >> + >> +static const struct iio_chan_spec bh1745_channels[] = { >> + BH1745_CHANNEL(RED, 0, BH1745_RED_LSB), >> + BH1745_CHANNEL(GREEN, 1, BH1745_GREEN_LSB), >> + BH1745_CHANNEL(BLUE, 2, BH1745_BLUE_LSB), >> + BH1745_CHANNEL(CLEAR, 3, BH1745_CLEAR_LSB), >> + IIO_CHAN_SOFT_TIMESTAMP(4), >> +}; >> + >> +static int bh1745_reset(struct bh1745_data *data) >> +{ >> + int ret; >> + int value; >> + >> + ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &value); >> + if (ret) >> + return ret; >> + >> + value |= (BH1745_SW_RESET | BH1745_INT_RESET); >> + >> + return regmap_write(data->regmap, BH1745_SYS_CTRL, value); > > Would it work if you used regmap_write_bits() instead? Yes, it should work. Jonathan suggested 'regmap_set_bits()' for this and it looks like a good fit for this as well. I will look through the regmap API once again and update similar cases. > > ... Sorry, my reviewing time is out :/ I may continue later but no need > to wait for my comments if I am not responding. I've too much stuff > piling on :( > > > Yours, > -- Matti > Best regards, Mudit Sharma
diff --git a/MAINTAINERS b/MAINTAINERS index 2ca8f35dfe03..e9ff6f465e7f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19404,6 +19404,12 @@ S: Supported F: drivers/power/supply/bd99954-charger.c F: drivers/power/supply/bd99954-charger.h +ROHM BH1745 COLOUR SENSOR +M: Mudit Sharma <muditsharma.info@gmail.com> +L: linux-iio@vger.kernel.org +S: Maintained +F: drivers/iio/light/bh1745.c + ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER M: Tomasz Duszynski <tduszyns@gmail.com> S: Maintained diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 9a587d403118..6cde702fa78d 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -114,6 +114,19 @@ config AS73211 This driver can also be built as a module. If so, the module will be called as73211. +config BH1745 + tristate "ROHM BH1745 colour sensor" + depends on I2C + select REGMAP_I2C + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + select IIO_GTS_HELPER + help + Say Y here to build support for the ROHM bh1745 colour sensor. + + To compile this driver as a module, choose M here: the module will + be called bh1745. + config BH1750 tristate "ROHM BH1750 ambient light sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index a30f906e91ba..939a701a06ac 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_APDS9300) += apds9300.o obj-$(CONFIG_APDS9306) += apds9306.o obj-$(CONFIG_APDS9960) += apds9960.o obj-$(CONFIG_AS73211) += as73211.o +obj-$(CONFIG_BH1745) += bh1745.o obj-$(CONFIG_BH1750) += bh1750.o obj-$(CONFIG_BH1780) += bh1780.o obj-$(CONFIG_CM32181) += cm32181.o diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c new file mode 100644 index 000000000000..8412d5da3019 --- /dev/null +++ b/drivers/iio/light/bh1745.c @@ -0,0 +1,931 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ROHM BH1745 digital colour sensor driver + * + * Copyright (C) Mudit Sharma <muditsharma.info@gmail.com> + * + * 7-bit I2C slave addresses: + * 0x38 (ADDR pin low) + * 0x39 (ADDR pin high) + */ + +#include <linux/i2c.h> +#include <linux/mutex.h> +#include <linux/util_macros.h> +#include <linux/iio/events.h> +#include <linux/regmap.h> +#include <linux/bits.h> +#include <linux/bitfield.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/iio/iio-gts-helper.h> + +/* BH1745 config regs */ +#define BH1745_SYS_CTRL 0x40 + +#define BH1745_MODE_CTRL_1 0x41 +#define BH1745_MODE_CTRL_2 0x42 +#define BH1745_MODE_CTRL_3 0x44 + +#define BH1745_INTR 0x60 +#define BH1745_INTR_STATUS BIT(7) + +#define BH1745_PERSISTENCE 0x61 + +#define BH1745_TH_LSB 0x62 +#define BH1745_TH_MSB 0x63 + +#define BH1745_TL_LSB 0x64 +#define BH1745_TL_MSB 0x65 + +#define BH1745_MANU_ID 0x92 + +/* BH1745 output regs */ +#define BH1745_RED_LSB 0x50 +#define BH1745_RED_MSB 0x51 +#define BH1745_GREEN_LSB 0x52 +#define BH1745_GREEN_MSB 0x53 +#define BH1745_BLUE_LSB 0x54 +#define BH1745_BLUE_MSB 0x55 +#define BH1745_CLEAR_LSB 0x56 +#define BH1745_CLEAR_MSB 0x57 + +#define BH1745_SW_RESET BIT(7) +#define BH1745_INT_RESET BIT(6) + +#define BH1745_MEASUREMENT_TIME_MASK GENMASK(2, 0) + +#define BH1745_RGBC_EN BIT(4) + +#define BH1745_ADC_GAIN_MASK GENMASK(1, 0) + +#define BH1745_INT_ENABLE BIT(0) +#define BH1745_INT_SIGNAL_ACTIVE BIT(7) + +#define BH1745_INT_SIGNAL_LATCHED BIT(4) + +#define BH1745_INT_SOURCE_MASK GENMASK(3, 2) + +#define BH1745_PART_ID 0x0B +#define BH1745_PART_ID_MASK GENMASK(5, 0) + +// From 16x max HW gain and 32x max integration time +#define BH1745_MAX_GAIN 512 + +static const int bh1745_int_time[][2] = { + { 0, 160000 }, /* 160 ms */ + { 0, 320000 }, /* 320 ms */ + { 0, 640000 }, /* 640 ms */ + { 1, 280000 }, /* 1280 ms */ + { 2, 560000 }, /* 2560 ms */ + { 5, 120000 }, /* 5120 ms */ +}; + +static const u8 bh1745_gain_factor[] = { 1, 2, 16 }; + +static const int bh1745_int_time_us[] = { 160000, 320000, 640000, + 1280000, 2560000, 5120000 }; + +enum bh1745_int_source { + BH1745_INT_SOURCE_RED, + BH1745_INT_SOURCE_GREEN, + BH1745_INT_SOURCE_BLUE, + BH1745_INT_SOURCE_CLEAR, +}; + +enum bh1745_gain { + BH1745_ADC_GAIN_1X, + BH1745_ADC_GAIN_2X, + BH1745_ADC_GAIN_16X, +}; + +enum bh1745_measurement_time { + BH1745_MEASUREMENT_TIME_160MS, + BH1745_MEASUREMENT_TIME_320MS, + BH1745_MEASUREMENT_TIME_640MS, + BH1745_MEASUREMENT_TIME_1280MS, + BH1745_MEASUREMENT_TIME_2560MS, + BH1745_MEASUREMENT_TIME_5120MS, +}; + +enum bh1745_presistence_value { + BH1745_PRESISTENCE_UPDATE_TOGGLE, + BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT, + BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT, + BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT, +}; + +static const struct iio_gain_sel_pair bh1745_gain[] = { + GAIN_SCALE_GAIN(1, BH1745_ADC_GAIN_1X), + GAIN_SCALE_GAIN(2, BH1745_ADC_GAIN_2X), + GAIN_SCALE_GAIN(16, BH1745_ADC_GAIN_16X), +}; + +static const struct iio_itime_sel_mul bh1745_itimes[] = { + GAIN_SCALE_ITIME_US(5120000, BH1745_MEASUREMENT_TIME_5120MS, 32), + GAIN_SCALE_ITIME_US(2560000, BH1745_MEASUREMENT_TIME_2560MS, 16), + GAIN_SCALE_ITIME_US(1280000, BH1745_MEASUREMENT_TIME_1280MS, 8), + GAIN_SCALE_ITIME_US(640000, BH1745_MEASUREMENT_TIME_640MS, 4), + GAIN_SCALE_ITIME_US(320000, BH1745_MEASUREMENT_TIME_320MS, 2), + GAIN_SCALE_ITIME_US(160000, BH1745_MEASUREMENT_TIME_160MS, 1), +}; + +struct bh1745_data { + /* + * Lock to prevent device setting update or read before related + * calculations 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 const struct regmap_range bh1745_volatile_ranges[] = { + regmap_reg_range(BH1745_MODE_CTRL_2, BH1745_MODE_CTRL_2), /* VALID */ + regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB), /* Data */ + regmap_reg_range(BH1745_INTR, BH1745_INTR), /* Interrupt */ +}; + +static const struct regmap_access_table bh1745_volatile_regs = { + .yes_ranges = bh1745_volatile_ranges, + .n_yes_ranges = ARRAY_SIZE(bh1745_volatile_ranges), +}; + +static const struct regmap_range bh1745_read_ranges[] = { + regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2), + regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB), + regmap_reg_range(BH1745_INTR, BH1745_INTR), + regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB), + regmap_reg_range(BH1745_MANU_ID, BH1745_MANU_ID), +}; + +static const struct regmap_access_table bh1745_ro_regs = { + .yes_ranges = bh1745_read_ranges, + .n_yes_ranges = ARRAY_SIZE(bh1745_read_ranges), +}; + +static const struct regmap_range bh1745_writable_ranges[] = { + regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2), + regmap_reg_range(BH1745_INTR, BH1745_INTR), + regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB), +}; + +static const struct regmap_access_table bh1745_wr_regs = { + .yes_ranges = bh1745_writable_ranges, + .n_yes_ranges = ARRAY_SIZE(bh1745_writable_ranges), +}; + +static const struct regmap_config bh1745_regmap = { + .reg_bits = 8, + .val_bits = 8, + .max_register = BH1745_MANU_ID, + .cache_type = REGCACHE_RBTREE, + .volatile_table = &bh1745_volatile_regs, + .wr_table = &bh1745_wr_regs, + .rd_table = &bh1745_ro_regs, +}; + +static const struct iio_event_spec bh1745_event_spec[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER, + .mask_shared_by_type = BIT(IIO_EV_INFO_PERIOD), + .mask_separate = BIT(IIO_EV_INFO_ENABLE), + }, +}; + +#define BH1745_CHANNEL(_colour, _si, _addr) \ + { \ + .type = IIO_INTENSITY, .modified = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_INT_TIME), \ + .info_mask_shared_by_all_available = \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_INT_TIME), \ + .event_spec = bh1745_event_spec, \ + .num_event_specs = ARRAY_SIZE(bh1745_event_spec), \ + .channel2 = IIO_MOD_LIGHT_##_colour, .address = _addr, \ + .scan_index = _si, \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 16, \ + .storagebits = 16, \ + .endianness = IIO_CPU, \ + }, \ + } + +static const struct iio_chan_spec bh1745_channels[] = { + BH1745_CHANNEL(RED, 0, BH1745_RED_LSB), + BH1745_CHANNEL(GREEN, 1, BH1745_GREEN_LSB), + BH1745_CHANNEL(BLUE, 2, BH1745_BLUE_LSB), + BH1745_CHANNEL(CLEAR, 3, BH1745_CLEAR_LSB), + IIO_CHAN_SOFT_TIMESTAMP(4), +}; + +static int bh1745_reset(struct bh1745_data *data) +{ + int ret; + int value; + + ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &value); + if (ret) + return ret; + + value |= (BH1745_SW_RESET | BH1745_INT_RESET); + + return regmap_write(data->regmap, BH1745_SYS_CTRL, value); +} + +static int bh1745_power_on(struct bh1745_data *data) +{ + return regmap_write_bits(data->regmap, BH1745_MODE_CTRL_2, + BH1745_RGBC_EN, BH1745_RGBC_EN); +} + +static void bh1745_power_off(void *data_ptr) +{ + struct bh1745_data *data = data_ptr; + struct device *dev = &data->client->dev; + int ret; + + ret = regmap_write_bits(data->regmap, BH1745_MODE_CTRL_2, + BH1745_RGBC_EN, 0); + if (ret) + dev_err(dev, "Failed to turn off device\n"); +} + +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]; + } + + return -EINVAL; +} + +static int bh1745_get_scale(struct bh1745_data *data, int *val, int *val2) +{ + int ret; + int gain_sel; + int int_time_sel; + + ret = regmap_read(data->regmap, BH1745_MODE_CTRL_2, &gain_sel); + if (ret) + return ret; + + gain_sel = FIELD_GET(BH1745_ADC_GAIN_MASK, gain_sel); + + ret = regmap_read(data->regmap, BH1745_MODE_CTRL_1, &int_time_sel); + if (ret) + return ret; + + int_time_sel = FIELD_GET(BH1745_MEASUREMENT_TIME_MASK, int_time_sel); + + return iio_gts_get_scale(&data->gts, bh1745_gain_factor[gain_sel], + bh1745_int_time_us[int_time_sel], val, val2); +} + +static int bh1745_set_scale(struct bh1745_data *data, int val, int val2) +{ + int ret; + int hw_gain_sel; + int current_int_time_sel; + int new_int_time_sel; + + ret = regmap_read(data->regmap, BH1745_MODE_CTRL_1, + ¤t_int_time_sel); + if (ret) + return ret; + + current_int_time_sel = + FIELD_GET(BH1745_MEASUREMENT_TIME_MASK, current_int_time_sel); + + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, + current_int_time_sel, + val, val2, &hw_gain_sel); + if (ret) { + for (int i = 0; i < data->gts.num_itime; i++) { + new_int_time_sel = data->gts.itime_table[i].sel; + + if (new_int_time_sel == current_int_time_sel) + continue; + + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, + new_int_time_sel, + val, val2, + &hw_gain_sel); + if (ret == 0) + break; + } + if (ret) + return -EINVAL; + + ret = regmap_write_bits(data->regmap, BH1745_MODE_CTRL_1, + BH1745_MEASUREMENT_TIME_MASK, + new_int_time_sel); + if (ret) + return ret; + } + + return regmap_write_bits(data->regmap, BH1745_MODE_CTRL_2, + BH1745_ADC_GAIN_MASK, hw_gain_sel); +} + +static int bh1745_get_int_time(struct bh1745_data *data, int *val) +{ + int ret; + int int_time; + + ret = regmap_read(data->regmap, BH1745_MODE_CTRL_1, &int_time); + if (ret) + return ret; + + int_time = FIELD_GET(BH1745_MEASUREMENT_TIME_MASK, int_time); + + ret = iio_gts_find_int_time_by_sel(&data->gts, int_time); + if (ret < 0) + return ret; + + *val = ret; + + return 0; +} + +static int bh1745_set_int_time(struct bh1745_data *data, int req_int_time) +{ + struct device *dev = &data->client->dev; + int ret; + int current_int_time, current_hwgain_sel, current_hwgain; + int new_hwgain, new_hwgain_sel, new_int_time_sel; + + if (!iio_gts_valid_time(&data->gts, req_int_time)) { + dev_err(dev, "Unsupported integration time requested: %d\n", + req_int_time); + return -EINVAL; + } + + ret = bh1745_get_int_time(data, ¤t_int_time); + if (ret) + return ret; + + if (current_int_time == req_int_time) + return 0; + + ret = regmap_read(data->regmap, BH1745_MODE_CTRL_2, ¤t_hwgain_sel); + if (ret) + return ret; + + current_hwgain_sel = FIELD_GET(BH1745_ADC_GAIN_MASK, current_hwgain_sel); + current_hwgain = bh1745_gain_factor[current_hwgain_sel]; + + ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, current_hwgain, + current_int_time, req_int_time, + &new_hwgain); + + if (new_hwgain < 0) { + dev_dbg(dev, "No corrosponding gain for requested integration time\n"); + return ret; + } + + if (ret) { + bool in_range; + + ret = iio_find_closest_gain_low(&data->gts, new_hwgain, &in_range); + if (!in_range) + dev_dbg(dev, "Optimal gain out of range\n"); + + if (ret < 0) { + dev_dbg(dev, "Total gain increased"); + ret = iio_gts_get_min_gain(&data->gts); + if (ret < 0) + return ret; + } + new_hwgain = ret; + dev_dbg(dev, "Scale changed, new hw_gain %d\n", + new_hwgain); + } + + ret = iio_gts_find_sel_by_gain(&data->gts, new_hwgain); + if (ret < 0) + return ret; + + new_hwgain_sel = FIELD_PREP(BH1745_ADC_GAIN_MASK, ret); + ret = regmap_write_bits(data->regmap, BH1745_MODE_CTRL_2, BH1745_ADC_GAIN_MASK, + new_hwgain_sel); + if (ret) + return ret; + + new_int_time_sel = iio_gts_find_sel_by_int_time(&data->gts, req_int_time); + if (new_int_time_sel < 0) + return new_int_time_sel; + + return regmap_write_bits(data->regmap, BH1745_MODE_CTRL_1, + BH1745_MEASUREMENT_TIME_MASK, new_int_time_sel); +} + +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) { + ret = regmap_bulk_read(data->regmap, chan->address, &value, 2); + if (ret) + return ret; + *val = value; + + return IIO_VAL_INT; + } + unreachable(); + } + + case IIO_CHAN_INFO_SCALE: { + guard(mutex)(&data->lock); + ret = bh1745_get_scale(data, val, val2); + if (ret) + return ret; + + return IIO_VAL_INT; + } + + case IIO_CHAN_INFO_INT_TIME: { + guard(mutex)(&data->lock); + *val = 0; + ret = bh1745_get_int_time(data, val2); + if (ret) + return 0; + + return IIO_VAL_INT_PLUS_MICRO; + } + + default: + return -EINVAL; + } +} + +static int bh1745_write_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; + + guard(mutex)(&data->lock); + switch (mask) { + case IIO_CHAN_INFO_SCALE: + return bh1745_set_scale(data, val, val2); + + case IIO_CHAN_INFO_INT_TIME: { + ret = bh1745_int_time_to_us(val, val2); + if (ret < 0) + return ret; + + return bh1745_set_int_time(data, ret); + } + default: + return -EINVAL; + } +} + +static int bh1745_read_thresh(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int *val, int *val2) +{ + struct bh1745_data *data = iio_priv(indio_dev); + int ret; + + switch (info) { + case IIO_EV_INFO_VALUE: + switch (dir) { + case IIO_EV_DIR_RISING: + ret = regmap_bulk_read(data->regmap, BH1745_TH_LSB, val, 2); + if (ret) + return ret; + + return IIO_VAL_INT; + + case IIO_EV_DIR_FALLING: + ret = regmap_bulk_read(data->regmap, BH1745_TL_LSB, val, 2); + if (ret) + return ret; + + return IIO_VAL_INT; + + default: + return -EINVAL; + } + case IIO_EV_INFO_PERIOD: + ret = regmap_read(data->regmap, BH1745_PERSISTENCE, val); + if (ret) + return ret; + + return IIO_VAL_INT; + + default: + return -EINVAL; + } +} + +static int bh1745_write_thresh(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int val, int val2) +{ + struct bh1745_data *data = iio_priv(indio_dev); + int ret; + + switch (info) { + case IIO_EV_INFO_VALUE: + if (val < 0x0 || val > 0xFFFF) + return -EINVAL; + + switch (dir) { + case IIO_EV_DIR_RISING: + ret = regmap_bulk_write(data->regmap, BH1745_TH_LSB, &val, 2); + if (ret) + return ret; + + return IIO_VAL_INT; + + case IIO_EV_DIR_FALLING: + ret = regmap_bulk_write(data->regmap, BH1745_TL_LSB, &val, 2); + if (ret) + return ret; + + return IIO_VAL_INT; + + default: + return -EINVAL; + } + case IIO_EV_INFO_PERIOD: + if (val < BH1745_PRESISTENCE_UPDATE_TOGGLE || + val > BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT) + return -EINVAL; + ret = regmap_write(data->regmap, BH1745_PERSISTENCE, val); + if (ret) + return ret; + + return IIO_VAL_INT; + + default: + return -EINVAL; + } +} + +static int bh1745_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct bh1745_data *data = iio_priv(indio_dev); + + guard(mutex)(&data->lock); + + return data->int_src; +} + +static int bh1745_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, int state) +{ + struct bh1745_data *data = iio_priv(indio_dev); + int ret; + + /* + * We do not update the interrupt source if 'state' value is '0' as + * there isn't a provision to not have an interrupt source at all. + */ + if (state == 1) { + guard(mutex)(&data->lock); + switch (chan->channel2) { + case IIO_MOD_LIGHT_RED: + ret = regmap_write_bits(data->regmap, BH1745_INTR, + BH1745_INT_SOURCE_MASK, + BH1745_INT_SOURCE_RED); + if (ret) + return ret; + + data->int_src = BH1745_INT_SOURCE_RED; + break; + + case IIO_MOD_LIGHT_GREEN: + ret = regmap_write_bits(data->regmap, BH1745_INTR, + BH1745_INT_SOURCE_MASK, + BH1745_INT_SOURCE_GREEN); + if (ret) + return ret; + + data->int_src = BH1745_INT_SOURCE_GREEN; + break; + + case IIO_MOD_LIGHT_BLUE: + ret = regmap_write_bits(data->regmap, BH1745_INTR, + BH1745_INT_SOURCE_MASK, + BH1745_INT_SOURCE_BLUE); + if (ret) + return ret; + + data->int_src = BH1745_INT_SOURCE_BLUE; + break; + + case IIO_MOD_LIGHT_CLEAR: + ret = regmap_write_bits(data->regmap, BH1745_INTR, + BH1745_INT_SOURCE_MASK, + BH1745_INT_SOURCE_CLEAR); + if (ret) + return ret; + + data->int_src = BH1745_INT_SOURCE_CLEAR; + break; + + default: + return -EINVAL; + } + } + + return 0; +} + +static int bh1745_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, const int **vals, + int *type, int *length, long mask) +{ + struct bh1745_data *data = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + return iio_gts_avail_times(&data->gts, vals, type, length); + + case IIO_CHAN_INFO_SCALE: + return iio_gts_all_avail_scales(&data->gts, vals, type, length); + + default: + return -EINVAL; + } +} + +static const struct iio_info bh1745_info = { + .read_raw = bh1745_read_raw, + .write_raw = bh1745_write_raw, + .read_event_value = bh1745_read_thresh, + .write_event_value = bh1745_write_thresh, + .read_event_config = bh1745_read_event_config, + .write_event_config = bh1745_write_event_config, + .read_avail = bh1745_read_avail, +}; + +static int bh1745_set_trigger_state(struct iio_trigger *trig, bool state) +{ + int value = 0; + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); + struct bh1745_data *data = iio_priv(indio_dev); + + if (state) { + guard(mutex)(&data->lock); + // 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); +} + +static const struct iio_trigger_ops bh1745_trigger_ops = { + .set_trigger_state = bh1745_set_trigger_state, +}; + +static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p) +{ + struct iio_dev *indio_dev = p; + struct bh1745_data *data = iio_priv(indio_dev); + int ret; + int value; + + ret = regmap_read(data->regmap, BH1745_INTR, &value); + if (ret) + return IRQ_NONE; + + if (value & BH1745_INTR_STATUS) { + guard(mutex)(&data->lock); + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, data->int_src, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_EITHER), + iio_get_time_ns(indio_dev)); + + iio_trigger_poll_nested(data->trig); + + return IRQ_HANDLED; + } + + return IRQ_NONE; +} + +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, j = 0; + + for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) { + 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_setup_trigger(struct iio_dev *indio_dev, struct device *parent) +{ + struct bh1745_data *data = iio_priv(indio_dev); + struct device *dev = &data->client->dev; + int ret; + + data->trig = devm_iio_trigger_alloc(parent, "%sdata-rdy-dev%d", + indio_dev->name, + iio_device_id(indio_dev)); + if (!data->trig) + return -ENOMEM; + + data->trig->ops = &bh1745_trigger_ops; + iio_trigger_set_drvdata(data->trig, indio_dev); + + ret = devm_iio_trigger_register(dev, data->trig); + if (ret) + return dev_err_probe(dev, ret, "Trigger registration failed\n"); + + ret = devm_iio_triggered_buffer_setup(parent, indio_dev, NULL, + bh1745_trigger_handler, NULL); + if (ret) + return dev_err_probe(dev, ret, "Triggered buffer setup failed\n"); + + if (data->client->irq) { + ret = devm_request_threaded_irq(dev, data->client->irq, NULL, + bh1745_interrupt_handler, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "bh1745_interrupt", indio_dev); + if (ret) + return dev_err_probe(dev, ret, "Request for IRQ failed\n"); + } + + return 0; +} + +static int bh1745_init(struct bh1745_data *data) +{ + int ret; + struct device *dev = &data->client->dev; + + mutex_init(&data->lock); + data->int_src = BH1745_INT_SOURCE_RED; + + ret = devm_iio_init_iio_gts(dev, BH1745_MAX_GAIN, 0, bh1745_gain, + ARRAY_SIZE(bh1745_gain), bh1745_itimes, + ARRAY_SIZE(bh1745_itimes), &data->gts); + if (ret) + return ret; + + ret = bh1745_reset(data); + if (ret) { + dev_err_probe(dev, ret, "Failed to reset sensor\n"); + return ret; + } + + ret = bh1745_power_on(data); + if (ret) { + dev_err_probe(dev, ret, "Failed to turn on sensor\n"); + return ret; + } + + return 0; +} + +static int bh1745_probe(struct i2c_client *client) +{ + int ret; + 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; + + ret = devm_regulator_get_enable(dev, "vdd"); + if (ret) + return dev_err_probe(dev, ret, "Failed to get and enable regulator\n"); + + data = iio_priv(indio_dev); + i2c_set_clientdata(client, indio_dev); + data->client = client; + indio_dev->info = &bh1745_info; + indio_dev->name = "bh1745"; + indio_dev->channels = bh1745_channels; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->num_channels = ARRAY_SIZE(bh1745_channels); + + data->regmap = devm_regmap_init_i2c(client, &bh1745_regmap); + if (IS_ERR(data->regmap)) + return dev_err_probe(dev, PTR_ERR(data->regmap), + "Failed to initialize Regmap\n"); + + ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &part_id); + if (ret) + return ret; + + part_id = FIELD_GET(BH1745_PART_ID_MASK, part_id); + if (part_id != BH1745_PART_ID) + dev_warn(dev, "Unknown part ID 0x%x\n", part_id); + + ret = bh1745_init(data); + if (ret) + return ret; + + ret = devm_add_action_or_reset(dev, bh1745_power_off, data); + if (ret) + return dev_err_probe(dev, ret, "Failed to add action or reset\n"); + + ret = bh1745_setup_trigger(indio_dev, indio_dev->dev.parent); + if (ret < 0) + return ret; + + ret = devm_iio_device_register(dev, indio_dev); + if (ret < 0) { + dev_err(dev, "Failed to register device\n"); + return ret; + } + + return 0; +} + +static const struct i2c_device_id bh1745_idtable[] = { + { "bh1745" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, bh1745_idtable); + +static const struct of_device_id bh1745_of_match[] = { + { .compatible = "rohm,bh1745" }, + { } +}; +MODULE_DEVICE_TABLE(of, bh1745_of_match); + +static struct i2c_driver bh1745_driver = { + .driver = { + .name = "bh1745", + .of_match_table = bh1745_of_match, + }, + .probe = bh1745_probe, + .id_table = bh1745_idtable, +}; +module_i2c_driver(bh1745_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Mudit Sharma <muditsharma.info@gmail.com>"); +MODULE_DESCRIPTION("BH1745 colour sensor driver"); +MODULE_IMPORT_NS(IIO_GTS_HELPER);