Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

[Srinivas Kandagatla]

Thanks for taking time to review,

On 07/06/2019 14:36, Pierre-Louis Bossart wrote:
> > +config SOUNDWIRE_QCOM
> > +ÂÂÂ tristate "Qualcomm SoundWire Master driver"
> > +ÂÂÂ select SOUNDWIRE_BUS
> > +ÂÂÂ depends on SND_SOC
>
> depends on SLIMBUS if you need the SlimBus link to talk to your 
> SoundWire Master?
>
> Also depends on device tree since you use of_ functions?
Thats true, will fix this in next version.
> > +#define SWRM_COMP_HW_VERSIONÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00
>
> Can we please use SDW_ or QCOM_SDW_ as prefix?

SWRM prefix is as per the data sheet register names, If it help am happy 
to add QCOM_ prefix it.

> > +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHEDÂÂÂ BIT(11)
> > +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILEDÂÂÂÂÂÂÂÂÂÂÂ BIT(12)
> > +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULLÂÂÂÂÂÂÂ BIT(13)
>
> This hints at hardware support to assign Device Numbers automagically so 
> will likely have impacts on the bus driver code, no?

yes, this controller has autoenumeration support, however I had disabled 
this code due to the fact that we need some more support in bus driver 
to accommodate this feature.

I will explore this side once again, may be before sending next version.


> > +#define SWRM_MAX_ROW_VALÂÂÂ 0 /* Rows = 48 */
> > +#define SWRM_DEFAULT_ROWSÂÂÂ 48
> > +#define SWRM_MIN_COL_VALÂÂÂ 0 /* Cols = 2 */
> > +#define SWRM_DEFAULT_COLÂÂÂ 16
> > +#define SWRM_SPECIAL_CMD_IDÂÂÂ 0xF
> > +#define MAX_FREQ_NUMÂÂÂÂÂÂÂ 1
> > +#define TIMEOUT_MSÂÂÂÂÂÂÂ 1000
> > +#define QCOM_SWRM_MAX_RD_LENÂÂÂ 0xf
> > +#define DEFAULT_CLK_FREQÂÂÂ 9600000
>
> The clocks and frame shape don't match usual expectations for PDM.
> For a 9.6 MHz support, you would typically use 8 columns and 50 rows to 
> transport PDM with a 50x oversampling. I've never seen anyone use 48x 
> for PDM.
>
> > +#define SWRM_MAX_DAISÂÂÂÂÂÂÂ 0xF
> > +
> > +struct qcom_swrm_port_config {
> > +ÂÂÂ u8 si;
> > +ÂÂÂ u8 off1;
> > +ÂÂÂ u8 off2;
> > +};
> > +
> > +struct qcom_swrm_ctrl {
> > +ÂÂÂ struct sdw_bus bus;
> > +ÂÂÂ struct device *dev;
> > +ÂÂÂ struct regmap *regmap;
> > +ÂÂÂ struct completion sp_cmd_comp;
> > +ÂÂÂ struct work_struct slave_work;
> > +ÂÂÂ /* read/write lock */
> > +ÂÂÂ struct mutex lock;
> > +ÂÂÂ /* Port alloc/free lock */
> > +ÂÂÂ struct mutex port_lock;
> > +ÂÂÂ struct clk *hclk;
> > +ÂÂÂ int fifo_status;
> > +ÂÂÂ void __iomem *base;
> > +ÂÂÂ u8 wr_cmd_id;
> > +ÂÂÂ u8 rd_cmd_id;
> > +ÂÂÂ int irq;
> > +ÂÂÂ unsigned int version;
> > +ÂÂÂ int num_din_ports;
> > +ÂÂÂ int num_dout_ports;
> > +ÂÂÂ unsigned long dout_port_mask;
> > +ÂÂÂ unsigned long din_port_mask;
> > +ÂÂÂ struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];
>
> this is not necessarily correct. the initial definitions for 
> SDW_MAX_PORTS was for Slave devices. There is no definitions for Masters 
> in the SoundWire spec, so you could use whatever constant you want for 
> your hardware.

Yes, I can move this define to this driver specific.

