Re: [PATCH 4/4] ASoC: TWL4030: Driver registration via twl4030_codec MFD

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

On Tuesday 20 October 2009 13:25:40 ext Mark Brown wrote:
> On Mon, Oct 19, 2009 at 03:42:20PM +0300, Peter Ujfalusi wrote:
> > TWL4030 codec is now using the device registration via
> > tlw4030_codec MFD device.
> 
> This looks pretty good but obviously depends on the MFD changes.

Thanks, yes it all depends on the MFD changes.

> The major thing that jumps out at me is the removal of the register
> definitions from the ASoC headers - it might be nice to have that done
> as part of the MFD patch, or as a separate patch.

The reason why I have done it like this is that with one patch I only touch one 
subsystem at the time:
1. MFD changes only
2. OMAP related changes
3. soc codec driver change

In patch 1, the register definitions had to be added, so that the twl4030_codec 
driver knows the registers (and there could be the vibra driver placed 
separately from the soc codec driver).
In patch 3, where I modify the soc codec driver to use the new method, than I 
remove the definitions and use the existing header file, introduced by the first 
patch.
All in all, after each patch the kernel can be builds, boots and works as 
before.

> 
> You've also got the bias being brought up when the ASoC system comes up
> rather than when the driver comes up.  To be honest it doesn't really
> make any difference either way, it's just slightly different to other
> drivers.

I was thinking that if you built the kernel with SND_SOC_ALL_CODECS on OMAP 
platform for some reason and you don't actually use the twl4030 as audio device 
-> no machine driver, which would use it, than the codec part would be off.
But yes, probably I can move the povering up to the probe function.

> What is useful with things like twl4030 which take a little
> while to come up is if you can do the bias bringup out of line from
> device probe, avoiding blocking system startup on CODEC bringup.  That's
> definitely a separate patch, I'm just mentioning it for interest here.

I'm sure there will be another round for this series, so I can make this change 
at the same time within the patch, since this anyway changes the way how the 
driver is loaded/probed.

> 
> There's also a couple of debug prints (like in the remove function) and

I'll get rid of them.
But at least this time I did not had those unneeded casts, which I usually have 
;)

> 
> > +MODULE_ALIAS("platform:twl4030_codec:audio");
> 
> Is that second colon right given...

I'm not sure about it at all either. I did not found any other 'nested MFD' 
drivers around, so this is just a guess
Should it be:

 +MODULE_ALIAS("platform:twl4030_codec_audio");


> 
> > +	.driver		= {
> > +		.name	= "twl4030_codec_audio",
> > +		.owner	= THIS_MODULE,
> 
> this.
> 

-- 
Péter