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

[lihuisong (C)]

在 2023/11/30 22:49, Jonathan Cameron 写道:
> On Thu, 30 Nov 2023 21:45:50 +0800
> Huisong Li <lihuisong@xxxxxxxxxx> 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@xxxxxxxxxx>
> Hi.
>
> A few trivial things inline but now looks good to me!
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> > ---
> >   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,
> > +};
> .