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

On Fri, Nov 20, 2020 at 10:59:12AM -0800, Evan Green wrote:

On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko
[off-list ref] wrote:

quoted

On Thu, Nov 19, 2020 at 1:40 AM Evan Green [off-list ref] wrote:

quoted

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.

...

quoted

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

I think it's complementary. The code above is "I have a device, I want
its _ADR". whereas acpi_find_child_device() is "I have an _ADR, I want
its device". I could flip things around to use this, but it would turn
the code from linear into quadratic. I'd have to scan each possible
address and call acpi_find_child_device() with that _ADR to see if
there's a child device there.

quoted

...

quoted

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

Oh, I'm not aware of those threads. I'd need some advice: I guess a
new fwnode_* API would make sense for this, but I had trouble coming
up with a generic interface. _ADR is just a blobbo 64 bit int, but
DT's "reg" is a little more flexible, having a length, and potentially
being an array. I suppose it would have to be something like:

int fwnode_property_read_reg(const struct fwnode_handle *fwnode,
                                 size_t index, uint64_t *addr, uint64_t *len);

But then ACPI would always return 0 for length, and only index 0 would
ever work? I'm worried I'm designing an API that's only useful to me.

I tried to look around for other examples of this specific pattern of
_ADR then "reg", but struggled to turn up much.
-Evan

Andy, is Evan's answer satisfying for you?