Messages in this thread | | | Date | Sat, 26 Aug 2023 13:01:08 +0100 | Subject | Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function | From | Bryan O'Donoghue <> |
| |
On 26/08/2023 11:02, Konrad Dybcio wrote: > On 23.08.2023 12:44, Bryan O'Donoghue wrote: >> From sdm845 onwards we need to ensure the VFE is powered on prior to >> switching on the CSID. > And what's the symptom if we fail to ensure this? How can someone > adding support for another platform tell whether the match-list > should be expanded?
That person has to understand the dependency.
The first version of this patch >= SDM845 would mitigate needing to know to expand the list.
Rather than revisit that discussion, I will amend the commit log.
> >> >> 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 encoded the dependency in a helper function. >> >> Use csid_depends_vfe() for this purpose. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> .../media/platform/qcom/camss/camss-csid.c | 20 +++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c >> index 08991b070bd61..fd04ed112b564 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csid.c >> +++ b/drivers/media/platform/qcom/camss/camss-csid.c >> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid) >> return 0; >> } >> >> +static bool csid_depends_vfe(u32 version) > toggle_vfe_before_csid?
If that's clearer np.
>> +{ >> + bool ret = false; >> + >> + switch (version) { >> + case CAMSS_845: >> + case CAMSS_8250: >> + ret = true; >> + break; >> + default: >> + break; >> + } >> + >> + return ret; > I'm not sure if it would be okay with like C conventions and > stuff, but this can be made shorter by returning from within > the switch statement
Yes but you still need the explicit return at the end of the function or from memory at least some of the compiler/static analysis or checkpatch stuff - I forget which - will complain.
--- bod
| |