+#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.
That'd be fine. As long as there is no duplication and two
terms/prefixes used for the same thing I am happy.
+
+ 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.
Right, and it's fine to add a timeout. The question is why add a timeout
*only* for a broadcast operation? It should be added for every
transaction IMO, unless you have a reason not to do so.
+
+ /* 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.
Right, so it's probably the comment that's too strict. It's not no pings
it's no pings during a sequence of up to N read/writes.
