Re: [PATCH v4 1/7] ASoC: hda - add ASoC HDA codec match function

[Takashi Iwai <tiwai@suse.de>]

At Mon, 25 May 2015 14:58:54 +0100,
Mark Brown wrote:
> 
> On Mon, May 25, 2015 at 01:55:59PM +0200, Takashi Iwai wrote:
> > Vinod Koul wrote:
> 
> > > The hdac core doesn't actually do matching. If you see the match
> > > function provided by core (hda_bus_match), it is a wrapper and
> > > actual matching for legacy devices is being done in legacy code, see
> > > hda_codec_match. This match function expects the hdac_device to be
> > > wrapped in legacy hda_codec, which we don't need here.
> 
> > > So for ASoC we are embedding hdac_device in soc_hda_codec and using
> > > the vendor_id and revision_id to match, so hda to write a new one.
> 
> You keep on describing what the code does; I can mostly see what the
> code does (and could probably figure out extra bits if I wasn't getting
> scared off by what I do see) - that's basically what I'm flagging as an
> issue.  What I'm having trouble seeing is why this makes sense.  The
> question isn't what, it's why.

OK, let's see below.

> > > I do not mind if we commonize this and have common match function in
> > > hdac, if legacy is happy with it. Or perhaps the move to core later on
> > > as ASoC HDA matures, either way whatever you n Takashi agree with
> > > would be okay by me.
> 
> > This is the next step.  It would need a fairly big amount of rewrite
> > in each legacy HDA codec driver, and I don't want it for 4.2.
> 
> So how does this actually work then?  If we need to rework all the HDA
> CODEC drivers before they work with this new HDA bus (or at least this
> new HDA bus when used with controllers that implement a custom match
> function) then does it make sense to start trying to use it now?
> 
> I would have expected this to be the /first/ step - refactor the
> existing HDA code, then add new users.

I don't agree with it.

> > Once when we get the common criteria for matching (both for asoc and
> > legacy), we can move to the unified match method.
> 
> I really don't understand this idea that we need to work out what the
> common criteria for matching are, HDA is an enumerable bus isn't it?

The common match method *does* exist.  It's just shifted to two parts,
one for ASoC and one for legacy, because of two different objects.

> > That said, the reason for individual match mechanism is not to break
> > the legacy hda code.  If anyone can provide a patch to achieve that
> > within 100 LOCs and without regression, I'd happily take it :)
> 
> Another way of looking at this is to ask why the CODEC driver matching
> can't continue to work the way it currently does - we're transitioning
> to a bus (which makes sense as a cleanup) but I've not seen any reason
> articulated for tying that up the merge of the Sky Lake code and it
> seems like we're not very near being in a position to be able to do
> that.  Or could we just keep the bigger hda_codec struct around in the
> bus code for now even if it winds up being bloated?

The most of needed changes are not about hda_bus stuff.  This has been
already done for 4.2, in for-next branch.  The rest works for HDA
legacy codes are rather self-contained, further refactoring of the
legacy code itself.  This refactoring has little merit from
functionality POV; the move to hda_bus was already finished, and user
would see no difference at all.  The resultant code wouldn't be much
better from readability POV, rather the size would bloat.  (Remember
that standardization means nothing but limited customization.)

One of few merits would a possible common match method in hda/bus, but
that's all.  So it's prioritized low for now.

> Like I say it's all the why questions that aren't making sense to me
> beyond the "we're too scared to touch the existing code" ones.

Avoiding a regression is the very first priority, indeed, for a driver
like HDA, which is over 10 years old, fat, complex for literally
thousands of various devices, and still with millions of users and

> I'm also wondering how this interacts with the Tegra HDA device by the
> way...

It's a part of HDA legacy.  No difference from any other HDA devices.
What's the problem with Tegra?


thanks,

Takashi