Messages in this thread | | | Date | Sat, 4 Mar 2023 22:28:43 +0200 | From | Matti Vaittinen <> | 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! ~~
| |