| Message ID | 20211115154201.46579-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1,1/3] i2c: mux: gpio: Replace custom acpi_get_local_address() | expand |
On Mon, Nov 15, 2021 at 7:42 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > Recently ACPI gained the acpi_get_local_address() API which may be used > instead of home grown i2c_mux_gpio_get_acpi_adr(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Evan Green <evgreen@chromium.org>
Hi! On 2021-11-15 16:41, Andy Shevchenko wrote: > Recently ACPI gained the acpi_get_local_address() API which may be used > instead of home grown i2c_mux_gpio_get_acpi_adr(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/i2c/muxes/i2c-mux-gpio.c | 43 ++------------------------------ > 1 file changed, 2 insertions(+), 41 deletions(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c > index bac415a52b78..31e6eb1591bb 100644 > --- a/drivers/i2c/muxes/i2c-mux-gpio.c > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c > @@ -49,45 +49,6 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc, u32 chan) > return 0; > } > > -#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; > - } In the conversion, I read it as if we lose this overflow check. Why is that not a problem? Cheers, Peter > - > - 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) > { > @@ -141,9 +102,9 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux, > 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); > + rc = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), values + i); > if (rc) > - return rc; > + return dev_err_probe(dev, rc, "Cannot get address\n"); > } > > i++; >
+Cc: Rafael On Thu, Nov 18, 2021 at 12:24 PM Peter Rosin <peda@axentia.se> wrote: > On 2021-11-15 16:41, Andy Shevchenko wrote: ... > > - *adr = adr64; > > - if (*adr != adr64) { > > - dev_err(dev, "Address out of range\n"); > > - return -ERANGE; > > - } > > In the conversion, I read it as if we lose this overflow check. It depends from which angle you look at this. We relaxed requirements. > Why is that > not a problem? The idea behind the acpi_get_local_address() is to provide a unified way between DT and ACPI for the same value. In either case we take only a 32-bit value. We might nevertheless add that check to the API. Rafael, what do you think? P.S. Just realized that in ACPI the higher part of the address may be used as flags by some interfaces (SoundWire is one of them), this is not applicable to I²C muxes right now, but who knows... So I prefer a relaxed version and, if necessary, documentation should be amended/updated.
On 2021-11-18 11:33, Andy Shevchenko wrote: > +Cc: Rafael > > On Thu, Nov 18, 2021 at 12:24 PM Peter Rosin <peda@axentia.se> wrote: >> On 2021-11-15 16:41, Andy Shevchenko wrote: > > ... > >>> - *adr = adr64; >>> - if (*adr != adr64) { >>> - dev_err(dev, "Address out of range\n"); >>> - return -ERANGE; >>> - } >> >> In the conversion, I read it as if we lose this overflow check. > > It depends from which angle you look at this. We relaxed requirements. > >> Why is that >> not a problem? > > The idea behind the acpi_get_local_address() is to provide a unified > way between DT and ACPI for the same value. In either case we take > only a 32-bit value. We might nevertheless add that check to the API. > Rafael, what do you think? > > P.S. Just realized that in ACPI the higher part of the address may be > used as flags by some interfaces (SoundWire is one of them), this is > not applicable to I²C muxes right now, but who knows... So I prefer a > relaxed version and, if necessary, documentation should be > amended/updated. Splendid, just checking that you're on top of things. I don't think any doc update is needed on the i2c-mux end, until flags in the upper bits are introduced? So, looks good to me, thanks! Acked-by: Peter Rosin <peda@axentia.se> @Wolfram: You're finding this series in patchwork and will be picking it up as usual, right? Thanks! Cheers, Peter
> @Wolfram: You're finding this series in patchwork and will be picking it > up as usual, right? Thanks! Right, will do so now.
On Mon, Nov 15, 2021 at 05:41:59PM +0200, Andy Shevchenko wrote: > Recently ACPI gained the acpi_get_local_address() API which may be used > instead of home grown i2c_mux_gpio_get_acpi_adr(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c index bac415a52b78..31e6eb1591bb 100644 --- a/drivers/i2c/muxes/i2c-mux-gpio.c +++ b/drivers/i2c/muxes/i2c-mux-gpio.c @@ -49,45 +49,6 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc, u32 chan) return 0; } -#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) { @@ -141,9 +102,9 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux, 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); + rc = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), values + i); if (rc) - return rc; + return dev_err_probe(dev, rc, "Cannot get address\n"); } i++;
Recently ACPI gained the acpi_get_local_address() API which may be used instead of home grown i2c_mux_gpio_get_acpi_adr(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/muxes/i2c-mux-gpio.c | 43 ++------------------------------ 1 file changed, 2 insertions(+), 41 deletions(-)