| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6e656981-e141-2831-0732-55f4beca565e@linaro.org>
Date: Sun, 9 Jun 2019 13:15:37 +0100
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Cezary Rojewski <cezary.rojewski@...el.com>, broonie@...nel.org,
vkoul@...nel.org
Cc: robh+dt@...nel.org, devicetree@...r.kernel.org,
mark.rutland@....com, pierre-louis.bossart@...ux.intel.com,
alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire
controller
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@...aro.org>
>> ---
>> 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 *)®, 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 *)®, 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;
>> +}
>> +
Powered by blists - more mailing lists