Re: [PATCH 2/3] iio: light: bu27008: add chip info

[Matti Vaittinen]

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