Re: [PATCH 6/6] soundwire: qcom: add support to new interrupts

[Srinivas Kandagatla]

On 01/02/2021 14:31, Vinod Koul wrote:
> On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> > Add support to new interrupts and update irq routine in a way
> > to deal with multiple pending interrupts with in a single interrupt!
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> >   drivers/soundwire/qcom.c | 191 ++++++++++++++++++++++++++++++---------
> >   1 file changed, 146 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> > index d61b204dc284..c699bd51d29a 100644
> > --- a/drivers/soundwire/qcom.c
> > +++ b/drivers/soundwire/qcom.c
> > @@ -28,10 +28,21 @@
> >   #define SWRM_COMP_PARAMS_DIN_PORTS_MASK				GENMASK(9, 5)
> >   #define SWRM_INTERRUPT_STATUS					0x200
> >   #define SWRM_INTERRUPT_STATUS_RMSK				GENMASK(16, 0)
> > +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ			BIT(0)
> >   #define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED		BIT(1)
> >   #define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS		BIT(2)
> > +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET			BIT(3)
> > +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW			BIT(4)
> > +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW			BIT(5)
> > +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW		BIT(6)
> >   #define SWRM_INTERRUPT_STATUS_CMD_ERROR				BIT(7)
> > +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION		BIT(8)
> > +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH		BIT(9)
> >   #define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED		BIT(10)
> > +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2             BIT(13)
> > +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2              BIT(14)
> > +#define SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP               BIT(16)
> > +#define SWRM_INTERRUPT_MAX					17
> >   #define SWRM_INTERRUPT_MASK_ADDR				0x204
> >   #define SWRM_INTERRUPT_CLEAR					0x208
> >   #define SWRM_INTERRUPT_CPU_EN					0x210
> > @@ -105,11 +116,8 @@ struct qcom_swrm_ctrl {
> >   	struct device *dev;
> >   	struct regmap *regmap;
> >   	void __iomem *mmio;
> > -	struct completion *comp;
> >   	struct completion broadcast;
> >   	struct work_struct slave_work;
> > -	/* read/write lock */
> > -	spinlock_t comp_lock;
> >   	/* Port alloc/free lock */
> >   	struct mutex port_lock;
> >   	struct mutex io_lock;
> > @@ -126,6 +134,7 @@ struct qcom_swrm_ctrl {
> >   	int rows_index;
> >   	unsigned long dout_port_mask;
> >   	unsigned long din_port_mask;
> > +	u32 intr_mask;
> >   	u8 rcmd_id;
> >   	u8 wcmd_id;
> >   	struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
> > @@ -315,6 +324,27 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
> >   	return ret;
> >   }
> >   
> > +static int qcom_swrm_get_alert_slave(struct qcom_swrm_ctrl *ctrl)
> > +{
> > +	u32 val;
> > +	int i;
> > +
> > +	ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
> > +
> > +	for (i = 0; i < SDW_MAX_DEVICES; i++) {
> > +		u32 s;
>
> define at top of the function pls, also maybe better name status?
Sure,
> > +
> > +		s = (val >> (i * 2));
>
> why * 2 ? Maybe add a comment for this logic

This is status mask of 2 bits! Will clarify this in next version!

> > +
> > +		if ((s & SWRM_MCP_SLV_STATUS_MASK) == SDW_SLAVE_ALERT) {
> > +			ctrl->status[i] = s;
> > +			return i;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >   static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> >   {
> >   	u32 val;
> > @@ -333,40 +363,122 @@ static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> >   
> >   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);
> > -
> > -	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);
> > -	}
> > +	struct qcom_swrm_ctrl *swrm = dev_id;
> > +	u32 value, intr_sts, intr_sts_masked;
> > +	u32 i;
> > +	u8 devnum = 0;
> > +	int ret = IRQ_HANDLED;
> > +
> > +
> > +	swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
> > +	intr_sts_masked = intr_sts & swrm->intr_mask;
> > +
> > +handle_irq:
>
> maybe move this into a fn and avoid a goto for non err path?
Makes sense!


--srini