linux-kernel - Re: [PATCH v3 1/2] ASoC: codecs: Add support for AK5558 ADC driver

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:   Tue, 13 Feb 2018 12:15:20 +0000
From:   Daniel Baluta <daniel.baluta@....com>
To:     "broonie@...nel.org" <broonie@...nel.org>
CC:     dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Cosmin Samoila <cosmin.samoila@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "wakasugi.jb@...asahi-kasei.co.jp" <wakasugi.jb@...asahi-kasei.co.jp>,
        Fabio Estevam <fabio.estevam@....com>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "mihai.serban@...il.com" <mihai.serban@...il.com>,
        "robh@...nel.org" <robh@...nel.org>,
        "S.j. Wang" <shengjiu.wang@....com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "mihai.serban@....com" <mihai.serban@....com>
Subject: Re: [PATCH v3 1/2] ASoC: codecs: Add support for AK5558 ADC driver

On Lu, 2018-02-12 at 12:02 +0000, Mark Brown wrote:
> On Mon, Feb 05, 2018 at 07:01:54PM +0200, Daniel Baluta wrote:
> > 
> > AK5558 is a 32-bit, 768 kHZ sampling, differential input ADC
> > for digital audio systems.
> > 
> > --- /dev/null
> > +++ b/sound/soc/codecs/ak5558.c
> > @@ -0,0 +1,618 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Audio driver for AK5558 ADC
> Please don't mix C++ and C style comments - just make the entire comment
> C++.
> 
Sure. Will use:
// SPDX-License-Identifier: GPL-2.0

> > 
> > +static const char * const tdm_texts[] = {
> > +	"Off", "TDM128",  "TDM256", "TDM512",
> > +};
> This looks like it should be a set_tdm_slot() operation, and indeed
> set_tdm_slot() appears to be implemented and duplicate this.
> 

Yup, will remove this. At first there was no set_tdm_slot.
> > 
> > +static const char * const dsdon_texts[] = {
> > +	"PCM", "DSD",
> > +};
> This looks like it's setting the DAI format?

Ditto. Will remove.

> 
> > 
> > +	SND_SOC_DAPM_MUX("AK5558 Ch1 Enable", SND_SOC_NOPM, 0, 0,
> > +			 &ak5558_channel1_mux_control),
> On/off controls should be switches not muxes, though if this is just
> selecting which channels are active (rather than a mute) I'd not expect
> it to be a control at all - the board can say if inputs are disabled.

OK, agree that if we want to have a way to select which channels are active
we should use a switch.

Will remove this control for now.

> 
> > 
> > +static int ak5558_set_mcki(struct snd_soc_codec *codec, int fs, int rclk)
> > +{
> > +	u8  mode;
> > +
> > +	mode = snd_soc_read(codec, AK5558_02_CONTROL1);
> > +	mode &= ~AK5558_CKS;
> > +	mode |= AK5558_CKS_AUTO;
> > +
> > +	snd_soc_update_bits(codec, AK5558_02_CONTROL1, AK5558_CKS, mode);
> > +
> > +	return 0;
> > +}
> This appears to just ignore the parameters?

These are left overs from a v1 cleanup. Will fix.


thanks Mark! Will send v4 asap.

thanks,
Daniel.