| Message ID | 20230817143812.677554-8-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand |
On 17.08.2023 16:38, Bryan O'Donoghue wrote: > From sdm845 onwards we need to ensure the VFE is powered on prior to > switching on the CSID. > > Alternatively we could model up the GDSCs and clocks the CSID needs > without the VFE but, there's a real question of the legitimacy of such a > use-case. > > For now drawing a line at sdm845 and switching on the associated VFEs is > a perfectly valid thing to do. > > Rather than continually extend out this clause for at least two new SoCs > with this same model - making the vfe_get/vfe_put path start to look > like spaghetti we can simply test for >= sdm845 here. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- Using >= here is veeery arbitrary and depends on the next person adding a SoC in chronological, or used-tech-chronological order correctly.. Not a fan! Konrad
On 18.08.2023 14:28, Konrad Dybcio wrote: > On 17.08.2023 16:38, Bryan O'Donoghue wrote: >> From sdm845 onwards we need to ensure the VFE is powered on prior to >> switching on the CSID. >> >> Alternatively we could model up the GDSCs and clocks the CSID needs >> without the VFE but, there's a real question of the legitimacy of such a >> use-case. >> >> For now drawing a line at sdm845 and switching on the associated VFEs is >> a perfectly valid thing to do. >> >> Rather than continually extend out this clause for at least two new SoCs >> with this same model - making the vfe_get/vfe_put path start to look >> like spaghetti we can simply test for >= sdm845 here. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- > Using >= here is veeery arbitrary and depends on the next person > adding a SoC in chronological, or used-tech-chronological order > correctly.. Not a fan! Perhaps some sort of a compatible-bound flag would be better suited Konrad
On 18/08/2023 13:29, Konrad Dybcio wrote: > On 18.08.2023 14:28, Konrad Dybcio wrote: >> On 17.08.2023 16:38, Bryan O'Donoghue wrote: >>> From sdm845 onwards we need to ensure the VFE is powered on prior to >>> switching on the CSID. >>> >>> Alternatively we could model up the GDSCs and clocks the CSID needs >>> without the VFE but, there's a real question of the legitimacy of such a >>> use-case. >>> >>> For now drawing a line at sdm845 and switching on the associated VFEs is >>> a perfectly valid thing to do. >>> >>> Rather than continually extend out this clause for at least two new SoCs >>> with this same model - making the vfe_get/vfe_put path start to look >>> like spaghetti we can simply test for >= sdm845 here. >>> >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> --- >> Using >= here is veeery arbitrary and depends on the next person >> adding a SoC in chronological, or used-tech-chronological order >> correctly.. Not a fan! > > Perhaps some sort of a compatible-bound flag would be better suited > > Konrad I take the point. I'll look at a macro or a helper function if (csid_within_vfe(version)) {} That way there's just one source of truth and the chronology is irrelevant. --- bod
diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c index 08991b070bd61..7ff450039ec3f 100644 --- a/drivers/media/platform/qcom/camss/camss-csid.c +++ b/drivers/media/platform/qcom/camss/camss-csid.c @@ -163,7 +163,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on) int ret = 0; if (on) { - if (version == CAMSS_8250 || version == CAMSS_845) { + if (version >= CAMSS_845) { ret = vfe_get(vfe); if (ret < 0) return ret; @@ -217,7 +217,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); - if (version == CAMSS_8250 || version == CAMSS_845) + if (version >= CAMSS_845) vfe_put(vfe); }
From sdm845 onwards we need to ensure the VFE is powered on prior to switching on the CSID. Alternatively we could model up the GDSCs and clocks the CSID needs without the VFE but, there's a real question of the legitimacy of such a use-case. For now drawing a line at sdm845 and switching on the associated VFEs is a perfectly valid thing to do. Rather than continually extend out this clause for at least two new SoCs with this same model - making the vfe_get/vfe_put path start to look like spaghetti we can simply test for >= sdm845 here. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/media/platform/qcom/camss/camss-csid.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)