On 19/07/2024 00:02, 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@xxxxxxxxx> > Reviewed-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx> > Reviewed-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> Hi Mudit, a couple of nitpicks inline. ... > +static int bh1745_set_int_time(struct bh1745_data *data, int val, int val2) > +{ > + struct device *dev = data->dev; > + int ret; > + int value; > + int current_int_time, current_hwgain_sel, current_hwgain; > + int new_hwgain, new_hwgain_sel, new_int_time_sel; > + int req_int_time = (1000000 * val) + val2; > + > + if (!iio_gts_valid_time(&data->gts, req_int_time)) { > + dev_dbg(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_CTRL2, &value); > + if (ret) > + return ret; > + > + current_hwgain_sel = FIELD_GET(BH1745_CTRL2_ADC_GAIN_MASK, value); > + current_hwgain = iio_gts_find_gain_by_sel(&data->gts, 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) { Typo in debug message: corresponding. I would recommend you to pass codespell to checkpatch. It will often catch such things. > + dev_dbg(dev, "No corrosponding gain for requested integration time\n"); > + return ret; > + } > + ... > +static int bh1745_write_raw_get_fmt(struct iio_dev *indio_dev, Nit: the alignment seems to be a bit off here. > + struct iio_chan_spec const *chan, > + long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_INT_TIME: > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + return -EINVAL; > + } > +} > + ... > +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 value; > + > + if (state == 0) > + return regmap_clear_bits(data->regmap, BH1745_INTR, BH1745_INTR_ENABLE); > + > + if (state == 1) { Nit: empty line at the beginning of the scope. > + > + /* Latch is always enabled when enabling interrupt */ > + value = BH1745_INTR_ENABLE; > + > + switch (chan->channel2) { > + case IIO_MOD_LIGHT_RED: > + return regmap_write(data->regmap, BH1745_INTR, > + value | FIELD_PREP(BH1745_INTR_SOURCE_MASK, > + BH1745_INTR_SOURCE_RED)); > + > + case IIO_MOD_LIGHT_GREEN: > + return regmap_write(data->regmap, BH1745_INTR, > + value | FIELD_PREP(BH1745_INTR_SOURCE_MASK, > + BH1745_INTR_SOURCE_GREEN)); > + > + case IIO_MOD_LIGHT_BLUE: > + return regmap_write(data->regmap, BH1745_INTR, > + value | FIELD_PREP(BH1745_INTR_SOURCE_MASK, > + BH1745_INTR_SOURCE_BLUE)); > + > + case IIO_MOD_LIGHT_CLEAR: > + return regmap_write(data->regmap, BH1745_INTR, > + value | FIELD_PREP(BH1745_INTR_SOURCE_MASK, > + BH1745_INTR_SOURCE_CLEAR)); > + > + default: > + return -EINVAL; > + } > + } > + > + return -EINVAL; > +} > + > Best regards, Javier Carrasco