> > +ÂÂÂ struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
> > +ÂÂÂ enum sdw_slave_status status[SDW_MAX_DEVICES];
> > +ÂÂÂ u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
> > +ÂÂÂ int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
> > +};
> > +
> > +#define to_qcom_sdw(b)ÂÂÂ container_of(b, struct qcom_swrm_ctrl, bus)
> > +
> > +struct usecase {
> > +ÂÂÂ u8 num_port;
> > +ÂÂÂ u8 num_ch;
> > +ÂÂÂ u32 chrate;
> > +};
>
> this structure doesn't seem to be used?
looks like left over, I will remove this.

> > +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 
> > cmd_data,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u8 dev_addr, u16 reg_addr)
> > +{
> > +ÂÂÂ int ret = 0;
> > +ÂÂÂ u8 cmd_id;
> > +ÂÂÂ u32 val;
> > +
> > +ÂÂÂ mutex_lock(&ctrl->lock);
> > +ÂÂÂ if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> > +ÂÂÂÂÂÂÂ cmd_id = SWRM_SPECIAL_CMD_ID;
> > +ÂÂÂ } else {
> > +ÂÂÂÂÂÂÂ if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
> > +ÂÂÂÂÂÂÂÂÂÂÂ ctrl->wr_cmd_id = 0;
> > +
> > +ÂÂÂÂÂÂÂ cmd_id = ctrl->wr_cmd_id;
> > +ÂÂÂ }
>
> might be worth having a helper/macro since you are doing the same thing 
> below.

I will do that!

> > +
> > +ÂÂÂ val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
> > +ÂÂÂ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> > +ÂÂÂ if (ret < 0) {
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂ __func__, val, ret);
> > +ÂÂÂÂÂÂÂ goto err;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> > +ÂÂÂÂÂÂÂ ctrl->fifo_status = 0;
> > +ÂÂÂÂÂÂÂ ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ msecs_to_jiffies(TIMEOUT_MS));
>
> This is odd. The SoundWire spec does not handle writes to a single 
> device or broadcast writes differently. I don't see a clear reason why 
> you would only timeout for a broadcast write.

There is danger of blocking here without timeout.

