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