On 2021-10-19 8:42 PM, Pierre-Louis Bossart wrote:
On 10/19/21 12:32 PM, Cezary Rojewski wrote:
On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:
...
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
Reason for renaming snd_hdac_ext_bus_init() to snd_hda_ext_bus_init() is
argument type change: 'struct hdac_bus *' to struct hda_bus *'. Don't
believe that's confusing to anybody.
No problem with reverting naming change for now - we can streamline
naming later.
In regard to sof_hda_bus_init(): I don't see any renaming here, just
argument type changes to match new snd_hda_ext_bus_init() usage.
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.
hda_bus is the base for all HDAudio drivers:
struct azx
struct skl_dev
struct sof_intel_hda_dev
So no, what vast majority of code actually does is hda_bus/codec to
hdac_bus/codec (and vice-versa) translation when in fact all the drivers
are hda_* based. If you speak of confusion, that's the confusion that
should be addressed in the future..
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?
Well, please see the updated declaration of snd_hda_ext_bus_init() in
this very patch and then the existing code of
sound/soc/intel/skylake/skl.c - skl_create().
Last argument in updated declaration reads 'modelname'. Skylake-driver
has its own, SOF initializes it differently.
Regads,
Czarek
