Re: [PATCH v2 2/3] powerpc/pseries: Handle and log pseries-wdt registration failures

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 13/07/26 09:59, Ritesh Harjani (IBM) wrote:
> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>
>> The pseries watchdog initialization registers the pseries-wdt platform
>> device using platform_device_register_simple(), but currently ignores
>> its return value.
>>
>> Check the returned pointer for errors, log a descriptive error message
>> when registration fails, and propagate the failure code to the caller.
>> This avoids silently ignoring platform device registration failures.
>>
> Fair enough.
>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>   arch/powerpc/platforms/pseries/setup.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
>> index 1223dc961242..bbb2813f8ede 100644
>> --- a/arch/powerpc/platforms/pseries/setup.c
>> +++ b/arch/powerpc/platforms/pseries/setup.c
>> @@ -191,8 +191,18 @@ static void __init fwnmi_init(void)
>>    */
>>   static __init int pseries_wdt_init(void)
>>   {
>> -	if (firmware_has_feature(FW_FEATURE_WATCHDOG))
>> -		platform_device_register_simple("pseries-wdt", 0, NULL, 0);
>> +	struct platform_device *pseries_wdt_dev;
> minor nit: we should rename this to pdev, since it is already under
> pseries_wdt_init(). That is generally how all platform drivers use it
> unless it requires more than one platform device.
>
> But either ways the patch looks good to me:
>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Thanks for the review. I will rename the variable in next version.

- Sourabh Jain

>
>> +
>> +	if (!firmware_has_feature(FW_FEATURE_WATCHDOG))
>> +		return 0;
>> +
>> +	pseries_wdt_dev = platform_device_register_simple("pseries-wdt", 0, NULL, 0);
>> +
>> +	if (IS_ERR(pseries_wdt_dev)) {
>> +		pr_err("Failed to register pseries-wdt platform device\n");
>> +		return PTR_ERR(pseries_wdt_dev);
>> +	}
>> +
>>   	return 0;
>>   }
>>   machine_subsys_initcall(pseries, pseries_wdt_init);
>> -- 
>> 2.52.0