Re: [PATCH 2/3] ASoC: Intel: kbl_da7219_max98357a_rt5660: Add a new codec rt5660

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 12/6/18 7:01 PM, Hui Wang wrote:
> On 2018/12/6 下午11:08, Pierre-Louis Bossart wrote:
>> On 12/6/18 8:52 AM, Hui Wang wrote:
>>> The new Dell IoT platform uses kabylake + alc3277 codec, and alc3277
>>> shares the driver with the codec rt5660, here we choose the
>>> closest machine driver kbl_da7219_max98357a, and based on this driver,
>>> we add a new codec rt5660 to it.
>>>
>>> The audio design on this IoT platform is as below:
>>>   - Intel kabylake platform
>>>   - connect the codec ALC3277 via SSP0
>>>   - line-out and line-in with Micbias jacks
>>>   - line-out mute control and jack detection of line-out and line-in
>>>   - two HDMI ports with audio capability
>>
>> I am not sure it makes sense to stuff the rt5660 support in a machine 
>> driver that's meant mostly for Chromebooks, and I am not sure why you 
>> picked this one specifically.
>
> Since this is a kabylake platform, we have 4 candidates to choose if 
> we want to stuff the code into an existing machine driver, they are:
>
> kbl_da7219_max98357a.c  kbl_da7219_max98927.c kbl_rt5663_max98927.c  
> kbl_rt5663_rt5514_max98927.c
>
> kbl_da7219_max98927.c: it connects 2 codecs via SSP0, so we can't 
> share the ssp0_hw_params and ssp_fixup functions with this Dell IoT 
> design
>
> kbl_rt5663_max98927.c:  it connects 2 codes via SSP0, and the rt5663 
> is connected to SSP1,  we can't share the ssp0_hw_params and ssp_fixup 
> functions with this Dell IoT design, more over, it requires the clock 
> of "ssp1_mclk" and "ssp1_sclk" which is not needed on Dell IoT.
>
> kbl_rt5663_rt5514_max98927.c:  nearly same as kbl_rt5663_max98927.c
>
> So in order to share the existing code with Dell IoT maximumly, I 
> choose kbl_da7219_max98357a.c, it connects 1 codec via SSP0 with 
> SND_SOC_DAIFMT_I2S_B, this is same as the Dell IoT's design.
This description shows you've done your homework, but...
>
>
>> Since there is no reuse of the DA7219, DMIC or MAX98357a, I would 
>> argue that you want to define a new machine driver so that you only 
>> select and compile in what you need, not to mention that it'll be 
>> easier to maintain, both for upstream and for Chrome backports.
>
> Yes, a standalone driver for a specific HW is easy to maintain, but it 
> will add lots of copy-and-paste code. If it is really not good to 
> stuff Dell IoT into an existing machine driver, I will prepare a 
> standalone machine driver for it in the V2.
>
...the only thing that's really copy-paste is the HDMI part, which can 
be factored without too many issues. All the maps, routing, codec 
controls are different, and there is no benefit in having them in a 
single file. There are multiple derivatives of those Chromebook designs 
and you really don't want to have to deal with their updates. And last 
the downside is really more complicated Kconfig dependencies.

I am all for reuse, and just spent the last hour making sure we can 
reuse between APL and GLK, but in this case I really strongly encourage 
you to have a different machine driver, it'll be simpler to review and 
maintain.

Thanks

-Pierre



_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel