[PATCH 0/3] platform/chrome: notify: Use PD_HOST_EVENT_STATUS

[PATCH 0/3] platform/chrome: notify: Use PD_HOST_EVENT_STATUS

[Prashant Malani]

This series improves the PD notifier driver to get the
EC_CMD_PD_HOST_EVENT_STATUS bits from the Chrome EC, and send those to
the notifier listeners. Earlier, the "event" param of the notifier was
always being hard-coded to a single value (corresponding to PD_MCU
events on ACPI and DT platforms) which wasn't of much use to the
listeners.

Prashant Malani (3):
  platform/chrome: notify: Add driver data struct
  platform/chrome: notify: Amend ACPI driver to plat
  platform/chrome: notify: Pull PD_HOST_EVENT status

 drivers/platform/chrome/cros_usbpd_notify.c | 197 +++++++++++++++++---
 1 file changed, 174 insertions(+), 23 deletions(-)

-- 
2.25.1.696.g5e7596f4ac-goog

[PATCH 1/3] platform/chrome: notify: Add driver data struct

[Prashant Malani]

Introduce a device driver data structure, cros_usbpd_notify_data, in
which we can store the notifier block object and pointers to the struct
cros_ec_device and struct device objects.

This will make it more convenient to access these pointers when
executing both platform and ACPI callbacks.

While we are here, also add a dev_info print declaring successful device
registration at the end of the platform probe function.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_usbpd_notify.c | 30 ++++++++++++++-------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
index 3851bbd6e9a39..edcb346024b07 100644
--- a/drivers/platform/chrome/cros_usbpd_notify.c
+++ b/drivers/platform/chrome/cros_usbpd_notify.c
@@ -16,6 +16,12 @@
 
 static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list);
 
+struct cros_usbpd_notify_data {
+	struct device *dev;
+	struct cros_ec_device *ec;
+	struct notifier_block nb;
+};
+
 /**
  * cros_usbpd_register_notify - Register a notifier callback for PD events.
  * @nb: Notifier block pointer to register
@@ -98,23 +104,28 @@ static int cros_usbpd_notify_probe_plat(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct cros_ec_dev *ecdev = dev_get_drvdata(dev->parent);
-	struct notifier_block *nb;
+	struct cros_usbpd_notify_data *pdnotify;
 	int ret;
 
-	nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL);
-	if (!nb)
+	pdnotify = devm_kzalloc(dev, sizeof(*pdnotify), GFP_KERNEL);
+	if (!pdnotify)
 		return -ENOMEM;
 
-	nb->notifier_call = cros_usbpd_notify_plat;
-	dev_set_drvdata(dev, nb);
+	pdnotify->dev = dev;
+	pdnotify->ec = ecdev->ec_dev;
+	pdnotify->nb.notifier_call = cros_usbpd_notify_plat;
+
+	dev_set_drvdata(dev, pdnotify);
 
 	ret = blocking_notifier_chain_register(&ecdev->ec_dev->event_notifier,
-					       nb);
+					       &pdnotify->nb);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register notifier\n");
 		return ret;
 	}
 
+	dev_info(dev, "Chrome EC PD notify device registered.\n");
+
 	return 0;
 }
 
@@ -122,10 +133,11 @@ static int cros_usbpd_notify_remove_plat(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct cros_ec_dev *ecdev = dev_get_drvdata(dev->parent);
-	struct notifier_block *nb =
-		(struct notifier_block *)dev_get_drvdata(dev);
+	struct cros_usbpd_notify_data *pdnotify =
+		(struct cros_usbpd_notify_data *)dev_get_drvdata(dev);
 
-	blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier, nb);
+	blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier,
+					   &pdnotify->nb);
 
 	return 0;
 }
-- 
2.25.1.696.g5e7596f4ac-goog

[PATCH 2/3] platform/chrome: notify: Amend ACPI driver to plat

[Prashant Malani]

Convert the ACPI driver into the equivalent platform driver, with the
same ACPI match table as before. This allows the device driver to access
the parent platform EC device and its cros_ec_device struct, which will
be required to communicate with the EC to pull PD Host event information
from it.

Also change the ACPI driver name to "cros-usbpd-notify-acpi" so that
there is no confusion between it and the "regular" platform driver on
platforms that have both CONFIG_ACPI and CONFIG_OF enabled.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_usbpd_notify.c | 82 ++++++++++++++++++---
 1 file changed, 70 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
index edcb346024b07..d2dbf7017e29c 100644
--- a/drivers/platform/chrome/cros_usbpd_notify.c
+++ b/drivers/platform/chrome/cros_usbpd_notify.c
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 
 #define DRV_NAME "cros-usbpd-notify"
+#define DRV_NAME_PLAT_ACPI "cros-usbpd-notify-acpi"
 #define ACPI_DRV_NAME "GOOG0003"
 
 static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list);
@@ -54,14 +55,72 @@ EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify);
 
 #ifdef CONFIG_ACPI
 
-static int cros_usbpd_notify_add_acpi(struct acpi_device *adev)
+static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data)
 {
+	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
+}
+
+static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev)
+{
+	struct cros_usbpd_notify_data *pdnotify;
+	struct device *dev = &pdev->dev;
+	struct acpi_device *adev;
+	struct cros_ec_device *ec_dev;
+	acpi_status status;
+
+	adev = ACPI_COMPANION(dev);
+	if (!adev) {
+		dev_err(dev, "No ACPI device found.\n");
+		return -ENODEV;
+	}
+
+	pdnotify = devm_kzalloc(dev, sizeof(*pdnotify), GFP_KERNEL);
+	if (!pdnotify)
+		return -ENOMEM;
+
+	/* Get the EC device pointer needed to talk to the EC. */
+	ec_dev = dev_get_drvdata(dev->parent);
+	if (!ec_dev) {
+		/*
+		 * We continue even for older devices which don't have the
+		 * correct device heirarchy, namely, GOOG0003 is a child
+		 * of GOOG0004.
+		 */
+		dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");
+	}
+
+	pdnotify->dev = dev;
+	pdnotify->ec = ec_dev;
+
+	status = acpi_install_notify_handler(adev->handle,
+					     ACPI_ALL_NOTIFY,
+					     cros_usbpd_notify_acpi,
+					     pdnotify);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(dev, "Failed to register notify handler %08x\n",
+			 status);
+		return -EINVAL;
+	}
+
+	dev_info(dev, "Chrome EC PD notify device registered.\n");
+
 	return 0;
 }
 
