[PATCH 2/2] Documentation: cs35l41: Shared boost properties

Lucas Tanure posted 2 patches 2 years, 7 months ago
There is a newer version of this series
[PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by Lucas Tanure 2 years, 7 months ago
Describe the properties used for shared boost
configuration.

Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
---
 .../devicetree/bindings/sound/cirrus,cs35l41.yaml     | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
index 18fb471aa891..6f5f01bec6f1 100644
--- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
+++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
@@ -85,11 +85,20 @@ properties:
       boost-cap-microfarad.
       External Boost must have GPIO1 as GPIO output. GPIO1 will be set high to
       enable boost voltage.
+      Shared boost allows two amplifiers to share a single boost circuit by
+      communicating on the MDSYNC bus. The passive amplifier does not control
+      the boost and receives data from the active amplifier. GPIO1 should be
+      configured for Sync when shared boost is used. Shared boost is not
+      compatible with External boost. Active amplifier requires
+      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
       0 = Internal Boost
       1 = External Boost
+      2 = Reserved
+      3 = Shared Boost Active
+      4 = Shared Boost Passive
     $ref: /schemas/types.yaml#/definitions/uint32
     minimum: 0
-    maximum: 1
+    maximum: 4
 
   cirrus,gpio1-polarity-invert:
     description:
-- 
2.39.1
Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by Krzysztof Kozlowski 2 years, 7 months ago
On 07/02/2023 11:40, Lucas Tanure wrote:
> Describe the properties used for shared boost
> configuration.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

> 
> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
> ---
>  .../devicetree/bindings/sound/cirrus,cs35l41.yaml     | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> index 18fb471aa891..6f5f01bec6f1 100644
> --- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> @@ -85,11 +85,20 @@ properties:
>        boost-cap-microfarad.
>        External Boost must have GPIO1 as GPIO output. GPIO1 will be set high to
>        enable boost voltage.
> +      Shared boost allows two amplifiers to share a single boost circuit by
> +      communicating on the MDSYNC bus. The passive amplifier does not control
> +      the boost and receives data from the active amplifier. GPIO1 should be
> +      configured for Sync when shared boost is used. Shared boost is not
> +      compatible with External boost. Active amplifier requires
> +      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
>        0 = Internal Boost
>        1 = External Boost
> +      2 = Reserved

How binding can be reserved? For what and why? Drop. 2 is shared active,
3 is shared passive.

Best regards,
Krzysztof
Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by Lucas Tanure 2 years, 7 months ago
On 07-02-2023 10:42, Krzysztof Kozlowski wrote:
> On 07/02/2023 11:40, Lucas Tanure wrote:
>> Describe the properties used for shared boost
>> configuration.
> 
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
ack
> 
>>
>> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
>> ---
>>   .../devicetree/bindings/sound/cirrus,cs35l41.yaml     | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
>> index 18fb471aa891..6f5f01bec6f1 100644
>> --- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
>> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
>> @@ -85,11 +85,20 @@ properties:
>>         boost-cap-microfarad.
>>         External Boost must have GPIO1 as GPIO output. GPIO1 will be set high to
>>         enable boost voltage.
>> +      Shared boost allows two amplifiers to share a single boost circuit by
>> +      communicating on the MDSYNC bus. The passive amplifier does not control
>> +      the boost and receives data from the active amplifier. GPIO1 should be
>> +      configured for Sync when shared boost is used. Shared boost is not
>> +      compatible with External boost. Active amplifier requires
>> +      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
>>         0 = Internal Boost
>>         1 = External Boost
>> +      2 = Reserved
> 
> How binding can be reserved? For what and why? Drop. 2 is shared active,
> 3 is shared passive.
2 Is shared boost without VSPK switch, a mode not supported for new 
system designs. But there is laptops using it, so we need to keep 
supporting in the driver.

> 
> Best regards,
> Krzysztof
>
Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by Krzysztof Kozlowski 2 years, 7 months ago
On 07/02/2023 16:46, Lucas Tanure wrote:
>>> +      Shared boost allows two amplifiers to share a single boost circuit by
>>> +      communicating on the MDSYNC bus. The passive amplifier does not control
>>> +      the boost and receives data from the active amplifier. GPIO1 should be
>>> +      configured for Sync when shared boost is used. Shared boost is not
>>> +      compatible with External boost. Active amplifier requires
>>> +      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
>>>         0 = Internal Boost
>>>         1 = External Boost
>>> +      2 = Reserved
>>
>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>> 3 is shared passive.
> 2 Is shared boost without VSPK switch, a mode not supported for new 
> system designs. But there is laptops using it, so we need to keep 
> supporting in the driver.

That's not the answer. 2 is nothing here, so it cannot be reserved.
Aren't you mixing now some register value with bindings?

Best regards,
Krzysztof
Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by Lucas Tanure 2 years, 7 months ago
On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
> On 07/02/2023 16:46, Lucas Tanure wrote:
>>>> +      Shared boost allows two amplifiers to share a single boost circuit by
>>>> +      communicating on the MDSYNC bus. The passive amplifier does not control
>>>> +      the boost and receives data from the active amplifier. GPIO1 should be
>>>> +      configured for Sync when shared boost is used. Shared boost is not
>>>> +      compatible with External boost. Active amplifier requires
>>>> +      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
>>>>          0 = Internal Boost
>>>>          1 = External Boost
>>>> +      2 = Reserved
>>>
>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>>> 3 is shared passive.
>> 2 Is shared boost without VSPK switch, a mode not supported for new
>> system designs. But there is laptops using it, so we need to keep
>> supporting in the driver.
> 
> That's not the answer. 2 is nothing here, so it cannot be reserved.
> Aren't you mixing now some register value with bindings?
> 
> Best regards,
> Krzysztof
> 
> 
I have added a new patch with propper documentation.
And I would like to use 3 and 4 for shared boost as 
CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the 
current driver.
The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the 
property "cirrus,boost-type", but to make everything consistent I would 
prefer to use 3 and 4 for the new boost types.
Is that ok with you?

Thanks
Lucas
Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by Krzysztof Kozlowski 2 years, 7 months ago
On 07/02/2023 17:34, Lucas Tanure wrote:
> On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
>> On 07/02/2023 16:46, Lucas Tanure wrote:
>>>>> +      Shared boost allows two amplifiers to share a single boost circuit by
>>>>> +      communicating on the MDSYNC bus. The passive amplifier does not control
>>>>> +      the boost and receives data from the active amplifier. GPIO1 should be
>>>>> +      configured for Sync when shared boost is used. Shared boost is not
>>>>> +      compatible with External boost. Active amplifier requires
>>>>> +      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
>>>>>          0 = Internal Boost
>>>>>          1 = External Boost
>>>>> +      2 = Reserved
>>>>
>>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>>>> 3 is shared passive.
>>> 2 Is shared boost without VSPK switch, a mode not supported for new
>>> system designs. But there is laptops using it, so we need to keep
>>> supporting in the driver.
>>
>> That's not the answer. 2 is nothing here, so it cannot be reserved.
>> Aren't you mixing now some register value with bindings?
>>
>> Best regards,
>> Krzysztof
>>
>>
> I have added a new patch with propper documentation.
> And I would like to use 3 and 4 for shared boost as 
> CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the 
> current driver.

I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.

> The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the 
> property "cirrus,boost-type", but to make everything consistent I would 
> prefer to use 3 and 4 for the new boost types.
> Is that ok with you?

I don't see how it is related. The value does not exist, so whether
laptop has that property or not, is not really related, right?

Best regards,
Krzysztof
Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by lucas.tanure@collabora.com 2 years, 7 months ago
On 2/7/23 4:48 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> On 07/02/2023 17:34, Lucas Tanure wrote:
> > On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
> >> On 07/02/2023 16:46, Lucas Tanure wrote:
> >>>>> +      Shared boost allows two amplifiers to share a single boost circuit by
> >>>>> +      communicating on the MDSYNC bus. The passive amplifier does not control
> >>>>> +      the boost and receives data from the active amplifier. GPIO1 should be
> >>>>> +      configured for Sync when shared boost is used. Shared boost is not
> >>>>> +      compatible with External boost. Active amplifier requires
> >>>>> +      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
> >>>>>           0 = Internal Boost
> >>>>>           1 = External Boost
> >>>>> +      2 = Reserved
> >>>>
> >>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
> >>>> 3 is shared passive.
> >>> 2 Is shared boost without VSPK switch, a mode not supported for new
> >>> system designs. But there is laptops using it, so we need to keep
> >>> supporting in the driver.
> >>
> >> That's not the answer. 2 is nothing here, so it cannot be reserved.
> >> Aren't you mixing now some register value with bindings?
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >>
> > I have added a new patch with propper documentation.
> > And I would like to use 3 and 4 for shared boost as
> > CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the
> > current driver.
> 
> I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.
> 
> > The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the
> > property "cirrus,boost-type", but to make everything consistent I would
> > prefer to use 3 and 4 for the new boost types.
> > Is that ok with you?
> 
> I don't see how it is related. The value does not exist, so whether
> laptop has that property or not, is not really related, right?
> 
> Best regards,
> Krzysztof
> 
> 
The value does exist in the code, but no device should have that in ACPI/DTB, so yes the value doesn't exist for ACPI/DTB purposes.
I can change CS35L41_EXT_BOOST_NO_VSPK_SWITCH to another value, like 99, and use 2 and 3 for shared boost.
I will re-submit that with v3.
Is that ok with you?

Thanks
Lucas

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
Posted by Krzysztof Kozlowski 2 years, 7 months ago
On 07/02/2023 18:03, lucas.tanure@collabora.com wrote:
> On 2/7/23 4:48 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>> On 07/02/2023 17:34, Lucas Tanure wrote:
>>> On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
>>>> On 07/02/2023 16:46, Lucas Tanure wrote:
>>>>>>> +      Shared boost allows two amplifiers to share a single boost circuit by
>>>>>>> +      communicating on the MDSYNC bus. The passive amplifier does not control
>>>>>>> +      the boost and receives data from the active amplifier. GPIO1 should be
>>>>>>> +      configured for Sync when shared boost is used. Shared boost is not
>>>>>>> +      compatible with External boost. Active amplifier requires
>>>>>>> +      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
>>>>>>>           0 = Internal Boost
>>>>>>>           1 = External Boost
>>>>>>> +      2 = Reserved
>>>>>>
>>>>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>>>>>> 3 is shared passive.
>>>>> 2 Is shared boost without VSPK switch, a mode not supported for new
>>>>> system designs. But there is laptops using it, so we need to keep
>>>>> supporting in the driver.
>>>>
>>>> That's not the answer. 2 is nothing here, so it cannot be reserved.
>>>> Aren't you mixing now some register value with bindings?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>>
>>> I have added a new patch with propper documentation.
>>> And I would like to use 3 and 4 for shared boost as
>>> CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the
>>> current driver.
>>
>> I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.
>>
>>> The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the
>>> property "cirrus,boost-type", but to make everything consistent I would
>>> prefer to use 3 and 4 for the new boost types.
>>> Is that ok with you?
>>
>> I don't see how it is related. The value does not exist, so whether
>> laptop has that property or not, is not really related, right?
>>
>> Best regards,
>> Krzysztof
>>
>>
> The value does exist in the code, but no device should have that in ACPI/DTB, so yes the value doesn't exist for ACPI/DTB purposes.
> I can change CS35L41_EXT_BOOST_NO_VSPK_SWITCH to another value, like 99, and use 2 and 3 for shared boost.
> I will re-submit that with v3.
> Is that ok with you?

I guess we still talk about different things. The code does not have a
binding for the boost, therefore it does not use boost binding. Whatever
it does with CS35L41_EXT_BOOST_NO_VSPK_SWITCH outside of DT, is not my
topic and I don't care.

That's why I asked folks to use strings for such enumerations, not
register values - to avoid any confusion between the code and bindings
(and also make it more readable for humans).

Best regards,
Krzysztof