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.
+/**
- 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.
+/**
- 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.
- 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.
- 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?)
+/**
- 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.
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?
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?
if (strlen(hlink->codec[i]) == 0)break;
It can be simplified like if (!hlink->codec[i][0])
if (!strncmp(hlink->codec[i], codec_name,sizeof(codec_name)))
This looks buggy. sizeof(codec_name) == sizeof(const char *) == 4 or 8.
return hlink;}- }
- return hlink;
This also looks buggy. When the loop is out, hlink isn't 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.
+/* 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.
Takashi