[PATCH v1] ASoC: support machine driver with TAS2781

[PATCH v1] ASoC: support machine driver with TAS2781

[Rui Zhou]

Support amp TAS2781 for mt8188 platform

Signed-off-by: Rui Zhou <zhourui@huaqin.corp-partner.google.com>
---
 sound/soc/mediatek/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
index 5a8476e1ecca..07f67f96c889 100644
--- a/sound/soc/mediatek/Kconfig
+++ b/sound/soc/mediatek/Kconfig
@@ -233,6 +233,7 @@ config SND_SOC_MT8188_MT6359
 	select SND_SOC_HDMI_CODEC
 	select SND_SOC_DMIC
 	select SND_SOC_MAX98390
+	select SND_SOC_TAS2781_I2C
 	select SND_SOC_NAU8315
 	select SND_SOC_NAU8825
 	select SND_SOC_RT5682S
-- 
2.34.1

Re: [PATCH v1] ASoC: support machine driver with TAS2781

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Tue, Jun 11, 2024 at 10:22:27AM +0800, Rui Zhou wrote:
> Support amp TAS2781 for mt8188 platform
> 
> Signed-off-by: Rui Zhou <zhourui@huaqin.corp-partner.google.com>
> ---
>  sound/soc/mediatek/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
> index 5a8476e1ecca..07f67f96c889 100644
> --- a/sound/soc/mediatek/Kconfig
> +++ b/sound/soc/mediatek/Kconfig
> @@ -233,6 +233,7 @@ config SND_SOC_MT8188_MT6359
>  	select SND_SOC_HDMI_CODEC
>  	select SND_SOC_DMIC
>  	select SND_SOC_MAX98390
> +	select SND_SOC_TAS2781_I2C
>  	select SND_SOC_NAU8315
>  	select SND_SOC_NAU8825

Ideally this would be sorted to avoid spurious merge conficts.  Also do
we really have dependencies here or should these just be being enabled
by users - these drivers are all pretty generic now?  Not that this
change in itself makes a huge difference to the existing state.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] ASoC: support machine driver with TAS2781

[AngeloGioacchino Del Regno]

Il 11/06/24 12:52, Mark Brown ha scritto:
> On Tue, Jun 11, 2024 at 10:22:27AM +0800, Rui Zhou wrote:
>> Support amp TAS2781 for mt8188 platform
>>
>> Signed-off-by: Rui Zhou <zhourui@huaqin.corp-partner.google.com>
>> ---
>>   sound/soc/mediatek/Kconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
>> index 5a8476e1ecca..07f67f96c889 100644
>> --- a/sound/soc/mediatek/Kconfig
>> +++ b/sound/soc/mediatek/Kconfig
>> @@ -233,6 +233,7 @@ config SND_SOC_MT8188_MT6359
>>   	select SND_SOC_HDMI_CODEC
>>   	select SND_SOC_DMIC
>>   	select SND_SOC_MAX98390
>> +	select SND_SOC_TAS2781_I2C
>>   	select SND_SOC_NAU8315
>>   	select SND_SOC_NAU8825
> 
> Ideally this would be sorted to avoid spurious merge conficts.  Also do
> we really have dependencies here or should these just be being enabled
> by users - these drivers are all pretty generic now?

The only dependency is on the MT6359 PMIC's audio related IPs and nothing else.

I'd even remove the other select statements for the other codecs, honestly,
as there's no reason why MT8188 wouldn't support a codec or the other, and
there is no reason why that SoC needs exactly those codecs, apart from some
board (Chromebooks) having one, or the other.

Instead of adding a select statement on MT8188_MT6359, adding the TAS2781_I2C
driver as a module in defconfig is a better idea..... at least, IMO.

>  Not that this
> change in itself makes a huge difference to the existing state.

I agree it doesn't make any huge difference, but I also don't see why users
can't simply enable (y/m) that in the kernel configuration :-)

Cheers,
Angelo

Re: [PATCH v1] ASoC: support machine driver with TAS2781

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]

On Tue, Jun 11, 2024 at 02:38:09PM +0200, AngeloGioacchino Del Regno wrote:
> Il 11/06/24 12:52, Mark Brown ha scritto:
> > On Tue, Jun 11, 2024 at 10:22:27AM +0800, Rui Zhou wrote:
> > > Support amp TAS2781 for mt8188 platform

