[PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

Hi Jean!

> Yes, "probing" would be duplicated if we had to support more than one
> bus. But as far as I can see, the onyx and tas devices can only be
> found on i2c-powermac buses, so this shouldn't be an issue. And there
> isn't really any probing going on, it's really only a matter of walking
> a small subset of the device tree (the children of the I2C bus) and
> instantiating I2C devices.

Right, it was just an unrelated thought, it's not related to this in
particular.

> > Now, aoa will currently automatically load from the layout fabric
> > module, and then pull in the modules for the codecs it _knows_ to be
> > present on the bus. Therefore, it seems that your patch makes things
> > less efficient because you probe for all those codecs, and there's no
> > machine that has all of them. The aoa fabric only loads the modules for
> > those it knows to be present, and they then probe (and in reality the
> > probing never fails because they really are there).
> 
> Can you please point me to the layout fabric / aoa fabric? I'd like to
> understand how it works. Then I may be able to rewrite my patch
> completely differently.

That's in sound/aoa/fabric/layout.c

> There are basically two ways to instantiate I2C devices in the new
> model. The first method is to declare the I2C devices as platform data
> and let i2c-core instantiate them. The second method is to have the i2c
> bus driver instantiate the clients. My patch uses the second method
> because I didn't know how to use the first method. However using the
> first method would solve the issues you raised. But I need some help
> from someone more familiar with the powermac platform initialization
> code to get it right.

Interesting. Does it need to be _platform_ data, or could
sound/aoa/fabric/layout.c declare the devices instead?

> > Now, since aoa needs information on how the entire system is glued
> > together (the fabric I was talking about with the line-in example), it
> > has to infer that from platform data, in this case the device tree.
> > Because I do not have any older machines, am lazy and snd-powermac works
> > for the old machines, snd-powermac with its "tumbler" is a driver for
> > the same tas3004 codec, but on a different, older, fabric.
> 
> I think I've found that one by now (snd_pmac_detect in
> sound/ppc/pmac.c, right?), thanks for the clarification.

That's snd-powermac, yes.

> BTW, that code doesn't seem significantly different from what my patch
> is adding to i2c-powermac.

That would be true, yes, with your patch I don't understand how to load
snd-powermac _or_ snd-aoa based on the platform data (or in the case of
snd-powermac, not load it automatically at all).

> Hmm, OK, I expected the code I moved from the aoa drivers to
> i2c-powermac to only match the subset of machines actually supported by
> aoa. If that's not the case then indeed it is incorrect.

Yeah, that's not the case, I think the 'deq' node will be present on
older machines as well and you would load snd-aoa-codec-tas where
snd-powermac should be loaded.

> > Therefore, probing the codecs in i2c-powermac and automatically loading
> > the corresponding aoa module will break sound on old machines.
> 
> Does this mean that manually loading snd_aoa_codec_tas today on an old
> system with a TAS would break sound too?

Yes, I'm pretty sure it does on some systems. Actually, I'm not entirely
sure, because I don't know whether the i2c core will prevent two drivers
from talking to the same chip on the same bus, and if it doesn't then
this might still work but it would at least be very strange wrt. suspend
resume.

> Anyway, the key point of my patch is to get rid of the legacy i2c
> binding model, not efficiency.

Yes, I understand :)

> > Since I'm away from all machines I could test this with I have no data
> > on any that or the module device table thing I pointed out for now.
> > 
> > Anyway, some more technical comments on your patch:
> >  * I realise you just copied things around but it would be nice to clean
> >    up the coding style, especially comment style, a little while at it.
> >    (yeah, it's my fault)
> 
> I can fix anything you like, just tell me what :)

I think CodingStyle wants
 /*
  * ...
  * ...
  */

> >  * aoa_codec_* is the module name, I see no reason to use that as the
> >    i2c name, that should be the codec's name instead (aka pcm3052 etc)
> 
> The names are totally arbitrary and we can change them if you like.
> Keep in mind though that we should avoid using mere chip names for
> non-generic drivers. The aoa drivers are powermac-specific, we don't
> want the names we pick to collide with another driver, that's why I
> chose to keep the aoa prefix.

Oh ok, that kinda ties in with my very first question about what happens
if the same chip is known in different contexts, on different buses etc.
Makes sense in that case I guess. But I still think that you shouldn't
auto-load the aoa codec modules based on this anyway.

johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20090409/6be1c147/attachment.pgp>