Re: [PATCH v3 02/15] media: qcom: camss: Start to move to module compat matched resources

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:31AM +0100, Bryan O'Donoghue wrote:
> There is a lot of unnecessary if/elsing in this code that arguably
> should never have made it upstream when adding a second let alone
> subsequent SoC.
> 
> I'm guilty of not fixing the mess myself when adding in the sm8250.
> Before adding in any new SoCs or resources lets take the time to cleanup
> the resource passing.
> 
> First step is to pass the generic struct camss_resources as a parameter
> per the compatible list.
> 
> Subsequent patches will address the other somewhat dispirate strutures

s/dispirate/disparate/ ?

> which we are also doing if/else on and assigning statically.
> 
> Squashed down a commit to drop useless NULL assignment for ispif resources.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> Acked-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> ---
>  drivers/media/platform/qcom/camss/camss.c | 92 ++++++++++++-----------
>  drivers/media/platform/qcom/camss/camss.h |  8 ++
>  2 files changed, 56 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index de39dc987444f..82e679c8ca011 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_domain.h>
> @@ -1120,47 +1121,13 @@ static int camss_of_parse_ports(struct camss *camss)
>   */
>  static int camss_init_subdevices(struct camss *camss)
>  {
> -	const struct resources *csiphy_res;
> -	const struct resources *csid_res;
> -	const struct resources *ispif_res;
> -	const struct resources *vfe_res;
> +	const struct camss_resources *res = camss->res;
>  	unsigned int i;
>  	int ret;
>  
> -	if (camss->version == CAMSS_8x16) {
> -		csiphy_res = csiphy_res_8x16;
> -		csid_res = csid_res_8x16;
> -		ispif_res = &ispif_res_8x16;
> -		vfe_res = vfe_res_8x16;
> -	} else if (camss->version == CAMSS_8x96) {
> -		csiphy_res = csiphy_res_8x96;
> -		csid_res = csid_res_8x96;
> -		ispif_res = &ispif_res_8x96;
> -		vfe_res = vfe_res_8x96;
> -	} else if (camss->version == CAMSS_660) {
> -		csiphy_res = csiphy_res_660;
> -		csid_res = csid_res_660;
> -		ispif_res = &ispif_res_660;
> -		vfe_res = vfe_res_660;
> -	}  else if (camss->version == CAMSS_845) {
> -		csiphy_res = csiphy_res_845;
> -		csid_res = csid_res_845;
> -		/* Titan VFEs don't have an ISPIF  */
> -		ispif_res = NULL;
> -		vfe_res = vfe_res_845;
> -	} else if (camss->version == CAMSS_8250) {
> -		csiphy_res = csiphy_res_8250;
> -		csid_res = csid_res_8250;
> -		/* Titan VFEs don't have an ISPIF  */
> -		ispif_res = NULL;
> -		vfe_res = vfe_res_8250;
> -	} else {
> -		return -EINVAL;
> -	}
> -
>  	for (i = 0; i < camss->csiphy_num; i++) {
>  		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
> -					     &csiphy_res[i], i);
> +					     &res->csiphy_res[i], i);
>  		if (ret < 0) {
>  			dev_err(camss->dev,
>  				"Failed to init csiphy%d sub-device: %d\n",
> @@ -1172,7 +1139,7 @@ static int camss_init_subdevices(struct camss *camss)
>  	/* note: SM8250 requires VFE to be initialized before CSID */
>  	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++) {
>  		ret = msm_vfe_subdev_init(camss, &camss->vfe[i],
> -					  &vfe_res[i], i);
> +					  &res->vfe_res[i], i);
>  		if (ret < 0) {
>  			dev_err(camss->dev,
>  				"Fail to init vfe%d sub-device: %d\n", i, ret);
> @@ -1182,7 +1149,7 @@ static int camss_init_subdevices(struct camss *camss)
>  
>  	for (i = 0; i < camss->csid_num; i++) {
>  		ret = msm_csid_subdev_init(camss, &camss->csid[i],
> -					   &csid_res[i], i);
> +					   &res->csid_res[i], i);
>  		if (ret < 0) {
>  			dev_err(camss->dev,
>  				"Failed to init csid%d sub-device: %d\n",
> @@ -1191,7 +1158,7 @@ static int camss_init_subdevices(struct camss *camss)
>  		}
>  	}
>  
> -	ret = msm_ispif_subdev_init(camss, ispif_res);
> +	ret = msm_ispif_subdev_init(camss, res->ispif_res);
>  	if (ret < 0) {
>  		dev_err(camss->dev, "Failed to init ispif sub-device: %d\n",
>  		ret);
> @@ -1554,6 +1521,10 @@ static int camss_probe(struct platform_device *pdev)
>  	if (!camss)
>  		return -ENOMEM;
>  
> +	camss->res = of_device_get_match_data(dev);
> +	if (!camss->res)
> +		return -ENODEV;

You could possibly drop the error check, as this can't happen.

Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

> +
>  	atomic_set(&camss->ref_count, 0);
>  	camss->dev = dev;
>  	platform_set_drvdata(pdev, camss);
> @@ -1735,12 +1706,45 @@ static void camss_remove(struct platform_device *pdev)
>  		camss_delete(camss);
>  }
>  
> +static const struct camss_resources msm8916_resources = {
> +	.csiphy_res = csiphy_res_8x16,
> +	.csid_res = csid_res_8x16,
> +	.ispif_res = &ispif_res_8x16,
> +	.vfe_res = vfe_res_8x16,
> +};
> +
> +static const struct camss_resources msm8996_resources = {
> +	.csiphy_res = csiphy_res_8x96,
> +	.csid_res = csid_res_8x96,
> +	.ispif_res = &ispif_res_8x96,
> +	.vfe_res = vfe_res_8x96,
> +};
> +
> +static const struct camss_resources sdm660_resources = {
> +	.csiphy_res = csiphy_res_660,
> +	.csid_res = csid_res_660,
> +	.ispif_res = &ispif_res_660,
> +	.vfe_res = vfe_res_660,
> +};
> +
> +static const struct camss_resources sdm845_resources = {
> +	.csiphy_res = csiphy_res_845,
> +	.csid_res = csid_res_845,
> +	.vfe_res = vfe_res_845,
> +};
> +
> +static const struct camss_resources sm8250_resources = {
> +	.csiphy_res = csiphy_res_8250,
> +	.csid_res = csid_res_8250,
> +	.vfe_res = vfe_res_8250,
> +};
> +
>  static const struct of_device_id camss_dt_match[] = {
> -	{ .compatible = "qcom,msm8916-camss" },
> -	{ .compatible = "qcom,msm8996-camss" },
> -	{ .compatible = "qcom,sdm660-camss" },
> -	{ .compatible = "qcom,sdm845-camss" },
> -	{ .compatible = "qcom,sm8250-camss" },
> +	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
> +	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
> +	{ .compatible = "qcom,sdm660-camss", .data = &sdm660_resources },
> +	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
> +	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
>  	{ }
>  };
>  
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index e95211cdb1fd6..f632ee49ad83e 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -79,6 +79,13 @@ enum icc_count {
>  	ICC_SM8250_COUNT = 4,
>  };
>  
> +struct camss_resources {
> +	const struct resources *csiphy_res;
> +	const struct resources *csid_res;
> +	const struct resources *ispif_res;
> +	const struct resources *vfe_res;
> +};
> +
>  struct camss {
>  	enum camss_version version;
>  	struct v4l2_device v4l2_dev;
> @@ -99,6 +106,7 @@ struct camss {
>  	struct device_link **genpd_link;
>  	struct icc_path *icc_path[ICC_SM8250_COUNT];
>  	struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
> +	const struct camss_resources *res;
>  };
>  
>  struct camss_camera_interface {

-- 
Regards,

Laurent Pinchart