* Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
[not found] ` <20230207104021.2842-3-lucas.tanure@collabora.com>
@ 2023-02-07 10:42 ` Krzysztof Kozlowski
2023-02-07 15:46 ` Lucas Tanure
0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-07 10:42 UTC (permalink / raw)
To: Lucas Tanure, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai
Cc: devicetree, alsa-devel, kernel, linux-kernel, patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature
[not found] ` <20230207104021.2842-2-lucas.tanure@collabora.com>
@ 2023-02-07 11:48 ` Charles Keepax
2023-02-07 15:49 ` Lucas Tanure
2023-02-08 11:46 ` kernel test robot
1 sibling, 1 reply; 10+ messages in thread
From: Charles Keepax @ 2023-02-07 11:48 UTC (permalink / raw)
To: Lucas Tanure
Cc: devicetree, alsa-devel, kernel, patches, Mark Brown, Takashi Iwai,
Liam Girdwood, Rob Herring, Krzysztof Kozlowski, David Rhodes,
linux-kernel
On Tue, Feb 07, 2023 at 10:40:20AM +0000, 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.
>
> Shared Boost is not supported in HDA Systems.
>
Probably would be nice to put at least a note to say based on
David's patches.
> +static const struct reg_sequence cs35l41_shd_boost_seq[] = {
> + {CS35L41_PWR_CTRL3, 0x01000110},
This will blat whatever the user set in the DRE switch.
Technically blats the CLASS H enable from the DAPM widget too,
but as that always turns on should be a no-op. Probably should
either not register the DRE switch or have setting it return an
error for these boost modes.
> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
> + struct completion *pll_lock)
> {
> int ret;
> + unsigned int gpio1;
>
> switch (b_type) {
> + case CS35L41_SHD_BOOST_ACTV:
> + case CS35L41_SHD_BOOST_PASS:
> + regmap_update_bits(regmap, CS35L41_PWR_CTRL3, CS35L41_SYNC_EN_MASK, 0);
> +
> + gpio1 = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
> + regmap_update_bits(regmap, CS35L41_GPIO_PAD_CONTROL, CS35L41_GPIO1_CTRL_MASK,
> + gpio1 << CS35L41_GPIO1_CTRL_SHIFT);
> +
> + ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
> + enable << CS35L41_GLOBAL_EN_SHIFT);
> + usleep_range(3000, 3100);
> + if (!enable)
> + break;
> +
> + if (!pll_lock)
> + return -EINVAL;
> +
> + ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> + if (ret == 0)
> + ret = -ETIMEDOUT;
This feels kinda scary, in that you are relying on a 1 to 1
correspondence between this code running and getting a PLL lock
signal. The datasheet is helpfully completely vague on when PLL
locks are triggered.
The PLL enable seems to be set through set_sysclk, which could
be called multiple times, per DAPM power up. Does the PLL
lock only go once global enable has been set? Can't help
but wonder if a reinit_completion should probably go somewhere
to ensure we are getting this lock of the PLL not a past one.
> @@ -483,6 +483,11 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
> ret = IRQ_HANDLED;
> }
>
> + if (status[2] & CS35L41_PLL_LOCK) {
> + regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS3, CS35L41_PLL_LOCK);
> + complete(&cs35l41->pll_lock);
> + }
> +
If you fall into any of the error cases in this IRQ handler above
this, it will blat values you don't want into BST_EN although, to
be fair that does look currently broken for external boost as
well.
Thanks,
Charles
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
2023-02-07 10:42 ` [PATCH 2/2] Documentation: cs35l41: Shared boost properties Krzysztof Kozlowski
@ 2023-02-07 15:46 ` Lucas Tanure
2023-02-07 16:13 ` Krzysztof Kozlowski
0 siblings, 1 reply; 10+ messages in thread
From: Lucas Tanure @ 2023-02-07 15:46 UTC (permalink / raw)
To: Krzysztof Kozlowski, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai
Cc: alsa-devel, devicetree, patches, linux-kernel, kernel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature
2023-02-07 11:48 ` [PATCH 1/2] ALSA: cs35l41: Add shared boost feature Charles Keepax
@ 2023-02-07 15:49 ` Lucas Tanure
0 siblings, 0 replies; 10+ messages in thread
From: Lucas Tanure @ 2023-02-07 15:49 UTC (permalink / raw)
To: Charles Keepax
Cc: David Rhodes, Liam Girdwood, Krzysztof Kozlowski, Mark Brown,
Rob Herring, Takashi Iwai, alsa-devel, devicetree, patches,
linux-kernel, kernel
On 07-02-2023 11:48, Charles Keepax wrote:
> On Tue, Feb 07, 2023 at 10:40:20AM +0000, 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.
>>
>> Shared Boost is not supported in HDA Systems.
>>
>
> Probably would be nice to put at least a note to say based on
> David's patches.
ack
>
>> +static const struct reg_sequence cs35l41_shd_boost_seq[] = {
>> + {CS35L41_PWR_CTRL3, 0x01000110},
>
> This will blat whatever the user set in the DRE switch.
> Technically blats the CLASS H enable from the DAPM widget too,
> but as that always turns on should be a no-op. Probably should
> either not register the DRE switch or have setting it return an
> error for these boost modes.
Fixed in v2.
Changed to regmap_update_bits.
>
>> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
>> + struct completion *pll_lock)
>> {
>> int ret;
>> + unsigned int gpio1;
>>
>> switch (b_type) {
>> + case CS35L41_SHD_BOOST_ACTV:
>> + case CS35L41_SHD_BOOST_PASS:
>> + regmap_update_bits(regmap, CS35L41_PWR_CTRL3, CS35L41_SYNC_EN_MASK, 0);
>> +
>> + gpio1 = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
>> + regmap_update_bits(regmap, CS35L41_GPIO_PAD_CONTROL, CS35L41_GPIO1_CTRL_MASK,
>> + gpio1 << CS35L41_GPIO1_CTRL_SHIFT);
>> +
>> + ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
>> + enable << CS35L41_GLOBAL_EN_SHIFT);
>> + usleep_range(3000, 3100);
>> + if (!enable)
>> + break;
>> +
>> + if (!pll_lock)
>> + return -EINVAL;
>> +
>> + ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
>> + if (ret == 0)
>> + ret = -ETIMEDOUT;
>
> This feels kinda scary, in that you are relying on a 1 to 1
> correspondence between this code running and getting a PLL lock
> signal. The datasheet is helpfully completely vague on when PLL
> locks are triggered.
>
> The PLL enable seems to be set through set_sysclk, which could
> be called multiple times, per DAPM power up. Does the PLL
> lock only go once global enable has been set? Can't help
> but wonder if a reinit_completion should probably go somewhere
> to ensure we are getting this lock of the PLL not a past one.
Added a reinit_completion at cs35l41_pcm_startup
>
>> @@ -483,6 +483,11 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
>> ret = IRQ_HANDLED;
>> }
>>
>> + if (status[2] & CS35L41_PLL_LOCK) {
>> + regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS3, CS35L41_PLL_LOCK);
>> + complete(&cs35l41->pll_lock);
>> + }
>> +
>
> If you fall into any of the error cases in this IRQ handler above
> this, it will blat values you don't want into BST_EN although, to
> be fair that does look currently broken for external boost as
> well.
Fixed with a new patch in v2 series.
>
> Thanks,
> Charles
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
2023-02-07 15:46 ` Lucas Tanure
@ 2023-02-07 16:13 ` Krzysztof Kozlowski
2023-02-07 16:34 ` Lucas Tanure
0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-07 16:13 UTC (permalink / raw)
To: Lucas Tanure, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai
Cc: alsa-devel, devicetree, patches, linux-kernel, kernel
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
_______________________________________________
Alsa-devel mailing list -- alsa-devel@alsa-project.org
To unsubscribe send an email to alsa-devel-leave@alsa-project.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
2023-02-07 16:13 ` Krzysztof Kozlowski
@ 2023-02-07 16:34 ` Lucas Tanure
2023-02-07 16:48 ` Krzysztof Kozlowski
0 siblings, 1 reply; 10+ messages in thread
From: Lucas Tanure @ 2023-02-07 16:34 UTC (permalink / raw)
To: Krzysztof Kozlowski, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai
Cc: alsa-devel, devicetree, patches, linux-kernel, kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
2023-02-07 16:34 ` Lucas Tanure
@ 2023-02-07 16:48 ` Krzysztof Kozlowski
2023-02-07 17:03 ` lucas.tanure
0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-07 16:48 UTC (permalink / raw)
To: Lucas Tanure, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai
Cc: alsa-devel, devicetree, patches, linux-kernel, kernel
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
_______________________________________________
Alsa-devel mailing list -- alsa-devel@alsa-project.org
To unsubscribe send an email to alsa-devel-leave@alsa-project.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
2023-02-07 16:48 ` Krzysztof Kozlowski
@ 2023-02-07 17:03 ` lucas.tanure
2023-02-08 10:23 ` Krzysztof Kozlowski
0 siblings, 1 reply; 10+ messages in thread
From: lucas.tanure @ 2023-02-07 17:03 UTC (permalink / raw)
To: Krzysztof Kozlowski, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai, alsa-devel, devicetree, patches, linux-kernel,
kernel
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
_______________________________________________
Alsa-devel mailing list -- alsa-devel@alsa-project.org
To unsubscribe send an email to alsa-devel-leave@alsa-project.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties
2023-02-07 17:03 ` lucas.tanure
@ 2023-02-08 10:23 ` Krzysztof Kozlowski
0 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-08 10:23 UTC (permalink / raw)
To: lucas.tanure, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai, alsa-devel, devicetree, patches, linux-kernel,
kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature
[not found] ` <20230207104021.2842-2-lucas.tanure@collabora.com>
2023-02-07 11:48 ` [PATCH 1/2] ALSA: cs35l41: Add shared boost feature Charles Keepax
@ 2023-02-08 11:46 ` kernel test robot
1 sibling, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-02-08 11:46 UTC (permalink / raw)
To: Lucas Tanure, David Rhodes, Charles Keepax, Liam Girdwood,
Krzysztof Kozlowski, Mark Brown, Rob Herring, Jaroslav Kysela,
Takashi Iwai
Cc: llvm, oe-kbuild-all, alsa-devel, devicetree, patches,
linux-kernel, kernel, Lucas Tanure
Hi Lucas,
I love your patch! Perhaps something to improve:
[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next tiwai-sound/for-linus linus/master v6.2-rc7 next-20230208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lucas-Tanure/ALSA-cs35l41-Add-shared-boost-feature/20230207-184238
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link: https://lore.kernel.org/r/20230207104021.2842-2-lucas.tanure%40collabora.com
patch subject: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature
config: i386-randconfig-a011 (https://download.01.org/0day-ci/archive/20230208/202302081911.MDwfUTfx-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c1726800667180cd46986c3578e635bafa8bf01a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Lucas-Tanure/ALSA-cs35l41-Add-shared-boost-feature/20230207-184238
git checkout c1726800667180cd46986c3578e635bafa8bf01a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash sound/soc/codecs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> sound/soc/codecs/cs35l41-lib.c:1160:7: warning: variable 'ret' is used uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
case CS35L41_SHD_BOOST_PASS:
^~~~~~~~~~~~~~~~~~~~~~
sound/soc/codecs/cs35l41-lib.c:1169:9: note: uninitialized use occurs here
return ret;
^~~
sound/soc/codecs/cs35l41-lib.c:1136:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
1 warning generated.
vim +/ret +1160 sound/soc/codecs/cs35l41-lib.c
1132
1133 int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
1134 struct cs35l41_hw_cfg *hw_cfg)
1135 {
1136 int ret;
1137
1138 switch (hw_cfg->bst_type) {
1139 case CS35L41_SHD_BOOST_ACTV:
1140 regmap_multi_reg_write(regmap, cs35l41_actv_seq, ARRAY_SIZE(cs35l41_actv_seq));
1141 fallthrough;
1142 case CS35L41_INT_BOOST:
1143 ret = cs35l41_boost_config(dev, regmap, hw_cfg->bst_ind,
1144 hw_cfg->bst_cap, hw_cfg->bst_ipk);
1145 if (ret)
1146 dev_err(dev, "Error in Boost DT config: %d\n", ret);
1147 break;
1148 case CS35L41_EXT_BOOST:
1149 case CS35L41_EXT_BOOST_NO_VSPK_SWITCH:
1150 /* Only CLSA0100 doesn't use GPIO as VSPK switch, but even on that laptop we can
1151 * toggle GPIO1 as is not connected to anything.
1152 * There will be no other device without VSPK switch.
1153 */
1154 regmap_write(regmap, CS35L41_GPIO1_CTRL1, 0x00000001);
1155 regmap_multi_reg_write(regmap, cs35l41_reset_to_safe,
1156 ARRAY_SIZE(cs35l41_reset_to_safe));
1157 ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL2, CS35L41_BST_EN_MASK,
1158 CS35L41_BST_DIS_FET_OFF << CS35L41_BST_EN_SHIFT);
1159 break;
> 1160 case CS35L41_SHD_BOOST_PASS:
1161 regmap_multi_reg_write(regmap, cs35l41_pass_seq, ARRAY_SIZE(cs35l41_pass_seq));
1162 break;
1163 default:
1164 dev_err(dev, "Boost type %d not supported\n", hw_cfg->bst_type);
1165 ret = -EINVAL;
1166 break;
1167 }
1168
1169 return ret;
1170 }
1171 EXPORT_SYMBOL_GPL(cs35l41_init_boost);
1172
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-02-08 11:48 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230207104021.2842-1-lucas.tanure@collabora.com>
[not found] ` <20230207104021.2842-3-lucas.tanure@collabora.com>
2023-02-07 10:42 ` [PATCH 2/2] Documentation: cs35l41: Shared boost properties Krzysztof Kozlowski
2023-02-07 15:46 ` Lucas Tanure
2023-02-07 16:13 ` Krzysztof Kozlowski
2023-02-07 16:34 ` Lucas Tanure
2023-02-07 16:48 ` Krzysztof Kozlowski
2023-02-07 17:03 ` lucas.tanure
2023-02-08 10:23 ` Krzysztof Kozlowski
[not found] ` <20230207104021.2842-2-lucas.tanure@collabora.com>
2023-02-07 11:48 ` [PATCH 1/2] ALSA: cs35l41: Add shared boost feature Charles Keepax
2023-02-07 15:49 ` Lucas Tanure
2023-02-08 11:46 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox