[alsa-devel] [PATCH 2/3] ASoC: Intel: kbl_da7219_max98357a_rt5660: Add a new codec rt5660

[Hui Wang]

On 2018/12/7 上午9:18, Pierre-Louis Bossart wrote:
>
> 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.
>
OK, got it. will implement a new c file in the V2.

Thanks,

Hui.

> Thanks
>
> -Pierre
>
>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel