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

[Srinivas Kandagatla]

Thanks for taking time to review,
I agre with most of the comments specially handling returns and making 
code more readable.
Will fix them in next version.

On 08/06/2019 22:53, Cezary Rojewski wrote:
> On 2019-06-07 10:56, Srinivas Kandagatla wrote:
> > Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
> > either integrated as part of WCD audio codecs via slimbus or
> > as part of SOC I/O.
> >
> > This patchset adds support to a very basic controller which has been
> > tested with WCD934x SoundWire controller connected to WSA881x smart
> > speaker amplifiers.
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> >  drivers/soundwire/Kconfig | 9 +
> > Â drivers/soundwire/Makefile |ÂÂ 4 +
> > Â drivers/soundwire/qcom.cÂÂ | 983 +++++++++++++++++++++++++++++++++++++
> > Â 3 files changed, 996 insertions(+)
> > Â create mode 100644 drivers/soundwire/qcom.c
> >
> > diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> > index 53b55b79c4af..f44d4f36dbbb 100644
> > --- a/drivers/soundwire/Kconfig
> > +++ b/drivers/soundwire/Kconfig
> > @@ -34,4 +34,13 @@ config SOUNDWIRE_INTEL
> > ÂÂÂÂÂÂÂ enable this config option to get the SoundWire support for that
> > ÂÂÂÂÂÂÂ device.
> > +config SOUNDWIRE_QCOM
> > +ÂÂÂ tristate "Qualcomm SoundWire Master driver"
> > +ÂÂÂ select SOUNDWIRE_BUS
> > +ÂÂÂ depends on SND_SOC
> > +ÂÂÂ help
> > +ÂÂÂÂÂ SoundWire Qualcomm Master driver.
> > +ÂÂÂÂÂ If you have an Qualcomm platform which has a SoundWire Master then
> > +ÂÂÂÂÂ enable this config option to get the SoundWire support for that
> > +ÂÂÂÂÂ device
> > Â endif
> > diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> > index 5817beaca0e1..f4ebfde31372 100644
> > --- a/drivers/soundwire/Makefile
> > +++ b/drivers/soundwire/Makefile
> > @@ -16,3 +16,7 @@ obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel.o
> > Â soundwire-intel-init-objs := intel_init.o
> > Â obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel-init.o
> > +
> > +#Qualcomm driver
> > +soundwire-qcom-objs :=ÂÂÂ qcom.o
> > +obj-$(CONFIG_SOUNDWIRE_QCOM) += soundwire-qcom.o
> > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> > new file mode 100644
> > index 000000000000..d1722d44d217
> > --- /dev/null
> > +++ b/drivers/soundwire/qcom.c
> > @@ -0,0 +1,983 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019, Linaro Limited
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/slimbus.h>
> > +#include <linux/soundwire/sdw.h>
> > +#include <linux/soundwire/sdw_registers.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include "bus.h"
> > +
>
> Pierre already pointed this out - SWR looks odd. During my time with 
> Soundwire I've met SDW and SNDW, but it's the first time I see SWR.

These names are derived from register names from hw datasheet.
So I have not much choice here other than using them as it. May be 
adding QCOM_ prefix make it bit more non-confusing.
> You seem to shortcut every reg here similarly to how it's done in SDW 
> spec. INTERRUPT is represented by INT there, and by doing so, this 
> define block would look more like a real family.
Will do that in next version.!

> > +#define SWRM_CMD_FIFO_WR_CMDÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x300
> > +#define SWRM_CMD_FIFO_RD_CMDÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x304
> > +#define SWRM_CMD_FIFO_CMDÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x308
> > +#define SWRM_CMD_FIFO_STATUSÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x30C
> > +#define SWRM_CMD_FIFO_CFG_ADDRÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x314
> > +#define SWRM_CMD_FIFO_CFG_NUM_OF_CMD_RETRY_SHFTÂÂÂÂÂÂÂÂÂÂÂ 0x0
> > +#define SWRM_CMD_FIFO_RD_FIFO_ADDRÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x318
> > +#define SWRM_ENUMERATOR_CFG_ADDRÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x500
> > +#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)ÂÂÂÂÂÂÂ (0x101C + 0x40 * (m))
> > +#define SWRM_MCP_FRAME_CTRL_BANK_SSP_PERIOD_SHFTÂÂÂÂÂÂÂ 16
> > +#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFTÂÂÂÂÂÂÂÂÂÂÂ 3
> > +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSKÂÂÂÂÂÂÂÂÂÂÂ GENMASK(2, 0)
> > +#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFTÂÂÂÂÂÂÂÂÂÂÂ 0
> > +#define SWRM_MCP_CFG_ADDRÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x1048
> > +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSKÂÂÂÂÂÂÂ GENMASK(21, 17)
> > +#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFTÂÂÂÂÂÂÂ 0x11
> > +#define SWRM_MCP_STATUSÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x104C
> > +#define SWRM_MCP_STATUS_BANK_NUM_MASKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
> > +#define SWRM_MCP_SLV_STATUSÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x1090
> > +#define SWRM_MCP_SLV_STATUS_MASKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GENMASK(1, 0)
> > +#define SWRM_DP_PORT_CTRL_BANK(n, m)ÂÂÂ (0x1124 + 0x100 * (n - 1) + 
> > 0x40 * m)
>
> Some of these you align, others leave with the equal amount of tabs 
> despite different widths.
I will take a closer look at such instance before sending next version.

> > +#define SWRM_DP_PORT_CTRL2_BANK(n, m)ÂÂÂ (0x1126 + 0x100 * (n - 1) + 
> > 0x40 * m)
>
> Consider reusing _CTRL_ and simply adding offset for 2_.
Okay.

> > +#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFTÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x18
> > +#define SWRM_DP_PORT_CTRL_OFFSET2_SHFTÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x10
> > +#define SWRM_DP_PORT_CTRL_OFFSET1_SHFTÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x08
> > +#define SWRM_AHB_BRIDGE_WR_DATA_0ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0xc885
> > +#define SWRM_AHB_BRIDGE_WR_ADDR_0ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0xc889
> > +#define SWRM_AHB_BRIDGE_RD_ADDR_0ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0xc88d
> > +#define SWRM_AHB_BRIDGE_RD_DATA_0ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0xc891
> > +
> > +#define SWRM_REG_VAL_PACK(data, dev, id, reg)ÂÂÂ \
> > +ÂÂÂÂÂÂÂÂÂÂÂ ((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
> > +
> > +#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
> > +#define SWRM_MAX_DAISÂÂÂÂÂÂÂ 0xF
>
> Given the scale of this block, it might be good to reiterate all defines 
> and see if indeed all are QCom specific. Maybe some could be replaced by 
> equivalents from common code.
I did take a look at common defines, however these values are pretty 
much specific to this controller configuration.>> +
> > +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;
>
> Given the fact you don't use version field directly and always shift it, 
> I'd consider making use of union here to listing version bits 
> explicitly. Overall size won't change.
Okay, I tried to keep most of the driver simple as I could as this is 
the first version, I will explore adding union as you suggested.

> > +ÂÂÂ 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];
> > +ÂÂÂ 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;
> > +};
> > +
>
> "usecase" looks ambiguous at best.

Its leftover

