Re: [PATCH 1/5] PCI: kirin: Use helper function for_each_available_child_of_node_scoped()

["zhangzekun (A)" <zhangzekun11@huawei.com>]

在 2024/8/28 20:11, Jonathan Cameron 写道:
> On Wed, 28 Aug 2024 15:38:21 +0800
> Zhang Zekun <zhangzekun11@huawei.com> wrote:
> 
>> for_each_available_child_of_node_scoped() provides a scope-based cleanup
>> functinality to put the device_node automatically, and we don't need to
>> call of_node_put() directly.  Let's simplify the code a bit with the use
>> of these functions.
>>
>> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> Hi.
> 
> Looks good.  A passing comment on another ugly bit of code in his
> function that you could tidy up whilst here.
> 
> For what you have covered
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>   drivers/pci/controller/dwc/pcie-kirin.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
>> index 0a29136491b8..e9bda1746ca5 100644
>> --- a/drivers/pci/controller/dwc/pcie-kirin.c
>> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
>> @@ -452,7 +452,7 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
>>   				    struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> -	struct device_node *child, *node = dev->of_node;
>> +	struct device_node *node = dev->of_node;
>>   	void __iomem *apb_base;
>>   	int ret;
>>   
>> @@ -477,17 +477,13 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
>>   		return ret;
>>   
> Looking at this function I don't suppose you fancy also tidying up the oddity of:
> 	kirin_pcie->gpio_id_dwc_perst = of_get_named_gpio(dev->of_node,
> 							  "reset-gpios", 0);
> 	if (kirin_pcie->gpio_id_dwc_perst == -EPROBE_DEFER) {
> 		return -EPROBE_DEFER;
> 	} else if (!gpio_is_valid(kirin_pcie->gpio_id_dwc_perst)) {
> 		dev_err(dev, "unable to get a valid gpio pin\n");
> 		return -ENODEV;
> 	}
> 
> Where that else adds nothing and it could just be
> 	ret = of_get_named_gpio(dev->of_node, "reset-gpios", 0);
> 	if (ret < 0)
> 		return dev_err_probe(dev, ret,
> 				     "unable to get a valid gpio pin\n2);
> 
> 	kirin_pcie->gpio_id_dwc_perst = ret;
> 
> or even update the gpio handling in general to use non deprecated
> functions.
> 

Hi, Jonathan,

Thanks for your review. I will send v2 to tidy up together.

Beset Regards,
Zekun