[PATCH 0/4] Convert cros-ec-i2c-tunnel to MFD Cell

[PATCH 0/4] Convert cros-ec-i2c-tunnel to MFD Cell

[Raul E Rangel]

Having the cros-ec-i2c-tunnel behave as a MFD Cell fixes a race condition
that could happen if the cros-ec-i2c-tunnel gets probed before the
cros_ec_lpc init has finished.

There were also some bugs in the original cros-ec-i2c-tunnel ACPI patch.

Raul E Rangel (4):
  i2c: i2c-cros-ec-tunnel: Pass ACPI node to i2c adapter
  i2c: i2c-cros-ec-tunnel: Fix ACPI identifier
  platform/chrome: cros_ec: Pass firmware node to MFD device
  platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD
    Cell

 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 37 ++++++++++---------------
 drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++
 drivers/platform/chrome/cros_ec.c       | 13 +++++++--
 3 files changed, 43 insertions(+), 26 deletions(-)

-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 1/4] i2c: i2c-cros-ec-tunnel: Pass ACPI node to i2c adapter

[Raul E Rangel]

The I2C bus needs to share the same ACPI node as the tunnel device so
that the I2C bus can be referenced from ACPI.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
I would have added a Fixes annotation, but I wasn't able to find the
hash for https://lore.kernel.org/patchwork/patch/1151436/.

 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 958161c71985..ac2412755f0a 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -272,6 +272,7 @@ static int ec_i2c_probe(struct platform_device *pdev)
 	bus->adap.algo_data = bus;
 	bus->adap.dev.parent = &pdev->dev;
 	bus->adap.dev.of_node = pdev->dev.of_node;
+	ACPI_COMPANION_SET(&bus->adap.dev, ACPI_COMPANION(&pdev->dev));
 	bus->adap.retries = I2C_MAX_RETRIES;
 
 	err = i2c_add_adapter(&bus->adap);
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 2/4] i2c: i2c-cros-ec-tunnel: Fix ACPI identifier

[Raul E Rangel]

The initial patch was using the incorrect identifier.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
There are currently no boards using the identifier so it's fine to
change it.

 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index ac2412755f0a..5d91e33eb600 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -299,7 +299,7 @@ static const struct of_device_id cros_ec_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
 
 static const struct acpi_device_id cros_ec_i2c_tunnel_acpi_id[] = {
-	{ "GOOG001A", 0 },
+	{ "GOOG0012", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_tunnel_acpi_id);
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 3/4] platform/chrome: cros_ec: Pass firmware node to MFD device

[Raul E Rangel]

cros_ec_dev needs to have a firmware node associated with it so mfd
cells can be properly bound to the correct ACPI/DT nodes.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

Before this patch:
$ find /sys/bus/platform/devices/cros-ec-* -iname firmware_node -exec ls -l '{}' \;
<nothing>

After this patch:
$ find /sys/bus/platform/devices/cros-ec-dev.0.auto/ -iname firmware_node -exec ls -l '{}' \;
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-accel.1.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-chardev.7.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-debugfs.8.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-gyro.2.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-lid-angle.4.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-lightbar.9.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-pd-sysfs.10.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-ring.3.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-sysfs.11.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-usbpd-charger.5.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-usbpd-logger.6.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00

 drivers/platform/chrome/cros_ec.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 51d76037f52a..3c08c9098d29 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -167,9 +167,16 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 	}
 
 	/* Register a platform device for the main EC instance */
-	ec_dev->ec = platform_device_register_data(ec_dev->dev, "cros-ec-dev",
-					PLATFORM_DEVID_AUTO, &ec_p,
-					sizeof(struct cros_ec_platform));
+	ec_dev->ec =
+		platform_device_register_full(&(struct platform_device_info){
+			.parent = ec_dev->dev,
+			.name = "cros-ec-dev",
+			.id = PLATFORM_DEVID_AUTO,
+			.data = &ec_p,
+			.size_data = sizeof(struct cros_ec_platform),
+			.fwnode = ec_dev->dev->fwnode,
+			.of_node_reused = 1});
+
 	if (IS_ERR(ec_dev->ec)) {
 		dev_err(ec_dev->dev,
 			"Failed to create CrOS EC platform device\n");
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Raul E Rangel]

If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
has finished initializing and setting the drvdata. This would cause a
NULL pointer panic.

Converting this driver over to an MFD solves the problem and aligns with
where the cros_ec is going.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
You can now see the device node lives under the mfd device.

$ find /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/ -iname firmware_node -exec ls -l '{}' \;
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/firmware_node -> ../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
/sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/i2c-10EC5682:00/firmware_node -> ../../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00/10EC5682:00

 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 36 +++++++++----------------
 drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++
 2 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 5d91e33eb600..2e3217678fa3 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -6,6 +6,7 @@
 #include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
+#include <linux/mfd/cros_ec.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
 #include <linux/platform_device.h>
@@ -27,7 +28,6 @@
 struct ec_i2c_device {
 	struct device *dev;
 	struct i2c_adapter adap;
-	struct cros_ec_device *ec;
 
 	u16 remote_bus;
 
@@ -176,6 +176,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
 {
 	struct ec_i2c_device *bus = adap->algo_data;
 	struct device *dev = bus->dev;
+	struct cros_ec_dev *ec = dev_get_drvdata(dev->parent);
 	const u16 bus_num = bus->remote_bus;
 	int request_len;
 	int response_len;
@@ -183,6 +184,16 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
 	int result;
 	struct cros_ec_command *msg;
 
+	if (!ec) {
+		dev_err(dev, "%s: ec is missing!\n", __func__);
+		return -EINVAL;
+	}
+
+	if (!ec->ec_dev) {
+		dev_err(dev, "%s: ec->ec_dev is missing!\n", __func__);
+		return -EINVAL;
+	}
+
 	request_len = ec_i2c_count_message(i2c_msgs, num);
 	if (request_len < 0) {
 		dev_warn(dev, "Error constructing message %d\n", request_len);
@@ -212,7 +223,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
 	msg->outsize = request_len;
 	msg->insize = response_len;
 
-	result = cros_ec_cmd_xfer_status(bus->ec, msg);
+	result = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (result < 0) {
 		dev_err(dev, "Error transferring EC i2c message %d\n", result);
 		goto exit;
@@ -241,17 +252,11 @@ static const struct i2c_algorithm ec_i2c_algorithm = {
 
 static int ec_i2c_probe(struct platform_device *pdev)
 {
-	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
 	struct device *dev = &pdev->dev;
 	struct ec_i2c_device *bus = NULL;
 	u32 remote_bus;
 	int err;
 
-	if (!ec->cmd_xfer) {
-		dev_err(dev, "Missing sendrecv\n");
-		return -EINVAL;
-	}
-
 	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
 	if (bus == NULL)
 		return -ENOMEM;
@@ -263,7 +268,6 @@ static int ec_i2c_probe(struct platform_device *pdev)
 	}
 	bus->remote_bus = remote_bus;
 
-	bus->ec = ec;
 	bus->dev = dev;
 
 	bus->adap.owner = THIS_MODULE;
@@ -292,25 +296,11 @@ static int ec_i2c_remove(struct platform_device *dev)
 	return 0;
 }
 
-static const struct of_device_id cros_ec_i2c_of_match[] = {
-	{ .compatible = "google,cros-ec-i2c-tunnel" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
-
-static const struct acpi_device_id cros_ec_i2c_tunnel_acpi_id[] = {
-	{ "GOOG0012", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_tunnel_acpi_id);
-
 static struct platform_driver ec_i2c_tunnel_driver = {
 	.probe = ec_i2c_probe,
 	.remove = ec_i2c_remove,
 	.driver = {
 		.name = "cros-ec-i2c-tunnel",
-		.acpi_match_table = ACPI_PTR(cros_ec_i2c_tunnel_acpi_id),
-		.of_match_table = of_match_ptr(cros_ec_i2c_of_match),
 	},
 };
 
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 1efdba18f20b..61b20e061f75 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -113,6 +113,18 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
 	{ .name = "cros-ec-vbc", }
 };
 
+static struct mfd_cell_acpi_match cros_ec_i2c_tunnel_acpi_match = {
+	.pnpid = "GOOG0012"
+};
+
+static struct mfd_cell cros_ec_fw_cells[] = {
+	{
+		.name = "cros-ec-i2c-tunnel",
+		.acpi_match = &cros_ec_i2c_tunnel_acpi_match,
+		.of_compatible = "google,cros-ec-i2c-tunnel"
+	},
+};
+
 int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
 {
 	struct cros_ec_command *msg;
@@ -485,6 +497,13 @@ static int ec_device_probe(struct platform_device *pdev)
 			 "failed to add cros-ec platform devices: %d\n",
 			 retval);
 
+	retval = mfd_add_hotplug_devices(ec->dev, cros_ec_fw_cells,
+					 ARRAY_SIZE(cros_ec_fw_cells));
+	if (retval)
+		dev_warn(ec->dev,
+			 "failed to add cros-ec fw platform devices: %d\n",
+			 retval);
+
 	/* Check whether this EC instance has a VBC NVRAM */
 	node = ec->ec_dev->dev->of_node;
 	if (of_property_read_bool(node, "google,has-vbc-nvram")) {
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Guenter Roeck]

On Thu, Nov 21, 2019 at 1:11 PM Raul E Rangel <rrangel@chromium.org> wrote:
>
> If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
> possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
> has finished initializing and setting the drvdata. This would cause a
> NULL pointer panic.
>
> Converting this driver over to an MFD solves the problem and aligns with
> where the cros_ec is going.
>

I thought the mfd maintainer objects to the use of the mfd API outside
drivers/mfd. Did that change recently ?

Guenter

> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> You can now see the device node lives under the mfd device.
>
> $ find /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/ -iname firmware_node -exec ls -l '{}' \;
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/firmware_node -> ../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/i2c-10EC5682:00/firmware_node -> ../../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00/10EC5682:00
>
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 36 +++++++++----------------
>  drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++
>  2 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index 5d91e33eb600..2e3217678fa3 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -6,6 +6,7 @@
>  #include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/i2c.h>
> +#include <linux/mfd/cros_ec.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_ec_proto.h>
>  #include <linux/platform_device.h>
> @@ -27,7 +28,6 @@
>  struct ec_i2c_device {
>         struct device *dev;
>         struct i2c_adapter adap;
> -       struct cros_ec_device *ec;
>
>         u16 remote_bus;
>
> @@ -176,6 +176,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
>  {
>         struct ec_i2c_device *bus = adap->algo_data;
>         struct device *dev = bus->dev;
> +       struct cros_ec_dev *ec = dev_get_drvdata(dev->parent);
>         const u16 bus_num = bus->remote_bus;
>         int request_len;
>         int response_len;
> @@ -183,6 +184,16 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
>         int result;
>         struct cros_ec_command *msg;
>
> +       if (!ec) {
> +               dev_err(dev, "%s: ec is missing!\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       if (!ec->ec_dev) {
> +               dev_err(dev, "%s: ec->ec_dev is missing!\n", __func__);
> +               return -EINVAL;
> +       }
> +
>         request_len = ec_i2c_count_message(i2c_msgs, num);
>         if (request_len < 0) {
>                 dev_warn(dev, "Error constructing message %d\n", request_len);
> @@ -212,7 +223,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
>         msg->outsize = request_len;
>         msg->insize = response_len;
>
> -       result = cros_ec_cmd_xfer_status(bus->ec, msg);
> +       result = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>         if (result < 0) {
>                 dev_err(dev, "Error transferring EC i2c message %d\n", result);
>                 goto exit;
> @@ -241,17 +252,11 @@ static const struct i2c_algorithm ec_i2c_algorithm = {
>
>  static int ec_i2c_probe(struct platform_device *pdev)
>  {
> -       struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
>         struct device *dev = &pdev->dev;
>         struct ec_i2c_device *bus = NULL;
>         u32 remote_bus;
>         int err;
>
> -       if (!ec->cmd_xfer) {
> -               dev_err(dev, "Missing sendrecv\n");
> -               return -EINVAL;
> -       }
> -
>         bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
>         if (bus == NULL)
>                 return -ENOMEM;
> @@ -263,7 +268,6 @@ static int ec_i2c_probe(struct platform_device *pdev)
>         }
>         bus->remote_bus = remote_bus;
>
> -       bus->ec = ec;
>         bus->dev = dev;
>
>         bus->adap.owner = THIS_MODULE;
> @@ -292,25 +296,11 @@ static int ec_i2c_remove(struct platform_device *dev)
>         return 0;
>  }
>
> -static const struct of_device_id cros_ec_i2c_of_match[] = {
> -       { .compatible = "google,cros-ec-i2c-tunnel" },
> -       {},
> -};
> -MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
> -
> -static const struct acpi_device_id cros_ec_i2c_tunnel_acpi_id[] = {
> -       { "GOOG0012", 0 },
> -       { }
> -};
> -MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_tunnel_acpi_id);
> -
>  static struct platform_driver ec_i2c_tunnel_driver = {
>         .probe = ec_i2c_probe,
>         .remove = ec_i2c_remove,
>         .driver = {
>                 .name = "cros-ec-i2c-tunnel",
> -               .acpi_match_table = ACPI_PTR(cros_ec_i2c_tunnel_acpi_id),
> -               .of_match_table = of_match_ptr(cros_ec_i2c_of_match),
>         },
>  };
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 1efdba18f20b..61b20e061f75 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -113,6 +113,18 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
>         { .name = "cros-ec-vbc", }
>  };
>
> +static struct mfd_cell_acpi_match cros_ec_i2c_tunnel_acpi_match = {
> +       .pnpid = "GOOG0012"
> +};
> +
> +static struct mfd_cell cros_ec_fw_cells[] = {
> +       {
> +               .name = "cros-ec-i2c-tunnel",
> +               .acpi_match = &cros_ec_i2c_tunnel_acpi_match,
> +               .of_compatible = "google,cros-ec-i2c-tunnel"
> +       },
> +};
> +
>  int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>  {
>         struct cros_ec_command *msg;
> @@ -485,6 +497,13 @@ static int ec_device_probe(struct platform_device *pdev)
>                          "failed to add cros-ec platform devices: %d\n",
>                          retval);
>
> +       retval = mfd_add_hotplug_devices(ec->dev, cros_ec_fw_cells,
> +                                        ARRAY_SIZE(cros_ec_fw_cells));
> +       if (retval)
> +               dev_warn(ec->dev,
> +                        "failed to add cros-ec fw platform devices: %d\n",
> +                        retval);
> +
>         /* Check whether this EC instance has a VBC NVRAM */
>         node = ec->ec_dev->dev->of_node;
>         if (of_property_read_bool(node, "google,has-vbc-nvram")) {
> --
> 2.24.0.432.g9d3f5f5b63-goog
>

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Raul Rangel]

On Thu, Nov 21, 2019 at 2:40 PM Guenter Roeck <groeck@google.com> wrote:
>
> On Thu, Nov 21, 2019 at 1:11 PM Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
> > possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
> > has finished initializing and setting the drvdata. This would cause a
> > NULL pointer panic.
> >
> > Converting this driver over to an MFD solves the problem and aligns with
> > where the cros_ec is going.
> >
>
> I thought the mfd maintainer objects to the use of the mfd API outside
> drivers/mfd. Did that change recently ?

The MFD apis are only used in drivers/mfd/cros_ec_dev.c.
drivers/i2c/busses/i2c-cros-ec-tunnel.c just registers as a simple
driver.

Raul

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Lee Jones]

On Thu, 21 Nov 2019, Guenter Roeck wrote:

> On Thu, Nov 21, 2019 at 1:11 PM Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
> > possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
> > has finished initializing and setting the drvdata. This would cause a
> > NULL pointer panic.
> >
> > Converting this driver over to an MFD solves the problem and aligns with
> > where the cros_ec is going.
> >
> 
> I thought the mfd maintainer objects to the use of the mfd API outside
> drivers/mfd. Did that change recently ?

It hasn't changed, although I don't see that here?

> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> > You can now see the device node lives under the mfd device.
> >
> > $ find /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/ -iname firmware_node -exec ls -l '{}' \;
> > /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> > /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/firmware_node -> ../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> > /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/i2c-10EC5682:00/firmware_node -> ../../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00/10EC5682:00
> >
> >  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 36 +++++++++----------------
> >  drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++
> >  2 files changed, 32 insertions(+), 23 deletions(-)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Guenter Roeck]

On Fri, Nov 22, 2019 at 12:03 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 21 Nov 2019, Guenter Roeck wrote:
>
> > On Thu, Nov 21, 2019 at 1:11 PM Raul E Rangel <rrangel@chromium.org> wrote:
> > >
> > > If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
> > > possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
> > > has finished initializing and setting the drvdata. This would cause a
> > > NULL pointer panic.
> > >
> > > Converting this driver over to an MFD solves the problem and aligns with
> > > where the cros_ec is going.
> > >
> >
> > I thought the mfd maintainer objects to the use of the mfd API outside
> > drivers/mfd. Did that change recently ?
>
> It hasn't changed, although I don't see that here?
>
Sorry, I was confused.

Guenter

> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > ---
> > > You can now see the device node lives under the mfd device.
> > >
> > > $ find /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/ -iname firmware_node -exec ls -l '{}' \;
> > > /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> > > /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/firmware_node -> ../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> > > /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/i2c-10EC5682:00/firmware_node -> ../../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00/10EC5682:00
> > >
> > >  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 36 +++++++++----------------
> > >  drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++
> > >  2 files changed, 32 insertions(+), 23 deletions(-)
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] i2c: i2c-cros-ec-tunnel: Pass ACPI node to i2c adapter

[Enric Balletbo i Serra]

Hi,

On 21/11/19 22:10, Raul E Rangel wrote:
> The I2C bus needs to share the same ACPI node as the tunnel device so
> that the I2C bus can be referenced from ACPI.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

Hmm, isn't this change the same as https://lkml.org/lkml/2019/11/21/208 ?

Should I assume this deprecates the one pointed above? Who is the author? Please
maintain the Signed-off chain.

Thanks,
 Enric

> ---
> I would have added a Fixes annotation, but I wasn't able to find the
> hash for https://lore.kernel.org/patchwork/patch/1151436/.
> 
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index 958161c71985..ac2412755f0a 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -272,6 +272,7 @@ static int ec_i2c_probe(struct platform_device *pdev)
>  	bus->adap.algo_data = bus;
>  	bus->adap.dev.parent = &pdev->dev;
>  	bus->adap.dev.of_node = pdev->dev.of_node;
> +	ACPI_COMPANION_SET(&bus->adap.dev, ACPI_COMPANION(&pdev->dev));
>  	bus->adap.retries = I2C_MAX_RETRIES;
>  
>  	err = i2c_add_adapter(&bus->adap);
> 

Re: [PATCH 2/4] i2c: i2c-cros-ec-tunnel: Fix ACPI identifier

[Enric Balletbo i Serra]

Hi Raul,

On 21/11/19 22:10, Raul E Rangel wrote:
> The initial patch was using the incorrect identifier.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> There are currently no boards using the identifier so it's fine to
> change it.
> 

Right, I don't see GOOG001A being used but also I don't see GOOG0012. Anyway,
for the CrOS EC side, the change looks good to me. I think this will go through
the I2C tree?

Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks,
 Enric

>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index ac2412755f0a..5d91e33eb600 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -299,7 +299,7 @@ static const struct of_device_id cros_ec_i2c_of_match[] = {
>  MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
>  
>  static const struct acpi_device_id cros_ec_i2c_tunnel_acpi_id[] = {
> -	{ "GOOG001A", 0 },
> +	{ "GOOG0012", 0 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_tunnel_acpi_id);
> 

Re: [PATCH 3/4] platform/chrome: cros_ec: Pass firmware node to MFD device

[Enric Balletbo i Serra]

Hi Raul,

One comment below.

On 21/11/19 22:10, Raul E Rangel wrote:
> cros_ec_dev needs to have a firmware node associated with it so mfd
> cells can be properly bound to the correct ACPI/DT nodes.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> 
> Before this patch:
> $ find /sys/bus/platform/devices/cros-ec-* -iname firmware_node -exec ls -l '{}' \;
> <nothing>
> 
> After this patch:
> $ find /sys/bus/platform/devices/cros-ec-dev.0.auto/ -iname firmware_node -exec ls -l '{}' \;
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-accel.1.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-chardev.7.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-debugfs.8.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-gyro.2.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-lid-angle.4.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-lightbar.9.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-pd-sysfs.10.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-ring.3.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-sysfs.11.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-usbpd-charger.5.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-usbpd-logger.6.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00
> 
>  drivers/platform/chrome/cros_ec.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 51d76037f52a..3c08c9098d29 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -167,9 +167,16 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  	}
>  
>  	/* Register a platform device for the main EC instance */
> -	ec_dev->ec = platform_device_register_data(ec_dev->dev, "cros-ec-dev",
> -					PLATFORM_DEVID_AUTO, &ec_p,
> -					sizeof(struct cros_ec_platform));
> +	ec_dev->ec =
> +		platform_device_register_full(&(struct platform_device_info){
> +			.parent = ec_dev->dev,
> +			.name = "cros-ec-dev",
> +			.id = PLATFORM_DEVID_AUTO,
> +			.data = &ec_p,
> +			.size_data = sizeof(struct cros_ec_platform),
> +			.fwnode = ec_dev->dev->fwnode,
> +			.of_node_reused = 1});

Please create a local `struct platform_device_info`, fill and pass the &pdevinfo
in platform_device_register_full, I think is more clear.

Thanks,
 Enric

> +
>  	if (IS_ERR(ec_dev->ec)) {
>  		dev_err(ec_dev->dev,
>  			"Failed to create CrOS EC platform device\n");
> 

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Enric Balletbo i Serra]

Hi Raul,

On 21/11/19 22:10, Raul E Rangel wrote:
> If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
> possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
> has finished initializing and setting the drvdata. This would cause a
> NULL pointer panic.
> 
> Converting this driver over to an MFD solves the problem and aligns with
> where the cros_ec is going.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> You can now see the device node lives under the mfd device.
> 
> $ find /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/ -iname firmware_node -exec ls -l '{}' \;
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/firmware_node -> ../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/i2c-10EC5682:00/firmware_node -> ../../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00/10EC5682:00
> 
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 36 +++++++++----------------
>  drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++
>  2 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index 5d91e33eb600..2e3217678fa3 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -6,6 +6,7 @@
>  #include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/i2c.h>
> +#include <linux/mfd/cros_ec.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_ec_proto.h>
>  #include <linux/platform_device.h>
> @@ -27,7 +28,6 @@
>  struct ec_i2c_device {
>  	struct device *dev;
>  	struct i2c_adapter adap;
> -	struct cros_ec_device *ec;
>  
>  	u16 remote_bus;
>  
> @@ -176,6 +176,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
>  {
>  	struct ec_i2c_device *bus = adap->algo_data;
>  	struct device *dev = bus->dev;
> +	struct cros_ec_dev *ec = dev_get_drvdata(dev->parent);
>  	const u16 bus_num = bus->remote_bus;
>  	int request_len;
>  	int response_len;
> @@ -183,6 +184,16 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
>  	int result;
>  	struct cros_ec_command *msg;
>  
> +	if (!ec) {
> +		dev_err(dev, "%s: ec is missing!\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!ec->ec_dev) {
> +		dev_err(dev, "%s: ec->ec_dev is missing!\n", __func__);
> +		return -EINVAL;
> +	}
> +

Are those checks needed? Is that possible?

>  	request_len = ec_i2c_count_message(i2c_msgs, num);
>  	if (request_len < 0) {
>  		dev_warn(dev, "Error constructing message %d\n", request_len);
> @@ -212,7 +223,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
>  	msg->outsize = request_len;
>  	msg->insize = response_len;
>  
> -	result = cros_ec_cmd_xfer_status(bus->ec, msg);
> +	result = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>  	if (result < 0) {
>  		dev_err(dev, "Error transferring EC i2c message %d\n", result);
>  		goto exit;
> @@ -241,17 +252,11 @@ static const struct i2c_algorithm ec_i2c_algorithm = {
>  
>  static int ec_i2c_probe(struct platform_device *pdev)
>  {
> -	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
>  	struct device *dev = &pdev->dev;
>  	struct ec_i2c_device *bus = NULL;
>  	u32 remote_bus;
>  	int err;
>  
> -	if (!ec->cmd_xfer) {
> -		dev_err(dev, "Missing sendrecv\n");
> -		return -EINVAL;
> -	}
> -
>  	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
>  	if (bus == NULL)
>  		return -ENOMEM;
> @@ -263,7 +268,6 @@ static int ec_i2c_probe(struct platform_device *pdev)
>  	}
>  	bus->remote_bus = remote_bus;
>  
> -	bus->ec = ec;
>  	bus->dev = dev;
>  
>  	bus->adap.owner = THIS_MODULE;
> @@ -292,25 +296,11 @@ static int ec_i2c_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> -static const struct of_device_id cros_ec_i2c_of_match[] = {
> -	{ .compatible = "google,cros-ec-i2c-tunnel" },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
> -
> -static const struct acpi_device_id cros_ec_i2c_tunnel_acpi_id[] = {
> -	{ "GOOG0012", 0 },

So, you're removing something that you just added in a previous patch. So is
really the change in the previous patch needed?


> -	{ }
> -};
> -MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_tunnel_acpi_id);
> -
>  static struct platform_driver ec_i2c_tunnel_driver = {
>  	.probe = ec_i2c_probe,
>  	.remove = ec_i2c_remove,
>  	.driver = {
>  		.name = "cros-ec-i2c-tunnel",
> -		.acpi_match_table = ACPI_PTR(cros_ec_i2c_tunnel_acpi_id),
> -		.of_match_table = of_match_ptr(cros_ec_i2c_of_match),

I don't understand this change, why? The id should be in the driver to match.

>  	},
>  };
>  
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 1efdba18f20b..61b20e061f75 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -113,6 +113,18 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
>  	{ .name = "cros-ec-vbc", }
>  };
>  
> +static struct mfd_cell_acpi_match cros_ec_i2c_tunnel_acpi_match = {
> +	.pnpid = "GOOG0012"
> +};
> +
> +static struct mfd_cell cros_ec_fw_cells[] = {
> +	{
> +		.name = "cros-ec-i2c-tunnel",
> +		.acpi_match = &cros_ec_i2c_tunnel_acpi_match,
> +		.of_compatible = "google,cros-ec-i2c-tunnel"

Ah, I see. The acpi_match and the of_compatible should be in the
i2c-cros-ec-tunnel driver not here. Why you changed? Didn't work?

> +	},
> +};
> +
>  int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>  {
>  	struct cros_ec_command *msg;
> @@ -485,6 +497,13 @@ static int ec_device_probe(struct platform_device *pdev)
>  			 "failed to add cros-ec platform devices: %d\n",
>  			 retval);
>  
> +	retval = mfd_add_hotplug_devices(ec->dev, cros_ec_fw_cells,
> +					 ARRAY_SIZE(cros_ec_fw_cells));
> +	if (retval)
> +		dev_warn(ec->dev,
> +			 "failed to add cros-ec fw platform devices: %d\n",
> +			 retval);
> +

I think this should go inside the cros_ec_platform_cells, so drop this and
add the "cros-ec-i2c-tunnel" in the cros_ec_platform_cells[] table is enough.

Thanks,
 Enric


>  	/* Check whether this EC instance has a VBC NVRAM */
>  	node = ec->ec_dev->dev->of_node;
>  	if (of_property_read_bool(node, "google,has-vbc-nvram")) {
> 

Re: [PATCH 1/4] i2c: i2c-cros-ec-tunnel: Pass ACPI node to i2c adapter

[Raul Rangel]

On Mon, Nov 25, 2019 at 9:06 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi,
>
> On 21/11/19 22:10, Raul E Rangel wrote:
> > The I2C bus needs to share the same ACPI node as the tunnel device so
> > that the I2C bus can be referenced from ACPI.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>
> Hmm, isn't this change the same as https://lkml.org/lkml/2019/11/21/208 ?
>
We both found the same problem. I wasn't sure if it would be better to
include it as part of this patch set since it was required for the MFD
patch, or to reference it. Let's just take Akshu's patch and ignore
this one.

Re: [PATCH 2/4] i2c: i2c-cros-ec-tunnel: Fix ACPI identifier

[Raul Rangel]

On Mon, Nov 25, 2019 at 9:23 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Raul,
>
> On 21/11/19 22:10, Raul E Rangel wrote:
> > The initial patch was using the incorrect identifier.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> > There are currently no boards using the identifier so it's fine to
> > change it.
> >
>
> Right, I don't see GOOG001A being used but also I don't see GOOG0012. Anyway,
> for the CrOS EC side, the change looks good to me. I think this will go through
> the I2C tree?
>
Is you were curious, here is the firmware patch:
https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+/1928674

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Raul Rangel]

> > +     if (!ec) {
> > +             dev_err(dev, "%s: ec is missing!\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (!ec->ec_dev) {
> > +             dev_err(dev, "%s: ec->ec_dev is missing!\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +
>
> Are those checks needed? Is that possible?
I don't think it's possible anymore now that the driver has been
converted into an MFD Cell. Previously it would panic if the
i2c-cros-ec-tunnel was compiled as a built-in because it could be
probed before the cros_ec. If the i2c-cros-ec-tunnel was compiled as a
module it worked as expected. So this patch really just fixes the race
condition on initialization.

[    3.733397] BUG: kernel NULL pointer dereference, address: 0000000000000060
...
[    3.734358] RIP: 0010:ec_i2c_probe+0x2f/0x158
0xffffffff81834990 is in ec_i2c_probe
(/mnt/host/source/src/third_party/kernel/next/drivers/i2c/busses/i2c-cros-ec-tunnel.c:250).
245             struct device *dev = &pdev->dev;
246             struct ec_i2c_device *bus = NULL;
247             u32 remote_bus;
248             int err;
249
250             if (!ec->cmd_xfer) {
251                     dev_err(dev, "Missing sendrecv\n");
252                     return -EINVAL;
253             }
254

If you want, I can remove them. But I also don't think they hurt
anything either.

> > -static const struct of_device_id cros_ec_i2c_of_match[] = {
> > -     { .compatible = "google,cros-ec-i2c-tunnel" },
> > -     {},
> > -};
> > -MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
> > -
> > -static const struct acpi_device_id cros_ec_i2c_tunnel_acpi_id[] = {
> > -     { "GOOG0012", 0 },
>
> So, you're removing something that you just added in a previous patch. So is
> really the change in the previous patch needed?

The previous patch also switched over to using
device_property_read_u32, so it's still needed.

> > -     { }
> > -};
> > -MODULE_DEVICE_TABLE(acpi, cros_ec_i2c_tunnel_acpi_id);
> > -
> >  static struct platform_driver ec_i2c_tunnel_driver = {
> >       .probe = ec_i2c_probe,
> >       .remove = ec_i2c_remove,
> >       .driver = {
> >               .name = "cros-ec-i2c-tunnel",
> > -             .acpi_match_table = ACPI_PTR(cros_ec_i2c_tunnel_acpi_id),
> > -             .of_match_table = of_match_ptr(cros_ec_i2c_of_match),
>
> I don't understand this change, why? The id should be in the driver to match.

I removed it here because I want the driver to enumerate as an MFD
cell instead of a stand alone driver.

> >       },
> >  };
> >
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index 1efdba18f20b..61b20e061f75 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -113,6 +113,18 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
> >       { .name = "cros-ec-vbc", }
> >  };
> >
> > +static struct mfd_cell_acpi_match cros_ec_i2c_tunnel_acpi_match = {
> > +     .pnpid = "GOOG0012"
> > +};
> > +
> > +static struct mfd_cell cros_ec_fw_cells[] = {
> > +     {
> > +             .name = "cros-ec-i2c-tunnel",
> > +             .acpi_match = &cros_ec_i2c_tunnel_acpi_match,
> > +             .of_compatible = "google,cros-ec-i2c-tunnel"
>
> Ah, I see. The acpi_match and the of_compatible should be in the
> i2c-cros-ec-tunnel driver not here. Why you changed? Didn't work?

The cell needs to be the one that defines the ACPI match so the MFD
can find the node and set the parent correctly.

>
> > +     },
> > +};
> > +
> >  int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >  {
> >       struct cros_ec_command *msg;
> > @@ -485,6 +497,13 @@ static int ec_device_probe(struct platform_device *pdev)
> >                        "failed to add cros-ec platform devices: %d\n",
> >                        retval);
> >
> > +     retval = mfd_add_hotplug_devices(ec->dev, cros_ec_fw_cells,
> > +                                      ARRAY_SIZE(cros_ec_fw_cells));
> > +     if (retval)
> > +             dev_warn(ec->dev,
> > +                      "failed to add cros-ec fw platform devices: %d\n",
> > +                      retval);
> > +
>
> I think this should go inside the cros_ec_platform_cells, so drop this and
> add the "cros-ec-i2c-tunnel" in the cros_ec_platform_cells[] table is enough.

Good idea. I'll do that.

Thanks

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Lee Jones]

On Thu, 21 Nov 2019, Raul E Rangel wrote:

> If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
> possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
> has finished initializing and setting the drvdata. This would cause a
> NULL pointer panic.
> 
> Converting this driver over to an MFD solves the problem and aligns with
> where the cros_ec is going.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> You can now see the device node lives under the mfd device.
> 
> $ find /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/ -iname firmware_node -exec ls -l '{}' \;
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/firmware_node -> ../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/i2c-10EC5682:00/firmware_node -> ../../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00/10EC5682:00
> 
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 36 +++++++++----------------
>  drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] platform/chrome: i2c: i2c-cros-ec-tunnel: Convert i2c tunnel to MFD Cell

[Enric Balletbo i Serra]

Hi Lee,

On 9/12/19 14:17, Lee Jones wrote:
> On Thu, 21 Nov 2019, Raul E Rangel wrote:
> 
>> If the i2c-cros-ec-tunnel driver is compiled into the kernel, it is
>> possible that i2c-cros-ec-tunnel could be probed before cros_ec_XXX
>> has finished initializing and setting the drvdata. This would cause a
>> NULL pointer panic.
>>
>> Converting this driver over to an MFD solves the problem and aligns with
>> where the cros_ec is going.
>>
>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>> ---
>> You can now see the device node lives under the mfd device.
>>
>> $ find /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/ -iname firmware_node -exec ls -l '{}' \;
>> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/firmware_node -> ../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
>> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/firmware_node -> ../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00
>> /sys/bus/platform/devices/cros-ec-dev.0.auto/cros-ec-i2c-tunnel.12.auto/i2c-9/i2c-10EC5682:00/firmware_node -> ../../../../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1c/PNP0C09:00/GOOG0004:00/GOOG0012:00/10EC5682:00
>>
>>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 36 +++++++++----------------
>>  drivers/mfd/cros_ec_dev.c               | 19 +++++++++++++
> 
> For my own reference:
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> 

Note that there is a v2 for this patch and I did some comments [1]

Thanks,
 Enric

[1] https://patchwork.ozlabs.org/patch/1200560/

Re: [PATCH 1/4] i2c: i2c-cros-ec-tunnel: Pass ACPI node to i2c adapter

[Wolfram Sang]

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

On Thu, Nov 21, 2019 at 02:10:50PM -0700, Raul E Rangel wrote:
> The I2C bus needs to share the same ACPI node as the tunnel device so
> that the I2C bus can be referenced from ACPI.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

Dropped as requested.


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

Re: [PATCH 2/4] i2c: i2c-cros-ec-tunnel: Fix ACPI identifier

[Wolfram Sang]

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

On Thu, Nov 21, 2019 at 02:10:51PM -0700, Raul E Rangel wrote:
> The initial patch was using the incorrect identifier.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

Applied to for-next, thanks!


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