mbox series

[v3,00/16] Support external boost at CS35l41 ASoC driver

Message ID 20220308171730.454587-1-tanureal@opensource.cirrus.com
Headers show
Series Support external boost at CS35l41 ASoC driver | expand

Message

Lucas Tanure March 8, 2022, 5:17 p.m. UTC 
Move the support for CS35L41 external boost to its shared library
for ASoC use.
This move resulted in cs35l41_hda_reg_sequence being removed,
and its steps were broken down into regmap writes or functions
from the library. And hardware configuration struct was unified
for its use in the shared lib.
While at it, some minor bugs were found and fixed it.

v3 changelog:
 - Remove patches already accepted
 - Improved logic in documentation patch
 - Documentation patch goes before its code
 - Fixed missing Signed-off-by
 - Fixed subject for HDA patches

v2 changelog:
 - Instead of removing the log, playback actions will log the last regmap access.
 - Documentation patch with the correct subject line and fixed bug reported by Rob Herring on the
 provided example.

Previous versions:
 v1: https://lkml.org/lkml/2022/3/3/759
 v2: https://lkml.org/lkml/2022/3/4/743

David Rhodes (1):
  ASoC: dt-bindings: cs35l41: Document CS35l41 External Boost

Lucas Tanure (15):
  sound: cs35l41: Unify hardware configuration
  sound: cs35l41: Check hw_config before using it
  sound: cs35l41: Move cs35l41_gpio_config to shared lib
  ALSA: hda: cs35l41: Fix I2S params comments
  ALSA: hda: cs35l41: Always configure the DAI
  ALSA: hda: cs35l41: Add Boost type flag
  hda: cs35l41: Put the device into safe mode for external boost
  hda: cs35l41: Mute the device before shutdown
  sound: cs35l41: Enable Internal Boost in shared lib
  ALSA: hda: cs35l41: Move boost config to initialization code
  ALSA: hda: cs35l41: Remove cs35l41_hda_reg_sequence struct
  ALSA: hda: cs35l41: Reorganize log for playback actions
  ALSA: hda: cs35l41: Handle all external boost setups the same way
  ALSA: hda: cs35l41: Move external boost handling to lib for ASoC use
  ASoC: cs35l41: Support external boost

 .../bindings/sound/cirrus,cs35l41.yaml        |  44 ++-
 include/sound/cs35l41.h                       |  53 +++-
 sound/pci/hda/cs35l41_hda.c                   | 295 ++++++------------
 sound/pci/hda/cs35l41_hda.h                   |  27 +-
 sound/soc/codecs/cs35l41-i2c.c                |   4 +-
 sound/soc/codecs/cs35l41-lib.c                | 190 ++++++++++-
 sound/soc/codecs/cs35l41-spi.c                |   4 +-
 sound/soc/codecs/cs35l41.c                    | 166 +++++-----
 sound/soc/codecs/cs35l41.h                    |   5 +-
 9 files changed, 437 insertions(+), 351 deletions(-)

Comments

Charles Keepax March 9, 2022, 9:36 a.m. UTC | #1 
On Tue, Mar 08, 2022 at 05:17:30PM +0000, Lucas Tanure wrote:
> Add support for external boost voltage, where GPIO1 must control a
> switch to isolate CS35L41 from the external Boost Voltage
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles
Takashi Iwai March 10, 2022, 1:40 p.m. UTC | #2 
On Tue, 08 Mar 2022 18:17:17 +0100,
Lucas Tanure wrote:
> 
> ASoC and HDA can use a single function to configure the chip gpios.
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Just use "ALSA:" prefix for the subject instead of "sound:" in case
both ALSA HD-audio and ASoC are covered in the patch.


thanks,

