Re: [PATCH 3/5] MFD: twl4030-codec: APLL_INFREQ handling in the MFD driver

[Peter Ujfalusi <peter.ujfalusi@nokia.com>]

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