| Message ID | cover.1664193316.git.zhoubinbin@loongson.cn |
|---|---|
| Headers | show |
| Series | i2c: ls2x: Add support for the Loongson-2K/LS7A I2C | expand |
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", ®)) > + 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.
On Mon, Sep 26, 2022, at 4:59 PM, Mika Westerberg wrote: >> +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"); >> +} > > 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? I'm fairly sure it's just a mistake and it should use the regular binding. As far as I understand, there are still other incompatible changes being made to the firmware on these machines, so it's just a matter of updating this part as well. Arnd