| Message ID | 1431341645-2457-2-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 11, 2015 at 04:23:59PM +0530, Vinod Koul wrote: > From: Jeeja KP <jeeja.kp@intel.com> > > ASoC hda codec driver will use id_table for registration. > id_table has info of codec vendor_id, revision_id and name. > For ASoC device/driver matching, device vendor_id, rev_id > will be matched to id_table to identify the device/driver. I'm sorry but I still don't entirely understand what this is supposed to do. It *looks* like it's trying to create a bus for HDA with this: > +/* HDA device table */ > +#define HDA_NAME_SIZE 20 > +struct soc_hda_device_id { > + __u32 id; > + __u32 rev_id; > + char name[HDA_NAME_SIZE]; > + unsigned long driver_data; > +}; > + > +struct soc_hda_codec_driver { > + struct hdac_driver core; > + const struct soc_hda_device_id *id_table; > +}; but it's not actually defining a driver model bus type (though it does use driver_register()) and I'd expect the bus type to be provided by the generic HDA code. Looking (fairly quickly admittedly) at the HDA code it's not entirely obvious how it all fits together and the changelog for that just talks about moving code around. We really need to make sure we don't end up with the same shambles we've got for AC'97 here (though we're unlikely to get similar touchscreen type things so it should be less bad).
At Fri, 22 May 2015 13:56:34 +0100, Mark Brown wrote: > > On Mon, May 11, 2015 at 04:23:59PM +0530, Vinod Koul wrote: > > From: Jeeja KP <jeeja.kp@intel.com> > > > > ASoC hda codec driver will use id_table for registration. > > id_table has info of codec vendor_id, revision_id and name. > > For ASoC device/driver matching, device vendor_id, rev_id > > will be matched to id_table to identify the device/driver. > > I'm sorry but I still don't entirely understand what this is supposed to > do. It *looks* like it's trying to create a bus for HDA with this: > > > +/* HDA device table */ > > +#define HDA_NAME_SIZE 20 > > +struct soc_hda_device_id { > > + __u32 id; > > + __u32 rev_id; > > + char name[HDA_NAME_SIZE]; > > + unsigned long driver_data; > > +}; > > + > > +struct soc_hda_codec_driver { > > + struct hdac_driver core; > > + const struct soc_hda_device_id *id_table; > > +}; > > but it's not actually defining a driver model bus type (though it does > use driver_register()) and I'd expect the bus type to be provided by the > generic HDA code. Looking (fairly quickly admittedly) at the HDA code > it's not entirely obvious how it all fits together and the changelog for > that just talks about moving code around. The bus is already defined in sound/hda. There, the actual binding is done in each hda driver specifics, i.e. in sound/soc/hda and sound/pci/hda. It's the way to allow binding completely different drivers for the very same PCI ID. Takashi
On Fri, May 22, 2015 at 03:35:03PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > I'm sorry but I still don't entirely understand what this is supposed to > > do. It *looks* like it's trying to create a bus for HDA with this: > > but it's not actually defining a driver model bus type (though it does > > use driver_register()) and I'd expect the bus type to be provided by the > > generic HDA code. Looking (fairly quickly admittedly) at the HDA code > > it's not entirely obvious how it all fits together and the changelog for > > that just talks about moving code around. > The bus is already defined in sound/hda. There, the actual binding is I see there's a bus_type defined there but this is calling raw device_register() and providing a match function so it seems like there's something missing or an abstraction problem. I'd expect a bus to be providing things like match functions and bus specific registration functions. This is all setting off alarm bells, both the code and the very aggressive pushing. > done in each hda driver specifics, i.e. in sound/soc/hda and > sound/pci/hda. It's the way to allow binding completely different > drivers for the very same PCI ID. I don't really understand this, sorry. What impact would HDA bus implementation have on PCI device drivers (by the time you're registering HDA devices presumably the PCI device will already be enumerated)? Having two drivers for the same PCI function doesn't sound like an *obviously* good idea (as we discussed recently) but that's a bit of a separate thing.
At Fri, 22 May 2015 18:41:06 +0100, Mark Brown wrote: > > On Fri, May 22, 2015 at 03:35:03PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > I'm sorry but I still don't entirely understand what this is supposed to > > > do. It *looks* like it's trying to create a bus for HDA with this: > > > > but it's not actually defining a driver model bus type (though it does > > > use driver_register()) and I'd expect the bus type to be provided by the > > > generic HDA code. Looking (fairly quickly admittedly) at the HDA code > > > it's not entirely obvious how it all fits together and the changelog for > > > that just talks about moving code around. > > > The bus is already defined in sound/hda. There, the actual binding is > > I see there's a bus_type defined there but this is calling raw > device_register() and providing a match function so it seems like > there's something missing or an abstraction problem. I'd expect a bus > to be providing things like match functions and bus specific > registration functions. This was intentionally left in that way, so that the upper core layer (HDA ASoC or legacy core parts) can implement the matching method more specifically. It can be done in the hdac_bus itself, but currently still leaves more room until we see the exact matching condition for ASoC. Once when the matching method can be unified, we can move the it to hdac_bus. But it's not necessarily done now. > This is all setting off alarm bells, both the code and the very > aggressive pushing. > > > done in each hda driver specifics, i.e. in sound/soc/hda and > > sound/pci/hda. It's the way to allow binding completely different > > drivers for the very same PCI ID. > > I don't really understand this, sorry. What impact would HDA bus > implementation have on PCI device drivers (by the time you're > registering HDA devices presumably the PCI device will already be > enumerated)? > > Having two drivers for the same PCI function doesn't sound like an > *obviously* good idea (as we discussed recently) but that's a bit of a > separate thing. Suggesting two PCI IDs might have been confusing, sorry. The point is that a HD-audio object can be inherited to two different level of objects, legacy and ASoC. Both are bound on the same bus, but to the corresponding drivers. Both objects use the very same bus ops, thus they share the same hdac_bus. Takashi
On Fri, May 22, 2015 at 08:13:45PM +0200, Takashi Iwai wrote: > At Fri, 22 May 2015 18:41:06 +0100, > Mark Brown wrote: > > > > On Fri, May 22, 2015 at 03:35:03PM +0200, Takashi Iwai wrote: > > > Mark Brown wrote: > > > > > > I'm sorry but I still don't entirely understand what this is supposed to > > > > do. It *looks* like it's trying to create a bus for HDA with this: > > > > > > but it's not actually defining a driver model bus type (though it does > > > > use driver_register()) and I'd expect the bus type to be provided by the > > > > generic HDA code. Looking (fairly quickly admittedly) at the HDA code > > > > it's not entirely obvious how it all fits together and the changelog for > > > > that just talks about moving code around. > > > > > The bus is already defined in sound/hda. There, the actual binding is > > > > I see there's a bus_type defined there but this is calling raw > > device_register() and providing a match function so it seems like > > there's something missing or an abstraction problem. I'd expect a bus > > to be providing things like match functions and bus specific > > registration functions. > > This was intentionally left in that way, so that the upper core layer > (HDA ASoC or legacy core parts) can implement the matching method more > specifically. It can be done in the hdac_bus itself, but currently > still leaves more room until we see the exact matching condition for > ASoC. > > Once when the matching method can be unified, we can move the it to > hdac_bus. But it's not necessarily done now. > > > This is all setting off alarm bells, both the code and the very > > aggressive pushing. > > > > > done in each hda driver specifics, i.e. in sound/soc/hda and > > > sound/pci/hda. It's the way to allow binding completely different > > > drivers for the very same PCI ID. > > > > I don't really understand this, sorry. What impact would HDA bus > > implementation have on PCI device drivers (by the time you're > > registering HDA devices presumably the PCI device will already be > > enumerated)? > > > > Having two drivers for the same PCI function doesn't sound like an > > *obviously* good idea (as we discussed recently) but that's a bit of a > > separate thing. > > Suggesting two PCI IDs might have been confusing, sorry. > > The point is that a HD-audio object can be inherited to two different > level of objects, legacy and ASoC. Both are bound on the same bus, > but to the corresponding drivers. Both objects use the very same bus > ops, thus they share the same hdac_bus. Thanks a bunch Takashi for clarifying :) Mark, I guess one clarification may still help, The SKL Audio controller is PCI device, the PCM driver in this series will load against the PCI device. Now the driver will go ahead and initialize the HDA bus. Also we provide our own matching function here. The matching function is for matching the devices on HDA link. The probing of the bus will find the HDA codecs present in the bus and we will match them based on Vendor ID and Device ID in match function above. Let us know if you have more questions
On Sat, May 23, 2015 at 11:21:56AM +0530, Vinod Koul wrote: > On Fri, May 22, 2015 at 08:13:45PM +0200, Takashi Iwai wrote: > > The point is that a HD-audio object can be inherited to two different > > level of objects, legacy and ASoC. Both are bound on the same bus, > > but to the corresponding drivers. Both objects use the very same bus > > ops, thus they share the same hdac_bus. > Thanks a bunch Takashi for clarifying :) > I guess one clarification may still help, The SKL Audio controller is PCI > device, the PCM driver in this series will load against the PCI device. > Now the driver will go ahead and initialize the HDA bus. Also we provide our > own matching function here. > The matching function is for matching the devices on HDA link. The probing > of the bus will find the HDA codecs present in the bus and we will match > them based on Vendor ID and Device ID in match function above. > Let us know if you have more questions To be honest the above isn't really clarifying things for me. I know what a matching function is for, the thing that is really worrying about this is that we've got different matching functions depending on how the HDA bus is instantiated. Given that HDA is an enumerable bus why would we want or need that - why and how does the matching differ depending on the driver we're using for the bus? I would not expect that matching using the HDA identification registers would be something that varies depending on how we register the bus.
On Mon, May 25, 2015 at 11:48:28AM +0100, Mark Brown wrote: > On Sat, May 23, 2015 at 11:21:56AM +0530, Vinod Koul wrote: > > On Fri, May 22, 2015 at 08:13:45PM +0200, Takashi Iwai wrote: > > > > The point is that a HD-audio object can be inherited to two different > > > level of objects, legacy and ASoC. Both are bound on the same bus, > > > but to the corresponding drivers. Both objects use the very same bus > > > ops, thus they share the same hdac_bus. > > > Thanks a bunch Takashi for clarifying :) > > > I guess one clarification may still help, The SKL Audio controller is PCI > > device, the PCM driver in this series will load against the PCI device. > > Now the driver will go ahead and initialize the HDA bus. Also we provide our > > own matching function here. > > > The matching function is for matching the devices on HDA link. The probing > > of the bus will find the HDA codecs present in the bus and we will match > > them based on Vendor ID and Device ID in match function above. > > > Let us know if you have more questions > > To be honest the above isn't really clarifying things for me. I know > what a matching function is for, I was trying to clarify that we are matching hda codec device here and not PCI HDA controller > the thing that is really worrying about > this is that we've got different matching functions depending on how the > HDA bus is instantiated. Given that HDA is an enumerable bus why would > we want or need that - why and how does the matching differ depending on > the driver we're using for the bus? I would not expect that matching > using the HDA identification registers would be something that varies > depending on how we register the bus. 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. 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. HTH Thanks
At Mon, 25 May 2015 16:51:50 +0530, Vinod Koul wrote: > > On Mon, May 25, 2015 at 11:48:28AM +0100, Mark Brown wrote: > > On Sat, May 23, 2015 at 11:21:56AM +0530, Vinod Koul wrote: > > > On Fri, May 22, 2015 at 08:13:45PM +0200, Takashi Iwai wrote: > > > > > > The point is that a HD-audio object can be inherited to two different > > > > level of objects, legacy and ASoC. Both are bound on the same bus, > > > > but to the corresponding drivers. Both objects use the very same bus > > > > ops, thus they share the same hdac_bus. > > > > > Thanks a bunch Takashi for clarifying :) > > > > > I guess one clarification may still help, The SKL Audio controller is PCI > > > device, the PCM driver in this series will load against the PCI device. > > > Now the driver will go ahead and initialize the HDA bus. Also we provide our > > > own matching function here. > > > > > The matching function is for matching the devices on HDA link. The probing > > > of the bus will find the HDA codecs present in the bus and we will match > > > them based on Vendor ID and Device ID in match function above. > > > > > Let us know if you have more questions > > > > To be honest the above isn't really clarifying things for me. I know > > what a matching function is for, > I was trying to clarify that we are matching hda codec device here and not > PCI HDA controller > > > the thing that is really worrying about > > this is that we've got different matching functions depending on how the > > HDA bus is instantiated. Given that HDA is an enumerable bus why would > > we want or need that - why and how does the matching differ depending on > > the driver we're using for the bus? I would not expect that matching > > using the HDA identification registers would be something that varies > > depending on how we register the bus. > > 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. > > 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. Once when we get the common criteria for matching (both for asoc and legacy), we can move to the unified match method. 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 :) Takashi
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. > > 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. > 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? > 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? 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. I'm also wondering how this interacts with the Tegra HDA device by the way...
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
On Tue, May 26, 2015 at 07:24:03AM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > On Mon, May 25, 2015 at 01:55:59PM +0200, Takashi Iwai wrote: > > > 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. What are these objects supposed to be then and why does the bus for HDA CODECs know about the differences between them? They're both called HDA CODECs which might lead one to think that they are supposed to represent the same thing. > > 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? People keep talking about PCI as being a differentator here, though apparently the Intel systems are all PCI so perhaps not.
At Tue, 26 May 2015 14:32:56 +0100, Mark Brown wrote: > > On Tue, May 26, 2015 at 07:24:03AM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > On Mon, May 25, 2015 at 01:55:59PM +0200, Takashi Iwai wrote: > > > > > 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. > > What are these objects supposed to be then and why does the bus for HDA > CODECs know about the differences between them? They're both called HDA > CODECs which might lead one to think that they are supposed to represent > the same thing. Both legacy and ASoC drivers contain own codec drivers, but their classes inherit the same HD-audio core object class that is managed by HD-audio core bus. Meanwhile, since the id table is (still) not embedded into HD-audio core object, the match method is still placed in each (asoc and legacy) driver core code. > > > 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? > > People keep talking about PCI as being a differentator here, though > apparently the Intel systems are all PCI so perhaps not. Yeah, Tegra also hits this whole movement. Or better to say, the whole HD-audio picture has been rewritten, and the transition about Tegra was already finished. For the legacy HDA, the rest are rather about the codec side. Takashi
On Tue, May 26, 2015 at 03:41:32PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > What are these objects supposed to be then and why does the bus for HDA > > CODECs know about the differences between them? They're both called HDA > > CODECs which might lead one to think that they are supposed to represent > > the same thing. > Both legacy and ASoC drivers contain own codec drivers, but their > classes inherit the same HD-audio core object class that is managed by > HD-audio core bus. Meanwhile, since the id table is (still) not > embedded into HD-audio core object, the match method is still placed > in each (asoc and legacy) driver core code. This is still as clear as mud, sorry - why is the ID table not something that the bus handles? It seems like a core part of what a bus abstracts. Having two completely separate sets of controller and device drivers that can't interact with each other (which seems like what we're ending up with here) seems really worrying and I don't think I've seen any plan for unification either. It's not just the match function stuff by the way, some of the later patches also made me wonder about the mutiple implementations of the bus thing. I think I'm really not understanding what this bus is supposed to be doing. It's possible that there is something I'm missing here (I imagine there's been some off-list discussions of this) but right now what I'm seeing is that either the bus is not yet at a point where it abstracts enough to really be a bus (it's not clear to me how something would use the bus) or the existing HDA code has been converted to use the new bus before we're ready to do that. What you all seem to be saying is that the existing code for HDA is too fragile to touch much so we don't want to move much of its functionality to the new bus yet. I can believe that but I think I'd be a lot happier if we were handling that by having the existing HDA code override bus functionality rather than by implementing key bus functionality in random places. It feels like trying to use the bus now is adding technical debt.
At Tue, 26 May 2015 20:43:16 +0100, Mark Brown wrote: > > On Tue, May 26, 2015 at 03:41:32PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > What are these objects supposed to be then and why does the bus for HDA > > > CODECs know about the differences between them? They're both called HDA > > > CODECs which might lead one to think that they are supposed to represent > > > the same thing. > > > Both legacy and ASoC drivers contain own codec drivers, but their > > classes inherit the same HD-audio core object class that is managed by > > HD-audio core bus. Meanwhile, since the id table is (still) not > > embedded into HD-audio core object, the match method is still placed > > in each (asoc and legacy) driver core code. > > This is still as clear as mud, sorry - why is the ID table not something > that the bus handles? It seems like a core part of what a bus > abstracts. Having two completely separate sets of controller and device > drivers that can't interact with each other (which seems like what we're > ending up with here) seems really worrying and I don't think I've seen > any plan for unification either. It's not just the match function stuff > by the way, some of the later patches also made me wonder about the > mutiple implementations of the bus thing. HDA bus is the place for communication between a device and a controller. This is common, no matter whether asoc or legacy. That is, an ASoC HDA controller may communicate even with an HDA legacy codec device, if really wanted. There is no distinction there. The fact you might be missing from the beginning of the story is that we need to implement two different driver sets for ASoC, both for controller and codec drivers. They can't be unified with legacy HDA unless we merge ASoC core codes into ALSA core (e.g. DPCM or DAPM into ALSA core). Also ASoC has the different ways of registration, too. For managing these two sets of stuff better, HDA bus provides the place for controller/codec communication. All verbs are executed via snd_hdac_bus_exec_verb(), basically. Also, it provides dozens of helper codes to implement PCM and codec/controller/stream management codes. Then both HDA legacy and asoc drivers are built up on them. This is the current standing point. The lack of id table in hdac_bus object is just because of the current form of the legacy HDA *codec* driver. They aren't translated to a normal struct device_driver yet. It's not done because such a translation is a large amount even though it's done systematically, and there is always a risk of breakage by that. But the plan is to achieve it in 4.3 kernel. So you can see the unified match method sooner or later. No reason to stick with it. > I think I'm really not understanding what this bus is supposed to be > doing. It's possible that there is something I'm missing here (I > imagine there's been some off-list discussions of this) but right now > what I'm seeing is that either the bus is not yet at a point where it > abstracts enough to really be a bus (it's not clear to me how something > would use the bus) or the existing HDA code has been converted to use > the new bus before we're ready to do that. > > What you all seem to be saying is that the existing code for HDA is too > fragile to touch much so we don't want to move much of its functionality > to the new bus yet. I can believe that but I think I'd be a lot happier > if we were handling that by having the existing HDA code override bus > functionality rather than by implementing key bus functionality in > random places. It feels like trying to use the bus now is adding > technical debt. Well, the patchset looks more complicated because lots of codes you're seeing in this patchset are specific to SKL hardware extension, and it doesn't (maybe won't) exist in HDA legacy. For example, patch 3 contains many such codes, which is likely the point where you stopped reading the rest patches. If you look through the patches, most of codes are wrappers, just calling snd_hdac_*(). PCM and PCI driver stuff need more codes than that because of ASoC-specific data and ops (dai_ops, dai_driver, etc) and to be an individual PCI driver. Some codes might be able to be unified in future, but not everything, as long as they are implemented as individual drivers. Takashi
On Wed, May 27, 2015 at 08:05:47AM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > This is still as clear as mud, sorry - why is the ID table not something > > that the bus handles? It seems like a core part of what a bus > > abstracts. Having two completely separate sets of controller and device > > drivers that can't interact with each other (which seems like what we're > > ending up with here) seems really worrying and I don't think I've seen > > any plan for unification either. It's not just the match function stuff > > by the way, some of the later patches also made me wonder about the > > mutiple implementations of the bus thing. > HDA bus is the place for communication between a device and a > controller. This is common, no matter whether asoc or legacy. That > is, an ASoC HDA controller may communicate even with an HDA legacy > codec device, if really wanted. There is no distinction there. I'm unclear how that would (practically speaking) be accomplished if they're in two separate worlds for matching. Come to think of it how does this work if we have more than one matching system in use - how does the driver model code cope with that, what happens if one match function ends up looking at data for another match type? > The fact you might be missing from the beginning of the story is that > we need to implement two different driver sets for ASoC, both for > controller and codec drivers. They can't be unified with legacy HDA > unless we merge ASoC core codes into ALSA core (e.g. DPCM or DAPM into > ALSA core). Also ASoC has the different ways of registration, too. Wow, that's not what I'd have expected to be happening at all and really should have been mentioned at some point. I need to think about this further. What I had expected to see for ASoC/HDA integration was something where ASoC devices were providing a standard HDA controller and then use the same CODEC drivers as everything else. Not doing that means that we inveitably have some duplication for at least things like CODEC quirks and device specific support which doesn't seem like the best idea. There's also userspace interfaces for things like the HDA device graph that'd need looking at. Having HDA use ASoC doesn't seem like a terrible idea, there are aspects of HDA that map quite well onto ASoC, but doesn't seem to be the intention here and I'm ambivalent about it being worth the effort. I don't think it would need us to move ASoC into the core any more than any other driver, it'd just make it much more widely used. > > What you all seem to be saying is that the existing code for HDA is too > > fragile to touch much so we don't want to move much of its functionality > > to the new bus yet. I can believe that but I think I'd be a lot happier > > if we were handling that by having the existing HDA code override bus > > functionality rather than by implementing key bus functionality in > > random places. It feels like trying to use the bus now is adding > > technical debt. > Well, the patchset looks more complicated because lots of codes you're > seeing in this patchset are specific to SKL hardware extension, and it > doesn't (maybe won't) exist in HDA legacy. For example, patch 3 > contains many such codes, which is likely the point where you stopped > reading the rest patches. I'm too alarmed by what I'm seeing in the apparently generic code to look at the platform specific code.
At Wed, 27 May 2015 19:34:06 +0100, Mark Brown wrote: > > On Wed, May 27, 2015 at 08:05:47AM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > This is still as clear as mud, sorry - why is the ID table not something > > > that the bus handles? It seems like a core part of what a bus > > > abstracts. Having two completely separate sets of controller and device > > > drivers that can't interact with each other (which seems like what we're > > > ending up with here) seems really worrying and I don't think I've seen > > > any plan for unification either. It's not just the match function stuff > > > by the way, some of the later patches also made me wonder about the > > > mutiple implementations of the bus thing. > > > HDA bus is the place for communication between a device and a > > controller. This is common, no matter whether asoc or legacy. That > > is, an ASoC HDA controller may communicate even with an HDA legacy > > codec device, if really wanted. There is no distinction there. > > I'm unclear how that would (practically speaking) be accomplished if > they're in two separate worlds for matching. Come to think of it how > does this work if we have more than one matching system in use - how > does the driver model code cope with that, what happens if one match > function ends up looking at data for another match type? The matching function does the same thing, but it just looks at the different embedded id tables. The bus match function already checks each device/driver type beforehand. > > The fact you might be missing from the beginning of the story is that > > we need to implement two different driver sets for ASoC, both for > > controller and codec drivers. They can't be unified with legacy HDA > > unless we merge ASoC core codes into ALSA core (e.g. DPCM or DAPM into > > ALSA core). Also ASoC has the different ways of registration, too. > > Wow, that's not what I'd have expected to be happening at all and really > should have been mentioned at some point. I need to think about this > further. > > What I had expected to see for ASoC/HDA integration was something where > ASoC devices were providing a standard HDA controller and then use the > same CODEC drivers as everything else. Not doing that means that we > inveitably have some duplication for at least things like CODEC quirks > and device specific support which doesn't seem like the best idea. > There's also userspace interfaces for things like the HDA device graph > that'd need looking at. Actually that's the exact plan -- the codec graph will be parsed in user-space (or pre-parsed) and mapped via DFW. The quirks are still open questions. Currently, ASoC HDA is targeted only to the new systems with SKL. The rest will keep supported by the existing driver. So, we expect that the amount of quirks can be reduced much (starting from zero). > Having HDA use ASoC doesn't seem like a terrible idea, there are aspects > of HDA that map quite well onto ASoC, but doesn't seem to be the > intention here and I'm ambivalent about it being worth the effort. I > don't think it would need us to move ASoC into the core any more than > any other driver, it'd just make it much more widely used. Heh, that's the very reason why we started implementing a different driver set, after all. The new driver code is written in minimalistic manner to fit better with ASoC. The heavy part has been already shifted into HDA core side, that is basically a stripped version from the old HDA code. ASoC implementation was requested rather due to DPCM, compress support, etc, which is currently ASoC-specific features. The current patchset we've seen is really a tip of iceberg... > > > What you all seem to be saying is that the existing code for HDA is too > > > fragile to touch much so we don't want to move much of its functionality > > > to the new bus yet. I can believe that but I think I'd be a lot happier > > > if we were handling that by having the existing HDA code override bus > > > functionality rather than by implementing key bus functionality in > > > random places. It feels like trying to use the bus now is adding > > > technical debt. > > > Well, the patchset looks more complicated because lots of codes you're > > seeing in this patchset are specific to SKL hardware extension, and it > > doesn't (maybe won't) exist in HDA legacy. For example, patch 3 > > contains many such codes, which is likely the point where you stopped > > reading the rest patches. > > I'm too alarmed by what I'm seeing in the apparently generic code to > look at the platform specific code. Yeah, it's a bit unfortunate. Maybe it'd have been easier if they were split better. Takashi
On Wed, May 27, 2015 at 09:17:27PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > I'm unclear how that would (practically speaking) be accomplished if > > they're in two separate worlds for matching. Come to think of it how > > does this work if we have more than one matching system in use - how > > does the driver model code cope with that, what happens if one match > > function ends up looking at data for another match type? > The matching function does the same thing, but it just looks at the > different embedded id tables. The bus match function already checks > each device/driver type beforehand. How do we safely do the looking at different tables bit though? I should probably go back and look again at the original patch... > > > The fact you might be missing from the beginning of the story is that > > > we need to implement two different driver sets for ASoC, both for > > > controller and codec drivers. They can't be unified with legacy HDA > > > unless we merge ASoC core codes into ALSA core (e.g. DPCM or DAPM into > > > ALSA core). Also ASoC has the different ways of registration, too. > > > > Wow, that's not what I'd have expected to be happening at all and really > > should have been mentioned at some point. I need to think about this > > further. > > What I had expected to see for ASoC/HDA integration was something where > > ASoC devices were providing a standard HDA controller and then use the > > same CODEC drivers as everything else. Not doing that means that we > > inveitably have some duplication for at least things like CODEC quirks > > and device specific support which doesn't seem like the best idea. > > There's also userspace interfaces for things like the HDA device graph > > that'd need looking at. > Actually that's the exact plan -- the codec graph will be parsed in > user-space (or pre-parsed) and mapped via DFW. OK, more surprises! Looking at the Broadwell based I2S systems that don't work with any released set of upstream software (they need alsa-lib from git) I'm wondering how close these systems are to getting into the hands of users and what the transition from existing HDA to userspace parsing HDA is going to look like. I guess older distros will just bind the existing HDA controller drivers to them and use the hardware without any of the shiny new features which ought to be fine and I'm sure that once everyone has got the new stuff it will all be glorious, it's the space where people are upgrading (possibly upgrading the kernel or userspace separately) that worries me. Especially the bit where people ask me to make their sound work when they see their system uses ASoC! > The quirks are still open questions. Currently, ASoC HDA is targeted > only to the new systems with SKL. The rest will keep supported by the > existing driver. So, we expect that the amount of quirks can be > reduced much (starting from zero). I'm sure that our hardware engineering colleagues wouldn't want us to be bored and will give us something to do there :) > > Having HDA use ASoC doesn't seem like a terrible idea, there are aspects > > of HDA that map quite well onto ASoC, but doesn't seem to be the > > intention here and I'm ambivalent about it being worth the effort. I > > don't think it would need us to move ASoC into the core any more than > > any other driver, it'd just make it much more widely used. > Heh, that's the very reason why we started implementing a different > driver set, after all. The new driver code is written in minimalistic > manner to fit better with ASoC. The heavy part has been already > shifted into HDA core side, that is basically a stripped version from > the old HDA code. > ASoC implementation was requested rather due to DPCM, compress > support, etc, which is currently ASoC-specific features. The current > patchset we've seen is really a tip of iceberg... Right, and I'm aware of the reuse potential with sharing features with the embedded side (like we're already seeing with the I2S Broadwells) so if we don't integrate the two somehow we end up with duplicated drivers which would be sad. It's just that I'd anticipated that this would be being handled by black boxing the HDA part of the system from the ASoC point of view.
At Thu, 28 May 2015 20:53:59 +0100, Mark Brown wrote: > > On Wed, May 27, 2015 at 09:17:27PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > I'm unclear how that would (practically speaking) be accomplished if > > > they're in two separate worlds for matching. Come to think of it how > > > does this work if we have more than one matching system in use - how > > > does the driver model code cope with that, what happens if one match > > > function ends up looking at data for another match type? > > > The matching function does the same thing, but it just looks at the > > different embedded id tables. The bus match function already checks > > each device/driver type beforehand. > > How do we safely do the looking at different tables bit though? I > should probably go back and look again at the original patch... There is the device type check in hda_bus_match() before calling hda_driver->match(), so it must be safe. But as already mentioned, the match isn't the thing to worry too much, as it'll be unified sooner or later. The rest (still unseen) stuff is more worrisome. > > > > The fact you might be missing from the beginning of the story is that > > > > we need to implement two different driver sets for ASoC, both for > > > > controller and codec drivers. They can't be unified with legacy HDA > > > > unless we merge ASoC core codes into ALSA core (e.g. DPCM or DAPM into > > > > ALSA core). Also ASoC has the different ways of registration, too. > > > > > > Wow, that's not what I'd have expected to be happening at all and really > > > should have been mentioned at some point. I need to think about this > > > further. > > > > What I had expected to see for ASoC/HDA integration was something where > > > ASoC devices were providing a standard HDA controller and then use the > > > same CODEC drivers as everything else. Not doing that means that we > > > inveitably have some duplication for at least things like CODEC quirks > > > and device specific support which doesn't seem like the best idea. > > > There's also userspace interfaces for things like the HDA device graph > > > that'd need looking at. > > > Actually that's the exact plan -- the codec graph will be parsed in > > user-space (or pre-parsed) and mapped via DFW. > > OK, more surprises! > > Looking at the Broadwell based I2S systems that don't work with any > released set of upstream software (they need alsa-lib from git) I'm > wondering how close these systems are to getting into the hands of users > and what the transition from existing HDA to userspace parsing HDA is > going to look like. > > I guess older distros will just bind the existing HDA controller drivers > to them and use the hardware without any of the shiny new features which > ought to be fine and I'm sure that once everyone has got the new stuff > it will all be glorious, it's the space where people are upgrading > (possibly upgrading the kernel or userspace separately) that worries me. > Especially the bit where people ask me to make their sound work when > they see their system uses ASoC! Right, the time is still a question. Vinod and Liam can answer at best. And it's the reason I suggested for pending merge until the codec side is ready. We need the topology stuff merged at first, then HDA-codec driver together with user-space side. > > The quirks are still open questions. Currently, ASoC HDA is targeted > > only to the new systems with SKL. The rest will keep supported by the > > existing driver. So, we expect that the amount of quirks can be > > reduced much (starting from zero). > > I'm sure that our hardware engineering colleagues wouldn't want us to be > bored and will give us something to do there :) I have no doubt about it, too :) > > > Having HDA use ASoC doesn't seem like a terrible idea, there are aspects > > > of HDA that map quite well onto ASoC, but doesn't seem to be the > > > intention here and I'm ambivalent about it being worth the effort. I > > > don't think it would need us to move ASoC into the core any more than > > > any other driver, it'd just make it much more widely used. > > > Heh, that's the very reason why we started implementing a different > > driver set, after all. The new driver code is written in minimalistic > > manner to fit better with ASoC. The heavy part has been already > > shifted into HDA core side, that is basically a stripped version from > > the old HDA code. > > > ASoC implementation was requested rather due to DPCM, compress > > support, etc, which is currently ASoC-specific features. The current > > patchset we've seen is really a tip of iceberg... > > Right, and I'm aware of the reuse potential with sharing features with > the embedded side (like we're already seeing with the I2S Broadwells) so > if we don't integrate the two somehow we end up with duplicated drivers > which would be sad. It's just that I'd anticipated that this would be > being handled by black boxing the HDA part of the system from the ASoC > point of view. Yes, blackboxing the existing HDA was the first plan. But it has obvious drawbacks when joined to ASoC (inheriting a huge amount of quirks, different designs; HDA legacy is very self-contained, does DAPM-like path controls and jack-retasking by itself, etc). Potentially it's still possible to implement in that way, as a plan B, if a cleaner implementation from scratch fails. Let's see... Takashi
On Fri, May 29, 2015 at 06:58:24AM +0200, Takashi Iwai wrote: > At Thu, 28 May 2015 20:53:59 +0100, > Mark Brown wrote: > > > > On Wed, May 27, 2015 at 09:17:27PM +0200, Takashi Iwai wrote: > > > Mark Brown wrote: > > > > > > I'm unclear how that would (practically speaking) be accomplished if > > > > they're in two separate worlds for matching. Come to think of it how > > > > does this work if we have more than one matching system in use - how > > > > does the driver model code cope with that, what happens if one match > > > > function ends up looking at data for another match type? > > > > > The matching function does the same thing, but it just looks at the > > > different embedded id tables. The bus match function already checks > > > each device/driver type beforehand. > > > > How do we safely do the looking at different tables bit though? I > > should probably go back and look again at the original patch... > > There is the device type check in hda_bus_match() before calling > hda_driver->match(), so it must be safe. But as already mentioned, > the match isn't the thing to worry too much, as it'll be unified > sooner or later. The rest (still unseen) stuff is more worrisome. > > > > > > The fact you might be missing from the beginning of the story is that > > > > > we need to implement two different driver sets for ASoC, both for > > > > > controller and codec drivers. They can't be unified with legacy HDA > > > > > unless we merge ASoC core codes into ALSA core (e.g. DPCM or DAPM into > > > > > ALSA core). Also ASoC has the different ways of registration, too. > > > > > > > > Wow, that's not what I'd have expected to be happening at all and really > > > > should have been mentioned at some point. I need to think about this > > > > further. > > > > > > What I had expected to see for ASoC/HDA integration was something where > > > > ASoC devices were providing a standard HDA controller and then use the > > > > same CODEC drivers as everything else. Not doing that means that we > > > > inveitably have some duplication for at least things like CODEC quirks > > > > and device specific support which doesn't seem like the best idea. > > > > There's also userspace interfaces for things like the HDA device graph > > > > that'd need looking at. > > > > > Actually that's the exact plan -- the codec graph will be parsed in > > > user-space (or pre-parsed) and mapped via DFW. > > > > OK, more surprises! > > > > Looking at the Broadwell based I2S systems that don't work with any > > released set of upstream software (they need alsa-lib from git) I'm > > wondering how close these systems are to getting into the hands of users > > and what the transition from existing HDA to userspace parsing HDA is > > going to look like. well userspace parsing is only for the pin mapping and changes which users want to do on their systems. For basic things the default graph will be read by existing HDA parser and codec driver will create widgets based on that. The generic HDA codec library should do most and the HDA ASoC codecs should do device specfic work (like existing patch_xxx) Yes I agree it will take a while before all things fall into place and "Just work". > > I guess older distros will just bind the existing HDA controller drivers > > to them and use the hardware without any of the shiny new features which > > ought to be fine and I'm sure that once everyone has got the new stuff > > it will all be glorious, it's the space where people are upgrading > > (possibly upgrading the kernel or userspace separately) that worries me. > > Especially the bit where people ask me to make their sound work when > > they see their system uses ASoC! For HDA based systems, unless someone really wants ASoC features we should not enable them yet. I2S systems will be using ASoC. I expect the next generation to be mature enough for us to make HDA systems also use ASoC :) by default > > Right, the time is still a question. Vinod and Liam can answer at > best. For BDW, Liam is the best guy. I will be supporting platforms from SKL onwards, so feel free to redirect them to me. > > And it's the reason I suggested for pending merge until the codec side > is ready. We need the topology stuff merged at first, then HDA-codec > driver together with user-space side. I dont mind if code doesnt get merged into next merge window and put in one after that. But in needs to get accepted is some topic branch so that subsequent series can be psted and reviewed, merged. Btw users will not see unless we have machine driver. I will add the disable feature after this series, so no user conflicts occur > > > > The quirks are still open questions. Currently, ASoC HDA is targeted > > > only to the new systems with SKL. The rest will keep supported by the > > > existing driver. So, we expect that the amount of quirks can be > > > reduced much (starting from zero). > > > > I'm sure that our hardware engineering colleagues wouldn't want us to be > > bored and will give us something to do there :) > > I have no doubt about it, too :) > > > > > Having HDA use ASoC doesn't seem like a terrible idea, there are aspects > > > > of HDA that map quite well onto ASoC, but doesn't seem to be the > > > > intention here and I'm ambivalent about it being worth the effort. I > > > > don't think it would need us to move ASoC into the core any more than > > > > any other driver, it'd just make it much more widely used. > > > > > Heh, that's the very reason why we started implementing a different > > > driver set, after all. The new driver code is written in minimalistic > > > manner to fit better with ASoC. The heavy part has been already > > > shifted into HDA core side, that is basically a stripped version from > > > the old HDA code. > > > > > ASoC implementation was requested rather due to DPCM, compress > > > support, etc, which is currently ASoC-specific features. The current > > > patchset we've seen is really a tip of iceberg... > > > > Right, and I'm aware of the reuse potential with sharing features with > > the embedded side (like we're already seeing with the I2S Broadwells) so > > if we don't integrate the two somehow we end up with duplicated drivers > > which would be sad. It's just that I'd anticipated that this would be > > being handled by black boxing the HDA part of the system from the ASoC > > point of view. For us, the main issue was managing DSP along with HDA. The SKL DSP requires us to instantiate DSP toplogy and manage them, without DAPM n DPCM this was unthinkable. So we leaned on bringing HDA into ASoC. To use HDA controller the core HDA is in place now Codecs are different beast here, esp HDA issue of pin mapping and jack retasking. So idea is to have codec generic parser which creates ASOC widgets based on parsing and actual codec driver use this or load their own quirks. For pin mapping, machines can override configuration using toplogy and change a system to 7.1 audio or three stereo outputs or 2 stereo audio and 1 mic,. Using toplogy they can write and load their own configuration, thus moving these configuration out of kernel and getting us rid of platform hacks > > Yes, blackboxing the existing HDA was the first plan. But it has > obvious drawbacks when joined to ASoC (inheriting a huge amount of > quirks, different designs; HDA legacy is very self-contained, does > DAPM-like path controls and jack-retasking by itself, etc). > > Potentially it's still possible to implement in that way, as a plan B, > if a cleaner implementation from scratch fails. Let's see... Yes I would still think our plan A is a good one. I dont expect us to solve all probelms right away but by the time we meet again for u-Conf we shoudl have dealt with most and can discuss remaining ones. Pls do add this as topic :) So Mark, Takashi, Now that we have debated this and I guess all have the big picture here, how do we go about this. Do you guys need changes in this series or this is good for now?
On Fri, May 29, 2015 at 01:45:44PM +0530, Vinod Koul wrote: > well userspace parsing is only for the pin mapping and changes which > users want to do on their systems. For basic things the default graph > will be read by existing HDA parser and codec driver will create > widgets based on that. The generic HDA codec library should do most and > the HDA ASoC codecs should do device specfic work (like existing > patch_xxx) So we *are* planning to continue to use the existing CODEC support but with some different interfacing on top of it? > > And it's the reason I suggested for pending merge until the codec side > > is ready. We need the topology stuff merged at first, then HDA-codec > > driver together with user-space side. > I dont mind if code doesnt get merged into next merge window and put > in one after that. But in needs to get accepted is some topic branch > so that subsequent series can be psted and reviewed, merged. Btw > users will not see unless we have machine driver. I will add the > disable feature after this series, so no user conflicts occur I need to have another look through. TBH I suspect it would be easier with something that was more of a minimum viable prototype than with a part of the system - with something like this it's as much the overall shape of the solution that's being reviewed as anything else. > Now that we have debated this and I guess all have the big picture > here, how do we go about this. Do you guys need changes in this series > or this is good for now? Like I say I really need to review it again, and I'm still not entirely convinced I understand what's going on well enough.
On Fri, May 29, 2015 at 06:35:03PM +0100, Mark Brown wrote: > On Fri, May 29, 2015 at 01:45:44PM +0530, Vinod Koul wrote: > > > well userspace parsing is only for the pin mapping and changes which > > users want to do on their systems. For basic things the default graph > > will be read by existing HDA parser and codec driver will create > > widgets based on that. The generic HDA codec library should do most and > > the HDA ASoC codecs should do device specfic work (like existing > > patch_xxx) > > So we *are* planning to continue to use the existing CODEC support but > with some different interfacing on top of it? Nope. Existing codecs wont be reused here. We are doing new ASoC HDA codec drivers which use common hdac parsing logic. Since some codecs need own methods we can't write a single HDA generic codec, and had to do with generic codec parser and then specfic driver (HDMI, ALC to start with) on top. > > > > And it's the reason I suggested for pending merge until the codec side > > > is ready. We need the topology stuff merged at first, then HDA-codec > > > driver together with user-space side. > > > I dont mind if code doesnt get merged into next merge window and put > > in one after that. But in needs to get accepted is some topic branch > > so that subsequent series can be psted and reviewed, merged. Btw > > users will not see unless we have machine driver. I will add the > > disable feature after this series, so no user conflicts occur > > I need to have another look through. TBH I suspect it would be easier > with something that was more of a minimum viable prototype than with a > part of the system - with something like this it's as much the overall > shape of the solution that's being reviewed as anything else. Sure, please do let us know if you have more questions on this and overall approach.
On Mon, Jun 01, 2015 at 10:35:04AM +0530, Vinod Koul wrote: > On Fri, May 29, 2015 at 06:35:03PM +0100, Mark Brown wrote: > > On Fri, May 29, 2015 at 01:45:44PM +0530, Vinod Koul wrote: > > > well userspace parsing is only for the pin mapping and changes which > > > users want to do on their systems. For basic things the default graph > > > will be read by existing HDA parser and codec driver will create > > > widgets based on that. The generic HDA codec library should do most and > > > the HDA ASoC codecs should do device specfic work (like existing > > > patch_xxx) > > So we *are* planning to continue to use the existing CODEC support but > > with some different interfacing on top of it? > Nope. Existing codecs wont be reused here. > We are doing new ASoC HDA codec drivers which use common hdac parsing logic. > Since some codecs need own methods we can't write a single HDA generic > codec, and had to do with generic codec parser and then specfic driver > (HDMI, ALC to start with) on top. But I thought we already had a generic CODEC parser? Are you saying you're going to write a new one?
On Tue, Jun 02, 2015 at 11:38:43AM +0100, Mark Brown wrote: > On Mon, Jun 01, 2015 at 10:35:04AM +0530, Vinod Koul wrote: > > On Fri, May 29, 2015 at 06:35:03PM +0100, Mark Brown wrote: > > > On Fri, May 29, 2015 at 01:45:44PM +0530, Vinod Koul wrote: > > > > > well userspace parsing is only for the pin mapping and changes which > > > > users want to do on their systems. For basic things the default graph > > > > will be read by existing HDA parser and codec driver will create > > > > widgets based on that. The generic HDA codec library should do most and > > > > the HDA ASoC codecs should do device specfic work (like existing > > > > patch_xxx) > > > > So we *are* planning to continue to use the existing CODEC support but > > > with some different interfacing on top of it? > > > Nope. Existing codecs wont be reused here. > > > We are doing new ASoC HDA codec drivers which use common hdac parsing logic. > > Since some codecs need own methods we can't write a single HDA generic > > codec, and had to do with generic codec parser and then specfic driver > > (HDMI, ALC to start with) on top. > > But I thought we already had a generic CODEC parser? Are you saying > you're going to write a new one? No I meant we are going to use shiny new HDAC codec parser and build ASoC HDA codecs which use this new parser.
diff --git a/include/sound/soc-hda-codec.h b/include/sound/soc-hda-codec.h new file mode 100644 index 000000000000..850430a3855b --- /dev/null +++ b/include/sound/soc-hda-codec.h @@ -0,0 +1,49 @@ +/* + * ASoC High Definition Audio Codec interface Definitions + * + * Copyright (c) 2014-2015 Intel Corporation + * + * Author(s): Jeeja KP <jeeja.kp@intel.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + */ + +#ifndef __SOUND_SOC_HDA_CODEC_H +#define __SOUND_SOC_HDA_CODEC_H + +#include <sound/hdaudio.h> + +/* HDA device table */ +#define HDA_NAME_SIZE 20 +struct soc_hda_device_id { + __u32 id; + __u32 rev_id; + char name[HDA_NAME_SIZE]; + unsigned long driver_data; +}; + +struct soc_hda_codec_driver { + struct hdac_driver core; + const struct soc_hda_device_id *id_table; +}; + +#define to_hdac_driver(drv) (container_of((drv), \ + struct hdac_driver, driver)) +#define to_soc_hda_codec_driver(hdrv) (container_of((hdrv), \ + struct soc_hda_codec_driver, core)) + +int snd_soc_hda_codec_driver_register(struct soc_hda_codec_driver *drv); +void snd_soc_hda_codec_driver_unregister(struct soc_hda_codec_driver *drv); +const struct soc_hda_device_id *snd_soc_hda_get_device_id( + struct hdac_device *hdev, struct soc_hda_codec_driver *drv); + +#endif diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 3ba52da18bc6..d3903580cb10 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -40,6 +40,7 @@ source "sound/soc/cirrus/Kconfig" source "sound/soc/davinci/Kconfig" source "sound/soc/dwc/Kconfig" source "sound/soc/fsl/Kconfig" +source "sound/soc/hda/Kconfig" source "sound/soc/jz4740/Kconfig" source "sound/soc/nuc900/Kconfig" source "sound/soc/omap/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index 974ba708b482..8741d6a38bf6 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -21,6 +21,7 @@ obj-$(CONFIG_SND_SOC) += cirrus/ obj-$(CONFIG_SND_SOC) += davinci/ obj-$(CONFIG_SND_SOC) += dwc/ obj-$(CONFIG_SND_SOC) += fsl/ +obj-$(CONFIG_SND_SOC) += hda/ obj-$(CONFIG_SND_SOC) += jz4740/ obj-$(CONFIG_SND_SOC) += intel/ obj-$(CONFIG_SND_SOC) += mxs/ diff --git a/sound/soc/hda/Kconfig b/sound/soc/hda/Kconfig new file mode 100644 index 000000000000..815943360bc5 --- /dev/null +++ b/sound/soc/hda/Kconfig @@ -0,0 +1,3 @@ +config SND_SOC_HDA_CORE + tristate + select SND_HDA_CORE diff --git a/sound/soc/hda/Makefile b/sound/soc/hda/Makefile new file mode 100644 index 000000000000..9585ab180a55 --- /dev/null +++ b/sound/soc/hda/Makefile @@ -0,0 +1,3 @@ +snd-soc-hda-core-objs := soc-hda-codec.o + +obj-$(CONFIG_SND_SOC_HDA_CORE) += snd-soc-hda-core.o diff --git a/sound/soc/hda/soc-hda-codec.c b/sound/soc/hda/soc-hda-codec.c new file mode 100644 index 000000000000..90c8fa53e2cc --- /dev/null +++ b/sound/soc/hda/soc-hda-codec.c @@ -0,0 +1,89 @@ +/* + * soc-hda-codec.c ASoC High Definition Audio Codec interface Definitions + * + * Copyright (c) 2014-2015 Intel Corporation + * + * Author(s): Jeeja KP <jeeja.kp@intel.com> + * + * + * This driver is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This driver is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/module.h> +#include <sound/hdaudio.h> +#include <sound/soc-hda-codec.h> + +/** + * snd_soc_hda_get_device_id - gets the hdac device id entry + * @hdev: HD-audio core device + * @drv: HD-audio soc codec driver + * + * Compares the hdac device vendor_id and revision_id to the hdac_soc_codec + * driver id_table and returns the matching device id entry. + */ +const struct soc_hda_device_id * +snd_soc_hda_get_device_id(struct hdac_device *hdev, + struct soc_hda_codec_driver *drv) +{ + if (drv->id_table) { + const struct soc_hda_device_id *id = drv->id_table; + + while (id->name[0]) { + if (hdev->vendor_id == id->id && + (!id->rev_id || id->rev_id == hdev->revision_id)) + return id; + id++; + } + } + return NULL; +} +EXPORT_SYMBOL_GPL(snd_soc_hda_get_device_id); + +/** + * soc_hda_codec_match - ASoC HDA codec match function + * @dev: HD-audio core device + * @@drv: HD-audio soc codec driver + * + * This tries to match the hdac device with hdac driver and return 1 on + * match, 0 otherwise + */ +static int soc_hda_codec_match(struct hdac_device *dev, struct hdac_driver *drv) +{ + struct soc_hda_codec_driver *driver = to_soc_hda_codec_driver(drv); + + if (snd_soc_hda_get_device_id(dev, driver)) + return 1; + else + return 0; +} + +/** + * snd_soc_hda_codec_driver_register - register hda codec driver + * @drv: HD Audio soc codec driver + */ +int snd_soc_hda_codec_driver_register(struct soc_hda_codec_driver *drv) +{ + drv->core.driver.bus = &snd_hda_bus_type; + drv->core.type = HDA_DEV_ASOC; + drv->core.match = soc_hda_codec_match; + return driver_register(&drv->core.driver); +} +EXPORT_SYMBOL_GPL(snd_soc_hda_codec_driver_register); + +/** + * snd_soc_hda_codec_driver_unregister - unregister hda codec driver + * @drv: HD Audio soc codec driver + */ +void snd_soc_hda_codec_driver_unregister(struct soc_hda_codec_driver *drv) +{ + driver_unregister(&drv->core.driver); +} +EXPORT_SYMBOL_GPL(snd_soc_hda_codec_driver_unregister);