-static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event)
+static int cros_usbpd_notify_remove_acpi(struct platform_device *pdev)
 {
-	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
+	struct device *dev = &pdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (!adev) {
+		dev_err(dev, "No ACPI device found.\n");
+		return -ENODEV;
+	}
+
+	acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
+				   cros_usbpd_notify_acpi);
+
+	return 0;
 }
 
 static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
@@ -70,14 +129,13 @@ static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids);
 
-static struct acpi_driver cros_usbpd_notify_acpi_driver = {
-	.name = DRV_NAME,
-	.class = DRV_NAME,
-	.ids = cros_usbpd_notify_acpi_device_ids,
-	.ops = {
-		.add = cros_usbpd_notify_add_acpi,
-		.notify = cros_usbpd_notify_acpi,
+static struct platform_driver cros_usbpd_notify_acpi_driver = {
+	.driver = {
+		.name = DRV_NAME_PLAT_ACPI,
+		.acpi_match_table = cros_usbpd_notify_acpi_device_ids,
 	},
+	.probe = cros_usbpd_notify_probe_acpi,
+	.remove = cros_usbpd_notify_remove_acpi,
 };
 
 #endif /* CONFIG_ACPI */
@@ -159,7 +217,7 @@ static int __init cros_usbpd_notify_init(void)
 		return ret;
 
 #ifdef CONFIG_ACPI
-	acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver);
+	platform_driver_register(&cros_usbpd_notify_acpi_driver);
 #endif
 	return 0;
 }
@@ -167,7 +225,7 @@ static int __init cros_usbpd_notify_init(void)
 static void __exit cros_usbpd_notify_exit(void)
 {
 #ifdef CONFIG_ACPI
-	acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver);
+	platform_driver_unregister(&cros_usbpd_notify_acpi_driver);
 #endif
 	platform_driver_unregister(&cros_usbpd_notify_plat_driver);
 }
-- 
2.25.1.696.g5e7596f4ac-goog

[PATCH 3/3] platform/chrome: notify: Pull PD_HOST_EVENT status

[Prashant Malani]

Read the PD host even status from the EC and send that to the notifier
listeners, for more fine-grained event information.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_usbpd_notify.c | 87 ++++++++++++++++++++-
 1 file changed, 84 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
index d2dbf7017e29c..3d9db4146217e 100644
--- a/drivers/platform/chrome/cros_usbpd_notify.c
+++ b/drivers/platform/chrome/cros_usbpd_notify.c
@@ -53,11 +53,91 @@ void cros_usbpd_unregister_notify(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify);
 
+/**
+ * cros_ec_pd_command - Send a command to the EC.
+ *
+ * @ec_dev: EC device
+ * @command: EC command
+ * @outdata: EC command output data
+ * @outsize: Size of outdata
+ * @indata: EC command input data
+ * @insize: Size of indata
+ *
+ * Return: 0 on success, < 0 on failure.
+ */
+static int cros_ec_pd_command(struct cros_ec_device *ec_dev,
+			      int command,
+			      uint8_t *outdata,
+			      int outsize,
+			      uint8_t *indata,
+			      int insize)
+{
+	int ret;
+	struct cros_ec_command *msg;
+
+	msg = kzalloc(sizeof(*msg) + max(insize, outsize), GFP_KERNEL);
+	if (!msg)
+		return -EC_RES_ERROR;
+
+	msg->command = command;
+	msg->outsize = outsize;
+	msg->insize = insize;
+
+	if (outsize)
+		memcpy(msg->data, outdata, outsize);
+
+	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+	if (ret < 0)
+		goto error;
+
+	if (insize)
+		memcpy(indata, msg->data, insize);
+	ret = EC_RES_SUCCESS;
+error:
+	kfree(msg);
+	return ret;
+}
+
+static void cros_usbpd_get_event_and_notify(struct device  *dev,
+					    struct cros_ec_device *ec_dev)
+{
+	struct ec_response_host_event_status host_event_status;
+	u32 event = 0;
+	int ret;
+
+	/*
+	 * We still send a 0 event out to older devices which don't
+	 * have the updated device heirarchy.
+	 */
+	if (!ec_dev) {
+		dev_dbg(dev,
+			"EC device inaccessible; sending 0 event status.\n");
+		goto send_notify;
+	}
+
+	/* Check for PD host events on EC. */
+	ret = cros_ec_pd_command(ec_dev, EC_CMD_PD_HOST_EVENT_STATUS,
+				 NULL, 0,
+				 (uint8_t *)&host_event_status,
+				 sizeof(host_event_status));
+	if (ret < 0) {
+		dev_warn(dev, "Can't get host event status (err: %d)\n", ret);
+		goto send_notify;
+	}
+
+	event = host_event_status.status;
+
+send_notify:
+	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
+}
+
 #ifdef CONFIG_ACPI
 
 static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data)
 {
-	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
+	struct cros_usbpd_notify_data *pdnotify = data;
+
+	cros_usbpd_get_event_and_notify(pdnotify->dev, pdnotify->ec);
 }
 
 static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev)
