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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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>

Who is this and why this SoB in the chain?

> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>

...

>  #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>

Seems you misinterpret ordering.

Besides that I don't see the needs of acpi.h. The header missed the
mod_devicetable.h (even without this patch), and your code needs property.h.

...

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

+1 to Mika's objection. Instead, make the common bindings and convert
the driver from OF to be agnostic.

...

>  MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
>  #endif
>  
> +#ifdef CONFIG_ACPI

Please, drop these ifdefferies (including OF one), it's more harmful
than useful.

> +#endif

...

>  		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
> +		.acpi_match_table = ACPI_PTR(i2c_gpio_acpi_match),

No ACPI_PTR(), and accordingly no of_match_ptr(). See above.

-- 
With Best Regards,
Andy Shevchenko