[PATCH 0/2] Only wait for INT3394 extcon to show ip if enabled

[PATCH 0/2] Only wait for INT3394 extcon to show ip if enabled

[Hans de Goede]

Hi All,

As with my previous (somewhat related) series the last patch of this series
depend on the first, as such it might be better to merge both through the
acpi tree if Sebastius is ok with that.

Otherwise these patches too are pretty self-explanatory.

Regards,

Hans

[PATCH 1/2] acpi: utils: Add new acpi_dev_present helper

[Hans de Goede]

acpi_dev_found just iterates over all acpi-ids and sees if one matches.
This means that it will return true for devices which are in the dsdt
but disabled (their _STA method returns 0).

For some drivers it is useful to be able to check if a certain hid
is not only present in the namespace, but also actually present as in
acpi_device_is_present() will return true for the device. For example
because if a certain device is present then the driver will want to use
an extcon or IIO adc channel provided by that device.

This commit adds a new acpi_dev_present helper which drivers can use
to this end.

Arguably acpi_dev_present is what acpi_dev_found should have been, but
there are too many users to just change acpi_dev_found without the risk
of breaking something.

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

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 22c0995..40f8953 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -736,6 +736,40 @@ bool acpi_dev_found(const char *hid)
 }
 EXPORT_SYMBOL(acpi_dev_found);
 
+static acpi_status acpi_dev_present_cb(acpi_handle ah, u32 level, void *ctx,
+				     void **retval)
+{
+	/*
+	 * acpi_get_devices() does all the work for us, if we get called
+	 * a device with a matching hid has been found and its _STA indicates
+	 * it is present.
+	 */
+	*(bool *)ctx = true;
+	return AE_CTRL_TERMINATE;
+}
+
+/**
+ * acpi_dev_present - Detect that a given ACPI device is present
+ * @hid: Hardware ID of the device.
+ *
+ * Return %true if the device was present at the moment of invocation.
+ * Note that if the device is pluggable, it may since have disappeared.
+ *
+ * Note that unlike acpi_dev_found() this function checks the status
+ * of the device so for devices which are present in the dsdt, but
+ * which are disabled (their _STA callback returns 0) this function
+ * will return false.
+ */
+bool acpi_dev_present(const char *hid)
+{
+	bool present = false;
+
+	acpi_get_devices(hid, acpi_dev_present_cb, &present, NULL);
+
+	return present;
+}
+EXPORT_SYMBOL(acpi_dev_present);
+
 /*
  * acpi_backlight= handling, this is done here rather then in video_detect.c
  * because __setup cannot be used in modules.
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ef0ae8a..29f6b5f 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
 	}
 
 bool acpi_dev_found(const char *hid);
+bool acpi_dev_present(const char *hid);
 
 #ifdef CONFIG_ACPI
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 673acda..059ffdc 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -614,6 +614,11 @@ static inline bool acpi_dev_found(const char *hid)
 	return false;
 }
 
+static inline bool acpi_dev_present(const char *hid)
+{
+	return false;
+}
+
 static inline bool is_acpi_node(struct fwnode_handle *fwnode)
 {
 	return false;
-- 
2.9.3

[PATCH 2/2] power: supply: axp288_charger: Only wait for INT3496 device if present

[Hans de Goede]

On some devices with an axp288 pmic setting vbus path based on the
id-pin is handled by an ACPI _AIE interrupt on the gpio and the
INT3496 device is disabled.

Instead of returning -EPROBE_DEFER on these devices waiting for the
never to show up INT3496 device, check for its presence and only
request and monitor the matching extcon if the device is there,
otherwise let the firmware handle the vbus path control.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index d3bf4b2..6c4ad66 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -14,6 +14,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/regmap.h>
@@ -114,7 +115,8 @@
 #define ILIM_3000MA			3000	/* 3000mA */
 
 #define AXP288_EXTCON_DEV_NAME		"axp288_extcon"
