linux-kernel - Re: [PATCH 3/5] MFD: twl4030-codec: APLL

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, 3 Nov 2009 15:37:50 +0200
From:	Peter Ujfalusi <peter.ujfalusi@...ia.com>
To:	ext Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"tony@...mide.com" <tony@...mide.com>
Subject: Re: [PATCH 3/5] MFD: twl4030-codec: APLL_INFREQ handling in the MFD driver

On Monday 02 November 2009 19:30:35 ext Mark Brown wrote:
> On Mon, Nov 02, 2009 at 02:34:53PM +0200, Peter Ujfalusi wrote:
> > Move the APLL_CTL register configuration to the twl4030-codec
> > MFD driver.
> > Provide also a function for childs to query the audio_mclk
> > frequency.
> 
> This all looks good to me, some nitpicks below.
> 
> > +unsigned int twl4030_codec_get_mclk(void)
> > +{
> > +	struct twl4030_codec *codec = platform_get_drvdata(twl4030_codec_dev);
> > +
> > +	return codec->audio_mclk;
> > +}
> > +EXPORT_SYMBOL_GPL(twl4030_codec_get_mclk);
> 
> As I said in my followup to patch 5 this feels like it should have a
> parameter to specify the twl4030 though in practical systems it won't
> matter.

I agree that this does not look quite nice, but as you already mentioned, it is 
highly unlikely that one system would have more than one twl series of PM chip 
on board. Another reason is that we have other part of the twl, which needs 
resources from the codec part, but it is not loaded through the codec MFD, so 
providing the needed information is kind of tricky with that setup.

> 
> > +	if (!(pdata->audio_mclk == 19200000 ||
> > +	      pdata->audio_mclk == 26000000 ||
> > +	      pdata->audio_mclk == 38400000)) {
> > +		dev_err(&pdev->dev, "Invalid audio_mclk\n");
> > +		return -EINVAL;
> > +	}
> 
> Might flow more naturally with a switch statement?

Yes, true. This looks weird, I'll change it.

> 

-- 
Péter
--
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/