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@xxxxxxxxxxx>
Who is this and why this SoB in the chain?
> Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
...
> #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", ®))
> + 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
