[PATCH v3 0/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[PATCH v3 0/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[Evan Green]

The i2c-mux-gpio driver is a handy driver to have in your bag of tricks,
but it currently only works with DT-based firmware. Enable this driver
on ACPI platforms as well.

The first patch is a little dinky. Peter, if it turns out you'd rather
just take this all as a single patch, feel free to squash the first
patch into the second. Or I can resend a squashed patch if needed.

Changes in v3:
 - Introduced minor &pdev->dev to dev refactor (Peter)
 - Update commit message again (Peter)
 - Added missing \n (Peter)
 - adr64 overflow check (Peter)
 - Don't initialize child (Peter)
 - Limit scope of dev_handle (Peter)

Changes in v2:
 - Make it compile properly when !CONFIG_ACPI (Randy)
 - Update commit message regarding i2c-parent (Peter)

Evan Green (2):
  i2c: i2c-mux-gpio: Factor out pdev->dev in _probe_dt()
  i2c: i2c-mux-gpio: Enable this driver in ACPI land

 drivers/i2c/muxes/i2c-mux-gpio.c | 112 ++++++++++++++++++++++---------
 1 file changed, 82 insertions(+), 30 deletions(-)

-- 
2.26.2

[PATCH v3 1/2] i2c: i2c-mux-gpio: Factor out pdev->dev in _probe_dt()

[Evan Green]

Factor out &pdev->dev into a local variable in preparation for
the ACPI enablement of this function, which will utilize the variable
more.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

Changes in v3:
 - Introduced minor &pdev->dev to dev refactor (Peter)

 drivers/i2c/muxes/i2c-mux-gpio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 4effe563e9e8d..caaa782b50d83 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -53,6 +53,7 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc, u32 chan)
 static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 					struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *adapter_np, *child;
 	struct i2c_adapter *adapter;
@@ -77,11 +78,11 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 
 	mux->data.n_values = of_get_child_count(np);
 
-	values = devm_kcalloc(&pdev->dev,
+	values = devm_kcalloc(dev,
 			      mux->data.n_values, sizeof(*mux->data.values),
 			      GFP_KERNEL);
 	if (!values) {
-		dev_err(&pdev->dev, "Cannot allocate values array");
+		dev_err(dev, "Cannot allocate values array");
 		return -ENOMEM;
 	}
 
-- 
2.26.2

[PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[Evan Green]

Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
property translates directly to a fwnode_property_*() call. The child
reg property translates naturally into _ADR in ACPI.

The i2c-parent binding is a relic from the days when the bindings
dictated that all direct children of an I2C controller had to be I2C
devices. These days that's no longer required. The i2c-mux can sit as a
direct child of its parent controller, which is where it makes the most
sense from a hardware description perspective. For the ACPI
implementation we'll assume that's always how the i2c-mux-gpio is
instantiated.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

Changes in v3:
 - Update commit message again (Peter)
 - Added missing \n (Peter)
 - adr64 overflow check (Peter)
 - Don't initialize child (Peter)
 - Limit scope of dev_handle (Peter)

Changes in v2:
 - Make it compile properly when !CONFIG_ACPI (Randy)
 - Update commit message regarding i2c-parent (Peter)

 drivers/i2c/muxes/i2c-mux-gpio.c | 107 +++++++++++++++++++++++--------
 1 file changed, 79 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index caaa782b50d83..bac415a52b780 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -49,35 +49,85 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc, u32 chan)
 	return 0;
 }
 
-#ifdef CONFIG_OF
-static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
-					struct platform_device *pdev)
+#ifdef CONFIG_ACPI
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+				     struct fwnode_handle *fwdev,
+				     unsigned int *adr)
+
+{
+	unsigned long long adr64;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
+				       METHOD_NAME__ADR,
+				       NULL, &adr64);
+
+	if (!ACPI_SUCCESS(status)) {
+		dev_err(dev, "Cannot get address\n");
+		return -EINVAL;
+	}
+
+	*adr = adr64;
+	if (*adr != adr64) {
+		dev_err(dev, "Address out of range\n");
+		return -ERANGE;
+	}
+
+	return 0;
+}
+
+#else
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+				     struct fwnode_handle *fwdev,
+				     unsigned int *adr)
+{
+	return -EINVAL;
+}
+
+#endif
+
+static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
+				 struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = pdev->dev.of_node;
-	struct device_node *adapter_np, *child;
-	struct i2c_adapter *adapter;
+	struct device_node *np = dev->of_node;
+	struct device_node *adapter_np;
+	struct i2c_adapter *adapter = NULL;
+	struct fwnode_handle *child;
 	unsigned *values;
-	int i = 0;
+	int rc, i = 0;
+
+	if (is_of_node(dev->fwnode)) {
+		if (!np)
+			return -ENODEV;
 
-	if (!np)
-		return -ENODEV;
+		adapter_np = of_parse_phandle(np, "i2c-parent", 0);
+		if (!adapter_np) {
+			dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
+			return -ENODEV;
+		}
+		adapter = of_find_i2c_adapter_by_node(adapter_np);
+		of_node_put(adapter_np);
+
+	} else if (is_acpi_node(dev->fwnode)) {
+		/*
+		 * In ACPI land the mux should be a direct child of the i2c
+		 * bus it muxes.
+		 */
+		acpi_handle dev_handle = ACPI_HANDLE(dev->parent);
 
-	adapter_np = of_parse_phandle(np, "i2c-parent", 0);
-	if (!adapter_np) {
-		dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
-		return -ENODEV;
+		adapter = i2c_acpi_find_adapter_by_handle(dev_handle);
 	}
-	adapter = of_find_i2c_adapter_by_node(adapter_np);
-	of_node_put(adapter_np);
+
 	if (!adapter)
 		return -EPROBE_DEFER;
 
 	mux->data.parent = i2c_adapter_id(adapter);
 	put_device(&adapter->dev);
 
-	mux->data.n_values = of_get_child_count(np);
-
+	mux->data.n_values = device_get_child_node_count(dev);
 	values = devm_kcalloc(dev,
 			      mux->data.n_values, sizeof(*mux->data.values),
 			      GFP_KERNEL);
@@ -86,24 +136,25 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 		return -ENOMEM;
 	}
 
