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@xxxxxxxxx> wrote:
On 10/5/23 18:14, Jonathan Cameron wrote:
On Mon, 2 Oct 2023 15:33:41 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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! ~~
