Re: [PATCH v2 4/4] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

["lihuisong (C)" <lihuisong@huawei.com>]

在 2023/11/30 22:49, Jonathan Cameron 写道:
> On Thu, 30 Nov 2023 21:45:50 +0800
> Huisong Li <lihuisong@huawei.com> wrote:
>
>> Support the platform with PCC type3 and interrupt ack. And a version
>> specific structure is introduced to handle the difference between the
>> device in the code.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Hi.
>
> A few trivial things inline but now looks good to me!
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>> ---
>>   drivers/soc/hisilicon/kunpeng_hccs.c | 136 ++++++++++++++++++++++-----
>>   drivers/soc/hisilicon/kunpeng_hccs.h |  15 +++
>>   2 files changed, 126 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
>> index 15125f1e0f3e..d2302ff8c0e9 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> ...
>
>>   
>> -static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
>> +static inline int hccs_wait_cmd_complete_by_poll(struct hccs_dev *hdev)
> Why inline?  Do we have numbers to support this hint to the compiler being
> useful?
No testing for this, but here might not be really useful.
So will remove this inline.
>>   {
>>   	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>>   	struct acpi_pcct_shared_memory __iomem *comm_base =
>> @@ -194,30 +211,75 @@ static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
>>   	return ret;
>>   }
>>   
>> +static inline int hccs_wait_cmd_complete_by_irq(struct hccs_dev *hdev)
>> +{
>> +	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>> +	int ret = 0;
> Drop ret...
Ack
>
>> +
>> +	if (!wait_for_completion_timeout(&cl_info->done,
>> +			usecs_to_jiffies(cl_info->deadline_us))) {
>> +		dev_err(hdev->dev, "PCC command executed timeout!\n");
>> +		ret = -ETIMEDOUT;
> 		return -TIMEDOUT;
> ...
>> +	}
>> +
>> +	return ret;
> return 0;
Ack
>> +}
>> +static const struct hccs_verspecific_data hisi04b1_verspec_data = {
>> +	.rx_callback = NULL,
> It's harmless but no need to set callback to NULL. Maybe it acts as documentation?
Just to explicitly assign value and show the difference between the devices.
> It's common practice to just let C spec guarantees initialize any not implemented callbacks
> to 0 without them needing to be done explicitly.
Correct, but It's harmless.
>
>> +	.wait_cmd_complete = hccs_wait_cmd_complete_by_poll,
>> +	.fill_pcc_shared_mem = hccs_fill_pcc_shared_mem_region,
>> +	.shared_mem_size = sizeof(struct acpi_pcct_shared_memory),
>> +	.has_txdone_irq = false,
>> +};
>> +
>> +static const struct hccs_verspecific_data hisi04b2_verspec_data = {
>> +	.rx_callback = hccs_pcc_rx_callback,
>> +	.wait_cmd_complete = hccs_wait_cmd_complete_by_irq,
>> +	.fill_pcc_shared_mem = hccs_fill_ext_pcc_shared_mem_region,
>> +	.shared_mem_size = sizeof(struct acpi_pcct_ext_pcc_shared_memory),
>> +	.has_txdone_irq = true,
>> +};
> .