@@ -144,6 +224,8 @@ static int cros_usbpd_notify_plat(struct notifier_block *nb,
 				  unsigned long queued_during_suspend,
 				  void *data)
 {
+	struct cros_usbpd_notify_data *pdnotify = container_of(nb,
+			struct cros_usbpd_notify_data, nb);
 	struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
 	u32 host_event = cros_ec_get_host_event(ec_dev);
 
@@ -151,8 +233,7 @@ static int cros_usbpd_notify_plat(struct notifier_block *nb,
 		return NOTIFY_BAD;
 
 	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
-		blocking_notifier_call_chain(&cros_usbpd_notifier_list,
-					     host_event, NULL);
+		cros_usbpd_get_event_and_notify(pdnotify->dev, ec_dev);
 		return NOTIFY_OK;
 	}
 	return NOTIFY_DONE;
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [PATCH 1/3] platform/chrome: notify: Add driver data struct

[Enric Balletbo i Serra]

Hi Prashant,

On 12/3/20 11:08, Prashant Malani wrote:
> Introduce a device driver data structure, cros_usbpd_notify_data, in
> which we can store the notifier block object and pointers to the struct
> cros_ec_device and struct device objects.
> 
> This will make it more convenient to access these pointers when
> executing both platform and ACPI callbacks.
> 
> While we are here, also add a dev_info print declaring successful device
> registration at the end of the platform probe function.
> 

This info can be obtained by other means, i.e function tracing or
initcall_debug. There is no need to repeat the same explicitly in the driver.

> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_usbpd_notify.c | 30 ++++++++++++++-------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
> index 3851bbd6e9a39..edcb346024b07 100644
> --- a/drivers/platform/chrome/cros_usbpd_notify.c
> +++ b/drivers/platform/chrome/cros_usbpd_notify.c
> @@ -16,6 +16,12 @@
>  
>  static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list);
>  
> +struct cros_usbpd_notify_data {
> +	struct device *dev;
> +	struct cros_ec_device *ec;
> +	struct notifier_block nb;
> +};
> +
>  /**
>   * cros_usbpd_register_notify - Register a notifier callback for PD events.
>   * @nb: Notifier block pointer to register
> @@ -98,23 +104,28 @@ static int cros_usbpd_notify_probe_plat(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_dev *ecdev = dev_get_drvdata(dev->parent);
> -	struct notifier_block *nb;
> +	struct cros_usbpd_notify_data *pdnotify;
>  	int ret;
>  
> -	nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL);
> -	if (!nb)
> +	pdnotify = devm_kzalloc(dev, sizeof(*pdnotify), GFP_KERNEL);
> +	if (!pdnotify)
>  		return -ENOMEM;
>  
> -	nb->notifier_call = cros_usbpd_notify_plat;
> -	dev_set_drvdata(dev, nb);
> +	pdnotify->dev = dev;
> +	pdnotify->ec = ecdev->ec_dev;
> +	pdnotify->nb.notifier_call = cros_usbpd_notify_plat;
> +
> +	dev_set_drvdata(dev, pdnotify);
>  
>  	ret = blocking_notifier_chain_register(&ecdev->ec_dev->event_notifier,
> -					       nb);
> +					       &pdnotify->nb);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to register notifier\n");
>  		return ret;
>  	}
>  
> +	dev_info(dev, "Chrome EC PD notify device registered.\n");
> +

This is only noise to the kernel log, remove it.

>  	return 0;
>  }
>  
> @@ -122,10 +133,11 @@ static int cros_usbpd_notify_remove_plat(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_dev *ecdev = dev_get_drvdata(dev->parent);
> -	struct notifier_block *nb =
> -		(struct notifier_block *)dev_get_drvdata(dev);
> +	struct cros_usbpd_notify_data *pdnotify =
> +		(struct cros_usbpd_notify_data *)dev_get_drvdata(dev);
>  
> -	blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier, nb);
> +	blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier,
> +					   &pdnotify->nb);
>  
>  	return 0;
>  }
> 

Re: [PATCH 2/3] platform/chrome: notify: Amend ACPI driver to plat

[Enric Balletbo i Serra]

Hi Prashant,

On 12/3/20 11:08, Prashant Malani wrote:
> Convert the ACPI driver into the equivalent platform driver, with the
> same ACPI match table as before. This allows the device driver to access
> the parent platform EC device and its cros_ec_device struct, which will
> be required to communicate with the EC to pull PD Host event information
> from it.
> 
> Also change the ACPI driver name to "cros-usbpd-notify-acpi" so that
> there is no confusion between it and the "regular" platform driver on
> platforms that have both CONFIG_ACPI and CONFIG_OF enabled.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_usbpd_notify.c | 82 ++++++++++++++++++---
>  1 file changed, 70 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
> index edcb346024b07..d2dbf7017e29c 100644
> --- a/drivers/platform/chrome/cros_usbpd_notify.c
> +++ b/drivers/platform/chrome/cros_usbpd_notify.c
> @@ -12,6 +12,7 @@
>  #include <linux/platform_device.h>
>  
>  #define DRV_NAME "cros-usbpd-notify"
> +#define DRV_NAME_PLAT_ACPI "cros-usbpd-notify-acpi"
>  #define ACPI_DRV_NAME "GOOG0003"
>  
>  static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list);
> @@ -54,14 +55,72 @@ EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify);
>  
>  #ifdef CONFIG_ACPI
>  
> -static int cros_usbpd_notify_add_acpi(struct acpi_device *adev)
> +static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data)
>  {
> +	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> +}
> +
> +static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev)
> +{
> +	struct cros_usbpd_notify_data *pdnotify;
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev;
> +	struct cros_ec_device *ec_dev;
> +	acpi_status status;
> +
> +	adev = ACPI_COMPANION(dev);
> +	if (!adev) {

I still missing some bits of the ACPI devices but is this possible?

The ACPI probe only will be called if there is a match so an ACPI device, I guess.

> +		dev_err(dev, "No ACPI device found.\n");
> +		return -ENODEV;
> +	}
> +
> +	pdnotify = devm_kzalloc(dev, sizeof(*pdnotify), GFP_KERNEL);
> +	if (!pdnotify)
> +		return -ENOMEM;
> +
> +	/* Get the EC device pointer needed to talk to the EC. */
> +	ec_dev = dev_get_drvdata(dev->parent);
> +	if (!ec_dev) {
> +		/*
> +		 * We continue even for older devices which don't have the
> +		 * correct device heirarchy, namely, GOOG0003 is a child
> +		 * of GOOG0004.
> +		 */
> +		dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");

I'm not sure this is correctly handled, see below ...


> +	}
> +
> +	pdnotify->dev = dev;
> +	pdnotify->ec = ec_dev;

