On 10/19/21 12:32 PM, Cezary Rojewski wrote:
On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:
hmm. It's not clear whether we should initialize the regular hdac_bus fields in the ext_bus_init(). For plain HDA, these are also initialized in the caller. E.g. in sound/pci/hda/hda_controller.c.
So if we start cleaning up, should we go further put this in snd_hdac_bus_init()?
Then another is what is the right place for settings like "sync_write = 1". While this settings applies to all current users of the extended bus, this is really hw specific setting and not really a property of the extended bus, so this feels a bit out-of-place.
I'm rather in favor of bigger cleanups. We can definitely move further in the future : )
I am not opposed to updates in this hdaudio-ext part, but I am in favor of less ambitious step-by-step changes.
a) This is legacy code where the initial authors have moved on, and it's very hard to figure out what the intent was. It's quite common to have discussion on feature v. bug, see e.g. the discussion we had on this in https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
b) In addition, this code is shared between two teams with separate testing/CI capabilities and limited number of commercial/shipping devices. There is a very large risk of introducing bugs even with the best intentions.
Before we do any changes in functionality, there are already basic steps that can be done, e.g. using consistent naming between variables, the existing code is just confusing as can be:
struct hdac_stream *stream; struct hdac_ext_stream *stream; struct hdac_stream *hstream; struct hdac_ext_stream *hstream;
I started basic cleanups here: https://github.com/thesofproject/linux/pull/3158, I haven't had time to complete this because of more important locking issues, but I intend to send those clarifications for the next cycle.
Again before we do large changes let's think of smaller steps and how we are going to validate those changes.
Agree, step-by-step is the way to go.
Isn't this patch a 'step' though? This isn't remotely a refactor, just gathering of common parts of ext-bus initialization. We could start with this so legacy users are unaffected, then have snd_hdac_bus_init() updated so snd_hdac_ext_bus_init() is devoid of 'core' fields assignments as suggested by Kai.
If it was just moving common parts, I would have no issue. My main objection is that you went one step further and started renaming stuff in rather confusing ways, e.g.
-void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev) +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
- * snd_hdac_ext_bus_init - initialize a HD-audio extended bus + * snd_hda_ext_bus_init - initialize a HD-audio extended bus
hda_bus is a definition from hda_codec.h, I don't see a reason why we should use this structure when the vast majority of the code uses hdac_bus. And the purpose of hdac_ext is really to deal with *controller* extensions to couple/decouple DMAs. There is no dependency on codecs at that level.
Even if there was, I also don't really see why/when we should start using hda_ext v. hdac_ext, the naming convention completely escapes me. It would seem logical to me to only use hdac_ext as an extension of hdac_, no?
I also don't get what this means: + snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
what is 'sklbus' and what purpose are you trying to accomplish with this change?