[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9c62d3f1-7d07-a695-67a0-f4a373e96073@gmail.com>
Date: Sat, 4 Mar 2023 22:28:43 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Lars-Peter Clausen <lars@...afoo.de>,
Paul Gazzillo <paul@...zz.com>,
Zhigang Shi <Zhigang.Shi@...eon.com>,
Shreeya Patel <shreeya.patel@...labora.com>,
Dmitry Osipenko <dmitry.osipenko@...labora.com>,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [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! ~~
Powered by blists - more mailing lists