If !ec_dev you'll assign a NULL pointer to pdnotify->ec. On the cases that
GOOG0003 is not a child of GOOG0004 I suspect you will get a NULL dereference
later in some other part of the code?

> +
> +	status = acpi_install_notify_handler(adev->handle,
> +					     ACPI_ALL_NOTIFY,
> +					     cros_usbpd_notify_acpi,
> +					     pdnotify);
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(dev, "Failed to register notify handler %08x\n",
> +			 status);
> +		return -EINVAL;
> +	}
> +
> +	dev_info(dev, "Chrome EC PD notify device registered.\n");
> +

This is only noise to the kernel log, remove it.

>  	return 0;
>  }
>  
> -static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event)
> +static int cros_usbpd_notify_remove_acpi(struct platform_device *pdev)
>  {
> -	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> +	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +	if (!adev) {
> +		dev_err(dev, "No ACPI device found.\n");

Is this possible?

> +		return -ENODEV;
> +	}
> +
> +	acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
> +				   cros_usbpd_notify_acpi);
> +
> +	return 0;
>  }
>  
>  static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
> @@ -70,14 +129,13 @@ static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids);
>  
> -static struct acpi_driver cros_usbpd_notify_acpi_driver = {
> -	.name = DRV_NAME,
> -	.class = DRV_NAME,
> -	.ids = cros_usbpd_notify_acpi_device_ids,
> -	.ops = {
> -		.add = cros_usbpd_notify_add_acpi,
> -		.notify = cros_usbpd_notify_acpi,
> +static struct platform_driver cros_usbpd_notify_acpi_driver = {

Nice, so it is converted to a platform_driver, now. This makes me think again if
we could just use a single platform_driver and register the acpi notifier in the
ACPI match case and use the non-acpi notifier on the OF case.

> +	.driver = {
> +		.name = DRV_NAME_PLAT_ACPI,
> +		.acpi_match_table = cros_usbpd_notify_acpi_device_ids,
>  	},
> +	.probe = cros_usbpd_notify_probe_acpi,
> +	.remove = cros_usbpd_notify_remove_acpi,
>  };
>  
>  #endif /* CONFIG_ACPI */
> @@ -159,7 +217,7 @@ static int __init cros_usbpd_notify_init(void)
>  		return ret;
>  
>  #ifdef CONFIG_ACPI
> -	acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver);
> +	platform_driver_register(&cros_usbpd_notify_acpi_driver);
>  #endif
>  	return 0;
>  }
> @@ -167,7 +225,7 @@ static int __init cros_usbpd_notify_init(void)
>  static void __exit cros_usbpd_notify_exit(void)
>  {
>  #ifdef CONFIG_ACPI
> -	acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver);
> +	platform_driver_unregister(&cros_usbpd_notify_acpi_driver);
>  #endif
>  	platform_driver_unregister(&cros_usbpd_notify_plat_driver);
>  }
> 

Re: [PATCH 3/3] platform/chrome: notify: Pull PD_HOST_EVENT status

[Enric Balletbo i Serra]

Hi Prashant,