> > +static u32 qcom_swrm_slim_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
> > +{
> > +ÂÂÂ struct regmap *wcd_regmap = ctrl->regmap;
> > +ÂÂÂ u32 val = 0, ret;
> > +
> > +ÂÂÂ /* pg register + offset */
> > +ÂÂÂ regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_RD_ADDR_0,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ (u8 *)&reg, 4);
> > +
> > +ÂÂÂ ret = regmap_bulk_read(wcd_regmap, SWRM_AHB_BRIDGE_RD_DATA_0,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (u8 *)&val, 4);
> > +ÂÂÂ if (ret < 0)
> > +ÂÂÂÂÂÂÂ dev_err(ctrl->dev, "Read Failure (%d)\n", ret);
> > +
> > +ÂÂÂ return val;
> > +}
> > +
> > +static u32 qcom_swrm_mmio_reg_read(struct qcom_swrm_ctrl *ctrl, int reg)
> > +{
> > +ÂÂÂ return readl_relaxed(ctrl->base + reg);
> > +}
> > +
> > +static int qcom_swrm_mmio_reg_write(struct qcom_swrm_ctrl *ctrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int reg, int val)
> > +{
> > +ÂÂÂ writel_relaxed(val, ctrl->base + reg);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static int qcom_swrm_slim_reg_write(struct qcom_swrm_ctrl *ctrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int reg, int val)
> > +{
> > +ÂÂÂ struct regmap *wcd_regmap = ctrl->regmap;
> > +
> > +ÂÂÂ /* pg register + offset */
> > +ÂÂÂ regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_DATA_0,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ (u8 *)&val, 4);
> > +ÂÂÂ /* write address register */
> > +ÂÂÂ regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_WR_ADDR_0,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ (u8 *)&reg, 4);
> > +
> > +ÂÂÂ return 0;
> > +}
>
> Ok, so you choose to declare write op as returning "int" yet either it 
> cannot do so (void writel_relaxed) or ret is completely ignored 
> (regmap_bulk_write does return an int value). Either switch to void or 
> check against returned value whenever possible.

Its right to check the return values, I will change it this accordingly 
and fix all such occurances.

> > +
> > +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;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ 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));
> > +
> > +ÂÂÂÂÂÂÂ if (!ret || ctrl->fifo_status) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(ctrl->dev, "reg 0x%x write failed\n", val);
>
> Both, this and err msg above are generic enough to be put into goto to 
> save some space.

I agree!

> > +ÂÂÂÂÂÂÂÂÂÂÂ 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;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ 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);
>
> Same for _rt_.
>
> > +ÂÂÂÂÂÂÂ 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));
> > +
> > +ÂÂÂÂÂÂÂ if (!ret || ctrl->fifo_status) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(ctrl->dev, "reg 0x%x read failed\n", val);
>
> Just to be sure. It is really "read" that is failing here?
The final result is that read has not been sucessfull with a complete 
interrupt. So yes read is failing here.


> > +ÂÂÂÂÂÂÂÂÂÂÂ 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");
>
> If you do not handle these errors at all, consider declaring 
> ERROR-message table. I believe leaving erroneus status as is may lead to 
> fatal consequences. If there is no intention for handling even the most 
> critical cases, please add TODO/ comment here.

Yep, I agree will clean this up!

