lkml.org 
[lkml]   [2023]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/6] iio: light: ROHM BU27034 Ambient Light Sensor
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! ~~

\
 
 \ /
  Last update: 2023-03-27 00:44    [W:1.121 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site