On 12/3/20 11:08, Prashant Malani wrote:
> Read the PD host even status from the EC and send that to the notifier
> listeners, for more fine-grained event information.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_usbpd_notify.c | 87 ++++++++++++++++++++-
>  1 file changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
> index d2dbf7017e29c..3d9db4146217e 100644
> --- a/drivers/platform/chrome/cros_usbpd_notify.c
> +++ b/drivers/platform/chrome/cros_usbpd_notify.c
> @@ -53,11 +53,91 @@ void cros_usbpd_unregister_notify(struct notifier_block *nb)
>  }
>  EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify);
>  
> +/**
> + * cros_ec_pd_command - Send a command to the EC.
> + *
> + * @ec_dev: EC device
> + * @command: EC command
> + * @outdata: EC command output data
> + * @outsize: Size of outdata
> + * @indata: EC command input data
> + * @insize: Size of indata
> + *
> + * Return: 0 on success, < 0 on failure.
> + */
> +static int cros_ec_pd_command(struct cros_ec_device *ec_dev,
> +			      int command,
> +			      uint8_t *outdata,
> +			      int outsize,
> +			      uint8_t *indata,
> +			      int insize)
> +{
> +	int ret;
> +	struct cros_ec_command *msg;

Reverse x-mas tree, please.

struct cros_ec_command *msg;
int ret;

> +
> +	msg = kzalloc(sizeof(*msg) + max(insize, outsize), GFP_KERNEL);
> +	if (!msg)
> +		return -EC_RES_ERROR;

Use standard linux error codes please, in that case -ENOMEM.

> +
> +	msg->command = command;
> +	msg->outsize = outsize;
> +	msg->insize = insize;
> +
> +	if (outsize)
> +		memcpy(msg->data, outdata, outsize);
> +
> +	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> +	if (ret < 0)
> +		goto error;
> +
> +	if (insize)
> +		memcpy(indata, msg->data, insize);
> +	ret = EC_RES_SUCCESS;

Standard linux error codes, just return what cros_ec_cmd_xfer_status returns.

> +error:
> +	kfree(msg);
> +	return ret;
> +}
> +
> +static void cros_usbpd_get_event_and_notify(struct device  *dev,
> +					    struct cros_ec_device *ec_dev)
> +{
> +	struct ec_response_host_event_status host_event_status;
> +	u32 event = 0;
> +	int ret;
> +
> +	/*
> +	 * We still send a 0 event out to older devices which don't
> +	 * have the updated device heirarchy.
> +	 */
> +	if (!ec_dev) {

Ok, remembering my comment in previous patch it makes sense to check for ec_dev,
but see below ...

> +		dev_dbg(dev,
> +			"EC device inaccessible; sending 0 event status.\n");
> +		goto send_notify;
> +	}
> +
> +	/* Check for PD host events on EC. */
> +	ret = cros_ec_pd_command(ec_dev, EC_CMD_PD_HOST_EVENT_STATUS,
> +				 NULL, 0,
> +				 (uint8_t *)&host_event_status,
> +				 sizeof(host_event_status));
> +	if (ret < 0) {
> +		dev_warn(dev, "Can't get host event status (err: %d)\n", ret);

This print is unneeded, a error will be printed already if it fails.

> +		goto send_notify;
> +	}
> +
> +	event = host_event_status.status;
> +
> +send_notify:
> +	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> +}
> +
>  #ifdef CONFIG_ACPI
>  
>  static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data)
>  {
> -	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> +	struct cros_usbpd_notify_data *pdnotify = data;
> +
> +	cros_usbpd_get_event_and_notify(pdnotify->dev, pdnotify->ec);
>  }
>  
>  static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev)
> @@ -144,6 +224,8 @@ static int cros_usbpd_notify_plat(struct notifier_block *nb,
>  				  unsigned long queued_during_suspend,
>  				  void *data)
>  {
> +	struct cros_usbpd_notify_data *pdnotify = container_of(nb,
> +			struct cros_usbpd_notify_data, nb);
>  	struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
>  	u32 host_event = cros_ec_get_host_event(ec_dev);
>  

Not related to this patch but as you introduced the possibility to have ec_dev
NULL, crash here.


> @@ -151,8 +233,7 @@ static int cros_usbpd_notify_plat(struct notifier_block *nb,
>  		return NOTIFY_BAD;
>  
>  	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
> -		blocking_notifier_call_chain(&cros_usbpd_notifier_list,
> -					     host_event, NULL);
> +		cros_usbpd_get_event_and_notify(pdnotify->dev, ec_dev);
>  		return NOTIFY_OK;
>  	}
>  	return NOTIFY_DONE;
> 

Re: [PATCH 2/3] platform/chrome: notify: Amend ACPI driver to plat

[Prashant Malani]

Hi Enric,

Thanks a lot for reviewing the patch, kindly see inline:

On Fri, Mar 13, 2020 at 01:42:26PM +0100, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> On 12/3/20 11:08, Prashant Malani wrote:
> > Convert the ACPI driver into the equivalent platform driver, with the
> > same ACPI match table as before. This allows the device driver to access
> > the parent platform EC device and its cros_ec_device struct, which will
> > be required to communicate with the EC to pull PD Host event information
> > from it.
> > 
> > Also change the ACPI driver name to "cros-usbpd-notify-acpi" so that
> > there is no confusion between it and the "regular" platform driver on
> > platforms that have both CONFIG_ACPI and CONFIG_OF enabled.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_usbpd_notify.c | 82 ++++++++++++++++++---
> >  1 file changed, 70 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
> > index edcb346024b07..d2dbf7017e29c 100644
> > --- a/drivers/platform/chrome/cros_usbpd_notify.c
> > +++ b/drivers/platform/chrome/cros_usbpd_notify.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/platform_device.h>
> >  
> >  #define DRV_NAME "cros-usbpd-notify"
> > +#define DRV_NAME_PLAT_ACPI "cros-usbpd-notify-acpi"
> >  #define ACPI_DRV_NAME "GOOG0003"
> >  
> >  static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list);
> > @@ -54,14 +55,72 @@ EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify);
> >  
> >  #ifdef CONFIG_ACPI
> >  
> > -static int cros_usbpd_notify_add_acpi(struct acpi_device *adev)
> > +static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data)
> >  {
> > +	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> > +}
> > +
> > +static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev)
> > +{
> > +	struct cros_usbpd_notify_data *pdnotify;
> > +	struct device *dev = &pdev->dev;
> > +	struct acpi_device *adev;
> > +	struct cros_ec_device *ec_dev;
> > +	acpi_status status;
> > +
> > +	adev = ACPI_COMPANION(dev);
> > +	if (!adev) {
> 
> I still missing some bits of the ACPI devices but is this possible?
> 
> The ACPI probe only will be called if there is a match so an ACPI device, I guess.
> 
Ack. Will remove this check. I was following cros_ec_lpc.c but that is a
common driver.

> > +		dev_err(dev, "No ACPI device found.\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	pdnotify = devm_kzalloc(dev, sizeof(*pdnotify), GFP_KERNEL);
> > +	if (!pdnotify)
> > +		return -ENOMEM;
> > +
> > +	/* Get the EC device pointer needed to talk to the EC. */
> > +	ec_dev = dev_get_drvdata(dev->parent);
> > +	if (!ec_dev) {
> > +		/*
> > +		 * We continue even for older devices which don't have the
> > +		 * correct device heirarchy, namely, GOOG0003 is a child
> > +		 * of GOOG0004.
> > +		 */
> > +		dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");
> 
> I'm not sure this is correctly handled, see below ...
> 
> 
> > +	}
> > +
> > +	pdnotify->dev = dev;
> > +	pdnotify->ec = ec_dev;
> 
> If !ec_dev you'll assign a NULL pointer to pdnotify->ec. On the cases that
> GOOG0003 is not a child of GOOG0004 I suspect you will get a NULL dereference
> later in some other part of the code?
> 

I think there is a comment about this in the Patch 3/3 review, so will
also address it there. Basically, cros_usbpd_notify_plat() will not have
a NULL ec_dev, because the platform_probe() only happens for a cros MFD,
which will be a child of the parent EC device always.

> > +
> > +	status = acpi_install_notify_handler(adev->handle,
> > +					     ACPI_ALL_NOTIFY,
> > +					     cros_usbpd_notify_acpi,
> > +					     pdnotify);
> > +	if (ACPI_FAILURE(status)) {
> > +		dev_warn(dev, "Failed to register notify handler %08x\n",
> > +			 status);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_info(dev, "Chrome EC PD notify device registered.\n");
> > +
> 
> This is only noise to the kernel log, remove it.

Done.
> 
> >  	return 0;
> >  }
> >  
> > -static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event)
> > +static int cros_usbpd_notify_remove_acpi(struct platform_device *pdev)
> >  {
> > -	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> > +	struct device *dev = &pdev->dev;
> > +	struct acpi_device *adev = ACPI_COMPANION(dev);
> > +
> > +	if (!adev) {
> > +		dev_err(dev, "No ACPI device found.\n");
> 
> Is this possible?
> 
Ack. For ACPI probe not possible. Will remove it.
> > +		return -ENODEV;
> > +	}
> > +
> > +	acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
> > +				   cros_usbpd_notify_acpi);
> > +
> > +	return 0;
> >  }
> >  
> >  static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
> > @@ -70,14 +129,13 @@ static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
> >  };
> >  MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids);
> >  
> > -static struct acpi_driver cros_usbpd_notify_acpi_driver = {
> > -	.name = DRV_NAME,
> > -	.class = DRV_NAME,
> > -	.ids = cros_usbpd_notify_acpi_device_ids,
> > -	.ops = {
> > -		.add = cros_usbpd_notify_add_acpi,
> > -		.notify = cros_usbpd_notify_acpi,
> > +static struct platform_driver cros_usbpd_notify_acpi_driver = {
> 
> Nice, so it is converted to a platform_driver, now. This makes me think again if
> we could just use a single platform_driver and register the acpi notifier in the
> ACPI match case and use the non-acpi notifier on the OF case.
> 
I'd like that as well. But, I'm hesitant to make the change now, since I
don't have a platform which has CONFIG_OF and CONFIG_ACPI on which to
test the common platform driver with (which is what you use IIRC).

Would something as follows work (pseudo code to follow):

static int cros_usbpd_notify_probe_plat(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	struct acpi_device *adev = ACPI_COMPANION(dev);

	/* "Non-ACPI case"
	if (dev->parent->of_node) {
		/* Do non-ACPI case probe work here */

	} else if (adev) {
		/* Do ACPI case probe work here */
	} else {
		return -EINVAL;
	}
}

and similarly for remove ?

If so, I can change Patch 2/2 to do this :)

Best regards,

-Prashant

> > +	.driver = {
> > +		.name = DRV_NAME_PLAT_ACPI,
> > +		.acpi_match_table = cros_usbpd_notify_acpi_device_ids,
> >  	},
> > +	.probe = cros_usbpd_notify_probe_acpi,
> > +	.remove = cros_usbpd_notify_remove_acpi,
> >  };
> >  
> >  #endif /* CONFIG_ACPI */
> > @@ -159,7 +217,7 @@ static int __init cros_usbpd_notify_init(void)
> >  		return ret;
> >  
> >  #ifdef CONFIG_ACPI
> > -	acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver);
> > +	platform_driver_register(&cros_usbpd_notify_acpi_driver);
> >  #endif
> >  	return 0;
> >  }
> > @@ -167,7 +225,7 @@ static int __init cros_usbpd_notify_init(void)
> >  static void __exit cros_usbpd_notify_exit(void)
> >  {
> >  #ifdef CONFIG_ACPI
> > -	acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver);
> > +	platform_driver_unregister(&cros_usbpd_notify_acpi_driver);
> >  #endif
> >  	platform_driver_unregister(&cros_usbpd_notify_plat_driver);
> >  }
> > 

