Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 17 Jun 2023 20:48:24 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] iio: light: bu27008: add chip info
Message-ID: <20230617204824.6f3d9b20@jic23-huawei>
In-Reply-To: <e898bb74afdbc9aa030932e82c36b4ce6d13bab7.1686651445.git.mazziesaccount@gmail.com>
References: <cover.1686650184.git.mazziesaccount@gmail.com>
        <e898bb74afdbc9aa030932e82c36b4ce6d13bab7.1686651445.git.mazziesaccount@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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 and assigning
can be done in an order that groups things better?)
Cost of out of order is that it's hard to check if everything is assigned.

> +	.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? 

> +
>  #define BU27008_MAX_VALID_RESULT_WAIT_US	50000
>  #define BU27008_VALID_RESULT_WAIT_QUANTA_US	1000

> -	idev->channels = bu27008_channels;
> -	idev->num_channels = ARRAY_SIZE(bu27008_channels);
> -	idev->name = "bu27008";
> +	idev->channels = /* data->cd->cspec; */ &bu27008_channels[0];
?   Seems you didn't put the channels cd

> +	idev->num_channels = /* data->cd->num_channels; */ ARRAY_SIZE(bu27008_channels);
?


> +	idev->name = data->cd->name;
>  	idev->info = &bu27008_info;
>  	idev->modes = INDIO_DIRECT_MODE;
>  	idev->available_scan_masks = bu27008_scan_masks;
>  
> -	ret = bu27008_chip_init(data);
> +	ret = data->cd->chip_init(data);
>  	if (ret)
>  		return ret;
>  
> @@ -1021,7 +1112,7 @@ static int bu27008_probe(struct i2c_client *i2c)
>  }
>  
>  static const struct of_device_id bu27008_of_match[] = {
> -	{ .compatible = "rohm,bu27008" },
> +	{ .compatible = "rohm,bu27008", .data = &bu27008_chip },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, bu27008_of_match);