Re: [RFC][PATCH 6/7] ASoC: hisilicon: Add hi6210 i2s audio driver for hdmi audio

[John Stultz]

On Sat, Jul 16, 2016 at 4:44 AM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Fri, Jul 15, 2016 at 07:13:26PM -0700, John Stultz wrote:
>>
>> +     .cpu_dai_name = "f7118000.hi6210_i2s",
>
> Why is this not connected up in DT - as things stand the card has zero
> reuse potential.  Though given that this has
>
>> +     .codec_name = "0.hi6210_hdmi_card",
>
> If you've got a CODEC with hdmi_card in the name that suggests there's a
> serious abstraction problem going on somewhere.

Yea. So I've been splitting up the patch and looking at how this might
be wired up via devicetree instead.

I've basically stripped the hdmi-card driver down and it really just
seems to be:

static struct snd_soc_dai_driver hi6210_hdmi_dai = {
        .name = "hi6210_hdmi_dai",
        .playback = {
                .channels_min = 2,
                .channels_max = 2,
                .rates = SNDRV_PCM_RATE_48000,
                .formats = SNDRV_PCM_FMTBIT_S16_LE,
        },
};

Then the probe/remove logic to register the codec, and connected it up
with the simple card driver.

Though it seems kind of silly to have a whole driver (and devicetree
entry for the probe hook) just to fill and install the above
structure. Is there a simpler way to specify that via a DT node
instead?


>> +static const struct of_device_id hi6210_i2s_dt_ids[] = {
>> +     { .compatible = "hisilicon,hi6210-i2s" },
>> +     { /* sentinel */ }
>> +};
>
> The code makes this look like it's not just an I2S controller.

I'm not sure I follow this? The dt ids make it seem like this driver
is not an i2s controller?

I think I've addressed the other issue you've pointed out in this
patch, so I'll likely be sending out another revision later today.

thanks
-john