Alsa-Devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* 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