lkml.org 
[lkml]   [2019]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller
    From
    Date

    >>> +#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.

    \
     
     \ /
      Last update: 2019-06-10 16:12    [W:2.513 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site