mbox series

[PATCHv3,RESEND,00/10] ASoC: fsl-asoc-card: compatibility integration of a generic codec use case for use with S/PDIF controller

Message ID 20231218124058.2047167-1-elinor.montmasson@savoirfairelinux.com (mailing list archive)
Headers show
Series ASoC: fsl-asoc-card: compatibility integration of a generic codec use case for use with S/PDIF controller | expand

Message

Elinor Montmasson Dec. 18, 2023, 12:40 p.m. UTC
Hello,

This is the v3 of the series of patch aiming to make the machine driver
"fsl-asoc-card" compatible with use cases where there is no real codec
driver.
It proposes to use the "spdif_receiver" and "spdif_transmitter"
drivers instead of the dummy codec.
This is a first step in using the S/PDIF controller with the ASRC.

The five first patches add compatibility with the pair of codecs
"spdif_receiver" and "spdif_transmitter" with a new compatible,
"fsl,imx-audio-generic".
Codec parameters are set with default values.
Consequently, the driver is modified to work with multi-codec use cases.
It can get 2 codecs phandles from the device tree, and the
"fsl_asoc_card_priv" struct now has 2 "codec_priv" to store properties
of both codecs. It is fixed to 2 codecs as only "fsl,imx-audio-generic"
uses multiple codecs at the moment.
However, the driver now uses for_each_codecs macros when possible to
ease future implementations of multi-codec configurations.

The remaining patches add configuration options for the device tree.
They configure the CPU DAI when using "fsl,imx-audio-generic".
These options are usually hard-coded in "fsl-asoc-card.c" for each
audio codec.
Because the generic codec could be used with other CPU DAIs than
the S/PDIF controller, setting these parameters could be required.

This series of patch was successfully built for arm64 and x86 on top of
the latest for-next branch of the ASoC git tree on the 14th of December.
These modifications have also been tested on an i.MX8MN evaluation
board, with a linux kernel RT v6.1.26-rt8.


We also have a few questions, following remarks made by
Krzysztof Kozlowski in a previous email for patch 10/10:

> >>> The compatible list for this generic sound card currently:
> >>> @@ -48,6 +51,8 @@ The compatible list for this generic sound card
> >>> currently:
> >>>
> >>> "fsl,imx-audio-nau8822"
> >>>
> >>> + "fsl,imx-audio-generic"
> >>
> >> Generic does not look like hardware specific.
> >
> > Even if our end goal is to use it with the S/PDIF controller, this new
> > support can be used with different hardware that doesn't
> > require a codec. Thus, we don't really want to specify "spdif" in it.
> >
> > Is this compatible string not suitable ?
> > Should we rename it to something else, like "fsl,imx-audio-no-codec" ?
> 
> Maybe Mark or Rob will help here, but for me "imx-audio" is just way too
> generic.

* Which generic name should we use ? Or how should we change it?
 
> Also, you add several new properties, so I really expect either
> converting old binding to DT schema first or adding new device in DT
> schema format.

* fsl-asoc-card.txt currently follows the old dt-bindings format.
Should we update it to DT schema format in this patch series
before adding my new properties?


Best regards,
Elinor Montmasson


Changelog:
v2 -> v3:
* when the bitmaster or framemaster are retrieved from the device tree,
  the driver will now compare them with the two codecs possibly given in
  the device tree, and not just the first codec.
* improve driver modifications to use multiple codecs for better
  integration of future multi-codec use cases:
    * use for_each_codec macros when possible.
    * "fsl_asoc_card_priv" struct now has 2 "codec_priv" as the driver
      can currently retrieve 2 codec phandles from the device tree.
* fix subject of patch 10/10 to follow the style of the subsystem and
  previous commits of the file.
* v2 patch series at:
https://lore.kernel.org/alsa-devel/20231027144734.3654829-1-elinor.montmasson@savoirfairelinux.com/

v1 -> v2:
* replace use of the dummy codec by the pair of codecs
  "spdif_receiver" / " spdif_transmitter".
* adapt how dai links codecs are used to take into account the
  possibility for multiple codecs per link.
* change compatible name.
* adapt driver to be able to register two codecs given in the device
  tree.
* v1 patch series at:
https://lore.kernel.org/alsa-devel/20230901144550.520072-1-elinor.montmasson@savoirfairelinux.com/


Elinor Montmasson (10):
  ASoC: fsl-asoc-card: add support for dai links with multiple codecs
  ASoC: fsl-asoc-card: add second dai link component for codecs
  ASoC: fsl-asoc-card: add compatibility to use 2 codecs in dai-links
  ASoC: fsl-asoc-card: add new compatible for a generic codec use case
  ASoC: fsl-asoc-card: set generic codec as clock provider
  ASoC: fsl-asoc-card: add dts property "cpu-slot-width"
  ASoC: fsl-asoc-card: add dts property "cpu-slot-num"
  ASoC: fsl-asoc-card: add dts properties "cpu-sysclk-freq"
  ASoC: fsl-asoc-card: add dts properties "cpu-sysclk-dir-out"
  ASoC: bindings: fsl-asoc-card: add compatible for generic codec

 .../bindings/sound/fsl-asoc-card.txt          |  28 +-
 sound/soc/fsl/fsl-asoc-card.c                 | 299 +++++++++++-------
 2 files changed, 218 insertions(+), 109 deletions(-)

