[PATCH 0/3] ACPI / utils: Add acpi_evaluate_reg() helper

[PATCH 0/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Hans de Goede]

Hi All,

Here is a small series adding an acpi_evaluate_reg() helper, note
the third patch sits on top of a fix for the pinctrl-cherryview
driver which I recently submitted and which is still finding its
way upstream.

Since this is not urgent (just a small code cleanup) I suggest
that the ACPI people can pick up patches 1-2 and then the last patch
can be merged post 5.8-rc1, at which point all the dependencies for
it should have landed already.

Regards,

Hans

[PATCH 1/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Hans de Goede]

With a recent fix to the pinctrl-cherryview driver we know have
2 drivers open-coding the parameter building / passing for calling
_REG on an ACPI handle.

Add a helper for this, so that these 2 drivers can be converted to this
helper.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/utils.c    | 25 +++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 804ac0df58ec..838b719ec7ce 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -605,6 +605,31 @@ acpi_status acpi_evaluate_lck(acpi_handle handle, int lock)
 	return status;
 }
 
+/**
+ * acpi_evaluate_reg: Evaluate _REG method to register OpRegion presence
+ * @handle: ACPI device handle
+ * @space_id: ACPI address space id to register OpRegion presence for
+ * @function: Parameter to pass to _REG one of ACPI_REG_CONNECT or
+ *            ACPI_REG_DISCONNECT
+ *
+ * Evaluate device's _REG method to register OpRegion presence.
+ */
+acpi_status acpi_evaluate_reg(acpi_handle handle, u8 space_id, u32 function)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object params[2];
+
+	params[0].type = ACPI_TYPE_INTEGER;
+	params[0].integer.value = space_id;
+	params[1].type = ACPI_TYPE_INTEGER;
+	params[1].integer.value = function;
+	arg_list.count = 2;
+	arg_list.pointer = params;
+
+	return acpi_evaluate_object(handle, "_REG", &arg_list, NULL);
+}
+EXPORT_SYMBOL(acpi_evaluate_reg);
+
 /**
  * acpi_evaluate_dsm - evaluate device's _DSM method
  * @handle: ACPI device handle
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a92bea7184a8..5afb6ceb284f 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -44,6 +44,7 @@ acpi_status acpi_execute_simple_method(acpi_handle handle, char *method,
 				       u64 arg);
 acpi_status acpi_evaluate_ej0(acpi_handle handle);
 acpi_status acpi_evaluate_lck(acpi_handle handle, int lock);
+acpi_status acpi_evaluate_reg(acpi_handle handle, u8 space_id, u32 function);
 bool acpi_ata_match(acpi_handle handle);
 bool acpi_bay_match(acpi_handle handle);
 bool acpi_dock_match(acpi_handle handle);
-- 
2.26.0

[PATCH 2/3] ACPI / hotplug / PCI: Use the new acpi_evaluate_reg() helper

[Hans de Goede]

Use the new acpi_evaluate_reg() helper in the acpiphp_glue.c code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b3869951c0eb..fd77eb1d460c 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -385,20 +385,11 @@ static unsigned char acpiphp_max_busnr(struct pci_bus *bus)
 static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
 {
 	struct acpiphp_func *func;
-	union acpi_object params[2];
-	struct acpi_object_list arg_list;
 
-	list_for_each_entry(func, &slot->funcs, sibling) {
-		arg_list.count = 2;
-		arg_list.pointer = params;
-		params[0].type = ACPI_TYPE_INTEGER;
-		params[0].integer.value = ACPI_ADR_SPACE_PCI_CONFIG;
-		params[1].type = ACPI_TYPE_INTEGER;
-		params[1].integer.value = 1;
-		/* _REG is optional, we don't care about if there is failure */
-		acpi_evaluate_object(func_to_handle(func), "_REG", &arg_list,
-				     NULL);
-	}
+	list_for_each_entry(func, &slot->funcs, sibling)
+		acpi_evaluate_reg(func_to_handle(func),
+				  ACPI_ADR_SPACE_PCI_CONFIG,
+				  ACPI_REG_CONNECT);
 }
 
 static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
