Re: [PATCH 2/2] firmware: tpm: Introduce tpm-qcom driver

[Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>]

On 31-08-2026 16:45, Konrad Dybcio wrote:
> On 8/31/26 11:43 AM, Kuldeep Singh wrote:
>> Add a TPM chip driver for platforms where a TPM 2.0 instance is
>> implemented by a Trusted Application (TA) running in Qualcomm's Trusted
>> Execution Environment (QTEE), reachable over the QCOMTEE object-IPC
>> transport.
> 
> [...]
> 
>> +static int tpm_qcom_get_client_env_obj(struct tee_context *ctx,
>> +				       struct tee_param_objref *client_env_obj)
>> +{
>> +	int ret;
>> +	struct tee_ioctl_object_invoke_arg inv_arg;
>> +	struct tee_param param[2];
> 
> nit: Reverse-Christmas-tree would be preferred

Ok.

> 
>> +
>> +	memset(&inv_arg, 0, sizeof(inv_arg));
>> +	memset(&param, 0, sizeof(param));
> 
> You can zero-initialize local struct variables like this:
> 
> struct foo bar = { };
> 
> [...]
> 
>> +static int tpm_qcom_send(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
>> +			 size_t cmd_len)
>> +{
>> +	struct tpm_qcom_private *pvt_data = dev_get_drvdata(chip->dev.parent);
>> +	size_t rsp_len = PAGE_ALIGN(MAX_RESPONSE_SIZE);
>> +	size_t copy_len;
>> +	int ret;
>> +
>> +	if (cmd_len > MAX_COMMAND_SIZE) {
>> +		dev_err(&chip->dev,
>> +			"%s: len=%zd exceeds MAX_COMMAND_SIZE\n",
>> +			__func__, cmd_len);
> 
> The name of the function isn't helpful here, this is the only time this
> message appears, so it's easy to grep

Sure.

> 
> [...]
> 
> 
>> +	err = tpm_chip_register(pvt_data->chip);
>> +	if (err) {
>> +		dev_err(dev, "%s: tpm_chip_register failed with rc=%d\n",
>> +			__func__, err);
> 
> Likewise

Since it's dev_err so dev name should be sufficient i think.
Let me drop function naming from log.

> 
> [...]
> 
>> +#define QCOMTEE_TPM_GET_TA_VERSION_ID		0x0001000
>> +#define QCOMTEE_TPM_TA_VERSION_GET_MAJOR(ver)	((u32)(ver) >> 16)
>> +#define QCOMTEE_TPM_TA_VERSION_GET_MINOR(ver)	((u32)(ver) & 0x0000ffffU)
> 
> That's FIELD_GET(mask, x)

Sounds good, Let me use FIELD_GET.

-- 
Regards
Kuldeep