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

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

[Andy Shevchenko]

Replace open coded variant of recently introduced acpi_get_subsystem_id().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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);
 
 	snprintf(icn8505->firmware_name, sizeof(icn8505->firmware_name),
 		 "chipone/icn8505-%s.fw", subsys);
 
-	kfree(buffer.pointer);
+	kfree_const(subsys);
 	return 0;
 }
 
-- 
2.35.1

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@linux.intel.com>
> ---
>  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

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

[Andy Shevchenko]

On Mon, Sep 05, 2022 at 12:35:42PM -0700, Dmitry Torokhov wrote:
> On Mon, Sep 05, 2022 at 08:20:01PM +0300, Andy Shevchenko wrote:

...

> > +	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)?

Which makes code inconsistent. But okay, no big deal.

> 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;

Would it matter? The generated code will be the same in both cases, no?

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

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Torokhov]

On Tue, Sep 06, 2022 at 03:54:06PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 05, 2022 at 12:35:42PM -0700, Dmitry Torokhov wrote:
> > On Mon, Sep 05, 2022 at 08:20:01PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > +	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)?
> 
> Which makes code inconsistent. But okay, no big deal.

To me the *_const() APIs are needed when the code does not really know
if it deals with a const/read-only object or not. If we know for sure we
are dealing with a const/read-only object, we can skip allocation and
freeing, so I do not see any inconsistencies.

> 
> > 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;
> 
> Would it matter? The generated code will be the same in both cases, no?

No, in the end I think the optimizer will reduce both variants to the
same thing. I do find mine a bit more compact and thus easier to read,
but I will not insist.

Thanks.

-- 
Dmitry