Takashi
Takashi Iwai March 10, 2022, 1:44 p.m. UTC | #3 
On Tue, 08 Mar 2022 18:17:15 +0100,
Lucas Tanure wrote:
> +enum cs35l41_gpio_func {
> +	CS35L41_HIZ,
> +	CS35L41_GPIO,
> +	CS35L41_INT_OPEN_DRAIN_GPIO2,
> +	CS35L41_MCLK,
> +	CS35L41_INT_PUSH_PULL_LOW_GPIO2,
> +	CS35L41_INT_PUSH_PULL_HIGH_GPIO2,
> +	CS35L41_PDM_CLK_GPIO2,
> +	CS35L41_PDM_DATA_GPIO2,
> +	CS35L41_MDSYNC_GPIO1 = 2,
> +	CS35L41_PDM_CLK_GPIO1 = 4,
> +	CS35L41_PDM_DATA_GPIO1 = 5,
>  };

So CS35L41_MDSYNC_GPIO1 is identical with *_DRAIN_GPIO2, i.e. it's an
alias?


thanks,

Takashi
Lucas Tanure March 10, 2022, 4:40 p.m. UTC | #4 
On 3/10/22 1:44 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 08 Mar 2022 18:17:15 +0100,
> Lucas Tanure wrote:
> > +enum cs35l41_gpio_func {
> > +	CS35L41_HIZ,
> > +	CS35L41_GPIO,
> > +	CS35L41_INT_OPEN_DRAIN_GPIO2,
> > +	CS35L41_MCLK,
> > +	CS35L41_INT_PUSH_PULL_LOW_GPIO2,
> > +	CS35L41_INT_PUSH_PULL_HIGH_GPIO2,
> > +	CS35L41_PDM_CLK_GPIO2,
> > +	CS35L41_PDM_DATA_GPIO2,
> > +	CS35L41_MDSYNC_GPIO1 = 2,
> > +	CS35L41_PDM_CLK_GPIO1 = 4,
> > +	CS35L41_PDM_DATA_GPIO1 = 5,
> >   };
> 
> So CS35L41_MDSYNC_GPIO1 is identical with *_DRAIN_GPIO2, i.e. it's an
> alias?
> 
> 
> thanks,
> 
> Takashi
> 

The value 2 sets GPIO1 as MDSYNC pin and GPIO2 as IRQ pin (Open drain).
It could be the same label, like CS35L41_GPIO1_MDSYNC_GPIO2_IRQ, but I think separating them is better for understanding the code and organizing the code.
Takashi Iwai March 10, 2022, 4:43 p.m. UTC | #5 
On Thu, 10 Mar 2022 17:40:45 +0100,
<tanureal@opensource.cirrus.com> wrote:
> 
> On 3/10/22 1:44 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Tue, 08 Mar 2022 18:17:15 +0100,
> > Lucas Tanure wrote:
> > > +enum cs35l41_gpio_func {
> > > +	CS35L41_HIZ,
> > > +	CS35L41_GPIO,
> > > +	CS35L41_INT_OPEN_DRAIN_GPIO2,
> > > +	CS35L41_MCLK,
> > > +	CS35L41_INT_PUSH_PULL_LOW_GPIO2,
> > > +	CS35L41_INT_PUSH_PULL_HIGH_GPIO2,
> > > +	CS35L41_PDM_CLK_GPIO2,
> > > +	CS35L41_PDM_DATA_GPIO2,
> > > +	CS35L41_MDSYNC_GPIO1 = 2,
> > > +	CS35L41_PDM_CLK_GPIO1 = 4,
> > > +	CS35L41_PDM_DATA_GPIO1 = 5,
> > >   };
> >
> > So CS35L41_MDSYNC_GPIO1 is identical with *_DRAIN_GPIO2, i.e. it's an
> > alias?
> >
> >
> > thanks,
> >
> > Takashi
> >
> 
> The value 2 sets GPIO1 as MDSYNC pin and GPIO2 as IRQ pin (Open drain).
> It could be the same label, like CS35L41_GPIO1_MDSYNC_GPIO2_IRQ, but I think separating them is better for understanding the code and organizing the code.

Yes, separating GPIO1 and GPIO2 would be less confusing.


thanks,

Takashi