-	for_each_child_of_node(np, child) {
-		of_property_read_u32(child, "reg", values + i);
+	device_for_each_child_node(dev, child) {
+		if (is_of_node(child)) {
+			fwnode_property_read_u32(child, "reg", values + i);
+
+		} else if (is_acpi_node(child)) {
+			rc = i2c_mux_gpio_get_acpi_adr(dev, child, values + i);
+			if (rc)
+				return rc;
+		}
+
 		i++;
 	}
 	mux->data.values = values;
 
-	if (of_property_read_u32(np, "idle-state", &mux->data.idle))
+	if (fwnode_property_read_u32(dev->fwnode, "idle-state", &mux->data.idle))
 		mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
 
 	return 0;
 }
-#else
-static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
-					struct platform_device *pdev)
-{
-	return 0;
-}
-#endif
 
 static int i2c_mux_gpio_probe(struct platform_device *pdev)
 {
@@ -119,7 +170,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	if (!dev_get_platdata(&pdev->dev)) {
-		ret = i2c_mux_gpio_probe_dt(mux, pdev);
+		ret = i2c_mux_gpio_probe_fw(mux, pdev);
 		if (ret < 0)
 			return ret;
 	} else {
-- 
2.26.2

Re: [PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[Andy Shevchenko]

On Sat, Oct 17, 2020 at 8:30 AM Evan Green <evgreen@chromium.org> wrote:
>
> Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> property translates directly to a fwnode_property_*() call. The child
> reg property translates naturally into _ADR in ACPI.
>
> The i2c-parent binding is a relic from the days when the bindings
> dictated that all direct children of an I2C controller had to be I2C
> devices. These days that's no longer required. The i2c-mux can sit as a
> direct child of its parent controller, which is where it makes the most
> sense from a hardware description perspective. For the ACPI
> implementation we'll assume that's always how the i2c-mux-gpio is
> instantiated.

Can you tell me if the following is relevant to what you are looking for?
https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-mux.c#L393

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[Evan Green]

On Sun, Oct 18, 2020 at 11:58 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Oct 17, 2020 at 8:30 AM Evan Green <evgreen@chromium.org> wrote:
> >
> > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > property translates directly to a fwnode_property_*() call. The child
> > reg property translates naturally into _ADR in ACPI.
> >
> > The i2c-parent binding is a relic from the days when the bindings
> > dictated that all direct children of an I2C controller had to be I2C
> > devices. These days that's no longer required. The i2c-mux can sit as a
> > direct child of its parent controller, which is where it makes the most
> > sense from a hardware description perspective. For the ACPI
> > implementation we'll assume that's always how the i2c-mux-gpio is
> > instantiated.
>
> Can you tell me if the following is relevant to what you are looking for?
> https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-mux.c#L393

I don't think so, but let me know if I'm reading between the lines incorrectly.

The code you pointed to links the newly-minted fake i2c controller
back together with its ACPI node. This is important, since I think
that's how child I2C devices underneath the fake busses get populated
in ACPI land. But the paragraph above is discussing how to identify
the parent adapter (ie the real hardware) for an i2c-mux-gpio device.

In DT-land, the i2c-mux-gpio floats at the top of the tree directly
under /, and then uses a phandle to point to where transactions should
be forwarded. I'm told the reason for this is historical limitations
with the DT bindings. Rather than trying to translate the phandle over
1:1 into ACPI-land, I'm asserting that the mux device should live
underneath the adapter it wants to forward traffic to.

-Evan

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[Evan Green]

On Mon, Oct 19, 2020 at 9:53 AM Evan Green <evgreen@chromium.org> wrote:
>
> On Sun, Oct 18, 2020 at 11:58 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sat, Oct 17, 2020 at 8:30 AM Evan Green <evgreen@chromium.org> wrote:
> > >
> > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > > property translates directly to a fwnode_property_*() call. The child
> > > reg property translates naturally into _ADR in ACPI.
> > >
> > > The i2c-parent binding is a relic from the days when the bindings
> > > dictated that all direct children of an I2C controller had to be I2C
> > > devices. These days that's no longer required. The i2c-mux can sit as a
> > > direct child of its parent controller, which is where it makes the most
> > > sense from a hardware description perspective. For the ACPI
> > > implementation we'll assume that's always how the i2c-mux-gpio is
> > > instantiated.
> >
> > Can you tell me if the following is relevant to what you are looking for?
> > https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-mux.c#L393
>
> I don't think so, but let me know if I'm reading between the lines incorrectly.
>
> The code you pointed to links the newly-minted fake i2c controller
> back together with its ACPI node. This is important, since I think
> that's how child I2C devices underneath the fake busses get populated
> in ACPI land. But the paragraph above is discussing how to identify
> the parent adapter (ie the real hardware) for an i2c-mux-gpio device.
>
> In DT-land, the i2c-mux-gpio floats at the top of the tree directly
> under /, and then uses a phandle to point to where transactions should
> be forwarded. I'm told the reason for this is historical limitations
> with the DT bindings. Rather than trying to translate the phandle over
> 1:1 into ACPI-land, I'm asserting that the mux device should live
> underneath the adapter it wants to forward traffic to.

Andy or Peter, Any other thoughts on this series?
-Evan