Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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