Re: [PATCH v2 5/6] iio: light: ROHM BU27034 Ambient Light Sensor

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 3/4/23 21:02, Jonathan Cameron wrote:

>>> ...
>>>    
>>>> +/*
>>>> + * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
>>>> + * Time impacts to gain: 1x, 2x, 4x, 8x.
>>>> + *
>>>> + * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768
>>>> + *
>>>> + * Using NANO precision for scale we must use scale 64x corresponding gain 1x
>>>> + * to avoid precision loss. (32x would result scale 976 562.5(nanos).
>>>> + */
>>>> +#define BU27034_SCALE_1X	64
>>>> +
>>>> +#define BU27034_GSEL_1X		0x00
>>>> +#define BU27034_GSEL_4X		0x08
>>>> +#define BU27034_GSEL_16X	0x0a
>>>> +#define BU27034_GSEL_32X	0x0b
>>>> +#define BU27034_GSEL_64X	0x0c
>>>> +#define BU27034_GSEL_256X	0x18
>>>> +#define BU27034_GSEL_512X	0x19
>>>> +#define BU27034_GSEL_1024X	0x1a
>>>> +#define BU27034_GSEL_2048X	0x1b
>>>> +#define BU27034_GSEL_4096X	0x1c
>>>
>>> Shouldn't the values be in plain decimal?
>>
>> Why?
> 
> Normally go with datasheet on this as it makes reviewer simpler.
> But datasheet is in binary so meh.
> 
>>
>>> Otherwise I would like to understand bit mapping inside these hex values.
>>
>> I like having register values in hex. It makes it obvious they don't
>> necessarily directly match any 'real world' human-readable values.
> 
> Perhaps a cross reference to the table in the spec is appropriate here?

I think adding a reference to the table in the data-sheet is good. 
Although - I gave some feedback about the data-sheet inside the company. 
It may be we will eventually see another version of it...

> whilst there are some patterns they aren't terribly consistent so probably
> best to just point at the table in the spec.
> 
> 
>>>> +		if (helper64 < 0xFFFFFFFFFFFFFLLU) {
>>>
>>> Perhaps this needs a definition.
>>
>> I like seeing the value here. It makes this less obfuscating. Comment
>> makes the purpose obvious so adding a define would not really give any
>> extra advantage.
> 
> It's not immediately obvious why it is that many f's.  Perhaps change
> to refer to number of bits (which is what matters really I think)
> and then use GENMASK() to fill this in?  I think it's 52 bits?

I am personally not used to the GENMASK(). I am always wondering whether 
the parameters were start + end bits, or star bit + number of set bits 
or ... It's somehow easier for me to understand the hex values - 
(especially when they are composed of all fff's or 1, 3, 7 or 2, 4, 8 ... ).

Well, I understand that is not universally true though so GENMASK() can 
for sure be simpler for most others - and it does not hide the value as 
define would do. So yes, GENMASK() could make sense here.

>>>> +			helper64 *= gain0;
>>>> +			do_div(helper64, ch0);
>>>> +		} else {
>>>> +			do_div(helper64, ch0);
>>>> +			helper64 *= gain0;
>>>> +		}
> 
>>>
>>> ...> >> +	if (!res[0])
>>>
>>> Positive conditional?
>>
>> No. Again, we check for the very specific case where res has all bits
>> zeroed. Inverse condition is counter intuitive.
>>
>>>    
>>>> +		ch0 = 1;
>>>> +	else
>>>> +		ch0 = le16_to_cpu(res[0]);
>>>> +
>>>> +	if (!res[1])
>>>> +		ch1 = 1;
>>>
>>> Ditto.
>>>    
>>>> +	else
>>>> +		ch1 = le16_to_cpu(res[1]);
>>>
>>> But why not to read and convert first and then check.
>>
>> Because conversion is not needed if channel data is zero.
> 
> Conversion is trivial. I agree with Andy here that the logic would look
> a bit simpler as (taking it a little further)
> 
> 	ch0 = max(1, le16_to_cpu(res[0]));

It's strange how differently we read the code. For me:

if (!val)
	ch = 1;
else
	ch =  le16_to_cpu(val);

tells what is happening at a glance whereas the:
ch0 = max(1, le16_to_cpu(res[0]));

really requires some focusing to see what is going on. For me it is both 
less clear and less efficient :(

But alas, if this is what is preferred by both of you, then I guess 
that's what it will look like.

>>>
>>>> +	switch (mask) {
>>>> +	case IIO_CHAN_INFO_INT_TIME:
>>>> +		return iio_gts_avail_times(&data->gts, vals, type, length);
>>>> +	case IIO_CHAN_INFO_SCALE:
>>>> +		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return -EINVAL;
>>>
>>> You may do it from default case.
>>>    
>>
>> I think we have discussed this one in the past too. I like having return
>> at the end of a non void function.
> 
> Given all the earlier returns and the fact that the compiler will shout at
> you if it you can get here and it is missing, I'd also suggest just putting
> it in the switch statement.

Ok. IIO is your territory. If the roles were switched I would definitely 
ask for having the returns at the end of the function - and at the end 
of the function only (when possible w/o lot of extra complication).

So perhaps I just have to accept that you want to have it your way with 
IIO :)

Yours,
	-- Matti

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

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