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

[Matti Vaittinen]

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! ~~