Re: [PATCH v2 4/4] ASOC: sunxi: Add support for the spdif block

[Code Kipper]

On 6 October 2015 at 11:00, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Oct 02, 2015 at 08:44:03AM +0200, Code Kipper wrote:
>> >> +config SND_SOC_SUNXI_DAI_SPDIF
>> >> +        tristate
>> >> +     depends on OF
>> >> +        select SND_SOC_GENERIC_DMAENGINE_PCM
>> >> +        select REGMAP_MMIO
>> >> +
>> >> +config SND_SOC_SUNXI_MACHINE_SPDIF
>> >> +        tristate "APB on-chip sun4i/sun5i/sun7i SPDIF"
>> >> +     depends on OF
>> >> +        select SND_SOC_SUNXI_DAI_SPDIF
>> >> +        help
>> >> +          Say Y if you want to add support for SoC S/PDIF audio as simple audio card.
>> >
>> > You still haven't said why you can't use simple-card...
>>
>> I mentioned in the covering letter that I thought that simple-card was
>> overkill.
>
> Overkill for what? It adds no code, that will put no new maintainance
> burden, without any duplication. While your code also adds all that.
>
>> There is also a thread concerning issues with the ordering
>> of module bringup here
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/070534.html.
>> I was initially trying to use the dummy spdif transmitter but couldn't
>> get it working, this set up works for me. I haven't got an audio guy
>> sitting next to me to ping and have reached out for some guidance. I
>> can do this using simple-card, it just with all the driver refactoring
>> it was the main place where I thought things would break.
>
> We're all on IRC.

OK, let me sit on the next patch release until I've properly
investigated this. I'm not a big IRCer so I will need to change my
habits.

>
>
>> >> +static void sun4i_spdif_configure(struct sun4i_spdif_dev *host)
>> >> +{
>> >> +     u32 reg_val;
>> >> +
>> >> +     /* soft reset SPDIF */
>> >> +     regmap_write(host->regmap, SUN4I_SPDIF_CTL, SUN4I_SPDIF_CTL_RESET);
>> >> +
>> >> +     /* MCLK OUTPUT enable */
>> >> +     regmap_update_bits(host->regmap, SUN4I_SPDIF_CTL,
>> >> +                     SUN4I_SPDIF_CTL_MCLKOUTEN, SUN4I_SPDIF_CTL_MCLKOUTEN);
>> >
>> > The alignment is still not right....
>>
>> I'm not even sure if we need mclk output enabled. Let me see what
>> happens when I remove this.
>
> It's not really the point. The alignment of all your wrapped lines is
> wrong.

Ahhhh....I was brought up to not mix tabs and spaces and I now see
with a quick check that checkpatch doesn't barf...I'll fix this.

>
>> >> +static int sun4i_spdif_startup(struct snd_pcm_substream *substream,
>> >> +                             struct snd_soc_dai *cpu_dai)
>> >> +{
>> >> +     struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> >> +     struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(rtd->cpu_dai);
>> >> +
>> >> +     if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
>> >> +             return -EINVAL;
>> >> +
>> >> +     sun4i_spdif_configure(host);
>> >> +
>> >> +     return clk_prepare_enable(host->clk);
>> >
>> > You're still not using pm_runtime...
>>
>> I've removed the pm stuff and this is the same as you have it in
>> sun4i-codec.
>
> You've removed the suspend code, and both Mark and I asked you to use
> runtime_pm to handle your bus clock.
>
> And this has also been asked for the codec.

You asked if I had tested the pm operations which I hadn't so I
removed them after looking at your driver and searching for pm_runtime
usage elsewhere in sound/soc. I will add them back.

>
>> >> +
>> >> +     ret = clk_set_rate(host->audio_clk, mclk);
>> >> +     if (ret < 0) {
>> >> +             dev_err(&pdev->dev,
>> >> +                     "Setting pll2 clock rate for %d Hz failed!\n", mclk);
>> >> +             return ret;
>> >> +     }
>> >
>> > You're still using the PLL2...
>>
>> I commented this out and it stopped working...let me check again.
>
> Then something is wrong somewhere else that needs to be fixed, either
> in the clock driver or in this driver. Did you update all the other
> references to PLL2 as well?

To be honest...the underlying clock code that I used wasn't based on
your latest patches. I'll relook at this, maybe my dsti is at fault.
>
>>
>> >
>> >> +
>> >> +     ret = clk_set_rate(host->clk, mclk);
>> >> +     if (ret < 0) {
>> >> +             dev_err(&pdev->dev,
>> >> +                     "Setting SPDIF clock rate for %d Hz failed!\n", mclk);
>> >> +             return ret;
>> >> +     }
>> >> +
>> >> +     reg_val = 0;
>> >> +     reg_val &= ~SUN4I_SPDIF_FCTL_FIFOSRC;
>> >> +     reg_val |= SUN4I_SPDIF_FCTL_TXTL_MASK;
>> >> +     reg_val |= SUN4I_SPDIF_FCTL_RXTL_MASK;
>> >> +     reg_val |= SUN4I_SPDIF_FCTL_TXIM;
>> >> +     reg_val |= SUN4I_SPDIF_FCTL_RXOM_MASK;
>> >> +     regmap_write(host->regmap, SUN4I_SPDIF_FCTL, reg_val);
>> >
>> > You're still not using regmap_update_bits...
>>
>> Why would I need to?, this is the first write to the register before
>> playback and I'm not interested in keeping any of the previous fifo
>> settings. Will remove"reg_val &= ~SUN4I_SPDIF_FCTL_FIFOSRC;" as that's
>> not doing anything.
>
> Dropping the masking is also an option.

Yeah...that still doesn't look right. I'll investigate.

>
>> > IF you're really going to ignore all the comments we did, please tell
>> > us upfront. That way, we will not waste our time doing a review of
>> > your patches.
>>
>> All is a strong word....did you even read my covering letter?....there
>> was a major refactoring of the code and I think I covered a majority
>> of the comments. Apologies if you feel that you'd wasted a lot of time
>> of this....it can't be any more that the EVB dts.
>
> The point of this is that this is a discussion. You simply ignored
> most of the comments, without even mentionning why you wanted to
> ignore them, and simply sent a new version.
>
> If you feel like one comment is invalid, let's discuss this, like you
> did here. But silently discarding them is not an option and actually
> quite rude.

I won't be in such a rush to resend the next patch set. I'll clear up
everything and if I run into any difficulties then I'll push to my
github and ping you on IRC.
Thanks,
CK
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/