Re: Why open-coding in sof_hda_bus_init()?

[Takashi Iwai <tiwai@suse.de>]

On Fri, 31 May 2019 19:43:59 +0200,
Pierre-Louis Bossart wrote:
> 
> On 5/31/19 12:11 PM, Takashi Iwai wrote:
> > Hi,
> >
> > while looking at SOF code due to the recent debugging session, I
> > noticed that sof_hda_bus_init() is basically an open-code of the
> > existing snd_hdac_ext_bus_init().  Why don't we simply call
> > snd_hdac_ext_bus_init() like below?
> 
> It's intentional.
> We've been asked since Day1 of SOF on ApolloLake to provide a
> 'self-contained' controller-only support that has no dependency on the
> snd_hdac library for solutions where HDaudio links+codecs are not used
> (typically IOT devices). This was driven by the lack of separation
> between layers in that library as well as a desire to have a
> dual-license. That's why you see the init and some of the basic
> utilities re-implemented for SOF.
> 
> However for cases where HDaudio+HDMI are required, we didn't want to
> reinvent the wheel - HDaudio is complicated enough - and do make use
> of this snd_hdac library.
> 
> We have a config SND_SOC_SOF_HDA that controls in which mode we
> operate, and it enables HDMI by default (for I2S+HDMI solutions). To
> get external HDaudio codecs you need the additional SOF_HDAUDIO_CODEC
> kconfig.
> 
> Does this help?

Well, what's wrong with the conditional build with Kconfig?
You can just wrap the call snd_hdac_ext_bus_init() with #if/endif,
e.g. in soc/sof/intel/hda.h,

static inline void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
				    const struct hdac_ext_bus_ops *ext_ops)
{
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
	snd_hdac_ext_bus_init(bus, dev, NULL, NULL, ext_ops);
#endif
}


In genral, the open-code is very bad from the maintenance POV.  And,
even worse, currently the hda-bus.c does only initialization, and the
release is with the hda-bus code.


thanks,

Takashi

