Re: [PATCH 05/13] media: qcom: camss: csiphy-3ph: Move CSIPHY variables to data field inside csiphy struct

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

On 8/12/24 17:41, Depeng Shao wrote:
> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> A .data field in the csiphy device structure allows us to extend out the
> register layout of the three phase capable CSIPHY layer.
> 
> Move the existing lane configuration structure to an encapsulating
> structure -> struct csiphy_device_regs which is derived from the .data
> field populated at PHY init time, as opposed to calculated at lane
> configuration.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   .../qcom/camss/camss-csiphy-3ph-1-0.c         | 55 ++++++++++++-------
>   .../media/platform/qcom/camss/camss-csiphy.h  |  1 +
>   2 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index b60c32a195df..93782ebfe0ea 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -63,6 +63,11 @@ struct csiphy_lane_regs {
>   	u32 csiphy_param_type;
>   };
>   
> +struct csiphy_device_regs {
> +	const struct csiphy_lane_regs *lane_regs;
> +	int lane_array_size;
> +};
> +
>   /* GEN2 1.0 2PH */
>   static const struct
>   csiphy_lane_regs lane_regs_sdm845[] = {
> @@ -470,28 +475,11 @@ static void csiphy_gen1_config_lanes(struct csiphy_device *csiphy,
>   static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>   				     u8 settle_cnt)
>   {
> -	const struct csiphy_lane_regs *r;
> -	int i, array_size;
> +	struct csiphy_device_regs *csiphy_regs = csiphy->data;
> +	const struct csiphy_lane_regs *r = csiphy_regs->lane_regs;
> +	int i, array_size = csiphy_regs->lane_array_size;
>   	u32 val;
>   
> -	switch (csiphy->camss->res->version) {
> -	case CAMSS_845:
> -		r = &lane_regs_sdm845[0];
> -		array_size = ARRAY_SIZE(lane_regs_sdm845);
> -		break;
> -	case CAMSS_8250:
> -		r = &lane_regs_sm8250[0];
> -		array_size = ARRAY_SIZE(lane_regs_sm8250);
> -		break;
> -	case CAMSS_8280XP:
> -		r = &lane_regs_sc8280xp[0];
> -		array_size = ARRAY_SIZE(lane_regs_sc8280xp);
> -		break;
> -	default:
> -		WARN(1, "unknown cspi version\n");
> -		return;
> -	}
> -
>   	for (i = 0; i < array_size; i++, r++) {
>   		switch (r->csiphy_param_type) {
>   		case CSIPHY_SETTLE_CNT_LOWER_BYTE:
> @@ -583,6 +571,33 @@ static void csiphy_lanes_disable(struct csiphy_device *csiphy,
>   
>   static int csiphy_init(struct csiphy_device *csiphy)
>   {
> +	struct device *dev = csiphy->camss->dev;
> +	struct csiphy_device_regs *regs;
> +
> +	regs = devm_kmalloc(dev, sizeof(*regs), GFP_KERNEL);
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	csiphy->data = regs;
> +
> +	switch (csiphy->camss->res->version) {
> +	case CAMSS_845:
> +		regs->lane_regs = &lane_regs_sdm845[0];
> +		regs->lane_array_size = ARRAY_SIZE(lane_regs_sdm845);
> +		break;
> +	case CAMSS_8250:
> +		regs->lane_regs = &lane_regs_sm8250[0];
> +		regs->lane_array_size = ARRAY_SIZE(lane_regs_sm8250);
> +		break;
> +	case CAMSS_8280XP:
> +		regs->lane_regs = &lane_regs_sc8280xp[0];
> +		regs->lane_array_size = ARRAY_SIZE(lane_regs_sc8280xp);
> +		break;
> +	default:
> +		WARN(1, "unknown csiphy version\n");
> +		return -ENODEV;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index bdf9a9c8bacc..cac1f800b7d8 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -95,6 +95,7 @@ struct csiphy_device {
>   	struct csiphy_config cfg;
>   	struct v4l2_mbus_framefmt fmt[MSM_CSIPHY_PADS_NUM];
>   	const struct csiphy_subdev_resources *res;
> +	void *data;

I would suggest to make the type/name above explicit:

struct csiphy_device_regs *regs;

>   };
>   
>   struct camss_subdev_resources;

--
Best wishes,
Vladimir