Re: [PATCH V1 2/3] rpmsg: glink: Add support to handle signals command

Stephen Boyd
2021-09-30T16:33:28Z

Quoting Deepak Kumar Singh (2021-09-30 08:32:03)
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 05533c7..384fcd2 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -976,6 +984,68 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
> return 0;
> }
>
> +/**
> + * qcom_glink_set_flow_control() - convert a signal cmd to wire format and
> + * transmit
> + * @ept: Rpmsg endpoint for channel.
> + * @enable: True/False - enable or disable flow control
> + *
> + * Return: 0 on success or standard Linux error code.
> + */
> +static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
> +{
> + struct glink_channel *channel = to_glink_channel(ept);
> + struct qcom_glink *glink = channel->glink;
> + struct glink_msg msg;
> + u32 sigs = 0;

Drop assignment.

> +
> + /**
> + * convert signals from TIOCM to NATIVE
> + * sigs = TIOCM_DTR|TIOCM_RTS
> + */
> + if (enable)
> + sigs |= (NATIVE_DTR_SIG | NATIVE_CTS_SIG);

sigs = NATIVE_DTR_SIG | NATIVE_CTS_SIG;

> + else
> + sigs |= (~(NATIVE_DTR_SIG | NATIVE_CTS_SIG));

sigs = ~(NATIVE_DTR_SIG | NATIVE_CTS_SIG);

> +
> + msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
> + msg.param1 = cpu_to_le16(channel->lcid);
> + msg.param2 = cpu_to_le32(sigs);
> +
> + return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
> +}
> +
> +static int qcom_glink_handle_signals(struct qcom_glink *glink,
> + unsigned int rcid, unsigned int sigs)
> +{
> + struct glink_channel *channel;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&glink->idr_lock, flags);

irqsave can be skipped because this is only called from an irq handler.

> + channel = idr_find(&glink->rcids, rcid);
> + spin_unlock_irqrestore(&glink->idr_lock, flags);
> + if (!channel) {
> + dev_err(glink->dev, "signal for non-existing channel\n");
> + return -EINVAL;
> + }
> +
> + /* convert signals from NATIVE to TIOCM */
> + if (sigs & NATIVE_DTR_SIG)
> + sigs |= TIOCM_DSR;
> + if (sigs & NATIVE_CTS_SIG)
> + sigs |= TIOCM_CTS;
> + if (sigs & NATIVE_CD_SIG)
> + sigs |= TIOCM_CD;
> + if (sigs & NATIVE_RI_SIG)
> + sigs |= TIOCM_RI;
> + sigs &= 0x0fff;

Is this to only keep TIOCM_* bits? Maybe make this an ORed together mask
of those bits so we know what 0xfff means.

> +
> + if (channel->ept.sig_cb)
> + channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
> +
> + return 0;
> +}
> +
> static irqreturn_t qcom_glink_native_intr(int irq, void *data)
> {
> struct qcom_glink *glink = data;
> @@ -1037,6 +1107,10 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
> qcom_glink_handle_intent_req_ack(glink, param1, param2);
> qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
> break;
> + case RPM_CMD_SIGNALS:
> + qcom_glink_handle_signals(glink, param1, param2);

Should the return value be handled and if there isn't a channel then
treat it as a spurious irq?

> + qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
> + break;
> default:
> dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
> ret = -EINVAL;