Date:   Thu, 19 Dec 2019 17:14:13 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        vkoul@...nel.org
Cc:     robh@...nel.org, bgoswami@...eaurora.org, broonie@...nel.org,
        lgirdwood@...il.com, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, spapothi@...eaurora.org
Subject: Re: [alsa-devel] [PATCH v5 2/2] soundwire: qcom: add support for
 SoundWire controller



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@...aro.org>
> 
> 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

Powered by blists - more mailing lists