Re: [PATCH] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[Peter Rosin]

Hi!

On 2020-10-10 00:43, 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 is a little trickier, since of's phandle definition
> suggests the i2c mux could live in a completely different part of
> the tree than its upstream i2c controller. For now in ACPI,

This is so since the I2C gpio-mux predates the "i2c-bus" sub-node of
I2C controllers. At that time *all* sub-nodes of I2C controllers
represented I2C client device, IIUC. With that knowledge, you could
perhaps rephrase the above?

Also, a nit below.

> just assume that the i2c-mux-gpio device will always be a direct
> child of the i2c controller. If the additional flexibility of
> defining muxes in wildly different locations from their parent
> controllers is required, it can be added later.
> 
> Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx>
> ---
> 
>  drivers/i2c/muxes/i2c-mux-gpio.c | 77 +++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 4effe563e9e8d..f195e95e8a037 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -49,34 +49,46 @@ 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)
> +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> +				 struct platform_device *pdev)
>  {
> -	struct device_node *np = pdev->dev.of_node;
> -	struct device_node *adapter_np, *child;
> -	struct i2c_adapter *adapter;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	acpi_handle dev_handle;
> +	struct device_node *adapter_np;
> +	struct i2c_adapter *adapter = NULL;
> +	struct fwnode_handle *child = NULL;
>  	unsigned *values;
>  	int i = 0;
>  
> -	if (!np)
> -		return -ENODEV;
> +	if (is_of_node(dev->fwnode)) {
> +		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_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.
> +		 */
> +		dev_handle = ACPI_HANDLE(dev->parent);
> +		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(&pdev->dev,
>  			      mux->data.n_values, sizeof(*mux->data.values),
>  			      GFP_KERNEL);
> @@ -85,24 +97,35 @@ 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)) {
> +			unsigned long long adr;
> +			acpi_status status;
> +
> +			status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> +						       METHOD_NAME__ADR,
> +						       NULL, &adr);
> +			if (ACPI_SUCCESS(status)) {
> +				*(values + i) = adr;

I would write that as
				values[i] = adr;

Cheers,
Peter

> +
> +			} else {
> +				dev_err(dev, "Cannot get address");
> +				return -EINVAL;
> +			}
> +		}
> +
>  		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)
>  {
> @@ -118,7 +141,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 {
>