Re: [PATCH v1 1/1] Input: icn8505 - Utilize acpi_get_subsystem_id()

[Dmitry Torokhov]

Hi Andy,

On Mon, Sep 05, 2022 at 08:20:01PM +0300, Andy Shevchenko wrote:
> Replace open coded variant of recently introduced acpi_get_subsystem_id().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/input/touchscreen/chipone_icn8505.c | 29 ++++++---------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
> index f9ca5502ac8c..bb5e63b87c5d 100644
> --- a/drivers/input/touchscreen/chipone_icn8505.c
> +++ b/drivers/input/touchscreen/chipone_icn8505.c
> @@ -364,32 +364,19 @@ static irqreturn_t icn8505_irq(int irq, void *dev_id)
>  
>  static int icn8505_probe_acpi(struct icn8505_data *icn8505, struct device *dev)
>  {
> -	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> -	const char *subsys = "unknown";
> -	struct acpi_device *adev;
> -	union acpi_object *obj;
> -	acpi_status status;
> -
> -	adev = ACPI_COMPANION(dev);
> -	if (!adev)
> -		return -ENODEV;
> +	const char *subsys;
>  
> -	status = acpi_evaluate_object(adev->handle, "_SUB", NULL, &buffer);
> -	if (ACPI_SUCCESS(status)) {
> -		obj = buffer.pointer;
> -		if (obj->type == ACPI_TYPE_STRING)
> -			subsys = obj->string.pointer;
> -		else
> -			dev_warn(dev, "Warning ACPI _SUB did not return a string\n");
> -	} else {
> -		dev_warn(dev, "Warning ACPI _SUB failed: %#x\n", status);
> -		buffer.pointer = NULL;
> -	}
> +	subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev));
> +	if (IS_ERR(subsys) && PTR_ERR(subsys) != -ENODATA)
> +		return PTR_ERR(subsys);
> +
> +	if (IS_ERR(subsys) && PTR_ERR(subsys) == -ENODATA)
> +		subsys = kstrdup_const("unknown", GFP_KERNEL);

Do we really need kstrdup_const() here? This makes me wonder if we need
to also have error handling here, and if we going to tip some automated
tools by not having it. Why can't we simply assign the constant here
(and continue using kfree_const() below)?

I think this is the case where PTR_ERR_OR_ZERO() might help avoid
multiple IS_ERR/PTR_ERR:

	subsys = acpi_get_subsystem_id(ACPI_HANDLE(dev));
	error = PTR_ERR_OR_ZERO(subsys);
	if (error == -ENODATA)
		subsys = "unknown";
	else if (error)
		return error;

>  
>  	snprintf(icn8505->firmware_name, sizeof(icn8505->firmware_name),
>  		 "chipone/icn8505-%s.fw", subsys);
>  
> -	kfree(buffer.pointer);
> +	kfree_const(subsys);
>  	return 0;
>  }
>  

Thanks.

-- 
Dmitry