| Message ID | 20201016152454.v3.2.Idef164c23d326f5e5edecfc5d3eb2a68fcf18be1@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i2c-mux-gpio: Enable this driver in ACPI land | expand |
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
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
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
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 {
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(-)