* 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 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 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
[parent not found: <20230207104021.2842-2-lucas.tanure@collabora.com>]
* 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 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 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