[V2,0/4] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C

Hi all:

This patch series adds support for the I2C module found on various
Loongson systems with the Loongson-2K SoC or the Loongson LS7A bridge chip.

For now, the I2C driver is suitable for DT-based or ACPI-based systems.

I have only tested the Loongson-3A5000+LS7A1000/LS7A2000 under LoongArch
architecture.

Thanks.

Changes since V1:
1. Remove the function of getting the static i2c bus number from ACPI "_UID";
2. Fix build warning from kernel test robot.

Binbin Zhou (4):
  i2c: gpio: Add support on ACPI-based system
  dt-bindings: i2c: add bindings for Loongson LS2X I2C
  i2c: Add driver for Loongson-2K/LS7A I2C controller
  LoongArch: Enable LS2X I2C in loongson3_defconfig

 .../bindings/i2c/loongson,ls2x-i2c.yaml       |  48 +++
 arch/loongarch/configs/loongson3_defconfig    |   1 +
 drivers/i2c/busses/Kconfig                    |   7 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-gpio.c                 |  30 ++
 drivers/i2c/busses/i2c-ls2x.c                 | 365 ++++++++++++++++++
 6 files changed, 452 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

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.

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", &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?

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