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

[Peter Rosin]

Hi!

On 2020-10-15 03:02, 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 all direct children
> of an i2c controller in Linux had to be i2c devices. These days that

I2C controller. I2C devices.

I fail to see why this "relic" has to be explicitly blamed on Linux? In the
beginning, the bindings for all I2C controllers (sometimes implicitely,
sometimes explicitely) specified that all child nodes had to be I2C devices.
The *bindings* were simply not as flexible before the i2c-bus subnode was
invented only a few years ago. So, there are arguments that the "problem"
was in DT-land and that Linux just followed suit.

> implementation detail has been worked out, so 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.

There is potential to match this and make i2c-parent optional for the
DT case and require it to be a child of its parent in such cases, if
someone has the time/energy...

> 
> Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx>
> ---
> 
> 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 | 103 ++++++++++++++++++++++---------
>  1 file changed, 75 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 4effe563e9e8d..8e4008f4a9b5d 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -49,34 +49,80 @@ 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");

Missing trailing \n

> +		return -EINVAL;
> +	}
> +
> +	*adr = adr64;

Maybe this is too pedantic? Optional, ignore if I'm just being insane...

	if (*adr != adr64) {
		dev_err(dev, "Address out of range\n");
		return -EINVAL;
	}

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

Remove the dev_handle declaration here...(push)...

> +	struct device_node *adapter_np;
> +	struct i2c_adapter *adapter = NULL;
> +	struct fwnode_handle *child = NULL;

Why do you need these two " = NULL" here? I can't believe compilers are
that stupid? If they are, fine. But otherwise, why single out these two
pointers and add NULL only there and not everywhere? But NULL everywhere
would be ugly...

>  	unsigned *values;
> -	int i = 0;
> +	int rc, 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");

You should do "&pdev->dev" -> "dev" here, because I hate having
the dev variable and then not use it. But that should perhaps be
a preparatory patch, because I see more instances and this is an
orthogonal change.

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

...(pop)...and perhaps say

		acpi_handle dev_handle = ACPI_HANDLE(dev->parent);

here?

Cheers,
Peter

> +		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 +131,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)
>  {
> @@ -118,7 +165,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 {
>