> > > +++ b/sound/soc/mediatek/Kconfig
> > > @@ -233,6 +233,7 @@ config SND_SOC_MT8188_MT6359
> > >   	select SND_SOC_HDMI_CODEC
> > >   	select SND_SOC_DMIC
> > >   	select SND_SOC_MAX98390
> > > +	select SND_SOC_TAS2781_I2C
> > >   	select SND_SOC_NAU8315
> > >   	select SND_SOC_NAU8825

> > Ideally this would be sorted to avoid spurious merge conficts.  Also do
> > we really have dependencies here or should these just be being enabled
> > by users - these drivers are all pretty generic now?

> The only dependency is on the MT6359 PMIC's audio related IPs and nothing else.

> I'd even remove the other select statements for the other codecs, honestly,
> as there's no reason why MT8188 wouldn't support a codec or the other, and
> there is no reason why that SoC needs exactly those codecs, apart from some
> board (Chromebooks) having one, or the other.

Right, that's what I meant - the drivers are now so generic that they
probably shouldn't be selecting everything they could possibly work
with.

> Instead of adding a select statement on MT8188_MT6359, adding the TAS2781_I2C
> driver as a module in defconfig is a better idea..... at least, IMO.

Yes.

> >  Not that this
> > change in itself makes a huge difference to the existing state.

> I agree it doesn't make any huge difference, but I also don't see why users
> can't simply enable (y/m) that in the kernel configuration :-)

I think this is a result of the drivers originally being very specific
to a particular setup and gradually getting made more generic, if the
driver can only be used on a specific machine then the selects make
sense but that's no longer the case for these drivers I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] ASoC: support machine driver with TAS2781

[AngeloGioacchino Del Regno]

Il 11/06/24 15:19, Mark Brown ha scritto:
> On Tue, Jun 11, 2024 at 02:38:09PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 11/06/24 12:52, Mark Brown ha scritto:
>>> On Tue, Jun 11, 2024 at 10:22:27AM +0800, Rui Zhou wrote:
>>>> Support amp TAS2781 for mt8188 platform
> 
>>>> +++ b/sound/soc/mediatek/Kconfig
>>>> @@ -233,6 +233,7 @@ config SND_SOC_MT8188_MT6359
>>>>    	select SND_SOC_HDMI_CODEC
>>>>    	select SND_SOC_DMIC
>>>>    	select SND_SOC_MAX98390
>>>> +	select SND_SOC_TAS2781_I2C
>>>>    	select SND_SOC_NAU8315
>>>>    	select SND_SOC_NAU8825
> 
>>> Ideally this would be sorted to avoid spurious merge conficts.  Also do
>>> we really have dependencies here or should these just be being enabled
>>> by users - these drivers are all pretty generic now?
> 
>> The only dependency is on the MT6359 PMIC's audio related IPs and nothing else.
> 
>> I'd even remove the other select statements for the other codecs, honestly,
>> as there's no reason why MT8188 wouldn't support a codec or the other, and
>> there is no reason why that SoC needs exactly those codecs, apart from some
>> board (Chromebooks) having one, or the other.
> 
> Right, that's what I meant - the drivers are now so generic that they
> probably shouldn't be selecting everything they could possibly work
> with.
> 
>> Instead of adding a select statement on MT8188_MT6359, adding the TAS2781_I2C
>> driver as a module in defconfig is a better idea..... at least, IMO.
> 
> Yes.
> 
>>>   Not that this
>>> change in itself makes a huge difference to the existing state.
> 
>> I agree it doesn't make any huge difference, but I also don't see why users
>> can't simply enable (y/m) that in the kernel configuration :-)
> 
> I think this is a result of the drivers originally being very specific
> to a particular setup

Yeah, I agree.

 >
> and gradually getting made more generic, if the
> driver can only be used on a specific machine then the selects make
> sense but that's no longer the case for these drivers I think.

No this is not going to be used only for a specific machine... so yeah your
thoughts are totally right.

Just as an example, MT8188 (Kompanio) and MT8390 (Genio) are the same SoC again,
like MT8195 (Kompanio) and MT8395 (Genio) - so this driver is also going to be
used for the Genio 700 EVK, other than some SBC, etc, which do anyway expose I2S
pins on some kind of header, so... :-)

Cheers!