Date:   Mon, 12 Oct 2020 10:46:56 -0700
From:   Evan Green <evgreen@...omium.org>
To:     Peter Rosin <peda@...ntia.se>
Cc:     Wolfram Sang <wsa@...nel.org>,
        Peter Korsgaard <peter.korsgaard@...co.com>,
        linux-i2c@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] i2c: i2c-mux-gpio: Enable this driver in ACPI land

On Sat, Oct 10, 2020 at 10:03 AM Peter Rosin <peda@...ntia.se> wrote:
>
> Hi!
>
> On 2020-10-10 00:43, Evan Green 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 is a little trickier, since of's phandle definition
> > suggests the i2c mux could live in a completely different part of
> > the tree than its upstream i2c controller. For now in ACPI,
>
> This is so since the I2C gpio-mux predates the "i2c-bus" sub-node of
> I2C controllers. At that time *all* sub-nodes of I2C controllers
> represented I2C client device, IIUC. With that knowledge, you could
> perhaps rephrase the above?

Ah I see, so this part of the binding was defined to work around
implementation details of Linux. But now those details are worked out,
so porting that part to ACPI isn't necessary. I'll rephrase to that
effect.

>
> Also, a nit below.
>
> > just assume that the i2c-mux-gpio device will always be a direct
> > child of the i2c controller. If the additional flexibility of
> > defining muxes in wildly different locations from their parent
> > controllers is required, it can be added later.
> >
> > Signed-off-by: Evan Green <evgreen@...omium.org>
> > ---
> >
> >  drivers/i2c/muxes/i2c-mux-gpio.c | 77 +++++++++++++++++++++-----------
> >  1 file changed, 50 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 4effe563e9e8d..f195e95e8a037 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -49,34 +49,46 @@ 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)
> > +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> > +                              struct platform_device *pdev)
> >  {
> > -     struct device_node *np = pdev->dev.of_node;
> > -     struct device_node *adapter_np, *child;
> > -     struct i2c_adapter *adapter;
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = dev->of_node;
> > +     acpi_handle dev_handle;
> > +     struct device_node *adapter_np;
> > +     struct i2c_adapter *adapter = NULL;
> > +     struct fwnode_handle *child = NULL;
> >       unsigned *values;
> >       int i = 0;
> >
> > -     if (!np)
> > -             return -ENODEV;
> > +     if (is_of_node(dev->fwnode)) {
> > +             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_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.
> > +              */
> > +             dev_handle = ACPI_HANDLE(dev->parent);
> > +             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(&pdev->dev,
> >                             mux->data.n_values, sizeof(*mux->data.values),
> >                             GFP_KERNEL);
> > @@ -85,24 +97,35 @@ 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)) {
> > +                     unsigned long long adr;
> > +                     acpi_status status;
> > +
> > +                     status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> > +                                                    METHOD_NAME__ADR,
> > +                                                    NULL, &adr);
> > +                     if (ACPI_SUCCESS(status)) {
> > +                             *(values + i) = adr;
>
> I would write that as
>                                 values[i] = adr;

Will fix. Let me get this compiling properly when ACPI is not defined
and I'll send and update. Thanks for reviewing.
-Evan

Powered by blists - more mailing lists