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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@gmail.com> 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@gmail.com>
> 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! ~~