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

["Maulik Shah (mkshah)" <maulik.shah@oss.qualcomm.com>]

On 6/24/2025 8:14 PM, Konrad Dybcio wrote:
> 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

I will update in v2.

> 
> [...]
> 
>> +/**
>> + * 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

I will update in v2.

> 
>> + */
>> +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

I will update in v2 per reverse-Christmas-tree sorting.

> 
>> +
>> +	if (n != 1 || state != RPMH_ACTIVE_ONLY_STATE)
> 
> if n must be one, why is it a parameter?

I will Remove n (which is always 1) and also the state (which is also always RPMH_ACTIVE_ONLY_STATE) from the parameters.

Thanks,
Maulik

> 
> Konrad