| Message ID | 20201118153951.RESEND.v3.2.Idef164c23d326f5e5edecfc5d3eb2a68fcf18be1@changeid |
|---|---|
| State | Accepted |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | i2c: i2c-mux-gpio: Enable this driver in ACPI land | expand |
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.
On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > 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? 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. > > ... > > > + 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 > > -- > With Best Regards, > Andy Shevchenko
Hi Andy, Peter, On Fri, Nov 20, 2020 at 10:59 AM Evan Green <evgreen@chromium.org> wrote: > > On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > 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? > > 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. > > > > > ... > > > > > + 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. Any thoughts on this? > -Evan > > > > > -- > > With Best Regards, > > Andy Shevchenko
Very sorry to ping. Is there anything I can do to help get this reviewed? -Evan On Mon, Nov 30, 2020 at 11:11 AM Evan Green <evgreen@chromium.org> wrote: > > Hi Andy, Peter, > > On Fri, Nov 20, 2020 at 10:59 AM Evan Green <evgreen@chromium.org> wrote: > > > > On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > 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? > > > > 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. > > > > > > > > ... > > > > > > > + 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. > > Any thoughts on this? > > > -Evan > > > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko
On Fri, Nov 20, 2020 at 10:59:12AM -0800, Evan Green wrote: > On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > 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? > > 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. > > > > > ... > > > > > + 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?
On Tue, Jan 5, 2021 at 2:25 AM Wolfram Sang <wsa@kernel.org> wrote: > > On Fri, Nov 20, 2020 at 10:59:12AM -0800, Evan Green wrote: > > On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > 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? > > > > 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. > > > > > > > > ... > > > > > > > + 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? Can this be accepted as-is, or should I resend? -Evan
> Can this be accepted as-is, or should I resend?
Peter, can you have a look here as well?
On 2021-01-14 20:53, Wolfram Sang wrote: >> Can this be accepted as-is, or should I resend? > > Peter, can you have a look here as well? I have no issues, but I was waiting for a response from Andy here. I have little knowledge of ACPI, and have previously made ignorant mistakes in that area. I would greatly appreciate Andy following through with his line of thinking... So, if we ignore Andys review comments, then: Acked-by: Peter Rosin <peda@axentia.se> Cheers, Peter
On Wed, Nov 18, 2020 at 03:40:25PM -0800, 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 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> Applied to for-next, thanks! The code Andy mentioned can still be refactored later if new ACPI helpers appear in the future.
On Sun, Jan 17, 2021 at 3:54 AM Wolfram Sang <wsa@kernel.org> wrote: > > On Wed, Nov 18, 2020 at 03:40:25PM -0800, 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 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> > > Applied to for-next, thanks! The code Andy mentioned can still be > refactored later if new ACPI helpers appear in the future. Thanks Wolfram and Peter!
On Fri, Jan 15, 2021 at 10:29:46AM +0100, Peter Rosin wrote: > On 2021-01-14 20:53, Wolfram Sang wrote: > >> Can this be accepted as-is, or should I resend? > > > > Peter, can you have a look here as well? > > I have no issues, but I was waiting for a response from Andy here. I have > little knowledge of ACPI, and have previously made ignorant mistakes in > that area. I would greatly appreciate Andy following through with his > line of thinking... > > So, if we ignore Andys review comments, then: > > Acked-by: Peter Rosin <peda@axentia.se> Sorry guys for me being silent, I have a lot of stuff which makes me under DDoS-like attack (metaphorically speaking). I'm going to look at this just now.
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(-)