diff mbox series

[v2,03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

Message ID 20230911-topic-mars-v2-3-3dac84b88c4b@linaro.org (mailing list archive)
State Superseded
Delegated to: Stanimir Varbanov
Headers
Series Venus cleanups |

Commit Message

Konrad Dybcio Feb. 9, 2024, 9:09 p.m. UTC 
  To make it easier to understand the various clock requirements within
this driver, add kerneldoc to venus_clk_get() explaining the fluff.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Bryan O'Donoghue March 6, 2024, 12:20 p.m. UTC | #1 
On 09/02/2024 21:09, Konrad Dybcio wrote:
> To make it easier to understand the various clock requirements within
> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index ac7c83404c6e..ea0a7d4601e2 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -23,6 +23,34 @@
>   
>   static bool legacy_binding;
>   
> +/**
> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec

Get non-codec Venus clocks.

> + * @core: A pointer to the venus core resource
> + *
> + * The Venus block (depending on the generation) can be split into a couple
> + * of clock domains: one for "main logic" and one for each video core (0-2pcs).

(0-2pcs) is hard for me to decode => zero to two parts?

Why are we double quoting "main logic" feels a bit "Dr Evil"

Suggest hyphenating which would do the same thing:

'one clock for the core-logic||main-logic'

> + *
> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
> + * domain, so this function is the only kind if clk_get necessary there.
> + *
> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
> + * statically proclaimed a decoder and core1 an encoder, with both having
> + * their own clock domains.

"statically defined" not "statically proclaimed"

> + *
> + * SDM845 features two video cores, each one of which may or may not be
> + * subdivided into 2 enc/dec threads.

"into two encoder/decoder threads."


> + *
> + * Other SoCs either feature a single video core (with its own clock domain)
> + * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases
> + * we treat it the same (CVP only happens to live near-by Venus on the SoC).

One not 1

> + *
> + * Due to unfortunate developments in the past, we have to support bindings
> + * (MSM8996, SDM660, SDM845) that require specifying the clocks and
> + * power-domains associated with a video core domain in a bogus subnode,
> + * which means that additional fluff is necessary..

"We need to support legacy bindings"

"sub-node"

> + *
> + * Return: 0 on success, negative errno on failure.
> + */
>   static int venus_clks_get(struct venus_core *core)
>   {
>   	const struct venus_resources *res = core->res;
> 

With that fixed.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Konrad Dybcio March 26, 2024, 9:23 p.m. UTC | #2 
On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
> On 09/02/2024 21:09, Konrad Dybcio wrote:
>> To make it easier to understand the various clock requirements within
>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index ac7c83404c6e..ea0a7d4601e2 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -23,6 +23,34 @@
>>     static bool legacy_binding;
>>   +/**
>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
> 
> Get non-codec Venus clocks.

No, this is not necessarily the case.. these may still include
vcodec clocks, that are specified under the root venus node (which
is the only way we'd like to keep)

I applied the rest of your suggestions, do I keep your rb?

Konrad
Bryan O'Donoghue March 27, 2024, 9:55 a.m. UTC | #3 
On 26/03/2024 21:23, Konrad Dybcio wrote:
> On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
>> On 09/02/2024 21:09, Konrad Dybcio wrote:
>>> To make it easier to understand the various clock requirements within
>>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> index ac7c83404c6e..ea0a7d4601e2 100644
>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> @@ -23,6 +23,34 @@
>>>      static bool legacy_binding;
>>>    +/**
>>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>>
>> Get non-codec Venus clocks.
> 
> No, this is not necessarily the case.. these may still include
> vcodec clocks, that are specified under the root venus node (which
> is the only way we'd like to keep)
> 
> I applied the rest of your suggestions, do I keep your rb?
> 
> Konrad
> 

Sure

BTW, I plan to test this series when I can - do you have a working tree ?

---
bod
Konrad Dybcio March 27, 2024, 5:23 p.m. UTC | #4 
On 27.03.2024 10:55 AM, Bryan O'Donoghue wrote:
> On 26/03/2024 21:23, Konrad Dybcio wrote:
>> On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
>>> On 09/02/2024 21:09, Konrad Dybcio wrote:
>>>> To make it easier to understand the various clock requirements within
>>>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> index ac7c83404c6e..ea0a7d4601e2 100644
>>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> @@ -23,6 +23,34 @@
>>>>      static bool legacy_binding;
>>>>    +/**
>>>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>>>
>>> Get non-codec Venus clocks.
>>
>> No, this is not necessarily the case.. these may still include
>> vcodec clocks, that are specified under the root venus node (which
>> is the only way we'd like to keep)
>>
>> I applied the rest of your suggestions, do I keep your rb?
>>
>> Konrad
>>
> 
> Sure
> 
> BTW, I plan to test this series when I can - do you have a working tree ?

next + the patchset in question is a working tree..

Konrad
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ac7c83404c6e..ea0a7d4601e2 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,6 +23,34 @@ 
 
 static bool legacy_binding;
 
+/**
+ * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
+ * @core: A pointer to the venus core resource
+ *
+ * The Venus block (depending on the generation) can be split into a couple
+ * of clock domains: one for "main logic" and one for each video core (0-2pcs).
+ *
+ * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
+ * domain, so this function is the only kind if clk_get necessary there.
+ *
+ * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
+ * statically proclaimed a decoder and core1 an encoder, with both having
+ * their own clock domains.
+ *
+ * SDM845 features two video cores, each one of which may or may not be
+ * subdivided into 2 enc/dec threads.
+ *
+ * Other SoCs either feature a single video core (with its own clock domain)
+ * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases
+ * we treat it the same (CVP only happens to live near-by Venus on the SoC).
+ *
+ * Due to unfortunate developments in the past, we have to support bindings
+ * (MSM8996, SDM660, SDM845) that require specifying the clocks and
+ * power-domains associated with a video core domain in a bogus subnode,
+ * which means that additional fluff is necessary..
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
 static int venus_clks_get(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;