Thanks for the reviews Jonathan!
I am a bit of overloaded right now so it may be reworking this series
will be postponed. Let's see. I will in any case take your feedback with
me and come back with the V2 of this series - later if not sooner :)
On 6/17/23 22:48, Jonathan Cameron wrote:
On Tue, 13 Jun 2023 13:20:07 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
The ROHM BU27010 RGB + flickering sensor is in many regards similar to
the BU27008. Prepare for adding support for BU27010 by allowing
chip-specific properties to be brought from the of_device_id data.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
A few things inline - including some commented out code you missed
when tidying up before sending.
Ouch. I must've done some of the tidying in latter patches. I'll do the
necessary cleanups and re-spin.
Jonathan
---
drivers/iio/light/rohm-bu27008.c | 185 +++++++++++++++++++++++--------
1 file changed, 138 insertions(+), 47 deletions(-)
diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
index b50bf8973d9a..8c7f6f20a523 100644
--- a/drivers/iio/light/rohm-bu27008.c
+++ b/drivers/iio/light/rohm-bu27008.c
@@ -211,7 +211,33 @@ static const struct iio_chan_spec bu27008_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS),
};
+struct bu27008_data;
+
+struct bu27_chip_data {
+ const char *name;
+ int (*chip_init)(struct bu27008_data *data);
+ int (*get_gain_sel)(struct bu27008_data *data, int *sel);
+ int (*write_gain_sel)(struct bu27008_data *data, int sel);
+ const struct regmap_config *regmap_cfg;
+ const struct iio_gain_sel_pair *gains;
+ const struct iio_gain_sel_pair *gains_ir;
+ int num_gains;
+ int num_gains_ir;
+ int scale1x;
+
+ int drdy_en_reg;
+ int drdy_en_mask;
+ int meas_en_reg;
+ int meas_en_mask;
+ int valid_reg;
+ int chan_sel_reg;
+ int chan_sel_mask;
+ int int_time_mask;
+ u8 part_id;
+};
+
struct bu27008_data {
+ const struct bu27_chip_data *cd;
struct regmap *regmap;
struct iio_trigger *trig;
struct device *dev;
@@ -282,6 +308,32 @@ static const struct regmap_config bu27008_regmap = {
.disable_locking = true,
};
+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 const struct bu27_chip_data bu27008_chip = {
+ .name = "bu27008",
+ .chip_init = bu27008_chip_init,
+ .scale1x = BU27008_SCALE_1X,
I'd keep this in same order as the definition unless there is a
strong reason for a different ordering (perhaps the structure
is ordered for packing purposes or something like that
I tried avoid adding much of padding. Didn't go through the embedded
structs to see alignment though.
I don't think this is a strong reason though. I don't expect many copies
of these structs being instantiated.
and assigning
can be done in an order that groups things better?)
Yes. I do like having some grouping there.
Cost of out of order is that it's hard to check if everything is assigned.
I'll revise the order. Thanks for pointing this out.
+ .get_gain_sel = bu27008_get_gain_sel,
+ .write_gain_sel = bu27008_write_gain_sel,
+ .part_id = BU27008_ID,
+ .regmap_cfg = &bu27008_regmap,
+ .drdy_en_reg = BU27008_REG_MODE_CONTROL3,
+ .drdy_en_mask = BU27008_MASK_INT_EN,
+ .valid_reg = BU27008_REG_MODE_CONTROL3,
+ .meas_en_reg = BU27008_REG_MODE_CONTROL3,
+ .meas_en_mask = BU27008_MASK_MEAS_EN,
+ .chan_sel_reg = BU27008_REG_MODE_CONTROL3,
+ .chan_sel_mask = BU27008_MASK_CHAN_SEL,
+ .int_time_mask = BU27008_MASK_MEAS_MODE,
+ .gains = &bu27008_gains[0],
+ .num_gains = ARRAY_SIZE(bu27008_gains),
+ .gains_ir = &bu27008_gains_ir[0],
+ .num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
+};
Could you move this down to below all the callbacks so that no need for forward
definitions of the functions?
Well, I will see how it works. I think there were some dependency -
chip_info is probably embedded in struct bu27008_data - which is needed
in these functions - but I'll check this.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