Comments

Krzysztof Kozlowski Dec. 18, 2023, 12:46 p.m. UTC | #1
On 18/12/2023 13:40, Elinor Montmasson wrote:
> Hello,
> 
> This is the v3 of the series of patch aiming to make the machine driver
> "fsl-asoc-card" compatible with use cases where there is no real codec
> driver.
> It proposes to use the "spdif_receiver" and "spdif_transmitter"
> drivers instead of the dummy codec.
> This is a first step in using the S/PDIF controller with the ASRC.
> 
> The five first patches add compatibility with the pair of codecs
> "spdif_receiver" and "spdif_transmitter" with a new compatible,
> "fsl,imx-audio-generic".
> Codec parameters are set with default values.
> Consequently, the driver is modified to work with multi-codec use cases.
> It can get 2 codecs phandles from the device tree, and the
> "fsl_asoc_card_priv" struct now has 2 "codec_priv" to store properties
> of both codecs. It is fixed to 2 codecs as only "fsl,imx-audio-generic"
> uses multiple codecs at the moment.
> However, the driver now uses for_each_codecs macros when possible to
> ease future implementations of multi-codec configurations.
> 
> The remaining patches add configuration options for the device tree.
> They configure the CPU DAI when using "fsl,imx-audio-generic".
> These options are usually hard-coded in "fsl-asoc-card.c" for each
> audio codec.
> Because the generic codec could be used with other CPU DAIs than
> the S/PDIF controller, setting these parameters could be required.
> 
> This series of patch was successfully built for arm64 and x86 on top of
> the latest for-next branch of the ASoC git tree on the 14th of December.
> These modifications have also been tested on an i.MX8MN evaluation
> board, with a linux kernel RT v6.1.26-rt8.
> 
> 
> We also have a few questions, following remarks made by
> Krzysztof Kozlowski in a previous email for patch 10/10:
> 
>>>>> The compatible list for this generic sound card currently:
>>>>> @@ -48,6 +51,8 @@ The compatible list for this generic sound card
>>>>> currently:
>>>>>
>>>>> "fsl,imx-audio-nau8822"
>>>>>
>>>>> + "fsl,imx-audio-generic"
>>>>
>>>> Generic does not look like hardware specific.
>>>
>>> Even if our end goal is to use it with the S/PDIF controller, this new
>>> support can be used with different hardware that doesn't
>>> require a codec. Thus, we don't really want to specify "spdif" in it.
>>>
>>> Is this compatible string not suitable ?
>>> Should we rename it to something else, like "fsl,imx-audio-no-codec" ?
>>
>> Maybe Mark or Rob will help here, but for me "imx-audio" is just way too
>> generic.
> 
> * Which generic name should we use ? Or how should we change it?

Compatible should be specific to one SoC, even if there is one driver
for entire family.

>  
>> Also, you add several new properties, so I really expect either
>> converting old binding to DT schema first or adding new device in DT
>> schema format.
> 
> * fsl-asoc-card.txt currently follows the old dt-bindings format.
> Should we update it to DT schema format in this patch series
> before adding my new properties?

You add six new properties, so from my point of view this cannot be in TXT.

Best regards,
Krzysztof
Daniel Baluta Dec. 18, 2023, 1:54 p.m. UTC | #2
>
> * fsl-asoc-card.txt currently follows the old dt-bindings format.
> Should we update it to DT schema format in this patch series
> before adding my new properties?
>
>

I know this is extra-work but we would greatly appreciate if you first
convert fsl-asoc-card.txt
to yml format and then add your new properties.
Elinor Montmasson Dec. 19, 2023, 12:39 p.m. UTC | #3
On Monday, 18 December, 2023 14:54:03, Daniel Baluta wrote
>
> > * fsl-asoc-card.txt currently follows the old dt-bindings format. 
> > Should we update it to DT schema format in this patch series 
> > before adding my new properties? 
> 
> I know this is extra-work but we would greatly appreciate if you first 
> convert fsl-asoc-card.txt 
> to yml format and then add your new properties. 

I will take some time next week to do the conversion, then I'll send
it in a v4 patch series.

Best regards,
Elinor Montmasson
Elinor Montmasson Dec. 29, 2023, 1:45 p.m. UTC | #4
Hello

On Monday, 18 December, 2023 14:54:03, Daniel Baluta wrote 
> I know this is extra-work but we would greatly appreciate if you first 
> convert fsl-asoc-card.txt 
> to yml format and then add your new properties. 

DT schema must have at least one maintainer in the "maintainers" field.
Who should I put for fsl-asoc-card.yaml ?

Best regards, 
Elinor Montmasson
Daniel Baluta Jan. 16, 2024, 11:41 a.m. UTC | #5
On Fri, Dec 29, 2023 at 3:45 PM Elinor Montmasson
<elinor.montmasson@savoirfairelinux.com> wrote:
>
> Hello
>
> On Monday, 18 December, 2023 14:54:03, Daniel Baluta wrote
> > I know this is extra-work but we would greatly appreciate if you first
> > convert fsl-asoc-card.txt
> > to yml format and then add your new properties.
>
> DT schema must have at least one maintainer in the "maintainers" field.
> Who should I put for fsl-asoc-card.yaml ?

I think it should be Shengjiu Wang, if he is OK with that.