Re: [PATCH] CHROMIUM: ASoC: amd: acp: Add tdm support for codecs in machine driver

[Venkata Prasad Potturu]

On 11/7/22 20:34, Pierre-Louis Bossart wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On 11/7/22 09:02, Venkata Prasad Potturu wrote:
> > On 11/7/22 19:44, Pierre-Louis Bossart wrote:
> > > Caution: This message originated from an External Source. Use proper
> > > caution when opening attachments, clicking links, or responding.
> > >
> > >
> > > On 11/7/22 04:34, Venkata Prasad Potturu wrote:
> > > > On 11/2/22 17:02, Mark Brown wrote:
> > > > > > On 11/1/22 20:01, Mark Brown wrote:
> > > > > > > On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu
> > > > > > > wrote:
> > > > > > > Right, that's what the code does but why is this something that
> > > > > > > should
> > > > > > > be controlled in this fashion?
> > > > > > This machine driver is common for TDM mode and I2S mode, user can
> > > > > > select TDM
> > > > > > mode or I2S mode.
> > > > > Why would the user choose one value or the other, and why would this
> > > > > choice be something that only changes at module load time?  If this is
> > > > > user controllable I'd really expect it to be runtime controllable.
> > > > > You're not explaining why this is a module parameter.
> > > > Different vendors/OEM's use the same hardware as one need I2S mode and
> > > > other need TDM mode, using common driver  to support  I2S and TDM mode
> > > > with this parameter.
> > > >
> > > >
> > > > static int tdm_mode = 0;
> > > > module_param_named(tdm_mode, tdm_mode, int, 0444);
> > > >
> > > > And this can be runtime controllable by setting permissions as 0644, we
> > > > will change and send next version patch.
> > > kernel parameters are difficult to manage for distributions using a
> > > single-build. Either all platforms use the kernel parameter or none of
> > > them do. That would not allow a per-platform choice of parameters.
> > > Using DMI quirks or ACPI identifiers would be a lot less problematic, no?
> > All platforms use the kernel parameter to select the I2S/TDM mode.
> > Runtime parameters are not required here, by default it is in I2S mode and
> > when the platform needs to enable TDM mode then pass tdm_mode module
> > parameter as 1.
> How would a distribution decide to set this kernel parameter, what
> criteria would be used to determine that the TDM mode is required?
> You've got to think of who uses that parameter.
> This may work for a Chrome solution given that there are per-product
> overlays but won't work in the general case IMHO.

Thanks for your time and  suggestion.

We will come back with DMI quirks.

>