-#define USB_HOST_EXTCON_DEV_NAME	"INT3496:00"
+#define USB_HOST_EXTCON_HID		"INT3496"
+#define USB_HOST_EXTCON_NAME		"INT3496:00"
 
 static const unsigned int cable_ids[] =
 	{ EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
@@ -808,10 +810,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
-	info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_DEV_NAME);
-	if (info->otg.cable == NULL) {
-		dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-		return -EPROBE_DEFER;
+	if (acpi_dev_present(USB_HOST_EXTCON_HID)) {
+		info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME);
+		if (info->otg.cable == NULL) {
+			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
+			return -EPROBE_DEFER;
+		}
 	}
 
 	platform_set_drvdata(pdev, info);
@@ -850,11 +854,13 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	/* Register for OTG notification */
 	INIT_WORK(&info->otg.work, axp288_charger_otg_evt_worker);
 	info->otg.id_nb.notifier_call = axp288_charger_handle_otg_evt;
-	ret = devm_extcon_register_notifier(&pdev->dev, info->otg.cable,
+	if (info->otg.cable) {
+		ret = devm_extcon_register_notifier(&pdev->dev, info->otg.cable,
 					EXTCON_USB_HOST, &info->otg.id_nb);
-	if (ret) {
-		dev_err(dev, "failed to register EXTCON_USB_HOST notifier\n");
-		return ret;
+		if (ret) {
+			dev_err(dev, "failed to register EXTCON_USB_HOST notifier\n");
+			return ret;
+		}
 	}
 	schedule_work(&info->otg.work);
 
-- 
2.9.3

Re: [PATCH 2/2] power: supply: axp288_charger: Only wait for INT3496 device if present

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

Hi,

On Thu, Mar 16, 2017 at 05:17:36PM +0100, Hans de Goede wrote:
> On some devices with an axp288 pmic setting vbus path based on the
> id-pin is handled by an ACPI _AIE interrupt on the gpio and the
> INT3496 device is disabled.
> 
> Instead of returning -EPROBE_DEFER on these devices waiting for the
> never to show up INT3496 device, check for its presence and only
> request and monitor the matching extcon if the device is there,
> otherwise let the firmware handle the vbus path control.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/axp288_charger.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index d3bf4b2..6c4ad66 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -14,6 +14,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/device.h>
>  #include <linux/regmap.h>
> @@ -114,7 +115,8 @@
>  #define ILIM_3000MA			3000	/* 3000mA */
>  
>  #define AXP288_EXTCON_DEV_NAME		"axp288_extcon"
> -#define USB_HOST_EXTCON_DEV_NAME	"INT3496:00"
> +#define USB_HOST_EXTCON_HID		"INT3496"
> +#define USB_HOST_EXTCON_NAME		"INT3496:00"
>  
>  static const unsigned int cable_ids[] =
>  	{ EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
> @@ -808,10 +810,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
>  		return -EPROBE_DEFER;
>  	}
>  
> -	info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_DEV_NAME);
> -	if (info->otg.cable == NULL) {
> -		dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
> -		return -EPROBE_DEFER;
> +	if (acpi_dev_present(USB_HOST_EXTCON_HID)) {

I may have missed something, but as far as I can see axp288 also
supports DT based init, so that should be handled here.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] power: supply: axp288_charger: Only wait for INT3496 device if present

[Chen-Yu Tsai]

Hi,

On Mon, Mar 20, 2017 at 9:30 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Thu, Mar 16, 2017 at 05:17:36PM +0100, Hans de Goede wrote:
>> On some devices with an axp288 pmic setting vbus path based on the
>> id-pin is handled by an ACPI _AIE interrupt on the gpio and the
>> INT3496 device is disabled.
>>
>> Instead of returning -EPROBE_DEFER on these devices waiting for the
>> never to show up INT3496 device, check for its presence and only
>> request and monitor the matching extcon if the device is there,
>> otherwise let the firmware handle the vbus path control.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/axp288_charger.c | 24 +++++++++++++++---------
>>  1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
>> index d3bf4b2..6c4ad66 100644
>> --- a/drivers/power/supply/axp288_charger.c
>> +++ b/drivers/power/supply/axp288_charger.c
>> @@ -14,6 +14,7 @@
>>   * GNU General Public License for more details.
>>   */
>>
>> +#include <linux/acpi.h>
>>  #include <linux/module.h>
>>  #include <linux/device.h>
>>  #include <linux/regmap.h>
>> @@ -114,7 +115,8 @@
>>  #define ILIM_3000MA                  3000    /* 3000mA */
>>
>>  #define AXP288_EXTCON_DEV_NAME               "axp288_extcon"
>> -#define USB_HOST_EXTCON_DEV_NAME     "INT3496:00"
>> +#define USB_HOST_EXTCON_HID          "INT3496"
>> +#define USB_HOST_EXTCON_NAME         "INT3496:00"
>>
>>  static const unsigned int cable_ids[] =
>>       { EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>> @@ -808,10 +810,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
>>               return -EPROBE_DEFER;
>>       }
>>
>> -     info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_DEV_NAME);
>> -     if (info->otg.cable == NULL) {
>> -             dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
>> -             return -EPROBE_DEFER;
>> +     if (acpi_dev_present(USB_HOST_EXTCON_HID)) {
>
> I may have missed something, but as far as I can see axp288 also
> supports DT based init, so that should be handled here.

I doubt that will happen. AXP288 was designed specifically for Intel
Bay Trail platforms, which AFAIK use ACPI. For the other AXP series
PMIC we have another battery driver for them coming in.

If you look in drivers/mfd/axp20x.c and drivers/mfd/axp20x-i2c.c,
you'll see that AXP288 is only instantiated from ACPI, and all
the axp288 named drivers are only instantiated for the AXP288
mfd device.

Regards
ChenYu

Re: [PATCH 2/2] power: supply: axp288_charger: Only wait for INT3496 device if present

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]

Hi,

On Mon, Mar 20, 2017 at 04:56:18PM +0800, Chen-Yu Tsai wrote:
> On Mon, Mar 20, 2017 at 9:30 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > On Thu, Mar 16, 2017 at 05:17:36PM +0100, Hans de Goede wrote:
> >> On some devices with an axp288 pmic setting vbus path based on the
> >> id-pin is handled by an ACPI _AIE interrupt on the gpio and the
> >> INT3496 device is disabled.
> >>
> >> Instead of returning -EPROBE_DEFER on these devices waiting for the
> >> never to show up INT3496 device, check for its presence and only
> >> request and monitor the matching extcon if the device is there,
> >> otherwise let the firmware handle the vbus path control.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/power/supply/axp288_charger.c | 24 +++++++++++++++---------
> >>  1 file changed, 15 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> >> index d3bf4b2..6c4ad66 100644
> >> --- a/drivers/power/supply/axp288_charger.c
> >> +++ b/drivers/power/supply/axp288_charger.c
> >> @@ -14,6 +14,7 @@
> >>   * GNU General Public License for more details.
> >>   */
> >>
> >> +#include <linux/acpi.h>
> >>  #include <linux/module.h>
> >>  #include <linux/device.h>
> >>  #include <linux/regmap.h>
> >> @@ -114,7 +115,8 @@
> >>  #define ILIM_3000MA                  3000    /* 3000mA */
> >>
> >>  #define AXP288_EXTCON_DEV_NAME               "axp288_extcon"
> >> -#define USB_HOST_EXTCON_DEV_NAME     "INT3496:00"
> >> +#define USB_HOST_EXTCON_HID          "INT3496"
> >> +#define USB_HOST_EXTCON_NAME         "INT3496:00"
> >>
> >>  static const unsigned int cable_ids[] =
> >>       { EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
> >> @@ -808,10 +810,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
> >>               return -EPROBE_DEFER;
> >>       }
> >>
> >> -     info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_DEV_NAME);
> >> -     if (info->otg.cable == NULL) {
> >> -             dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
> >> -             return -EPROBE_DEFER;
> >> +     if (acpi_dev_present(USB_HOST_EXTCON_HID)) {
> >
> > I may have missed something, but as far as I can see axp288 also
> > supports DT based init, so that should be handled here.
> 
> I doubt that will happen. AXP288 was designed specifically for Intel
> Bay Trail platforms, which AFAIK use ACPI. For the other AXP series
> PMIC we have another battery driver for them coming in.
> 
> If you look in drivers/mfd/axp20x.c and drivers/mfd/axp20x-i2c.c,
> you'll see that AXP288 is only instantiated from ACPI, and all
> the axp288 named drivers are only instantiated for the AXP288
> mfd device.

Ok. I just looked briefly at drivers/mfd/axp20x.c and saw, that it
contained of_compatible entries. On a closer look axp288 does not
have them, so:

Acked-by: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] acpi: utils: Add new acpi_dev_present helper

[Rafael J. Wysocki]

On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote:
> acpi_dev_found just iterates over all acpi-ids and sees if one matches.
> This means that it will return true for devices which are in the dsdt
> but disabled (their _STA method returns 0).
> 
> For some drivers it is useful to be able to check if a certain hid
> is not only present in the namespace, but also actually present as in
> acpi_device_is_present() will return true for the device. For example
> because if a certain device is present then the driver will want to use
> an extcon or IIO adc channel provided by that device.
> 
> This commit adds a new acpi_dev_present helper which drivers can use
> to this end.
> 
> Arguably acpi_dev_present is what acpi_dev_found should have been, but
> there are too many users to just change acpi_dev_found without the risk
> of breaking something.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Andy, Mika, any concerns about this patch, please?

> ---
>  drivers/acpi/utils.c    | 34 ++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |  1 +
>  include/linux/acpi.h    |  5 +++++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 22c0995..40f8953 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -736,6 +736,40 @@ bool acpi_dev_found(const char *hid)
>  }
>  EXPORT_SYMBOL(acpi_dev_found);
>  
> +static acpi_status acpi_dev_present_cb(acpi_handle ah, u32 level, void *ctx,
> +				     void **retval)
> +{
> +	/*
> +	 * acpi_get_devices() does all the work for us, if we get called
> +	 * a device with a matching hid has been found and its _STA indicates
> +	 * it is present.
> +	 */
> +	*(bool *)ctx = true;
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +/**
> + * acpi_dev_present - Detect that a given ACPI device is present
> + * @hid: Hardware ID of the device.
> + *
> + * Return %true if the device was present at the moment of invocation.
> + * Note that if the device is pluggable, it may since have disappeared.
> + *
> + * Note that unlike acpi_dev_found() this function checks the status
> + * of the device so for devices which are present in the dsdt, but
> + * which are disabled (their _STA callback returns 0) this function
> + * will return false.
> + */
> +bool acpi_dev_present(const char *hid)
> +{
> +	bool present = false;
> +
> +	acpi_get_devices(hid, acpi_dev_present_cb, &present, NULL);
> +
> +	return present;
> +}
> +EXPORT_SYMBOL(acpi_dev_present);
> +
>  /*
>   * acpi_backlight= handling, this is done here rather then in video_detect.c
>   * because __setup cannot be used in modules.
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ef0ae8a..29f6b5f 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
>  	}
>  
>  bool acpi_dev_found(const char *hid);
> +bool acpi_dev_present(const char *hid);
>  
>  #ifdef CONFIG_ACPI
>  
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 673acda..059ffdc 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -614,6 +614,11 @@ static inline bool acpi_dev_found(const char *hid)
>  	return false;
>  }
>  
> +static inline bool acpi_dev_present(const char *hid)
> +{
> +	return false;
> +}
> +
>  static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>  {
>  	return false;
> 

Re: [PATCH 1/2] acpi: utils: Add new acpi_dev_present helper

[Mika Westerberg]

On Tue, Mar 28, 2017 at 11:42:29PM +0200, Rafael J. Wysocki wrote:
> On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote:
> > acpi_dev_found just iterates over all acpi-ids and sees if one matches.
> > This means that it will return true for devices which are in the dsdt
> > but disabled (their _STA method returns 0).
> > 
> > For some drivers it is useful to be able to check if a certain hid
> > is not only present in the namespace, but also actually present as in
> > acpi_device_is_present() will return true for the device. For example
> > because if a certain device is present then the driver will want to use
> > an extcon or IIO adc channel provided by that device.
> > 
> > This commit adds a new acpi_dev_present helper which drivers can use
> > to this end.
> > 
> > Arguably acpi_dev_present is what acpi_dev_found should have been, but
> > there are too many users to just change acpi_dev_found without the risk
> > of breaking something.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Andy, Mika, any concerns about this patch, please?

No concerns from my side.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/2] acpi: utils: Add new acpi_dev_present helper

[Andy Shevchenko]

On Wed, 2017-03-29 at 12:26 +0300, Mika Westerberg wrote:
> On Tue, Mar 28, 2017 at 11:42:29PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote:
> > > acpi_dev_found just iterates over all acpi-ids and sees if one
> > > matches.
> > > This means that it will return true for devices which are in the
> > > dsdt
> > > but disabled (their _STA method returns 0).
> > > 
> > > For some drivers it is useful to be able to check if a certain hid
> > > is not only present in the namespace, but also actually present as
> > > in
> > > acpi_device_is_present() will return true for the device. For
> > > example
> > > because if a certain device is present then the driver will want
> > > to use
> > > an extcon or IIO adc channel provided by that device.
> > > 
> > > This commit adds a new acpi_dev_present helper which drivers can
> > > use
> > > to this end.
> > > 
> > > Arguably acpi_dev_present is what acpi_dev_found should have been,
> > > but
> > > there are too many users to just change acpi_dev_found without the
> > > risk
> > > of breaking something.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Andy, Mika, any concerns about this patch, please?
> 
> No concerns from my side.
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Neither from me.

I briefly looked in the code and indeed we have no existing helper as a
such, so, if you think this is the best way to go, I would agree.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] acpi: utils: Add new acpi_dev_present helper

[Lukas Wunner]

[cc += Robert Moore]

Hi Hans,

I'm the author of acpi_dev_found(), please in the future use git blame
to determine relevant authors of existing code that should be cc'ed,
I noticed your patch only now:

On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote:
> acpi_dev_found just iterates over all acpi-ids and sees if one matches.
> This means that it will return true for devices which are in the dsdt
> but disabled (their _STA method returns 0).
>
> For some drivers it is useful to be able to check if a certain hid
> is not only present in the namespace, but also actually present as in
> acpi_device_is_present() will return true for the device. For example
> because if a certain device is present then the driver will want to use
> an extcon or IIO adc channel provided by that device.
>
> This commit adds a new acpi_dev_present helper which drivers can use
> to this end.
>
> Arguably acpi_dev_present is what acpi_dev_found should have been, but
> there are too many users to just change acpi_dev_found without the risk
> of breaking something.

I originally did submit an acpi_dev_present() function which was identical
to what you've submitted now:
http://www.spinics.net/lists/linux-acpi/msg61865.html

However Robert Moore raised an objection that "Traversing the namespace
over and over is truly brute force":
http://www.spinics.net/lists/linux-acpi/msg61911.html

For my use case, which was apple_gmux_present(), just detecting presence
of the HID in the namespace was sufficient, I did not have the need to
execute _STA.  Hence to address Robert Moore's concern I switched to
simply traversing the acpi_bus_id_list.

Rafael objected to the acpi_dev_present() name as it suggested that _STA
is checked even though it wasn't, so I renamed the function to
acpi_dev_found() with commit c68ae33e7fb4.

The objection raised by Robert Moore applies to your patch as well since
it is identical to my original patch.  The return value of _STA seems to
be cached in the "status" field of struct acpi_device, so you may
be able to overcome Robert Moore's objection by calling bus_find_device()
with a callback which contains the check in acpi_device_is_present().
See drivers/firmware/efi/dev-path-parser.c for an example (parse_acpi_path()
and match_acpi_dev()).  This is probably faster than acpi_get_devices()
because it just traverses a list instead of walking the namespace and it
avoids the call to _STA.  (Some devices just return a constant when _STA
is called, others may take more time.)

FWIW, all existing users of acpi_dev_found(), except for the hisilicon
Ethernet driver, originally used acpi_get_devices() and I converted
them to acpi_dev_found to deduplicate code.  Thus it would be safe to
convert those to your new function acpi_dev_present().  I also converted
sound/soc/intel/common/sst-match-acpi.c to acpi_dev_found() but the
Intel folks switched back to acpi_get_devices() because just like you
they had the need to check _STA.  If you introduce a new helper which
checks _STA, it would be good if you could amend sst-match-acpi.c to
use it so as to deduplicate code.

Thanks,

Lukas

Re: [PATCH 1/2] acpi: utils: Add new acpi_dev_present helper

[Rafael J. Wysocki]

On Thu, Mar 30, 2017 at 10:33 AM, Lukas Wunner <lukas@wunner.de> wrote:
> [cc += Robert Moore]
>
> Hi Hans,
>
> I'm the author of acpi_dev_found(), please in the future use git blame
> to determine relevant authors of existing code that should be cc'ed,
> I noticed your patch only now:
>
> On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote:
>> acpi_dev_found just iterates over all acpi-ids and sees if one matches.
>> This means that it will return true for devices which are in the dsdt
>> but disabled (their _STA method returns 0).
>>
>> For some drivers it is useful to be able to check if a certain hid
>> is not only present in the namespace, but also actually present as in
>> acpi_device_is_present() will return true for the device. For example
>> because if a certain device is present then the driver will want to use
>> an extcon or IIO adc channel provided by that device.
>>
>> This commit adds a new acpi_dev_present helper which drivers can use
>> to this end.
>>
>> Arguably acpi_dev_present is what acpi_dev_found should have been, but
>> there are too many users to just change acpi_dev_found without the risk
>> of breaking something.
>
> I originally did submit an acpi_dev_present() function which was identical
> to what you've submitted now:
> http://www.spinics.net/lists/linux-acpi/msg61865.html
>
> However Robert Moore raised an objection that "Traversing the namespace
> over and over is truly brute force":
> http://www.spinics.net/lists/linux-acpi/msg61911.html
>
> For my use case, which was apple_gmux_present(), just detecting presence
> of the HID in the namespace was sufficient, I did not have the need to
> execute _STA.  Hence to address Robert Moore's concern I switched to
> simply traversing the acpi_bus_id_list.
>
> Rafael objected to the acpi_dev_present() name as it suggested that _STA
> is checked even though it wasn't, so I renamed the function to
> acpi_dev_found() with commit c68ae33e7fb4.
>
> The objection raised by Robert Moore applies to your patch as well since
> it is identical to my original patch.

Good point, actually.

> The return value of _STA seems to
> be cached in the "status" field of struct acpi_device, so you may
> be able to overcome Robert Moore's objection by calling bus_find_device()
> with a callback which contains the check in acpi_device_is_present().
> See drivers/firmware/efi/dev-path-parser.c for an example (parse_acpi_path()
> and match_acpi_dev()).  This is probably faster than acpi_get_devices()
> because it just traverses a list instead of walking the namespace and it
> avoids the call to _STA.  (Some devices just return a constant when _STA
> is called, others may take more time.)

Yes, that would be preferred.

Thanks,
Rafael

Re: [PATCH 1/2] acpi: utils: Add new acpi_dev_present helper

[Hans de Goede]

Hi Lukas,

On 30-03-17 10:33, Lukas Wunner wrote:
> [cc += Robert Moore]
>
> Hi Hans,
>
> I'm the author of acpi_dev_found(), please in the future use git blame
> to determine relevant authors of existing code that should be cc'ed,

Right, sorry about that.

> I noticed your patch only now:
>
> On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote:
>> acpi_dev_found just iterates over all acpi-ids and sees if one matches.
>> This means that it will return true for devices which are in the dsdt
>> but disabled (their _STA method returns 0).
>>
>> For some drivers it is useful to be able to check if a certain hid
>> is not only present in the namespace, but also actually present as in
>> acpi_device_is_present() will return true for the device. For example
>> because if a certain device is present then the driver will want to use
>> an extcon or IIO adc channel provided by that device.
>>
>> This commit adds a new acpi_dev_present helper which drivers can use
>> to this end.
>>
>> Arguably acpi_dev_present is what acpi_dev_found should have been, but
>> there are too many users to just change acpi_dev_found without the risk
>> of breaking something.
>
> I originally did submit an acpi_dev_present() function which was identical
> to what you've submitted now:
> http://www.spinics.net/lists/linux-acpi/msg61865.html
>
> However Robert Moore raised an objection that "Traversing the namespace
> over and over is truly brute force":
> http://www.spinics.net/lists/linux-acpi/msg61911.html
>
> For my use case, which was apple_gmux_present(), just detecting presence
> of the HID in the namespace was sufficient, I did not have the need to
> execute _STA.  Hence to address Robert Moore's concern I switched to
> simply traversing the acpi_bus_id_list.
>
> Rafael objected to the acpi_dev_present() name as it suggested that _STA
> is checked even though it wasn't, so I renamed the function to
> acpi_dev_found() with commit c68ae33e7fb4.
>
> The objection raised by Robert Moore applies to your patch as well since
> it is identical to my original patch.  The return value of _STA seems to
> be cached in the "status" field of struct acpi_device, so you may
> be able to overcome Robert Moore's objection by calling bus_find_device()
> with a callback which contains the check in acpi_device_is_present().
> See drivers/firmware/efi/dev-path-parser.c for an example (parse_acpi_path()
> and match_acpi_dev()).  This is probably faster than acpi_get_devices()
> because it just traverses a list instead of walking the namespace and it
> avoids the call to _STA.  (Some devices just return a constant when _STA
> is called, others may take more time.)

Thank you for your input. I've prepared a new version using your suggestion
and slightly extended the function with a uid and hrv argument so that
it becomes more generally useful, e.g. it can be used now to replace
the code from drivers/firmware/efi/dev-path-parser.c you gave as an
example how to do this.

I'm going to submit a new version of this patch-set now, including
a few patches replacing another series from me which can also use the
new extended functionality.

If people like the new v2 acpi_dev_present() I will submit patches
to replace the custom code for this in drivers/firmware/efi/dev-path-parser.c
and sound/soc/intel/common/sst-match-acpi.c once it is merged.

Talking about merging, I also have some none ACPI patches which
need this pending (such as 2/2 of the orgiinal series) it would be
great of at least the first patch of the new series could be merged
in time for 4.12, or alternatively an immutable branch with it
can be created for 4.13 once 4.12-rc1 is released.

Regards,

Hans


>
> FWIW, all existing users of acpi_dev_found(), except for the hisilicon
> Ethernet driver, originally used acpi_get_devices() and I converted
> them to acpi_dev_found to deduplicate code.  Thus it would be safe to
> convert those to your new function acpi_dev_present().  I also converted
> sound/soc/intel/common/sst-match-acpi.c to acpi_dev_found() but the
> Intel folks switched back to acpi_get_devices() because just like you
> they had the need to check _STA.  If you introduce a new helper which
> checks _STA, it would be good if you could amend sst-match-acpi.c to
> use it so as to deduplicate code.
>
> Thanks,
>
> Lukas
>