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

lihuisong (C)
2023-12-01T02:47:06Z

在 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,
>> +};
> .