| Message ID | 20240411124543.199-9-quic_grosikop@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Move camss version related defs in to resources | expand |
On 11/04/2024 13: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> > --- Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp/sm8250/sdm845/apq8016
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
Hi Vladimir, Thanks for the review, On 5/13/2024 6:58 PM, Vladimir Zapolskiy wrote: > 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... The error is fatal and driver probe will fail if this happens, it is good to have it in kernel log buffer. However i agree it can be changed to dev_warn_once. > >> + "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; :-). I believe this is metter of personal taste. I also like the code which you have posted. But with function of this size i dont see that it will make any difference. > >> + 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. As general rule i agree. But here either we have address or not, i dont see the reason to return an error code. Also i dont see what caller will do if he gets error code instead of NULL. I am refering in particular this case. If we have different error paths of failiure maybe it will more sense. > >> +} >> + >> +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
On 13/05/2024 17:26, Gjorgji Rosikopulos (Consultant) wrote: >>> +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. > As general rule i agree. But here either we have address or not, > i dont see the reason to return an error code. Also i dont see what > caller will do if he gets error code instead of NULL. > I am refering in particular this case. If we have different error paths > of failiure maybe it will more sense. I don't see a compelling reason to change the submitted code. I'd leave well-enough alone for v4. Please keep changes for V4 restricted to formatting/line indentation/SPDX. I don't want to have to reverify all of this code unless a bug is found. --- bod
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, + "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) { + 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; +} + +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 */