Re: [PATCH] iio: bu27008: Add processed illuminance channel

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 10/10/23 13:01, Matti Vaittinen wrote:
> On 10/10/23 12:40, Jonathan Cameron wrote:
>> On Fri, 6 Oct 2023 08:01:15 +0300
>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>>> On 10/5/23 18:14, Jonathan Cameron wrote:
>>>> On Mon, 2 Oct 2023 15:33:41 +0300
>>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> //snip
> 
>>>>> +static int bu27008_get_rgb_ir(struct bu27008_data *data, unsigned 
>>>>> int *red,
>>>>> +            unsigned int *green, unsigned int *blue, unsigned int 
>>>>> *ir)
>>>>> +{
>>>>> +    int ret, chan_sel, int_time, tmpret, valid;
>>>>> +    __le16 chans[BU27008_NUM_HW_CHANS];
>>>>> +
>>>>> +    chan_sel = BU27008_BLUE2_IR3 << (ffs(data->cd->chan_sel_mask) 
>>>>> - 1);
>>>>> +
>>>>> +    ret = regmap_update_bits(data->regmap, data->cd->chan_sel_reg,
>>>>> +                 data->cd->chan_sel_mask, chan_sel);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    ret = bu27008_meas_set(data, true);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    ret = bu27008_get_int_time_us(data);
>>>>> +    if (ret < 0)
>>>>> +        int_time = BU27008_MEAS_TIME_MAX_MS;
>>>>> +    else
>>>>> +        int_time = ret / USEC_PER_MSEC;
>>>>> +
>>>>> +    msleep(int_time);
>>>>> +
>>>>> +    ret = regmap_read_poll_timeout(data->regmap, data->cd->valid_reg,
>>>>> +                       valid, (valid & BU27008_MASK_VALID),
>>>>> +                       BU27008_VALID_RESULT_WAIT_QUANTA_US,
>>>>> +                       BU27008_MAX_VALID_RESULT_WAIT_US);
>>>>> +    if (ret)
>>>>> +        goto out;
>>>>> +
>>>>> +    ret = regmap_bulk_read(data->regmap, BU27008_REG_DATA0_LO, chans,
>>>>> +                   sizeof(chans));
>>>>> +    if (ret)
>>>>> +        goto out;
>>>>> +
>>>>> +    *red = le16_to_cpu(chans[0]);
>>>>> +    *green = le16_to_cpu(chans[1]);
>>>>> +    *blue = le16_to_cpu(chans[2]);
>>>>> +    *ir = le16_to_cpu(chans[3]);
>>>>
>>>> I'd be tempted to use an array + definitely pass them as u16 rather
>>>> than unsigned int.
>>>
>>> I'm not really convinced the u16 is better here. We need the 32 bits
>>> later for the calculations - and (afaics) using natural size int for
>>> arguments shouldn't harm. We read the channel data to correct type array
>>> so code should be pretty clear as to what we have in HW.
>>
>> ok.  I don't like lack of range clamping - so at the point of the caller
>> I can't immediately see that these will be sub 16bit value.  Not htat
>> important I guess.
>>
>>>
>>> Also, I think that having an array obfuscates what element is which
>>> channel because these ICs didn't have the 1 to 1 mapping from channel
>>> index to colour. I was thinking of adding a struct for this but decided
>>> to just keep it simple and clear.
>> A struct or array + enum would work.
>> I just don't much like lots of very similar parameters.
> 
> Right. A struct is not a problem. I am less fond with an enum because 
> the HW channel which carries a specific color may change. I think adding 
> an enum which indicates a place where a color is in array may be 
> misleading one to think that HW has a fixed channel (data register 
> address) for a colour. (I think that is quite usual while ICs where one 
> color may be in different channels depending on the config are likely to 
> be rare... I haven't read too many light sensor specs/drivers to know 
> for sure though).
  I should've re-read the whole driver code before replying. I see we 
already have an enum for colours (with comments telling that some of the 
colours do not have fixed channel). So, my point objecting the enum is 
kind of moot. Unfortunately I chose the clear to be before IR, which 
means that if we used existing enum we would have a gap in the array 
(because clear is not used for lux computation). Not sure it matters 
though :)

Yours,
	-- Matti

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

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