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
