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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 2 Oct 2015 08:44:03 +0200
From:	Code Kipper <codekipper@...il.com>
To:	Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Mark Brown <broonie@...nel.org>,
	linux-sunxi <linux-sunxi@...glegroups.com>,
	linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
	"Andrea Venturi (pers)" <be17068@...rbole.bo.it>
Subject: Re: [PATCH v2 4/4] ASOC: sunxi: Add support for the spdif block

>> +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. 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.

>
>> +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.

>
>> +     /* flush TX FIFO */
>> +     regmap_update_bits(host->regmap, SUN4I_SPDIF_FCTL,
>> +                     SUN4I_SPDIF_FCTL_FTX, SUN4I_SPDIF_FCTL_FTX);
>> +
>> +     /* clear interrupt status */
>> +     regmap_read(host->regmap, SUN4I_SPDIF_ISTA, &reg_val);
>> +     regmap_write(host->regmap, SUN4I_SPDIF_ISTA, reg_val);
>
> You're not using any interrupts. Why is this needed?

ditto. This wasn't brought up in the previous reviews.

>
>> +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.

>
>> +
>> +     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.

>
>> +
>> +     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.

>
> 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.
Thanks anyway,
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/