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

[Vaittinen, Matti]

Hi Andy

Thanks for the review.

On 5/2/23 23:40, Andy Shevchenko wrote:
> On Wed, Apr 26, 2023 at 11:08:17AM +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
> 
> ...
> 
>> +enum {
>> +	BU27008_RED,	/* Always data0 */
>> +	BU27008_GREEN,	/* Always data1 */
>> +	BU27008_BLUE,	/* data2, configurable (blue / clear) */
>> +	BU27008_CLEAR,	/* data2 or data3 */
>> +	BU27008_IR,	/* data3 */
>> +	BU27008_NUM_CHANS
> 
> Why not converting comments to a kernel-doc?
> 
>> +};
>> +
>> +enum {
>> +	BU27008_DATA0, /* Always RED */
>> +	BU27008_DATA1, /* Always GREEN */
>> +	BU27008_DATA2, /* Blue or Clear */
>> +	BU27008_DATA3, /* IR or Clear */
>> +	BU27008_NUM_HW_CHANS
>> +};
> 
> Ditto.

I see no value having entities which are not intended to be used outside 
this file documented in any "global" documentation. One who is ever 
going to use these or wonder what these are - will most likely be 
watching this file. My personal view is that the generated docs should 
be kept lean. In my opinion the problem of the day is the time we spend 
looking for a needle hidden in a haystack. In my opinion adding this to 
kernel-doc just adds hay :)

I still can do this if no-one else objects. I almost never look at the 
generated docs myself. Usually I just look the docs from code files - 
and kernel-doc format is not any worse for me to read. Still, I can 
imagine including this type of stuff to generic doc just bloats them and 
my not serve well those who use them.

> 
> ...
> 
>> +	if (int_time < 0)
>> +		int_time = 400000;
> 
> Adding 3 0:s to drop them below with a heavy division operation? Well done!
> Or did I miss anything?

No. You did not miss anything. This can be improved.

> 
>> +	msleep(int_time / 1000);
> 
> USEC_PER_MSEC ?

Ok.

> ...
> 
>> +	ret = devm_iio_device_register(dev, idev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Unable to register iio device\n");
>> +
>> +	return ret;
> 
> return 0 will suffice.

Ok.

Thanks,
	-- Matti

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

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