-- 
2.26.0

[PATCH 3/3] pinctrl: cherryview: Use the new acpi_evaluate_reg() helper

[Hans de Goede]

Use the new acpi_evaluate_reg() helper in the pinctrl-cherryview code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 4817aec114d6..579bdfcfab8f 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1693,8 +1693,6 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function,
 
 static int chv_pinctrl_probe(struct platform_device *pdev)
 {
-	struct acpi_object_list input;
-	union acpi_object params[2];
 	struct chv_pinctrl *pctrl;
 	struct acpi_device *adev;
 	acpi_status status;
@@ -1765,13 +1763,8 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 	 * the GeneralPurposeIo OpRegion. Manually call _REG here so that
 	 * the DSDT-s GeneralPurposeIo availability checks will succeed.
 	 */
-	params[0].type = ACPI_TYPE_INTEGER;
-	params[0].integer.value = ACPI_ADR_SPACE_GPIO;
-	params[1].type = ACPI_TYPE_INTEGER;
-	params[1].integer.value = 1;
-	input.count = 2;
-	input.pointer = params;
-	acpi_evaluate_object(adev->handle, "_REG", &input, NULL);
+	acpi_evaluate_reg(adev->handle, ACPI_ADR_SPACE_GPIO,
+			  ACPI_REG_CONNECT);
 
 	platform_set_drvdata(pdev, pctrl);
 
-- 
2.26.0

Re: [PATCH 2/3] ACPI / hotplug / PCI: Use the new acpi_evaluate_reg() helper

[Andy Shevchenko]

On Tue, May 05, 2020 at 03:21:27PM +0200, Hans de Goede wrote:
> Use the new acpi_evaluate_reg() helper in the acpiphp_glue.c code.

> -		/* _REG is optional, we don't care about if there is failure */

This may be left.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Andy Shevchenko]

On Tue, May 05, 2020 at 03:21:26PM +0200, Hans de Goede wrote:
> With a recent fix to the pinctrl-cherryview driver we know have
> 2 drivers open-coding the parameter building / passing for calling
> _REG on an ACPI handle.
> 
> Add a helper for this, so that these 2 drivers can be converted to this
> helper.

Suggested-by?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Andy Shevchenko]

On Tue, May 05, 2020 at 03:21:26PM +0200, Hans de Goede wrote:
> With a recent fix to the pinctrl-cherryview driver we know have
> 2 drivers open-coding the parameter building / passing for calling
> _REG on an ACPI handle.
> 
> Add a helper for this, so that these 2 drivers can be converted to this
> helper.

> + * @function: Parameter to pass to _REG one of ACPI_REG_CONNECT or
> + *            ACPI_REG_DISCONNECT

Is it enum or definitions? If former can we refer to it?


(Sorry, clicked reply to fast with previous mail)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] pinctrl: cherryview: Use the new acpi_evaluate_reg() helper

[Andy Shevchenko]

On Tue, May 05, 2020 at 03:21:28PM +0200, Hans de Goede wrote:
> Use the new acpi_evaluate_reg() helper in the pinctrl-cherryview code.

Will wait for Mika's Ack.

> +	acpi_evaluate_reg(adev->handle, ACPI_ADR_SPACE_GPIO,
> +			  ACPI_REG_CONNECT);

I believe it perfectly one line (perhaps description in kernel doc as well in patch 1).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Andy Shevchenko]

On Tue, May 05, 2020 at 03:21:25PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is a small series adding an acpi_evaluate_reg() helper, note
> the third patch sits on top of a fix for the pinctrl-cherryview
> driver which I recently submitted and which is still finding its
> way upstream.
> 
> Since this is not urgent (just a small code cleanup) I suggest
> that the ACPI people can pick up patches 1-2 and then the last patch
> can be merged post 5.8-rc1, at which point all the dependencies for
> it should have landed already.

