On 19/12/2019 16:07, Pierre-Louis Bossart wrote:
On 12/19/19 3:28 AM, 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>
This looks quite good, I only have a couple of nit-picks/questions below.
Thanks for taking time to review this.
+static int qcom_swrm_abh_reg_read(struct qcom_swrm_ctrl *ctrl, int reg,
+ u32 *val)
+{
+ struct regmap *wcd_regmap = ctrl->regmap;
+ int ret;
+
+ /* pg register + offset */
+ ret = regmap_bulk_write(wcd_regmap, SWRM_AHB_BRIDGE_RD_ADDR_0,
+ (u8 *)®, 4);
+ if (ret < 0)
+ return SDW_CMD_FAIL;
+
+ ret = regmap_bulk_read(wcd_regmap, SWRM_AHB_BRIDGE_RD_DATA_0,
+ val, 4);
+ if (ret < 0)
+ return SDW_CMD_FAIL;
+
+ return SDW_CMD_OK;
+}
I think I asked the question before but don't remember the answer so you
may want to add a comment explaining why SDW_CMD_IGNORED is not a
possible return value?
There is no way atleast in this version of the controller to know if the
command is ignored. Only error that can be detected atm is timeout
waiting for response. Hopefully new versions of this IP have that
ability to detect this.
The BER is supposed to be very very low but there is a non-zero
possibility of a device losing sync.
+
+static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8
cmd_data,
+ u8 dev_addr, u16 reg_addr)
+{
+ DECLARE_COMPLETION_ONSTACK(comp);
+ unsigned long flags;
+ u32 val;
+ int ret;
+
+ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ ctrl->comp = ∁
+ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+ val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
+ SWRM_SPECIAL_CMD_ID, reg_addr);
+ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
+ if (ret)
+ goto err;
the code is a bit inconsistent at the moment on how errors are handled.
In some cases you explicitly test for errors, but ...
I looked at our previous discussions and I think we decided not to do
error checking reading on controller registers.
"For the Intel stuff, we typically don't check the read/writes to
controller registers, but anything that goes out on the bus is checked. "
+
+ for (i = 0; i < len; i++) {
+ ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
... here you don't ...
+ rval[i] = val & 0xFF;
+ }
+
+err:
+ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ ctrl->comp = NULL;
+ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+
+ return ret;
+}
+
+static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
+{
+ u32 val;
+ int i;
+
+ ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
... and not here ...
+
+ for (i = 0; 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;
+ unsigned long flags;
+
+ ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
... and here same the reg_read/writes are no longer tested for?
+
+ if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
+ ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
+ dev_err_ratelimited(ctrl->dev,
+ "CMD error, fifo status 0x%x\n",
+ value);
+ ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
+ }
+
+ if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
+ sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS)
+ schedule_work(&ctrl->slave_work);
+
+ ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
is it intentional to clear the interrupts first, before doing additional
checks?
No, I can move it to right to the end!
Or could it be done immediately after reading the status. It's not clear
to me if the position of this clear matters, and if yes you should
probably add a comment?
Am not 100% if it matters, but Ideally I would like clear the interrupt
source before clearing the interrupt.
+
+ if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) {
+ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ if (ctrl->comp)
+ complete(ctrl->comp);
+ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+ }
+
+ return IRQ_HANDLED;
The rest looks fine. nice work.
Thanks,
srini
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