> > +
> > +ÂÂÂÂÂÂÂ if (!ret || ctrl->fifo_status) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(ctrl->dev, "reg 0x%x write failed\n", val);
> > +ÂÂÂÂÂÂÂÂÂÂÂ ret = -ENODATA;
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto err;
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +err:
> > +ÂÂÂ mutex_unlock(&ctrl->lock);
> > +ÂÂÂ return ret;
> > +}
> > +
> > +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u8 dev_addr, u16 reg_addr,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u32 len, u8 *rval)
> > +{
> > +ÂÂÂ int i, ret = 0;
> > +ÂÂÂ u8 cmd_id = 0;
> > +ÂÂÂ u32 val;
> > +
> > +ÂÂÂ mutex_lock(&ctrl->lock);
> > +ÂÂÂ if (dev_addr == SDW_ENUM_DEV_NUM) {
> > +ÂÂÂÂÂÂÂ cmd_id = SWRM_SPECIAL_CMD_ID;
> > +ÂÂÂ } else {
> > +ÂÂÂÂÂÂÂ if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
> > +ÂÂÂÂÂÂÂÂÂÂÂ ctrl->rd_cmd_id = 0;
> > +
> > +ÂÂÂÂÂÂÂ cmd_id = ctrl->rd_cmd_id;
> > +ÂÂÂ }
>
> helper?
yes will add that.

> > +ÂÂÂ val = SWRM_REG_VAL_PACK(len, dev_addr, cmd_id, reg_addr);
> > +ÂÂÂ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
> > +ÂÂÂ if (ret < 0) {
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "reg 0x%x write failed err:%d\n", val, ret);
> > +ÂÂÂÂÂÂÂ goto err;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if (dev_addr == SDW_ENUM_DEV_NUM) {
> > +ÂÂÂÂÂÂÂ ctrl->fifo_status = 0;
> > +ÂÂÂÂÂÂÂ ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ msecs_to_jiffies(TIMEOUT_MS));
> same comment here, there isn't a clear reason to only timeout for a read 
> from device0.
>
> > +ÂÂÂÂÂÂÂ if (!ret || ctrl->fifo_status) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(ctrl->dev, "reg 0x%x read failed\n", val);
> > +ÂÂÂÂÂÂÂÂÂÂÂ ret = -ENODATA;
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto err;
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
> > +ÂÂÂ for (i = 0; i < len; i++) {
> > +ÂÂÂÂÂÂÂ rval[i] = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR);
> > +ÂÂÂÂÂÂÂ rval[i] &= 0xFF;
> > +ÂÂÂ }
> > +
> > +err:
> > +ÂÂÂ mutex_unlock(&ctrl->lock);
> > +ÂÂÂ return ret;
> > +}
> > +
> > +static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> > +{
> > +ÂÂÂ u32 val = ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS);
> > +ÂÂÂ int i;
> > +
> > +ÂÂÂ for (i = 1; i < SDW_MAX_DEVICES; i++) {
> > +ÂÂÂÂÂÂÂ u32 s;
> > +
> > +ÂÂÂÂÂÂÂ s = (val >> (i * 2));
> > +ÂÂÂÂÂÂÂ s &= SWRM_MCP_SLV_STATUS_MASK;
> > +ÂÂÂÂÂÂÂ ctrl->status[i] = s;
> > +ÂÂÂ }
> > +}
> > +
> > +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_id;
> > +ÂÂÂ u32 sts, value;
> > +
> > +ÂÂÂ sts = ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS);
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> > +ÂÂÂÂÂÂÂ complete(&ctrl->sp_cmd_comp);
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> > +ÂÂÂÂÂÂÂ value = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS);
> > +ÂÂÂÂÂÂÂ dev_err_ratelimited(ctrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "CMD error, fifo status 0x%x\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ value);
> > +ÂÂÂÂÂÂÂ ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> > +ÂÂÂÂÂÂÂ if ((value & 0xF) == 0xF) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ ctrl->fifo_status = -ENODATA;
> > +ÂÂÂÂÂÂÂÂÂÂÂ complete(&ctrl->sp_cmd_comp);
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> > +ÂÂÂÂÂÂÂ sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS) {
> > +ÂÂÂÂÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> > +ÂÂÂÂÂÂÂÂÂÂÂ ctrl->status[0] = SDW_SLAVE_ATTACHED;
> > +
> > +ÂÂÂÂÂÂÂ schedule_work(&ctrl->slave_work);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ)
> > +ÂÂÂÂÂÂÂ dev_dbg(ctrl->dev, "Slave pend irq\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED)
> > +ÂÂÂÂÂÂÂ dev_dbg(ctrl->dev, "New slave attached\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET)
> > +ÂÂÂÂÂÂÂ dev_err_ratelimited(ctrl->dev, "Bus clash detected\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Read FIFO overflow\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Read FIFO underflow\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Write FIFO overflow\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Port collision detected\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Read enable valid mismatch\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Cmd id finished\n");
> > +
> > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Bus reset finished\n");
>
> This list is odd as well. It makes sense to only log error cases if you 
> don't really know how to handle them, but a 'NEW SLAVE ATTACHED' should 
> lead to an action, no?

NEW SLAVE ATTACHED interrupt already schedules action by reporting this 
to soudwire bus layer.
Some of them are leftover as part of debug, i will try to clean them up 
and see if some of them can be handled properly.
> > +
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
> > +
> > +ÂÂÂ return IRQ_HANDLED;
> > +}
> > +
> > +static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> > +{
> > +ÂÂÂ u32 val;
> > +ÂÂÂ u8 row_ctrl = SWRM_MAX_ROW_VAL;
> > +ÂÂÂ u8 col_ctrl = SWRM_MIN_COL_VAL;
> > +ÂÂÂ u8 ssp_period = 1;
> > +ÂÂÂ u8 retry_cmd_num = 3;
>
> probably want a define for those magic values, they are quite important.

Yep, will do that!

> > +
> > +ÂÂÂ /* Clear Rows and Cols */
> > +ÂÂÂ val = ((row_ctrl << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> > +ÂÂÂÂÂÂÂ (col_ctrl << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT) |
> > +ÂÂÂÂÂÂÂ (ssp_period << SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFT));
> > +
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
> > +
> > +ÂÂÂ /* Disable Auto enumeration */
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
>
> This goes back to my earlier comment. Do you disable this 
> auto-enumeration to avoid conflicts with the existing bus management? 
> That's not necessarily smart, we may want to change that bus layer to 
> reduce the enumeration time if hardware can do it.

I will explore add this support in bus befor next version.

> > +
> > +ÂÂÂ /* Mask soundwire interrupts */
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SWRM_INTERRUPT_STATUS_RMSK);
> > +
> > +ÂÂÂ /* Configure No pings */
> > +ÂÂÂ val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
>
> If there is any sort of PREQ signaling for Slave-initiated interrupts, 
> disabling PINGs is likely a non-conformant implementation since the 
> master is required to issue a PING command within 32 frames. That's also 
> the only way to know if a device is attached, so additional comments are 
> likely required.
This is the value of Maximum number of consiecutive read/write commands 
without ping command in between. I will try to collect more details and 
add some comments here.


> > +
> > +ÂÂÂ val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
> > +ÂÂÂ val |= (0x1f << SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT);
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
> > +
> > +ÂÂÂ /* Configure number of retries of a read/write cmd */
> > +ÂÂÂ val = (retry_cmd_num << SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFT);
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, val);
> > +
> > +ÂÂÂ /* Set IRQ to PULSE */
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SWRM_COMP_CFG_ENABLE_MSK);
>
> indentation seems off in this code?
Yes! I will fix this.

