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

[Srinivas Kandagatla]

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 *)&reg, 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 = &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