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

[Pierre-Louis Bossart]

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