> > +ÂÂÂ return 0;
> > +}
> > +
> > +static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_msg *msg)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> > +ÂÂÂ int ret, i, len;
> > +
> > +ÂÂÂ if (msg->flags == SDW_MSG_FLAG_READ) {
> > +ÂÂÂÂÂÂÂ for (i = 0; i < msg->len;) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ len = msg->len - i;
> > +ÂÂÂÂÂÂÂÂÂÂÂ else
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ len = QCOM_SWRM_MAX_RD_LEN;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂ ret = qcom_swrm_cmd_fifo_rd_cmd(ctrl, msg->dev_num,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ msg->addr + i, len,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &msg->buf[i]);
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (ret < 0) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (ret == -ENODATA)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return SDW_CMD_IGNORED;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂ i = i + len;
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ } else if (msg->flags == SDW_MSG_FLAG_WRITE) {
> > +ÂÂÂÂÂÂÂ for (i = 0; i < msg->len; i++) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ msg->dev_num,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ msg->addr + i);
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (ret < 0) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (ret == -ENODATA)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return SDW_CMD_IGNORED;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
> > +ÂÂÂ return SDW_CMD_OK;
> > +}
> > +
> > +static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
> > +{
> > +ÂÂÂ u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> > +ÂÂÂ u32 val;
> > +
> > +ÂÂÂ val = ctrl->reg_read(ctrl, reg);
> > +ÂÂÂ val |= ((0 << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT) |
> > +ÂÂÂÂÂÂÂ (7l << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT));
>
> magic values, probably need a macro here?
Okay, will add a proper define for this values.

> > +ÂÂÂ ctrl->reg_write(ctrl, reg, val);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
>
> > +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
> > +{
> > +ÂÂÂ int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
> > +ÂÂÂ struct snd_soc_dai_driver *dais;
> > +ÂÂÂ int i;
> > +
> > +ÂÂÂ /* PDM dais are only tested for now */
> > +ÂÂÂ dais = devm_kcalloc(ctrl->dev, num_dais, sizeof(*dais), GFP_KERNEL);
> > +ÂÂÂ if (!dais)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ for (i = 0; i < num_dais; i++) {
> > +ÂÂÂÂÂÂÂ dais[i].name = kasprintf(GFP_KERNEL, "SDW Pin%d", i);
>
> if (!dais[i].name)
>
> > +ÂÂÂÂÂÂÂ if (i < ctrl->num_dout_ports) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].playback.stream_name = kasprintf(GFP_KERNEL,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "SDW Tx%d", i);
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (!dais[i].playback.stream_name) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kfree(dais[i].name);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return -ENOMEM;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
>
> also need to free previously allocated memory in earlier iterations, or 
> use devm_
Yes, thats true, I will fix this in next version.


Thanks,
srini