Thank you!

Some minor comments to be addressed.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Bjorn Helgaas]

On Tue, May 05, 2020 at 03:21:26PM +0200, Hans de Goede wrote:
> With a recent fix to the pinctrl-cherryview driver we know have
> 2 drivers open-coding the parameter building / passing for calling
> _REG on an ACPI handle.

s/know/now/

Re: [PATCH 2/3] ACPI / hotplug / PCI: Use the new acpi_evaluate_reg() helper

[Bjorn Helgaas]

On Tue, May 05, 2020 at 03:21:27PM +0200, Hans de Goede wrote:
> Use the new acpi_evaluate_reg() helper in the acpiphp_glue.c code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

OK by me if this is merged with the first.

> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index b3869951c0eb..fd77eb1d460c 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -385,20 +385,11 @@ static unsigned char acpiphp_max_busnr(struct pci_bus *bus)
>  static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)
>  {
>  	struct acpiphp_func *func;
> -	union acpi_object params[2];
> -	struct acpi_object_list arg_list;
>  
> -	list_for_each_entry(func, &slot->funcs, sibling) {
> -		arg_list.count = 2;
> -		arg_list.pointer = params;
> -		params[0].type = ACPI_TYPE_INTEGER;
> -		params[0].integer.value = ACPI_ADR_SPACE_PCI_CONFIG;
> -		params[1].type = ACPI_TYPE_INTEGER;
> -		params[1].integer.value = 1;
> -		/* _REG is optional, we don't care about if there is failure */
> -		acpi_evaluate_object(func_to_handle(func), "_REG", &arg_list,
> -				     NULL);
> -	}
> +	list_for_each_entry(func, &slot->funcs, sibling)
> +		acpi_evaluate_reg(func_to_handle(func),
> +				  ACPI_ADR_SPACE_PCI_CONFIG,
> +				  ACPI_REG_CONNECT);
>  }
>  
>  static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev)
> -- 
> 2.26.0
> 

Re: [PATCH 0/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Hans de Goede]

Hi,

On 5/5/20 5:44 PM, Andy Shevchenko wrote:
> On Tue, May 05, 2020 at 03:21:25PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is a small series adding an acpi_evaluate_reg() helper, note
>> the third patch sits on top of a fix for the pinctrl-cherryview
>> driver which I recently submitted and which is still finding its
>> way upstream.
>>
>> Since this is not urgent (just a small code cleanup) I suggest
>> that the ACPI people can pick up patches 1-2 and then the last patch
>> can be merged post 5.8-rc1, at which point all the dependencies for
>> it should have landed already.
> 
> Thank you!
> 
> Some minor comments to be addressed.
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you, I will fix the remarks you have and send out a v2
with just patches 1 and 2 for now, then we can move forward
with those.

How to deal with the _REG not being called issue by the ACPICA
core issue on Cherryview devices is still being discussed, so
lets wait for the final fix for that and then I can send out
a new version of the 3th patch on top of that once the other
bits are merged.

Regards,

Hans

Re: [PATCH 1/3] ACPI / utils: Add acpi_evaluate_reg() helper

[Hans de Goede]

Hi,

On 5/5/20 5:42 PM, Andy Shevchenko wrote:
> On Tue, May 05, 2020 at 03:21:26PM +0200, Hans de Goede wrote:
>> With a recent fix to the pinctrl-cherryview driver we know have
>> 2 drivers open-coding the parameter building / passing for calling
>> _REG on an ACPI handle.
>>
>> Add a helper for this, so that these 2 drivers can be converted to this
>> helper.
> 
> Suggested-by?

Right sorry about that I will fix this for v2.

 >> + * @function: Parameter to pass to _REG one of ACPI_REG_CONNECT or
 >> + *            ACPI_REG_DISCONNECT
 >
 > Is it enum or definitions? If former can we refer to it?

These are #define-s.

Regards,

Hans