Re: [PATCH 1/2] soc: qcom: rpmh: Add support to read back resource settings

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 6/23/25 6:43 PM, Kamal Wadhwa wrote:
> From: Maulik Shah <maulik.shah@oss.qualcomm.com>
> 
> All rpmh_*() APIs so far have supported placing votes for various
> resource settings but the H/W also have option to read resource
> settings.
> 
> This change adds a new rpmh_read() API to allow clients
> to read back resource setting from H/W. This will be useful for
> clients like regulators, which currently don't have a way to know
> the settings applied during bootloader stage.
> 
> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> ---

[...]

>  	u32 msgid;
> -	u32 cmd_msgid = CMD_MSGID_LEN | CMD_MSGID_WRITE;
> +	u32 cmd_msgid = CMD_MSGID_LEN;
>  	u32 cmd_enable = 0;
>  	struct tcs_cmd *cmd;
>  	int i, j;
>  
>  	/* Convert all commands to RR when the request has wait_for_compl set */
>  	cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
> +	cmd_msgid |= (!msg->is_read) ? CMD_MSGID_WRITE : 0;

if (!msg->is_read)
	cmd_msgid |= CMD_MSGID_WRITE

looks more human-readable

[...]

> +/**
> + * rpmh_read: Read a resource value
> + *
> + * @dev: The device making the request
> + * @state: Must be Active state
> + * @cmd: The payload having address of resource to read
> + * @n: The number of elements in @cmd, must be single command
> + *
> + * Reads the value for the resource address given in tcs_cmd->addr
> + * and returns the tcs_cmd->data filled with same.
> + *
> + * May sleep. Do not call from atomic contexts.
> + *
> + * Return:
> + * * 0			- Success
> + * * -Error             - Error code

This isn't valid kerneldoc

> + */
> +int rpmh_read(const struct device *dev, enum rpmh_state state, struct tcs_cmd *cmd, u32 n)
> +{
> +	int ret;
> +	DECLARE_COMPLETION_ONSTACK(compl);
> +	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);

A reverse-Christmas-tree sorting would be nice here

> +
> +	if (n != 1 || state != RPMH_ACTIVE_ONLY_STATE)

if n must be one, why is it a parameter?

Konrad