Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201118234025.376412-1-evgreen@chromium.org> <20201118153951.RESEND.v3.2.Idef164c23d326f5e5edecfc5d3eb2a68fcf18be1@changeid>
In-Reply-To: <20201118153951.RESEND.v3.2.Idef164c23d326f5e5edecfc5d3eb2a68fcf18be1@changeid>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 19 Nov 2020 17:25:41 +0200
Message-ID: <CAHp75VdtwyCj7emc7Bk87q7kMQA0sSX81-aK-fMq4qTfTF-c_g@mail.gmail.com>
Subject: Re: [RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in
 ACPI land
To:     Evan Green <evgreen@chromium.org>
Cc:     Peter Rosin <peda@axentia.se>, Wolfram Sang <wsa@kernel.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        Peter Korsgaard <peter.korsgaard@barco.com>,
        linux-i2c <linux-i2c@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Nov 19, 2020 at 1:40 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.

...

> +#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

I'm wondering if you may use acpi_find_child_device() here.
Or is it a complementary function?

...

> +       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++;
>         }

And for this I already told in two different threads with similar code
that perhaps we need common helper that will check reg followed by
_ADR.

-- 
With Best Regards,
Andy Shevchenko