> > +
> > +ÂÂÂ 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;
> > +
> > +ÂÂÂ /* 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);
> > +
> > +ÂÂÂ /* 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);
> > +
> > +ÂÂÂ 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);
> > +ÂÂÂ return 0;
>
> As in my previous comment, you should check against ret from reg_write 
> if void approach is not chosen.

I agree!

> > +}
> > +
> > +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;
>
> Any reason for inlining this incrementation? If _rd_ fails, we leave the 
> loop anyway.
>
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ } 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));
> > +ÂÂÂ ctrl->reg_write(ctrl, reg, val);
> > +
> > +ÂÂÂ return 0;
>
> s/return 0/return ctrl->reg_write(ctrl, reg, val)/
>
> > +}
> > +
> > +static int qcom_swrm_port_params(struct sdw_bus *bus,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_port_params *p_params,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int bank)
> > +{
> > +ÂÂÂ /* TBD */
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static int qcom_swrm_transport_params(struct sdw_bus *bus,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_transport_params *params,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum sdw_reg_bank bank)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> > +ÂÂÂ u32 value;
> > +
> > +ÂÂÂ value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
> > +ÂÂÂ value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
> > +ÂÂÂ value |= params->sample_interval - 1;
> > +
> > +ÂÂÂ ctrl->reg_write(ctrl, SWRM_DP_PORT_CTRL_BANK((params->port_num), 
> > bank),
> > +ÂÂÂÂÂÂÂÂÂÂÂ value);
> > +
> > +ÂÂÂ return 0;
>
> Another "return issue" here.
>
> > +}
> > +
> > +static int qcom_swrm_port_enable(struct sdw_bus *bus,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_enable_ch *enable_ch,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int bank)
> > +{
> > +ÂÂÂ u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> > +ÂÂÂ u32 val;
> > +
> > +ÂÂÂ val = ctrl->reg_read(ctrl, reg);
> > +ÂÂÂ if (enable_ch->enable)
> > +ÂÂÂÂÂÂÂ val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> > +ÂÂÂ else
> > +ÂÂÂÂÂÂÂ val &= ~(enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> > +
> > +ÂÂÂ ctrl->reg_write(ctrl, reg, val);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static struct sdw_master_port_ops qcom_swrm_port_ops = {
> > +ÂÂÂ .dpn_set_port_params = qcom_swrm_port_params,
> > +ÂÂÂ .dpn_set_port_transport_params = qcom_swrm_transport_params,
> > +ÂÂÂ .dpn_port_enable_ch = qcom_swrm_port_enable,
> > +};
> > +
> > +static struct sdw_master_ops qcom_swrm_ops = {
> > +ÂÂÂ .xfer_msg = qcom_swrm_xfer_msg,
> > +ÂÂÂ .pre_bank_switch = qcom_swrm_pre_bank_switch,
> > +};
> > +
> > +static int qcom_swrm_compute_params(struct sdw_bus *bus)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> > +ÂÂÂ struct sdw_master_runtime *m_rt;
> > +ÂÂÂ struct sdw_slave_runtime *s_rt;
> > +ÂÂÂ struct sdw_port_runtime *p_rt;
> > +ÂÂÂ int i = 0;
> > +
> > +ÂÂÂ list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
> > +ÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.port_num = p_rt->num;
> > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.sample_interval =
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ctrl->pconfig[p_rt->num - 1].si + 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset1 =
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ctrl->pconfig[p_rt->num - 1].off1;
> > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset2 =
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ctrl->pconfig[p_rt->num - 1].off2;
>
> ctrl->pconfig[ <idx> ] colleagues bellow make use of local index 
> variable which clearly makes it more readable.
>
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.port_num = p_rt->num;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.sample_interval =
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ctrl->pconfig[i].si + 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset1 =
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ctrl->pconfig[i].off1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset2 =
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ctrl->pconfig[i].off2;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i++;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
> > +ÂÂÂ DEFAULT_CLK_FREQ,
> > +};
> > +
> > +static void qcom_swrm_slave_wq(struct work_struct *work)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl =
> > +ÂÂÂÂÂÂÂÂÂÂÂ container_of(work, struct qcom_swrm_ctrl, slave_work);
> > +
> > +ÂÂÂ qcom_swrm_get_device_status(ctrl);
> > +ÂÂÂ sdw_handle_slave_status(&ctrl->bus, ctrl->status);
> > +}
> > +
> > +static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> > +
> > +ÂÂÂ if (!ctrl->sruntime[dai->id])
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ return sdw_enable_stream(ctrl->sruntime[dai->id]);
> > +}
> > +
> > +static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_stream_runtime *stream)
> > +{
> > +ÂÂÂ struct sdw_master_runtime *m_rt;
> > +ÂÂÂ struct sdw_port_runtime *p_rt;
> > +ÂÂÂ unsigned long *port_mask;
> > +
> > +ÂÂÂ mutex_lock(&ctrl->port_lock);
> > +
> > +ÂÂÂ list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > +ÂÂÂÂÂÂÂ if (m_rt->direction == SDW_DATA_DIR_RX)
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->dout_port_mask;
> > +ÂÂÂÂÂÂÂ else
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->din_port_mask;
> > +
> > +ÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(p_rt->num - 1, port_mask);
> > +ÂÂÂÂÂÂÂ }
>
> Unnecessary brackets.
>
> > +ÂÂÂ }
> > +
> > +ÂÂÂ mutex_unlock(&ctrl->port_lock);
> > +}
> > +
> > +static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct sdw_stream_runtime *stream,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_pcm_hw_params *params,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int direction)
> > +{
> > +ÂÂÂ struct sdw_port_config pconfig[SDW_MAX_PORTS];
> > +ÂÂÂ struct sdw_stream_config sconfig;
> > +ÂÂÂ struct sdw_master_runtime *m_rt;
> > +ÂÂÂ struct sdw_slave_runtime *s_rt;
> > +ÂÂÂ struct sdw_port_runtime *p_rt;
> > +ÂÂÂ unsigned long *port_mask;
> > +ÂÂÂ int i, maxport, pn, nports = 0, ret = 0;
> > +
> > +ÂÂÂ mutex_lock(&ctrl->port_lock);
> > +ÂÂÂ list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > +ÂÂÂÂÂÂÂ if (m_rt->direction == SDW_DATA_DIR_RX) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ maxport = ctrl->num_dout_ports;
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->dout_port_mask;
> > +ÂÂÂÂÂÂÂ } else {
> > +ÂÂÂÂÂÂÂÂÂÂÂ maxport = ctrl->num_din_ports;
> > +ÂÂÂÂÂÂÂÂÂÂÂ port_mask = &ctrl->din_port_mask;
> > +ÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂ list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* Port numbers start from 1 - 14*/
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pn = find_first_zero_bit(port_mask, maxport);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (pn > (maxport - 1)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(ctrl->dev, "All ports busy\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -EBUSY;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto err;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ set_bit(pn, port_mask);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pconfig[nports].num = pn + 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pconfig[nports].ch_mask = p_rt->ch_mask;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ nports++;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if (direction == SNDRV_PCM_STREAM_CAPTURE)
> > +ÂÂÂÂÂÂÂ sconfig.direction = SDW_DATA_DIR_TX;
> > +ÂÂÂ else
> > +ÂÂÂÂÂÂÂ sconfig.direction = SDW_DATA_DIR_RX;
> > +
> > +ÂÂÂ sconfig.ch_count = 1;
> > +ÂÂÂ sconfig.frame_rate = params_rate(params);
> > +ÂÂÂ sconfig.type = stream->type;
> > +ÂÂÂ sconfig.bps = 1;
>
> Hmm. frame_rate and type gets assigned based on "input" data yet the 
> rest is hardcoded. Is this intended?

For now I only managed to test PDM on this controller, and I agree these 
need to be more generic..
> > +ÂÂÂ sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ nports, stream);
> > +err:
> > +ÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂ for (i = 0; i < nports; i++)
> > +ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(pconfig[i].num - 1, port_mask);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ mutex_unlock(&ctrl->port_lock);
> > +
> > +ÂÂÂ return ret;
> > +}
> > +
> > +static int qcom_swrm_hw_params(struct snd_pcm_substream *substream,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_pcm_hw_params *params,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> > +ÂÂÂ int ret;
> > +
> > +ÂÂÂ ret = qcom_swrm_stream_alloc_ports(ctrl, ctrl->sruntime[dai->id],
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ params, substream->stream);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ return sdw_prepare_stream(ctrl->sruntime[dai->id]);
> > +}
> > +
> > +static int qcom_swrm_hw_free(struct snd_pcm_substream *substream,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
> > +{
> > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> > +
> > +ÂÂÂ qcom_swrm_stream_free_ports(ctrl, ctrl->sruntime[dai->id]);
> > +ÂÂÂ sdw_stream_remove_master(&ctrl->bus, ctrl->sruntime[dai->id]);
> > +ÂÂÂ sdw_deprepare_stream(ctrl->sruntime[dai->id]);
> > +ÂÂÂ sdw_disable_stream(ctrl->sruntime[dai->id]);
>
> Declaring local variable initialized with ctrl->sruntime[dai->id] should 
> prove more readable.
I agree!

> > +
> > +ÂÂÂ 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 (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;
>
> Now this got me worried. What about memory allocated in iterations 
> before the failure? It must be freed in error handling path. goto should 
> be of help here.
I agree.

> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].playback.channels_min = 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].playback.channels_max = 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].playback.rates = SNDRV_PCM_RATE_48000;
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
>
> All of these formats are hardcoded. Consider declaring a "template" 
> format above and simply initialize each dai with it.

Okay!

> > +ÂÂÂÂÂÂÂ } else {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].capture.stream_name = kasprintf(GFP_KERNEL,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "SDW Rx%d", i);
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (!dais[i].capture.stream_name) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kfree(dais[i].name);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kfree(dais[i].playback.stream_name);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return -ENOMEM;
>
> Same memory deallocation issue here.
I see that, I will take care of this in next version.
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].capture.channels_min = 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].capture.channels_max = 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].capture.rates = SNDRV_PCM_RATE_48000;
> > +ÂÂÂÂÂÂÂÂÂÂÂ dais[i].capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
>
> Comment regarding playback dai initialization applies here too.
>
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ dais[i].ops = &qcom_swrm_pdm_dai_ops;
> > +ÂÂÂÂÂÂÂ dais[i].id = i;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ return devm_snd_soc_register_component(ctrl->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &qcom_swrm_dai_component,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dais, num_dais);
> > +}
> > +
> > +static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
> > +{
> > +ÂÂÂ struct device_node *np = ctrl->dev->of_node;
> > +ÂÂÂ u8 off1[SDW_MAX_PORTS];
> > +ÂÂÂ u8 off2[SDW_MAX_PORTS];
> > +ÂÂÂ u8 si[SDW_MAX_PORTS];
>
> Array of struct qcom_swrm_port_config instead of this trio?
Makes sense! Will do that in next version.
> > +ÂÂÂ int i, ret, nports, val;
> > +
> > +ÂÂÂ val = ctrl->reg_read(ctrl, SWRM_COMP_PARAMS);
> > +ÂÂÂ ctrl->num_dout_ports = val & SWRM_COMP_PARAMS_DOUT_PORTS_MASK;
> > +ÂÂÂ ctrl->num_din_ports = (val & SWRM_COMP_PARAMS_DIN_PORTS_MASK) >> 5;
> > +
> > +ÂÂÂ ret = of_property_read_u32(np, "qcom,din-ports", &val);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ if (val > ctrl->num_din_ports)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ ctrl->num_din_ports = val;
> > +
> > +ÂÂÂ ret = of_property_read_u32(np, "qcom,dout-ports", &val);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ if (val > ctrl->num_dout_ports)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ ctrl->num_dout_ports = val;
> > +
> > +ÂÂÂ nports = ctrl->num_dout_ports + ctrl->num_din_ports;
> > +
> > +ÂÂÂ ret = of_property_read_u8_array(np, "qcom,ports-offset1",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ off1, nports);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ ret = of_property_read_u8_array(np, "qcom,ports-offset2",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ off2, nports);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ ret = of_property_read_u8_array(np, "qcom,ports-sinterval-low",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ si, nports);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ for (i = 0; i < nports; i++) {
> > +ÂÂÂÂÂÂÂ ctrl->pconfig[i].si = si[i];
> > +ÂÂÂÂÂÂÂ ctrl->pconfig[i].off1 = off1[i];
> > +ÂÂÂÂÂÂÂ ctrl->pconfig[i].off2 = off2[i];
> > +ÂÂÂ }
>
> Using array I spoke of earlier leads to brackets being redundant here.
>
> > +
> > +ÂÂÂ return 0;
> > +}
> > +