Re: [PATCH 2/5] i2c: gpio: Add support on ACPI-based system

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

Hi,

On Thu, Sep 22, 2022 at 07:39:55PM +0800, Binbin Zhou wrote:
> Add support for the ACPI-based device registration so that the driver
> can be also enabled through ACPI table.
> 
> Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-gpio.c | 41 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index b1985c1667e1..ccea37e755e6 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -13,6 +13,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/acpi.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/i2c-gpio.h>
>  #include <linux/platform_device.h>
> @@ -318,6 +319,24 @@ static void of_i2c_gpio_get_props(struct device_node *np,
>  		of_property_read_bool(np, "i2c-gpio,scl-output-only");
>  }
>  
> +static void acpi_i2c_gpio_get_props(struct device *dev,
> +				  struct i2c_gpio_platform_data *pdata)
> +{
> +	u32 reg;
> +
> +	device_property_read_u32(dev, "delay-us", &pdata->udelay);
> +
> +	if (!device_property_read_u32(dev, "timeout-ms", &reg))
> +		pdata->timeout = msecs_to_jiffies(reg);
> +
> +	pdata->sda_is_open_drain =
> +		device_property_read_bool(dev, "sda-open-drain");
> +	pdata->scl_is_open_drain =
> +		device_property_read_bool(dev, "scl-open-drain");
> +	pdata->scl_is_output_only =
> +		device_property_read_bool(dev, "scl-output-only");
> +}

I think this would work with the DT description too as it is using
device_property_xxx() so I wonder if you can just do:

	i2c_gpio_get_props(dev, pdata);

instead of

 	if (np) {
 		of_i2c_gpio_get_props(np, pdata);
	} else if (ACPI_COMPANION(dev)) {
		acpi_i2c_gpio_get_props(dev, pdata);

> +
>  static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
>  					   const char *con_id,
>  					   unsigned int index,
> @@ -363,6 +382,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	enum gpiod_flags gflags;
> +	acpi_status status;
> +	unsigned long long id;
>  	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -375,6 +396,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>  
>  	if (np) {
>  		of_i2c_gpio_get_props(np, pdata);
> +	} else if (ACPI_COMPANION(dev)) {
> +		acpi_i2c_gpio_get_props(dev, pdata);
>  	} else {
>  		/*
>  		 * If all platform data settings are zero it is OK
> @@ -445,7 +468,14 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>  	adap->dev.parent = dev;
>  	adap->dev.of_node = np;
>  
> -	adap->nr = pdev->id;
> +	if (ACPI_COMPANION(dev)) {
> +		status = acpi_evaluate_integer(ACPI_HANDLE(dev),
> +						"_UID", NULL, &id);
> +		if (ACPI_SUCCESS(status) && (id >= 0))
> +			adap->nr = id;

Unrelated change? And if not then same comment about why you need the
static number in the first place ;-)