On Wed, May 03, 2023 at 12:50:14PM +0300, Matti Vaittinen wrote:
> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> and IR) with four configurable channels. Red and green being always
> available and two out of the rest three (blue, clear, IR) can be
> selected to be simultaneously measured. Typical application is adjusting
> LCD backlight of TVs, mobile phones and tablet PCs.
>
> Add initial support for the ROHM BU27008 color sensor.
> - raw_read() of RGB and clear channels
> - triggered buffer w/ DRDY interrtupt
...
> +config ROHM_BU27008
> + tristate "ROHM BU27008 color (RGB+C/IR) sensor"
> + depends on I2C
> + select REGMAP_I2C
> + select IIO_GTS_HELPER
> + help
> + Enable support for the ROHM BU27008 color sensor.
> + The ROHM BU27008 is a sensor with 5 photodiodes (red, green,
> + blue, clear and IR) with four configurable channels. Red and
> + green being always available and two out of the rest three
> + (blue, clear, IR) can be selected to be simultaneously measured.
> + Typical application is adjusting LCD backlight of TVs,
> + mobile phones and tablet PCs.
Module name?
...
> +static const struct regmap_range bu27008_read_only_ranges[] = {
> + {
> + .range_min = BU27008_REG_DATA0_LO,
> + .range_max = BU27008_REG_DATA3_HI,
> + }, {
> + .range_min = BU27008_REG_MANUFACTURER_ID,
> + .range_max = BU27008_REG_MANUFACTURER_ID,
> + }
+ trailing comma for consistency?
> +};
...
> +static const struct regmap_config bu27008_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = BU27008_REG_MAX,
> + .cache_type = REGCACHE_RBTREE,
> + .volatile_table = &bu27008_volatile_regs,
> + .wr_table = &bu27008_ro_regs,
Do you need regmap lock? If so, why (since you have mutex)?
> +};
...
> +static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
> + struct iio_chan_spec const *chan, int *val, int *val2)
> +{
> + int ret, int_time;
> +
> + ret = bu27008_chan_cfg(data, chan);
> + if (ret)
> + return ret;
> +
> + ret = bu27008_meas_set(data, BU27008_MEAS_EN);
> + if (ret)
> + return ret;
> +
> + int_time = bu27008_get_int_time_us(data);
> + if (int_time < 0)
> + int_time = BU27008_MEAS_TIME_MAX_MS;
> + else
> + int_time /= USEC_PER_MSEC;
The above function returns an error code when negative, so I would rather see
ret = bu27008_get_int_time_us(data);
if (ret < 0)
int_time = BU27008_MEAS_TIME_MAX_MS;
else
int_time = ret / USEC_PER_MSEC;
at least this explicitly shows the semantics of the "negative" time.
> + msleep(int_time);
> +
> + ret = bu27008_chan_read_data(data, chan->address, val);
> + if (!ret)
> + ret = IIO_VAL_INT;
> +
> + if (bu27008_meas_set(data, BU27008_MEAS_DIS))
> + dev_warn(data->dev, "measurement disabling failed\n");
> +
> + return ret;
> +}
...
> + ret = regmap_reinit_cache(data->regmap, &bu27008_regmap);
> + if (ret) {
> + dev_err(data->dev, "Failed to reinit reg cache\n");
> + return ret;
Dup is not needed.
> + }
> +
> + return ret;
...
> + if (i2c->irq) {
Instead of a long body, I would rather see a call to
ret = ..._setup_irq();
if (ret)
return ret;
> + ret = devm_iio_triggered_buffer_setup(dev, idev,
> + &iio_pollfunc_store_time,
> + bu27008_trigger_handler,
> + &bu27008_buffer_ops);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "iio_triggered_buffer_setup_ext FAIL\n");
> +
> + itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d",
> + idev->name, iio_device_id(idev));
> + if (!itrig)
> + return -ENOMEM;
> +
> + data->trig = itrig;
> +
> + itrig->ops = &bu27008_trigger_ops;
> + iio_trigger_set_drvdata(itrig, data);
> +
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008",
> + dev_name(dev));
> +
> + ret = devm_request_irq(dev, i2c->irq,
> + &bu27008_data_rdy_poll,
> + 0, name, itrig);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Could not request IRQ\n");
> +
> + ret = devm_iio_trigger_register(dev, itrig);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Trigger registration failed\n");
> +
> + /* set default trigger */
> + idev->trig = iio_trigger_get(itrig);
> + } else {
> + dev_info(dev, "No IRQ, buffered mode disabled\n");
> + }
--
With Best Regards,
Andy Shevchenko
