Re: [PATCH v3 3/7] ASoC: hda - adds SoC controller and stream operations

[Vinod Koul <vinod.koul@intel.com>]

On Wed, Apr 29, 2015 at 02:26:40PM +0200, Takashi Iwai wrote:
> At Wed, 29 Apr 2015 01:24:26 +0530,
> Vinod Koul wrote:
> > 
> > +struct soc_hdac_stream {
> > +	struct hdac_stream hstream;
> > +	unsigned int decoupled:1;
> > +	void __iomem *pphc_addr; /* processing pipe host stream reg pointer */
> > +	void __iomem *pplc_addr; /* processing pipe link stream reg pointer */
> > +	bool link_locked:1;
> > +	struct snd_pcm_substream *link_substream;
> > +	bool link_prepared;
> 
> The bit fields should be gathered into the same place so that the
> struct can be packed better.  Also, use bool consistently for bit
> fields, too.
Sure, btw should we use bitfields or bool alone. I dont see much reason to
use bitfields here?

> > +/**
> > + * snd_soc_hdac_bus_parse_capabilities - parse capablity structure
> > + * @sbus - HD-audio soc core bus
> > + */
> > +int snd_soc_hdac_bus_parse_capabilities(struct soc_hdac_bus *sbus)
> > +{
> > +	unsigned int cur_cap;
> > +	unsigned int offset;
> > +	struct hdac_bus *bus = &sbus->bus;
> > +
> > +	offset = snd_hdac_chip_readl(bus, LLCH);
> > +
> > +	sbus->ppcap = false;
> > +	sbus->mlcap = false;
> > +	sbus->spbcap = false;
> > +	sbus->gtscap = false;
> > +
> > +	/* Lets walk the linked capabilities list */
> > +	do {
> 
> I'd check the validity of the offset value, at least, to a negative
> value.  When a chip or bus is screwed up, it would return -1.
Ok

> 
> 
> > +/**
> > + * snd_soc_hdac_bus_get_ml_capablities - get multilink capablity
> > + * @sbus - HD-audio soc core bus
> > + */
> > +int snd_soc_hdac_bus_get_ml_capablities(struct soc_hdac_bus *sbus)
> > +{
> > +	int idx = 0;
> 
> Superfluous initialization.
> 
> > +	u32 link_count = 0;
> 
> Ditto.
will remove

> 
> > +	struct soc_hdac_link *hlink;
> > +	struct hdac_bus *bus = &sbus->bus;
> > +
> > +	INIT_LIST_HEAD(&sbus->hlink_list);
> 
> This should be done better in the initializer of soc_hdac_bus object.
Good catch

> 
> > +
> > +	link_count = soc_hdac_bus_mlcap_readb(sbus, ML_MLCD) + 1;
> > +
> > +	dev_dbg(bus->dev, "In %s Link count: %d\n", __func__, link_count);
> > +
> > +	for (idx = 0; idx < link_count; idx++) {
> > +		hlink  = devm_kzalloc(bus->dev, sizeof(*hlink), GFP_KERNEL);
> > +		if (!hlink)
> > +			return -ENOMEM;
> > +		hlink->index = idx;
> > +		hlink->bus = bus;
> > +		hlink->ml_addr = sbus->mlcap_addr +
> > +					ML_BASE +
> > +					(ML_INTERVAL *
> > +					idx);
> > +		hlink->lcaps  = soc_hdac_link_readw(hlink, ML_LCAP);
> > +		hlink->lsdiid = soc_hdac_link_readw(hlink, ML_LSDIID);
> > +
> > +		list_add(&hlink->list, &sbus->hlink_list);
> 
> list_add_tail() is used more often.  (Does the order matter?)
I dont think so... but i agree would make sense to order it

> 
> > +/**
> > + * snd_soc_hdac_bus_map_codec_to_link - maps codec to link
> > + * @sbus - HD-audio soc core bus
> > + * @addr - codec address
> > + */
> > +int snd_soc_hdac_bus_map_codec_to_link(struct soc_hdac_bus *sbus, int addr)
> > +{
> > +	struct soc_hdac_link *hlink;
> > +	struct hdac_bus *bus = &sbus->bus;
> > +
> > +	list_for_each_entry(hlink, &sbus->hlink_list, list) {
> > +		/*check if SDI bit number == Codec address */
> > +		dev_dbg(bus->dev, "lsdid for %d link %x\n", hlink->index, hlink->lsdiid);
> > +		if (!(hlink->lsdiid))
> 
> Superfluous parentheses.
will fix

> 
> > +			continue;
> > +
> > +		if (hlink->lsdiid && (0x1 << addr)) {
> > +			snprintf(hlink->codec[addr],
> > +					sizeof(hlink->codec[addr]),
> > +					"codec#%03x.%d", addr, addr);
> 
> Does repeating the address twice make sense?
It doesnt :)

> 
> 
> > +			break;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(snd_soc_hdac_bus_map_codec_to_link);
> > +
> > +/**
> > + * snd_soc_hdac_bus_get_link_index - get link based on codec name
> > + * @sbus - HD-audio soc core bus
> > + * @codec_name - codec name
> > + */
> > +struct soc_hdac_link *snd_soc_hdac_bus_get_link(struct soc_hdac_bus *sbus,
> > +						 const char *codec_name)
> > +{
> > +	int i = 0;
> > +	struct soc_hdac_link *hlink = NULL;
> > +
> > +	list_for_each_entry(hlink, &sbus->hlink_list, list) {
> > +		for (i = 0; i < 16 ; i++) {
> 
> Where does 16 comes from?  Not HDA_MAX_CODECS?
HDA Spec :) but i think we should use HDA_MAX_CODECS rather

> 
> > +			if (strlen(hlink->codec[i]) == 0)
> > +				break;
> 
> It can be simplified like
> 			if (!hlink->codec[i][0])
yes

> 
> > +			if (!strncmp(hlink->codec[i], codec_name,
> > +					sizeof(codec_name)))
> 
> This looks buggy.  sizeof(codec_name) == sizeof(const char *) == 4 or 8.
yes it should be strlen() instead

> 
> > +				return hlink;
> > +		}
> > +	}
> > +	return hlink;
> 
> This also looks buggy.  When the loop is out, hlink isn't NULL.
yes we should make it return NULL

> 
> > +}
> > +EXPORT_SYMBOL_GPL(snd_soc_hdac_bus_get_link);
> > +
> > +/**
> > + * snd_soc_hdac_bus_link_power_up -power up hda link
> > + * @link - HD-audio soc link
> > + */
> > +int snd_soc_hdac_bus_link_power_up(struct soc_hdac_link *link)
> > +{
> > +	int timeout;
> > +	u32 val;
> > +	int mask = (1 << MLCTL_CPA);
> > +
> > +	soc_hdac_link_updatel(link, ML_LCTL, 0, MLCTL_SPA);
> > +	udelay(3);
> > +	timeout = 300;
> > +
> > +	do {
> > +		val = soc_hdac_link_readl(link, ML_LCTL);
> > +		if (((val & mask) >> MLCTL_CPA))
> > +			return 0;
> > +	} while (--timeout);
> 
> How 300 reads timeout calculated?  There is no delay in the loop, so
> it's quite short.
I think we should to cpu_relax or add a delay here

> 
> 
> > +/* Module information */
> > +MODULE_AUTHOR("Jeeja KP <jeeja.kp@intel.com>");
> > +MODULE_DESCRIPTION("HDA SoC core");
> > +MODULE_LICENSE("GPL v2");
> 
> There is already module information in soc-hda-codec.c.
yes will eliminate the duplicate

-- 
~Vinod