Re: [PATCH 3/3] platform/chrome: notify: Pull PD_HOST_EVENT status

[Prashant Malani]

Hi Enric,

Thanks for the review as always. Kindly see inline:

On Fri, Mar 13, 2020 at 01:43:24PM +0100, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> On 12/3/20 11:08, Prashant Malani wrote:
> > Read the PD host even status from the EC and send that to the notifier
> > listeners, for more fine-grained event information.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_usbpd_notify.c | 87 ++++++++++++++++++++-
> >  1 file changed, 84 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
> > index d2dbf7017e29c..3d9db4146217e 100644
> > --- a/drivers/platform/chrome/cros_usbpd_notify.c
> > +++ b/drivers/platform/chrome/cros_usbpd_notify.c
> > @@ -53,11 +53,91 @@ void cros_usbpd_unregister_notify(struct notifier_block *nb)
> >  }
> >  EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify);
> >  
> > +/**
> > + * cros_ec_pd_command - Send a command to the EC.
> > + *
> > + * @ec_dev: EC device
> > + * @command: EC command
> > + * @outdata: EC command output data
> > + * @outsize: Size of outdata
> > + * @indata: EC command input data
> > + * @insize: Size of indata
> > + *
> > + * Return: 0 on success, < 0 on failure.
> > + */
> > +static int cros_ec_pd_command(struct cros_ec_device *ec_dev,
> > +			      int command,
> > +			      uint8_t *outdata,
> > +			      int outsize,
> > +			      uint8_t *indata,
> > +			      int insize)
> > +{
> > +	int ret;
> > +	struct cros_ec_command *msg;
> 
> Reverse x-mas tree, please.
> 
Done.
> struct cros_ec_command *msg;
> int ret;
> 
> > +
> > +	msg = kzalloc(sizeof(*msg) + max(insize, outsize), GFP_KERNEL);
> > +	if (!msg)
> > +		return -EC_RES_ERROR;
> 
> Use standard linux error codes please, in that case -ENOMEM.
> 
Done.
> > +
> > +	msg->command = command;
> > +	msg->outsize = outsize;
> > +	msg->insize = insize;
> > +
> > +	if (outsize)
> > +		memcpy(msg->data, outdata, outsize);
> > +
> > +	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	if (insize)
> > +		memcpy(indata, msg->data, insize);
> > +	ret = EC_RES_SUCCESS;
> 
> Standard linux error codes, just return what cros_ec_cmd_xfer_status returns.
Done.
> 
> > +error:
> > +	kfree(msg);
> > +	return ret;
> > +}
> > +
> > +static void cros_usbpd_get_event_and_notify(struct device  *dev,
> > +					    struct cros_ec_device *ec_dev)
> > +{
> > +	struct ec_response_host_event_status host_event_status;
> > +	u32 event = 0;
> > +	int ret;
> > +
> > +	/*
> > +	 * We still send a 0 event out to older devices which don't
> > +	 * have the updated device heirarchy.
> > +	 */
> > +	if (!ec_dev) {
> 
> Ok, remembering my comment in previous patch it makes sense to check for ec_dev,
> but see below ...
> 
> > +		dev_dbg(dev,
> > +			"EC device inaccessible; sending 0 event status.\n");
> > +		goto send_notify;
> > +	}
> > +
> > +	/* Check for PD host events on EC. */
> > +	ret = cros_ec_pd_command(ec_dev, EC_CMD_PD_HOST_EVENT_STATUS,
> > +				 NULL, 0,
> > +				 (uint8_t *)&host_event_status,
> > +				 sizeof(host_event_status));
> > +	if (ret < 0) {
> > +		dev_warn(dev, "Can't get host event status (err: %d)\n", ret);
> 
> This print is unneeded, a error will be printed already if it fails.
Done.
> 
> > +		goto send_notify;
> > +	}
> > +
> > +	event = host_event_status.status;
> > +
> > +send_notify:
> > +	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> > +}
> > +
> >  #ifdef CONFIG_ACPI
> >  
> >  static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data)
> >  {
> > -	blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> > +	struct cros_usbpd_notify_data *pdnotify = data;
> > +
> > +	cros_usbpd_get_event_and_notify(pdnotify->dev, pdnotify->ec);
> >  }
> >  
> >  static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev)
> > @@ -144,6 +224,8 @@ static int cros_usbpd_notify_plat(struct notifier_block *nb,
> >  				  unsigned long queued_during_suspend,
> >  				  void *data)
> >  {
> > +	struct cros_usbpd_notify_data *pdnotify = container_of(nb,
> > +			struct cros_usbpd_notify_data, nb);
> >  	struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
> >  	u32 host_event = cros_ec_get_host_event(ec_dev);
> >  
> 
> Not related to this patch but as you introduced the possibility to have ec_dev
> NULL, crash here.
notify_plat() would only be called for the cros-MFD initialization (i.e
the "platform" case) situation, so ec_dev can be guranteed to be present here.

> 
> 
> > @@ -151,8 +233,7 @@ static int cros_usbpd_notify_plat(struct notifier_block *nb,
> >  		return NOTIFY_BAD;
> >  
> >  	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
> > -		blocking_notifier_call_chain(&cros_usbpd_notifier_list,
> > -					     host_event, NULL);
> > +		cros_usbpd_get_event_and_notify(pdnotify->dev, ec_dev);
> >  		return NOTIFY_OK;
> >  	}
> >  	return NOTIFY_DONE;
> > 

Re: [PATCH 2/3] platform/chrome: notify: Amend ACPI driver to plat

[Prashant Malani]

Hi Enric,

On Sun, Mar 15, 2020 at 2:38 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Enric,
>
> Thanks a lot for reviewing the patch, kindly see inline:
>
> On Fri, Mar 13, 2020 at 01:42:26PM +0100, Enric Balletbo i Serra wrote:
> > Hi Prashant,
> >
> > On 12/3/20 11:08, Prashant Malani wrote:
> > > Convert the ACPI driver into the equivalent platform driver, with the
> > > same ACPI match table as before. This allows the device driver to access
> > > the parent platform EC device and its cros_ec_device struct, which will
> > > be required to communicate with the EC to pull PD Host event information
> > > from it.
> > >
> > > Also change the ACPI driver name to "cros-usbpd-notify-acpi" so that
> > > there is no confusion between it and the "regular" platform driver on
> > > platforms that have both CONFIG_ACPI and CONFIG_OF enabled.
> > >
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >  drivers/platform/chrome/cros_usbpd_notify.c | 82 ++++++++++++++++++---
> > >  1 file changed, 70 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
> > > index edcb346024b07..d2dbf7017e29c 100644
> > > --- a/drivers/platform/chrome/cros_usbpd_notify.c
> > > +++ b/drivers/platform/chrome/cros_usbpd_notify.c
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/platform_device.h>
> > >
> > >  #define DRV_NAME "cros-usbpd-notify"
> > > +#define DRV_NAME_PLAT_ACPI "cros-usbpd-notify-acpi"
> > >  #define ACPI_DRV_NAME "GOOG0003"
> > >
> > >  static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list);
> > > @@ -54,14 +55,72 @@ EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify);
> > >
> > >  #ifdef CONFIG_ACPI
> > >
> > > -static int cros_usbpd_notify_add_acpi(struct acpi_device *adev)
> > > +static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data)
> > >  {
> > > +   blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> > > +}
> > > +
> > > +static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev)
> > > +{
> > > +   struct cros_usbpd_notify_data *pdnotify;
> > > +   struct device *dev = &pdev->dev;
> > > +   struct acpi_device *adev;
> > > +   struct cros_ec_device *ec_dev;
> > > +   acpi_status status;
> > > +
> > > +   adev = ACPI_COMPANION(dev);
> > > +   if (!adev) {
> >
> > I still missing some bits of the ACPI devices but is this possible?
> >
> > The ACPI probe only will be called if there is a match so an ACPI device, I guess.
> >
> Ack. Will remove this check. I was following cros_ec_lpc.c but that is a
> common driver.
>
> > > +           dev_err(dev, "No ACPI device found.\n");
> > > +           return -ENODEV;
> > > +   }
> > > +
> > > +   pdnotify = devm_kzalloc(dev, sizeof(*pdnotify), GFP_KERNEL);
> > > +   if (!pdnotify)
> > > +           return -ENOMEM;
> > > +
> > > +   /* Get the EC device pointer needed to talk to the EC. */
> > > +   ec_dev = dev_get_drvdata(dev->parent);
> > > +   if (!ec_dev) {
> > > +           /*
> > > +            * We continue even for older devices which don't have the
> > > +            * correct device heirarchy, namely, GOOG0003 is a child
> > > +            * of GOOG0004.
> > > +            */
> > > +           dev_warn(dev, "Couldn't get Chrome EC device pointer.\n");
> >
> > I'm not sure this is correctly handled, see below ...
> >
> >
> > > +   }
> > > +
> > > +   pdnotify->dev = dev;
> > > +   pdnotify->ec = ec_dev;
> >
> > If !ec_dev you'll assign a NULL pointer to pdnotify->ec. On the cases that
> > GOOG0003 is not a child of GOOG0004 I suspect you will get a NULL dereference
> > later in some other part of the code?
> >
>
> I think there is a comment about this in the Patch 3/3 review, so will
> also address it there. Basically, cros_usbpd_notify_plat() will not have
> a NULL ec_dev, because the platform_probe() only happens for a cros MFD,
> which will be a child of the parent EC device always.
>
> > > +
> > > +   status = acpi_install_notify_handler(adev->handle,
> > > +                                        ACPI_ALL_NOTIFY,
> > > +                                        cros_usbpd_notify_acpi,
> > > +                                        pdnotify);
> > > +   if (ACPI_FAILURE(status)) {
> > > +           dev_warn(dev, "Failed to register notify handler %08x\n",
> > > +                    status);
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   dev_info(dev, "Chrome EC PD notify device registered.\n");
> > > +
> >
> > This is only noise to the kernel log, remove it.
>
> Done.
> >
> > >     return 0;
> > >  }
> > >
> > > -static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event)
> > > +static int cros_usbpd_notify_remove_acpi(struct platform_device *pdev)
> > >  {
> > > -   blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL);
> > > +   struct device *dev = &pdev->dev;
> > > +   struct acpi_device *adev = ACPI_COMPANION(dev);
> > > +
> > > +   if (!adev) {
> > > +           dev_err(dev, "No ACPI device found.\n");
> >
> > Is this possible?
> >
> Ack. For ACPI probe not possible. Will remove it.
> > > +           return -ENODEV;
> > > +   }
> > > +
> > > +   acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
> > > +                              cros_usbpd_notify_acpi);
> > > +
> > > +   return 0;
> > >  }
> > >
> > >  static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
> > > @@ -70,14 +129,13 @@ static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids);
> > >
> > > -static struct acpi_driver cros_usbpd_notify_acpi_driver = {
> > > -   .name = DRV_NAME,
> > > -   .class = DRV_NAME,
> > > -   .ids = cros_usbpd_notify_acpi_device_ids,
> > > -   .ops = {
> > > -           .add = cros_usbpd_notify_add_acpi,
> > > -           .notify = cros_usbpd_notify_acpi,
> > > +static struct platform_driver cros_usbpd_notify_acpi_driver = {
> >
> > Nice, so it is converted to a platform_driver, now. This makes me think again if
> > we could just use a single platform_driver and register the acpi notifier in the
> > ACPI match case and use the non-acpi notifier on the OF case.
> >
> I'd like that as well. But, I'm hesitant to make the change now, since I
> don't have a platform which has CONFIG_OF and CONFIG_ACPI on which to
> test the common platform driver with (which is what you use IIRC).
>
> Would something as follows work (pseudo code to follow):
>
> static int cros_usbpd_notify_probe_plat(struct platform_device *pdev)
> {
>         struct device *dev = &pdev->dev;
>         struct acpi_device *adev = ACPI_COMPANION(dev);
>
>         /* "Non-ACPI case"
>         if (dev->parent->of_node) {
>                 /* Do non-ACPI case probe work here */
>
>         } else if (adev) {
>                 /* Do ACPI case probe work here */
>         } else {
>                 return -EINVAL;
>         }
> }
>
> and similarly for remove ?
>
> If so, I can change Patch 2/2 to do this :)
Actually, I tried the above out and this causes a compilation failure
for the ARM case (i.e CONFIG_OF && !CONFIG_ACPI), because struct
acpi_device isn't defined for that case. It looks like we might have
to stick with two separate drivers, even if both are platform. Open to
suggestions though.

>
> Best regards,
>
> -Prashant
>
> > > +   .driver = {
> > > +           .name = DRV_NAME_PLAT_ACPI,
> > > +           .acpi_match_table = cros_usbpd_notify_acpi_device_ids,
> > >     },
> > > +   .probe = cros_usbpd_notify_probe_acpi,
> > > +   .remove = cros_usbpd_notify_remove_acpi,
> > >  };
> > >
> > >  #endif /* CONFIG_ACPI */
> > > @@ -159,7 +217,7 @@ static int __init cros_usbpd_notify_init(void)
> > >             return ret;
> > >
> > >  #ifdef CONFIG_ACPI
> > > -   acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver);
> > > +   platform_driver_register(&cros_usbpd_notify_acpi_driver);
> > >  #endif
> > >     return 0;
> > >  }
> > > @@ -167,7 +225,7 @@ static int __init cros_usbpd_notify_init(void)
> > >  static void __exit cros_usbpd_notify_exit(void)
> > >  {
> > >  #ifdef CONFIG_ACPI
> > > -   acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver);
> > > +   platform_driver_unregister(&cros_usbpd_notify_acpi_driver);
> > >  #endif
> > >     platform_driver_unregister(&cros_usbpd_notify_plat_driver);
> > >  }
> > >