Re: [PATCH V2 1/4] i2c: gpio: Add support on ACPI-based system

[Mika Westerberg <mika.westerberg@linux.intel.com>]

[+Rafael and Andy]

Hi,

On Mon, Sep 26, 2022 at 09:00:04PM +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@loongson.cn>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  drivers/i2c/busses/i2c-gpio.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index b1985c1667e1..417eb31e0971 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");
> +}

Otherwise this patch looks good but I'm concerned because we have two
kinds of bindings now. The DT one above uses "i2c-gpio,..." and this
ACPI one uses just "..." so the question is where did these come from?
Is there already some existing system out there with these bindings or
they are documented somewhere?

Ideally we would be able to just do:

	pdata->sda_is_open_drain =
		device_property_read_bool(dev, "i2c-gpio,sda-open-drain");

for any firmware description.

> +
>  static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
>  					   const char *con_id,
>  					   unsigned int index,
> @@ -375,6 +394,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
> @@ -491,10 +512,19 @@ static const struct of_device_id i2c_gpio_dt_ids[] = {
>  MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
>  #endif
>  
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id i2c_gpio_acpi_match[] = {
> +	{"LOON0005"}, /*LoongArch*/
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, i2c_gpio_acpi_match);
> +#endif
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
> +		.acpi_match_table = ACPI_PTR(i2c_gpio_acpi_match),
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= i2c_gpio_remove,
> -- 
> 2.31.1