From: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
To: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>,
rfoss@kernel.org, todor.too@gmail.com,
bryan.odonoghue@linaro.org, andersson@kernel.org,
konrad.dybcio@linaro.org, mchehab@kernel.org
Cc: linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org, laurent.pinchart@ideasonboard.com,
hverkuil-cisco@xs4all.nl, quic_hariramp@quicinc.com
Subject: Re: [PATCH v3 8/8] media: qcom: camss: Decouple VFE from CSID
Date: Mon, 13 May 2024 18:58:36 +0300 [thread overview]
Message-ID: <adbd0eeb-62c0-46a3-9cbb-92f6fde6c152@linaro.org> (raw)
In-Reply-To: <20240411124543.199-9-quic_grosikop@quicinc.com>
On 4/11/24 15:45, Gjorgji Rosikopulos wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>
> Decouple the direct calls to VFE's vfe_get/put in the CSID subdev
> in order to prepare for the introduction of IFE subdev.
>
> Also decouple CSID base address from VFE since on the Titan platform
> CSID register base address resides within VFE's base address.
>
> Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
> Signed-off-by: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>
> ---
> .../media/platform/qcom/camss/camss-csid.c | 16 +++--
> .../media/platform/qcom/camss/camss-csid.h | 1 +
> drivers/media/platform/qcom/camss/camss.c | 69 +++++++++++++++++++
> drivers/media/platform/qcom/camss/camss.h | 8 +++
> 4 files changed, 89 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 5b23f5b8746d..858db5d4ca75 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -602,7 +602,6 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
> struct csid_device *csid = v4l2_get_subdevdata(sd);
> struct camss *camss = csid->camss;
> struct device *dev = camss->dev;
> - struct vfe_device *vfe = &camss->vfe[csid->id];
> int ret = 0;
>
> if (on) {
> @@ -611,7 +610,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
> * switching on the CSID. Do so unconditionally, as there is no
> * drawback in following the same powering order on older SoCs.
> */
> - ret = vfe_get(vfe);
> + ret = csid->res->parent_dev_ops->get(camss, csid->id);
> if (ret < 0)
> return ret;
>
> @@ -663,7 +662,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
> regulator_bulk_disable(csid->num_supplies,
> csid->supplies);
> pm_runtime_put_sync(dev);
> - vfe_put(vfe);
> + csid->res->parent_dev_ops->put(camss, csid->id);
> }
>
> return ret;
> @@ -1021,6 +1020,11 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
> csid->id = id;
> csid->res = &res->csid;
>
> + if (dev_WARN_ONCE(dev, !csid->res->parent_dev_ops,
Please remove/replace dev_WARN_ONCE() to a lesser dev_warn_once(), wherever it's
possible please do not use/introduce WARN() type of writes to the kernel log buffer...
> + "Error: CSID depends on VFE/IFE device ops!\n")) {
> + return -EINVAL;
> + }
> +
> csid->res->hw_ops->subdev_init(csid);
>
> /* Memory */
> @@ -1031,9 +1035,11 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
> * VFE to be initialized before CSID
> */
> if (id >= 2) /* VFE/CSID lite */
> - csid->base = camss->vfe[id].base + VFE_480_LITE_CSID_OFFSET;
> + csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
> + + VFE_480_LITE_CSID_OFFSET;
> else
> - csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
> + csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
> + + VFE_480_CSID_OFFSET;
> } else {
> csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
> if (IS_ERR(csid->base))
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index 0e385d17c250..8cdae98e4dca 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -157,6 +157,7 @@ struct csid_hw_ops {
> struct csid_subdev_resources {
> bool is_lite;
> const struct csid_hw_ops *hw_ops;
> + const struct parent_dev_ops *parent_dev_ops;
> const struct csid_formats *formats;
> };
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 37060eaa0ba5..4d625ef59cf7 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -32,6 +32,8 @@
> #define CAMSS_CLOCK_MARGIN_NUMERATOR 105
> #define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
>
> +static const struct parent_dev_ops vfe_parent_dev_ops;
> +
> static const struct camss_subdev_resources csiphy_res_8x16[] = {
> /* CSIPHY0 */
> {
> @@ -87,6 +89,7 @@ static const struct camss_subdev_resources csid_res_8x16[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_1,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_1
> }
> },
> @@ -109,6 +112,7 @@ static const struct camss_subdev_resources csid_res_8x16[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_1,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_1
> }
> },
> @@ -226,6 +230,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> },
> @@ -248,6 +253,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> },
> @@ -270,6 +276,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> },
> @@ -292,6 +299,7 @@ static const struct camss_subdev_resources csid_res_8x96[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> }
> @@ -445,6 +453,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> },
> @@ -470,6 +479,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> },
> @@ -495,6 +505,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> },
> @@ -520,6 +531,7 @@ static const struct camss_subdev_resources csid_res_660[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_4_7,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_4_7
> }
> }
> @@ -714,6 +726,7 @@ static const struct camss_subdev_resources csid_res_845[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -739,6 +752,7 @@ static const struct camss_subdev_resources csid_res_845[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -765,6 +779,7 @@ static const struct camss_subdev_resources csid_res_845[] = {
> .csid = {
> .is_lite = true,
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> }
> @@ -957,6 +972,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -974,6 +990,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> .type = CAMSS_SUBDEV_TYPE_CSID,
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -991,6 +1008,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> .csid = {
> .is_lite = true,
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1008,6 +1026,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> .csid = {
> .is_lite = true,
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> }
> @@ -1212,6 +1231,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .interrupt = { "csid0" },
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1227,6 +1247,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .interrupt = { "csid1" },
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1242,6 +1263,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .interrupt = { "csid2" },
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1257,6 +1279,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .interrupt = { "csid3" },
> .csid = {
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1272,6 +1295,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .csid = {
> .is_lite = true,
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1287,6 +1311,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .csid = {
> .is_lite = true,
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1302,6 +1327,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .csid = {
> .is_lite = true,
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> },
> @@ -1317,6 +1343,7 @@ static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> .csid = {
> .is_lite = true,
> .hw_ops = &csid_ops_gen2,
> + .parent_dev_ops = &vfe_parent_dev_ops,
> .formats = &csid_formats_gen2
> }
> }
> @@ -1661,6 +1688,48 @@ void camss_pm_domain_off(struct camss *camss, int id)
> }
> }
>
> +static int vfe_parent_dev_ops_get(struct camss *camss, int id)
> +{
> + int ret = -EINVAL;
> +
> + if (id < camss->res->vfe_num) {
if (id >= camss->res->vfe_num)
return -EINVAL;
> + struct vfe_device *vfe = &camss->vfe[id];
> +
> + ret = vfe_get(vfe);
> + }
> +
> + return ret;
> +}
> +
> +static int vfe_parent_dev_ops_put(struct camss *camss, int id)
> +{
> + if (id < camss->res->vfe_num) {
> + struct vfe_device *vfe = &camss->vfe[id];
> +
> + vfe_put(vfe);
> + }
> +
> + return 0;
> +}
> +
> +static void __iomem
> +*vfe_parent_dev_ops_get_base_address(struct camss *camss, int id)
> +{
> + if (id < camss->res->vfe_num) {
> + struct vfe_device *vfe = &camss->vfe[id];
> +
> + return vfe->base;
> + }
> +
> + return NULL;
I can find code snippets above like
if (IS_ERR(csid->base))
...
So, is it really a good idea to return NULL on error? Probably it might be better
to return a reasonable error to the caller.
> +}
> +
> +static const struct parent_dev_ops vfe_parent_dev_ops = {
> + .get = vfe_parent_dev_ops_get,
> + .put = vfe_parent_dev_ops_put,
> + .get_base_address = vfe_parent_dev_ops_get_base_address
> +};
> +
> /*
> * camss_of_parse_endpoint_node - Parse port endpoint node
> * @dev: Device
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index a5be9e872992..b3c967bcf8a9 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -143,6 +143,12 @@ struct camss_clock {
> u32 nfreqs;
> };
>
> +struct parent_dev_ops {
> + int (*get)(struct camss *camss, int id);
> + int (*put)(struct camss *camss, int id);
> + void __iomem *(*get_base_address)(struct camss *camss, int id);
> +};
> +
> void camss_add_clock_margin(u64 *rate);
> int camss_enable_clocks(int nclocks, struct camss_clock *clock,
> struct device *dev);
> @@ -153,6 +159,8 @@ s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
> int camss_get_pixel_clock(struct media_entity *entity, u64 *pixel_clock);
> int camss_pm_domain_on(struct camss *camss, int id);
> void camss_pm_domain_off(struct camss *camss, int id);
> +int camss_vfe_get(struct camss *camss, int id);
> +void camss_vfe_put(struct camss *camss, int id);
> void camss_delete(struct camss *camss);
>
> #endif /* QC_MSM_CAMSS_H */
--
Best wishes,
Vladimir
next prev parent reply other threads:[~2024-05-13 15:58 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 12:45 [PATCH v3 0/8] Move camss version related defs in to resources Gjorgji Rosikopulos
2024-04-11 12:45 ` [PATCH v3 1/8] media: qcom: camss: Add per sub-device type resources Gjorgji Rosikopulos
2024-05-10 18:26 ` Bryan O'Donoghue
2024-05-10 22:18 ` Bryan O'Donoghue
2024-05-13 9:55 ` Gjorgji Rosikopulos (Consultant)
2024-05-10 23:09 ` [PATCH v3.1] " Bryan O'Donoghue
2024-04-11 12:45 ` [PATCH v3 2/8] media: qcom: camss: Attach formats to VFE resources Gjorgji Rosikopulos
2024-05-10 18:27 ` Bryan O'Donoghue
2024-05-13 15:15 ` Vladimir Zapolskiy
2024-05-13 15:35 ` Gjorgji Rosikopulos (Consultant)
2024-05-13 17:36 ` Bryan O'Donoghue
2024-05-14 8:39 ` Gjorgji Rosikopulos (Consultant)
2024-04-11 12:45 ` [PATCH v3 3/8] media: qcom: camss: Attach formats to CSID resources Gjorgji Rosikopulos
2024-05-10 18:27 ` Bryan O'Donoghue
2024-04-11 12:45 ` [PATCH v3 4/8] media: qcom: camss: Attach formats to CSIPHY resources Gjorgji Rosikopulos
2024-05-10 18:28 ` Bryan O'Donoghue
2024-04-11 12:45 ` [PATCH v3 5/8] media: qcom: camss: Move format related functions Gjorgji Rosikopulos
2024-05-10 18:28 ` Bryan O'Donoghue
2024-05-13 15:39 ` Vladimir Zapolskiy
2024-05-13 16:52 ` Gjorgji Rosikopulos (Consultant)
2024-05-13 17:43 ` Bryan O'Donoghue
2024-04-11 12:45 ` [PATCH v3 6/8] media: qcom: camss: Split testgen, RDI and RX for CSID 170 Gjorgji Rosikopulos
2024-05-10 18:28 ` Bryan O'Donoghue
2024-04-11 12:45 ` [PATCH v3 7/8] media: qcom: camss: Decompose register and link operations Gjorgji Rosikopulos
2024-05-10 18:29 ` Bryan O'Donoghue
2024-04-11 12:45 ` [PATCH v3 8/8] media: qcom: camss: Decouple VFE from CSID Gjorgji Rosikopulos
2024-05-10 18:29 ` Bryan O'Donoghue
2024-05-13 15:58 ` Vladimir Zapolskiy [this message]
2024-05-13 16:26 ` Gjorgji Rosikopulos (Consultant)
2024-05-13 17:48 ` Bryan O'Donoghue
2024-05-10 18:33 ` [PATCH v3 0/8] Move camss version related defs in to resources Bryan O'Donoghue
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=adbd0eeb-62c0-46a3-9cbb-92f6fde6c152@linaro.org \
--to=vladimir.zapolskiy@linaro.org \
--cc=andersson@kernel.org \
--cc=bryan.odonoghue@linaro.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=konrad.dybcio@linaro.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=quic_grosikop@quicinc.com \
--cc=quic_hariramp@quicinc.com \
--cc=rfoss@kernel.org \
--cc=todor.too@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox