[PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[Andy Shevchenko]

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

[PATCH v1 2/3] i2c: mux: gpio: Don't dereference fwnode from struct device

[Andy Shevchenko]

We have a special helper to get fwnode out of struct device.
Moreover, dereferencing it directly prevents the fwnode
modifications in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 31e6eb1591bb..b09c10f36ddb 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -53,6 +53,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
 				 struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct device_node *np = dev->of_node;
 	struct device_node *adapter_np;
 	struct i2c_adapter *adapter = NULL;
@@ -60,7 +61,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
 	unsigned *values;
 	int rc, i = 0;
 
-	if (is_of_node(dev->fwnode)) {
+	if (is_of_node(fwnode)) {
 		if (!np)
 			return -ENODEV;
 
@@ -72,7 +73,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
 		adapter = of_find_i2c_adapter_by_node(adapter_np);
 		of_node_put(adapter_np);
 
-	} else if (is_acpi_node(dev->fwnode)) {
+	} else if (is_acpi_node(fwnode)) {
 		/*
 		 * In ACPI land the mux should be a direct child of the i2c
 		 * bus it muxes.
@@ -111,7 +112,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
 	}
 	mux->data.values = values;
 
-	if (fwnode_property_read_u32(dev->fwnode, "idle-state", &mux->data.idle))
+	if (device_property_read_u32(dev, "idle-state", &mux->data.idle))
 		mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
 
 	return 0;
-- 
2.33.0

Re: [PATCH v1 2/3] i2c: mux: gpio: Don't dereference fwnode from struct device

[Evan Green]

On Mon, Nov 15, 2021 at 7:42 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> We have a special helper to get fwnode out of struct device.
> Moreover, dereferencing it directly prevents the fwnode
> modifications in the future.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH v1 2/3] i2c: mux: gpio: Don't dereference fwnode from struct device

[Peter Rosin]

Hi!

On 2021-11-15 16:42, Andy Shevchenko wrote:
> We have a special helper to get fwnode out of struct device.
> Moreover, dereferencing it directly prevents the fwnode
> modifications in the future.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/muxes/i2c-mux-gpio.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 31e6eb1591bb..b09c10f36ddb 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -53,6 +53,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
>  				 struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct device_node *np = dev->of_node;

It feels like there is opportunity to get rid of np, but I suppose that can be
done later...

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

>  	struct device_node *adapter_np;
>  	struct i2c_adapter *adapter = NULL;
> @@ -60,7 +61,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
>  	unsigned *values;
>  	int rc, i = 0;
>  
> -	if (is_of_node(dev->fwnode)) {
> +	if (is_of_node(fwnode)) {
>  		if (!np)
>  			return -ENODEV;
>  
> @@ -72,7 +73,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
>  		adapter = of_find_i2c_adapter_by_node(adapter_np);
>  		of_node_put(adapter_np);
>  
> -	} else if (is_acpi_node(dev->fwnode)) {
> +	} else if (is_acpi_node(fwnode)) {
>  		/*
>  		 * In ACPI land the mux should be a direct child of the i2c
>  		 * bus it muxes.
> @@ -111,7 +112,7 @@ static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
>  	}
>  	mux->data.values = values;
>  
> -	if (fwnode_property_read_u32(dev->fwnode, "idle-state", &mux->data.idle))
> +	if (device_property_read_u32(dev, "idle-state", &mux->data.idle))
>  		mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
>  
>  	return 0;
> 

Re: [PATCH v1 2/3] i2c: mux: gpio: Don't dereference fwnode from struct device

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

On Mon, Nov 15, 2021 at 05:42:00PM +0200, Andy Shevchenko wrote:
> We have a special helper to get fwnode out of struct device.
> Moreover, dereferencing it directly prevents the fwnode
> modifications in the future.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 3/3] i2c: mux: gpio: Use array_size() helper

[Andy Shevchenko]

Use array_size() helper to aid in 2-factor allocation instances.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index b09c10f36ddb..73a23e117ebe 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -7,6 +7,7 @@
 
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
+#include <linux/overflow.h>
 #include <linux/platform_data/i2c-mux-gpio.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
@@ -152,7 +153,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 
 	muxc = i2c_mux_alloc(parent, &pdev->dev, mux->data.n_values,
-			     ngpios * sizeof(*mux->gpios), 0,
+			     array_size(ngpios, sizeof(*mux->gpios)), 0,
 			     i2c_mux_gpio_select, NULL);
 	if (!muxc) {
 		ret = -ENOMEM;
-- 
2.33.0

Re: [PATCH v1 3/3] i2c: mux: gpio: Use array_size() helper

[Evan Green]

On Mon, Nov 15, 2021 at 7:42 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Use array_size() helper to aid in 2-factor allocation instances.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for the cleanup series!

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH v1 3/3] i2c: mux: gpio: Use array_size() helper

[Peter Rosin]

On 2021-11-15 16:42, Andy Shevchenko wrote:
> Use array_size() helper to aid in 2-factor allocation instances.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

Re: [PATCH v1 3/3] i2c: mux: gpio: Use array_size() helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

On Mon, Nov 15, 2021 at 05:42:01PM +0200, Andy Shevchenko wrote:
> Use array_size() helper to aid in 2-factor allocation instances.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[Evan Green]

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>

Re: [PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[Peter Rosin]

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

Re: [PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[Andy Shevchenko]

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


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[Peter Rosin]

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

Re: [PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 130 bytes --]

> @Wolfram: You're finding this series in patchwork and will be picking it
> up as usual, right? Thanks!

Right, will do so now.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/3] i2c: mux: gpio: Replace custom acpi_get_local_address()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]