Re: [RFC 3/7] firmware: arm_scmi: Add QCOM vendor protocol

Konrad Dybcio
2024-01-17T20:15:57Z

On 1/17/24 18:34, Sibi Sankar wrote:
> From: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>
> SCMI QCOM vendor protocol provides interface to communicate with SCMI
> controller and enable vendor specific features like bus scaling capable
> of running on it.
>
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
> Co-developed-by: Amir Vajid <avajid@quicinc.com>
> Signed-off-by: Amir Vajid <avajid@quicinc.com>
> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---

[...]

> +
> +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> + u32 param_id, size_t size)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;

After you apply Dmitry's suggestions on returning -EINVAL
directly, you can also sort definitions in a reverse-Christmas-
tree order throughout the file.

> + msg = t->tx.buf;
> + *msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);

lower/upper_32_bits()?

[...]

> + if (t->rx.len > rx_size) {
> + pr_err("SCMI received buffer size %zu is more than expected size %zu\n",
> + t->rx.len, rx_size);
> + return -EMSGSIZE;

No other driver seems to be checking for this, should this:

a) go to common code
b) be ignored

?

Konrad