> 
> >
> > The only thing that differs is the handling of bus->id number, where
> > SOF fixes to zero (which was actually done in a patch after the first
> > commit).  But judging from the comment, this doesn't seem like a big
> > matter, as we assume a single bus, so far...
> >
> >
> > thanks,
> >
> > Takashi
> >
> > ---
> > diff --git a/sound/soc/sof/intel/Makefile b/sound/soc/sof/intel/Makefile
> > index b8f58e006e29..f42450a9a7b6 100644
> > --- a/sound/soc/sof/intel/Makefile
> > +++ b/sound/soc/sof/intel/Makefile
> > @@ -7,7 +7,7 @@ snd-sof-intel-ipc-objs := intel-ipc.o
> >     snd-sof-intel-hda-common-objs := hda.o hda-loader.o hda-stream.o
> > hda-trace.o \
> >   				 hda-dsp.o hda-ipc.o hda-ctrl.o hda-pcm.o \
> > -				 hda-dai.o hda-bus.o \
> > +				 hda-dai.o \
> >   				 apl.o cnl.o
> >     snd-sof-intel-hda-objs := hda-codec.o
> > diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
> > deleted file mode 100644
> > index a7e6d8227df6..000000000000
> > --- a/sound/soc/sof/intel/hda-bus.c
> > +++ /dev/null
> > @@ -1,111 +0,0 @@
> > -// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> > -//
> > -// This file is provided under a dual BSD/GPLv2 license.  When using or
> > -// redistributing this file, you may do so under either license.
> > -//
> > -// Copyright(c) 2018 Intel Corporation. All rights reserved.
> > -//
> > -// Authors: Keyon Jie <yang.jie@linux.intel.com>
> > -
> > -#include <linux/io.h>
> > -#include <sound/hdaudio.h>
> > -#include "../sof-priv.h"
> > -#include "hda.h"
> > -
> > -#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
> > -
> > -static const struct hdac_bus_ops bus_ops = {
> > -	.command = snd_hdac_bus_send_cmd,
> > -	.get_response = snd_hdac_bus_get_response,
> > -};
> > -
> > -#endif
> > -
> > -static void sof_hda_writel(u32 value, u32 __iomem *addr)
> > -{
> > -	writel(value, addr);
> > -}
> > -
> > -static u32 sof_hda_readl(u32 __iomem *addr)
> > -{
> > -	return readl(addr);
> > -}
> > -
> > -static void sof_hda_writew(u16 value, u16 __iomem *addr)
> > -{
> > -	writew(value, addr);
> > -}
> > -
> > -static u16 sof_hda_readw(u16 __iomem *addr)
> > -{
> > -	return readw(addr);
> > -}
> > -
> > -static void sof_hda_writeb(u8 value, u8 __iomem *addr)
> > -{
> > -	writeb(value, addr);
> > -}
> > -
> > -static u8 sof_hda_readb(u8 __iomem *addr)
> > -{
> > -	return readb(addr);
> > -}
> > -
> > -static int sof_hda_dma_alloc_pages(struct hdac_bus *bus, int type,
> > -				   size_t size, struct snd_dma_buffer *buf)
> > -{
> > -	return snd_dma_alloc_pages(type, bus->dev, size, buf);
> > -}
> > -
> > -static void sof_hda_dma_free_pages(struct hdac_bus *bus,
> > -				   struct snd_dma_buffer *buf)
> > -{
> > -	snd_dma_free_pages(buf);
> > -}
> > -
> > -static const struct hdac_io_ops io_ops = {
> > -	.reg_writel = sof_hda_writel,
> > -	.reg_readl = sof_hda_readl,
> > -	.reg_writew = sof_hda_writew,
> > -	.reg_readw = sof_hda_readw,
> > -	.reg_writeb = sof_hda_writeb,
> > -	.reg_readb = sof_hda_readb,
> > -	.dma_alloc_pages = sof_hda_dma_alloc_pages,
> > -	.dma_free_pages = sof_hda_dma_free_pages,
> > -};
> > -
> > -/*
> > - * This can be used for both with/without hda link support.
> > - */
> > -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
> > -		      const struct hdac_ext_bus_ops *ext_ops)
> > -{
> > -	memset(bus, 0, sizeof(*bus));
> > -	bus->dev = dev;
> > -
> > -	bus->io_ops = &io_ops;
> > -	INIT_LIST_HEAD(&bus->stream_list);
> > -
> > -	bus->irq = -1;
> > -	bus->ext_ops = ext_ops;
> > -
> > -	/*
> > -	 * There is only one HDA bus atm. keep the index as 0.
> > -	 * Need to fix when there are more than one HDA bus.
> > -	 */
> > -	bus->idx = 0;
> > -
> > -	spin_lock_init(&bus->reg_lock);
> > -
> > -#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
> > -	INIT_LIST_HEAD(&bus->codec_list);
> > -	INIT_LIST_HEAD(&bus->hlink_list);
> > -
> > -	mutex_init(&bus->cmd_mutex);
> > -	mutex_init(&bus->lock);
> > -	bus->ops = &bus_ops;
> > -	INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
> > -	bus->cmd_dma_state = true;
> > -#endif
> > -
> > -}
> > diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> > index 92d45c43b4b1..a9de6a297b56 100644
> > --- a/sound/soc/sof/intel/hda.h
> > +++ b/sound/soc/sof/intel/hda.h
> > @@ -532,8 +532,11 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset);
> >   /*
> >    * HDA bus operations.
> >    */
> > -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
> > -		      const struct hdac_ext_bus_ops *ext_ops);
> > +static inline void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
> > +				    const struct hdac_ext_bus_ops *ext_ops)
> > +{
> > +	snd_hdac_ext_bus_init(bus, dev, NULL, NULL, ext_ops);
> > +}
> >     #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
> >   /*
> >
>