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

[Vaittinen, Matti]

On 5/4/23 17:33, Andy Shevchenko wrote:
> 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?

We have discussed this several times already.

https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@xxxxxxxxxxxxxxxxx/

Module name is completely irrelevant when selecting a kernel configuration.

> 
> ...
> 
>> +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? 

Thanks.

>> +};
> 
> ...
> 
>> +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)?

I believe you know that regmap uses a default lock when no external lock 
is given. So, I assume you mean that maybe we could set 
'disable_locking' for the regmap here.

It's nice to be occasionally pushed to think "out of the box". And yes, 
disabling regmap lock is really out of my "normal box" :)

I didn't go through all of the code yet, but I think pretty much all of 
the sequences which end up to register writes are indeed protected by 
the mutex. (Well, probe is not but it is expected to only update one bit 
while rest of the register should stay fixed).

It may be we could live without regmap_lock when driver is in it's 
current state, but I am not convinced the performance improvement is 
worth the risk. Having regmap unprotected is not common, and it is also 
not easy to spot when making changes to the driver. In my opinion it is 
a bit like asking for a nose-bleed unless there is really heavy reasons 
to drop the lock... In this case, having the regmap_lock (which is 
pretty much never locked because we have the mutex as you said) is 
probably not a penalty that matters.

> 
>> +};
> 
> ...
> 
>> +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.

Ok.

> 
>> +	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.

Thanks.

> 
>> +	}
>> +
>> +	return ret;
> 
> ...
> 
>> +	if (i2c->irq) {
> 
> Instead of a long body, I would rather see a call to

Could make sense indeed. I'll see how it would look like, thanks.

> 
> 		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");
>> +	}
> 
> 

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~