On Tue, 13 Jun 2023 13:20:26 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> The ROHM BU27010 is an RGBC sensor with a flickering detection FIFO. The
> RGBC+IR sensor functionality is largely similar to what the BU27008 has.
> There are some notable things though:
> - gain setting is once again new and exotic. Now, there is 6bit gain
> setting where 4 of the bits are common to all channels and 2 bits
> can be configured separately for each channel. The BU27010 has
> similar "1X on other channels vs 2X on IR when selector is 0x0"
> gain design as BU27008 had. So, we use same gain setting policy for
> BU27010 as we did for BU27008 - driver sets same gain selector for all
> channels but shows the gains separately for all channels so users
> can (at least in theory) detect this 1X vs 2X madness...
> - BU27010 has suffled all the control register bitfields to new
> addresses and bit positions while still keeping the register naming
> same.
> - Some more power/reset control is added.
> - FIFO for "flickering detection" is added.
>
> The control register suffling made this slightly nasty. Still, it is
> easier for maintenance perspective to add the BU27010 support in BU27008
> driver because - even though the bit positions/addresses were changed -
> most of the driver structure can be re-used. Writing own driver for
> BU27010 would mean plenty of duplicate code albeit a tad more clarity.
>
> The flickering FIFO is not supported by the driver.
>
> Add BU27010 RGBC+IR support to rohm-bu27008 driver.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
>
Resulting code looks more or less fine, but there is stuff in here that
belongs in previous patch - so send a v2 with the refactors all done
there and just support for the new part in here.
Thanks,
Jonathan
...
> +
> static int bu27008_chip_init(struct bu27008_data *data);
> static int bu27008_write_gain_sel(struct bu27008_data *data, int sel);
> static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel);
>
> +static int bu27010_chip_init(struct bu27008_data *data);
> +static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel);
> +static int bu27010_write_gain_sel(struct bu27008_data *data, int sel);
> +
> +static const struct bu27_chip_data bu27010_chip = {
> + .name = "bu27010",
> + .chip_init = bu27010_chip_init,
> + .get_gain_sel = bu27010_get_gain_sel,
> + .write_gain_sel = bu27010_write_gain_sel,
> + .scale1x = BU27010_SCALE_1X,
> + .part_id = BU27010_ID,
> + .regmap_cfg = &bu27010_regmap,
> + .drdy_en_reg = BU27010_REG_MODE_CONTROL4,
> + .drdy_en_mask = BU27010_DRDY_EN,
> + .valid_reg = BU27010_REG_MODE_CONTROL5,
> + .meas_en_reg = BU27010_REG_MODE_CONTROL5,
> + .meas_en_mask = BU27010_MASK_MEAS_EN,
> + .chan_sel_reg = BU27008_REG_MODE_CONTROL1,
> + .chan_sel_mask = BU27010_MASK_CHAN_SEL,
> + .int_time_mask = BU27010_MASK_MEAS_MODE,
> + .gains = &bu27010_gains[0],
> + .num_gains = ARRAY_SIZE(bu27010_gains),
> + .gains_ir = &bu27010_gains_ir[0],
> + .num_gains_ir = ARRAY_SIZE(bu27010_gains_ir),
> + .itimes = &bu27010_itimes[0],
> + .num_itimes = ARRAY_SIZE(bu27010_itimes),
> +};
> +
> static const struct bu27_chip_data bu27008_chip = {
> .name = "bu27008",
> .chip_init = bu27008_chip_init,
> @@ -332,6 +529,8 @@ static const struct bu27_chip_data bu27008_chip = {
> .num_gains = ARRAY_SIZE(bu27008_gains),
> .gains_ir = &bu27008_gains_ir[0],
> .num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
> + .itimes = &bu27008_itimes[0],
> + .num_itimes = ARRAY_SIZE(bu27008_itimes),
> };
>
> #define BU27008_MAX_VALID_RESULT_WAIT_US 50000
> @@ -358,6 +557,31 @@ static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
> return ret;
> }
>
> +static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel)
> +{
> + int ret;
> +
> + /*
> + * We always "lock" the gain selectors for all channels to prevent
> + * unsupported configs. It does not matter which channel is used
> + * we can just return selector from any of them.
> + */
> + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, sel);
> + if (!ret) {
if (ret)
return ret;
....
> + int tmp;
> +
> + *sel = FIELD_GET(BU27010_MASK_DATA0_GAIN, *sel);
> +
> + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &tmp);
> + if (ret)
> + return ret;
> +
> + *sel |= FIELD_GET(BU27010_MASK_RGBC_GAIN, tmp) << fls(BU27010_MASK_DATA0_GAIN);
> + }
> +
> + return ret;
> +}
> +
> static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel)
> {
> int ret;
> @@ -388,7 +612,7 @@ static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int
> {
> int ret, sel;
>
> - ret = bu27008_get_gain_sel(data, &sel);
> + ret = data->cd->get_gain_sel(data, &sel);
Again, belongs in previous patch.
> if (ret)
> return ret;
>
> @@ -403,6 +627,35 @@ static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int
> return 0;
> }
>
> +static int bu27010_write_gain_sel(struct bu27008_data *data, int sel)
> +{
> + unsigned int regval;
> + int ret;
> +
> + /*
> + * Gain 'selector' is composed of two registers. Selector is 6bit value,
> + * 4 high bits being the RGBC gain fieild in MODE_CONTROL1 register and
> + * two low bits being the channel specific gain in MODE_CONTROL2.
> + *
> + * Let's take the 4 high bits of whole 6 bit selector, and prepare
> + * the MODE_CONTROL1 value (RGBC gain part).
> + */
> + regval = FIELD_PREP(BU27010_MASK_RGBC_GAIN, (sel >> 2));
> +
> + ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
> + BU27010_MASK_RGBC_GAIN, regval);
ret not checked.
> + /*
> + * Two low two bits must be set for all 4 channels in the
> + * MODE_CONTROL2 register. Copy these two bits for all channels.
> + */
> + regval = sel & GENMASK(1, 0);
FIELD_PREP and all fields explicitly set.
> + regval = regval | regval >> 2 | regval >> 4 | regval >> 6;
> +
> + ret = regmap_write(data->regmap, BU27008_REG_MODE_CONTROL2, regval);
return regmap_write...
> +
> + return ret;
> +}
> +
> static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
> {
> int regval;
> @@ -452,7 +705,7 @@ static int bu27008_set_gain(struct bu27008_data *data, int gain)
> if (ret < 0)
> return ret;
>
> - return bu27008_write_gain_sel(data, ret);
> + return data->cd->write_gain_sel(data, ret);
Another one that wants to be in the refactor patch - not here.
> }
>
> static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
> @@ -460,13 +713,15 @@ static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
> int ret, val;
>
> ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &val);
> + if (ret)
> + return ret;
>
> val &= data->cd->int_time_mask;
> val >>= ffs(data->cd->int_time_mask) - 1;
>
> *sel = val;
>
> - return ret;
> + return 0;
This looks unrelated / fix for previous patch?
> }
>
> static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel)
> @@ -759,7 +1014,7 @@ static int bu27008_set_scale(struct bu27008_data *data,
> goto unlock_out;
>
> }
> - ret = bu27008_write_gain_sel(data, gain_sel);
> + ret = data->cd->write_gain_sel(data, gain_sel);
As below - make all these sort of refactoring changes in previous patch.
>
> unlock_out:
> mutex_unlock(&data->mutex);
> @@ -867,6 +1122,55 @@ static const struct iio_info bu27008_info = {
> .validate_trigger = iio_validate_own_trigger,
> };
>
> +static int bu27010_chip_init(struct bu27008_data *data)
> +{
> + int ret;
> +
> + /* Reset the IC to initial power-off state */
I'd argue the code is clear enough the comment adds little.
> + ret = regmap_write_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
> + BU27010_MASK_SW_RESET, BU27010_MASK_SW_RESET);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
> +
> + msleep(1);
> +
> + /* Power ON*/
> + ret = regmap_write_bits(data->regmap, BU27010_REG_POWER,
> + BU27010_MASK_POWER, BU27010_MASK_POWER);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Sensor power-on failed\n");
> +
> + msleep(1);
> +
> + /* Release blocks from reset */
> + ret = regmap_write_bits(data->regmap, BU27010_REG_RESET,
> + BU27010_MASK_RESET, BU27010_RESET_RELEASE);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Sensor powering failed\n");
> +
> + msleep(1);
> +
> + /*
> + * The IRQ enabling on BU27010 is done in a peculiar way. The IRQ
> + * enabling is not a bit mask where individual IRQs could be enabled but
> + * a field which values are:
> + * 00 => IRQs disabled
> + * 01 => Data-ready (RGBC/IR)
> + * 10 => Data-ready (flicker)
> + * 11 => Flicker FIFO
> + *
That's nasty :)
> + * So, only one IRQ can be enabled at a time and enabling for example
> + * flicker FIFO would automagically disable data-ready IRQ.
> + *
> + * Currently the driver does not support the flicker. Hence, we can
> + * just treat the RGBC data-ready as single bit which can be enabled /
> + * disabled. This works for as long as the second bit in the field
> + * stays zero. Here we ensure it gets zeroed.
> + */
> + return regmap_clear_bits(data->regmap, BU27010_REG_MODE_CONTROL4,
> + BU27010_IRQ_DIS_ALL);
> +}
> +
> static int bu27008_chip_init(struct bu27008_data *data)
> {
> int ret;
> @@ -1068,14 +1372,14 @@ static int bu27008_probe(struct i2c_client *i2c)
> dev_warn(dev, "unknown device 0x%x\n", part_id);
>
> ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains,
> - data->cd->num_gains, bu27008_itimes,
> - ARRAY_SIZE(bu27008_itimes), &data->gts);
> + data->cd->num_gains, data->cd->itimes,
> + data->cd->num_itimes, &data->gts);
> if (ret)
> return ret;
>
> ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains_ir,
> - data->cd->num_gains_ir, bu27008_itimes,
> - ARRAY_SIZE(bu27008_itimes), &data->gts_ir);
> + data->cd->num_gains_ir, data->cd->itimes,
> + data->cd->num_itimes, &data->gts_ir);
I'd expect the changes to make things part specific to all be in previous patch
- not mixed across that one and this one.
> if (ret)
> return ret;
