[PATCH V3 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller

[PATCH V3 0/5] i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller

[Binbin Zhou]

Hi all:

Sorry, it may have been a while since the V2 patchset.

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 tested on Loongson-3A5000LA+LS7A1000/LS7A2000, Loongson-2K1000LA
and Loongson-2K0500.

Thanks.

Changes since V2:
- Addressed all review comments from v2
  - Drop of_match_ptr() in i2c-gpio to avoid potential unused warnings
    (1/5);
  - Introduce i2c_gpio_get_props() function as the generic interface
    to get i2c-gpio props from DT or ACPI table (2/5);
  - Refact ls2x i2c code, similar to removing excessive goto tags (4/5).

Thanks to Andy and Mika for their suggestions.

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

Binbin Zhou (5):
  i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'
  i2c: gpio: Add support on ACPI-based system
  dt-bindings: i2c: add bindings for Loongson LS2X I2C
  i2c: ls2x: 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                    |  11 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-gpio.c                 |  30 +-
 drivers/i2c/busses/i2c-ls2x.c                 | 415 ++++++++++++++++++
 6 files changed, 494 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

-- 
2.31.1

[PATCH V3 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'

[Binbin Zhou]

of_match_ptr() compiles into NULL if CONFIG_OF is disabled.
Fix warning by dropping of_match_ptr().

Suggested by Andy Shevchenko, thanks.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/i2c-gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index b1985c1667e1..0e4385a9bcf7 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -482,19 +482,17 @@ static int i2c_gpio_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#if defined(CONFIG_OF)
 static const struct of_device_id i2c_gpio_dt_ids[] = {
 	{ .compatible = "i2c-gpio", },
 	{ /* sentinel */ }
 };
 
 MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
-#endif
 
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
-		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
+		.of_match_table	= i2c_gpio_dt_ids,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= i2c_gpio_remove,
-- 
2.31.1

Re: [PATCH V3 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'

[Andy Shevchenko]

On Fri, Nov 25, 2022 at 04:54:11PM +0800, Binbin Zhou wrote:
> of_match_ptr() compiles into NULL if CONFIG_OF is disabled.
> Fix warning by dropping of_match_ptr().

> Suggested by Andy Shevchenko, thanks.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>

We have a tag for that:

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 1/5] i2c: gpio: Fix potential unused warning for 'i2c_gpio_dt_ids'

[Arnd Bergmann]

On Fri, Nov 25, 2022, at 09:54, Binbin Zhou wrote:
> of_match_ptr() compiles into NULL if CONFIG_OF is disabled.
> Fix warning by dropping of_match_ptr().
>
> Suggested by Andy Shevchenko, thanks.
>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>

Acked-by: Arnd Bergmann <arnd@arndb.de>

I think this is a useful cleanup in general, though I don't think
there is an actual warning in this particular driver that gets fixed
by the patch, so it would be good to change the changelog text to
describe this better: dropping a matching #ifdef check along with
dropping of_match_ptr() is just a cleanup, while dropping
of_match_ptr() that has no corresponding #ifdef fixes an actual
warning.

[PATCH V3 2/5] i2c: gpio: Add support on ACPI-based system

[Binbin Zhou]

Add support for the ACPI-based device registration, so that the driver
can be also enabled through ACPI table.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/i2c-gpio.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 0e4385a9bcf7..652d1f39854e 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -300,22 +300,23 @@ static inline void i2c_gpio_fault_injector_init(struct platform_device *pdev) {}
 static inline void i2c_gpio_fault_injector_exit(struct platform_device *pdev) {}
 #endif /* CONFIG_I2C_GPIO_FAULT_INJECTOR*/
 
-static void of_i2c_gpio_get_props(struct device_node *np,
-				  struct i2c_gpio_platform_data *pdata)
+/* get i2c-gpio props from DT or ACPI table */
+static void i2c_gpio_get_props(struct device *dev,
+				struct i2c_gpio_platform_data *pdata)
 {
 	u32 reg;
 
-	of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay);
+	device_property_read_u32(dev, "i2c-gpio,delay-us", &pdata->udelay);
 
-	if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", &reg))
+	if (!device_property_read_u32(dev, "i2c-gpio,timeout-ms", &reg))
 		pdata->timeout = msecs_to_jiffies(reg);
 
 	pdata->sda_is_open_drain =
-		of_property_read_bool(np, "i2c-gpio,sda-open-drain");
+		device_property_read_bool(dev, "i2c-gpio,sda-open-drain");
 	pdata->scl_is_open_drain =
-		of_property_read_bool(np, "i2c-gpio,scl-open-drain");
+		device_property_read_bool(dev, "i2c-gpio,scl-open-drain");
 	pdata->scl_is_output_only =
-		of_property_read_bool(np, "i2c-gpio,scl-output-only");
+		device_property_read_bool(dev, "i2c-gpio,scl-output-only");
 }
 
 static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
@@ -373,8 +374,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 	bit_data = &priv->bit_data;
 	pdata = &priv->pdata;
 
-	if (np) {
-		of_i2c_gpio_get_props(np, pdata);
+	if (np || has_acpi_companion(dev)) {
+		i2c_gpio_get_props(dev, pdata);
 	} else {
 		/*
 		 * If all platform data settings are zero it is OK
@@ -489,10 +490,17 @@ static const struct of_device_id i2c_gpio_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
 
+static const struct acpi_device_id i2c_gpio_acpi_match[] = {
+	{"LOON0005", 0}, /*LoongArch*/
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, i2c_gpio_acpi_match);
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.of_match_table	= i2c_gpio_dt_ids,
+		.acpi_match_table = i2c_gpio_acpi_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= i2c_gpio_remove,
-- 
2.31.1

Re: [PATCH V3 2/5] i2c: gpio: Add support on ACPI-based system

[Andy Shevchenko]

On Fri, Nov 25, 2022 at 04:54:12PM +0800, Binbin Zhou wrote:
> Add support for the ACPI-based device registration, so that the driver
> can be also enabled through ACPI table.

...

> +/* get i2c-gpio props from DT or ACPI table */

Get

...

> -	if (np) {
> -		of_i2c_gpio_get_props(np, pdata);
> +	if (np || has_acpi_companion(dev)) {
> +		i2c_gpio_get_props(dev, pdata);

	if (dev_fwnode() {
		i2c_gpio_get_props(dev, pdata);

...

> +	{"LOON0005", 0}, /*LoongArch*/

", 0" part is redundant. Also missing spaces in the comment.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 2/5] i2c: gpio: Add support on ACPI-based system

[Binbin Zhou]

在 2022/11/25 18:23, Andy Shevchenko 写道:
> On Fri, Nov 25, 2022 at 04:54:12PM +0800, Binbin Zhou wrote:
>> Add support for the ACPI-based device registration, so that the driver
>> can be also enabled through ACPI table.
> ...
>
>> +/* get i2c-gpio props from DT or ACPI table */
> Get
>
> ...
>
>> -	if (np) {
>> -		of_i2c_gpio_get_props(np, pdata);
>> +	if (np || has_acpi_companion(dev)) {
>> +		i2c_gpio_get_props(dev, pdata);
> 	if (dev_fwnode() {
> 		i2c_gpio_get_props(dev, pdata);
>
> ...
>
>> +	{"LOON0005", 0}, /*LoongArch*/
> ", 0" part is redundant. Also missing spaces in the comment.
>
Hi Andy:

Thanks for your review. I will fix them in the V4 patchset.

Binbin

[PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Binbin Zhou]

Add device tree bindings for the i2c controller on the Loongson-2K Soc
or Loongosn LS7A bridge.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/i2c/loongson,ls2x-i2c.yaml       | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
new file mode 100644
index 000000000000..8c785f329d2f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/i2c/loongson,ls2x-i2c.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Loongson LS2X I2C Controller
+
+maintainers:
+  - Binbin Zhou <zhoubinbin@loongson.cn>
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - loongson,ls2k-i2c # Loongson-2K SoCs
+      - loongson,ls7a-i2c # Loongson LS7A Bridge
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@1fe21000 {
+        compatible = "loongson,ls2k-i2c";
+        reg = <0 0x1fe21000 0 0x8>;
+        interrupts = <22>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        eeprom@57{
+            compatible = "atmel,24c16";
+            reg = <0x57>;
+            pagesize = <16>;
+        };
+    };
-- 
2.31.1

Re: [PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Krzysztof Kozlowski]

On 25/11/2022 09:54, Binbin Zhou wrote:
> Add device tree bindings for the i2c controller on the Loongson-2K Soc
> or Loongosn LS7A bridge.

It's a v3 which is for the first time sent to DT maintainers...

Subject: drop second, redundant "bindings for".

> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../bindings/i2c/loongson,ls2x-i2c.yaml       | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> new file mode 100644
> index 000000000000..8c785f329d2f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/i2c/loongson,ls2x-i2c.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes form both.

> +
> +title: Loongson LS2X I2C Controller
> +
> +maintainers:
> +  - Binbin Zhou <zhoubinbin@loongson.cn>
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - loongson,ls2k-i2c # Loongson-2K SoCs
> +      - loongson,ls7a-i2c # Loongson LS7A Bridge

Isn't your comment exactly the same as compatible? Where is the
difference? I propose to drop the comment entirely, unless it explains
something.

> +
> +  reg:
> +    maxItems: 1
> +

No clocks? I2C controller without clocks? Are you sure the binding is
complete?

> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +

Best regards,
Krzysztof

Re: [PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Binbin Zhou]

Hi Krzysztof:

在 2022/11/28 04:49, Krzysztof Kozlowski 写道:
> On 25/11/2022 09:54, Binbin Zhou wrote:
>> Add device tree bindings for the i2c controller on the Loongson-2K Soc
>> or Loongosn LS7A bridge.
> It's a v3 which is for the first time sent to DT maintainers...
Sorry, it was my mistake, I didn't double check the mail recipients in 
my .git/config.
>
> Subject: drop second, redundant "bindings for".

Ok. I get it.


>
>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
>> ---
>>   .../bindings/i2c/loongson,ls2x-i2c.yaml       | 48 +++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>> new file mode 100644
>> index 000000000000..8c785f329d2f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>> @@ -0,0 +1,48 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/i2c/loongson,ls2x-i2c.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> Drop quotes form both.
>
>> +
>> +title: Loongson LS2X I2C Controller
>> +
>> +maintainers:
>> +  - Binbin Zhou <zhoubinbin@loongson.cn>
>> +
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - loongson,ls2k-i2c # Loongson-2K SoCs
>> +      - loongson,ls7a-i2c # Loongson LS7A Bridge
> Isn't your comment exactly the same as compatible? Where is the
> difference? I propose to drop the comment entirely, unless it explains
> something.

OK, I will drop the useless comment.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
> No clocks? I2C controller without clocks? Are you sure the binding is
> complete?

We previously set the default CLOCK in the driver. Of course, we also 
provide the path to read the clock-frequency field for redo. In any 
case, I will add the clock-frequency field to the V4 patchset.

Thanks for your review.

Binbin


>
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
> Best regards,
> Krzysztof

Re: [PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Krzysztof Kozlowski]

On 28/11/2022 13:24, Binbin Zhou wrote:

> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>> No clocks? I2C controller without clocks? Are you sure the binding is
>> complete?
> 
> We previously set the default CLOCK in the driver. Of course, we also 
> provide the path to read the clock-frequency field for redo. In any 
> case, I will add the clock-frequency field to the V4 patchset.

I am not thinking here about the driver. What your bindings said, is
that device does not have any clocks and I have doubts about it...

I also do not say anything about clock-frequency because it is already
there via i2c-controller.yaml.

Best regards,
Krzysztof

Re: [PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Krzysztof Kozlowski]

On 28/11/2022 13:24, Binbin Zhou wrote:
> Hi Krzysztof:
> 
> 在 2022/11/28 04:49, Krzysztof Kozlowski 写道:
>> On 25/11/2022 09:54, Binbin Zhou wrote:
>>> Add device tree bindings for the i2c controller on the Loongson-2K Soc
>>> or Loongosn LS7A bridge.
>> It's a v3 which is for the first time sent to DT maintainers...
> Sorry, it was my mistake, I didn't double check the mail recipients in 
> my .git/config.
>>
>> Subject: drop second, redundant "bindings for".
> 
> Ok. I get it.

Actually, sending bindings and patches for same devices is a waste of
everyone's time:
https://lore.kernel.org/all/20221117075938.23379-2-zhuyinbo@loongson.cn/

Get your upstream process synchronized. Perform reviews on each other
patches, check mailing lists.

Best regards,
Krzysztof

Re: [PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Binbin Zhou]

在 2022/11/28 22:15, Krzysztof Kozlowski 写道:
> On 28/11/2022 13:24, Binbin Zhou wrote:
>> Hi Krzysztof:
>>
>> 在 2022/11/28 04:49, Krzysztof Kozlowski 写道:
>>> On 25/11/2022 09:54, Binbin Zhou wrote:
>>>> Add device tree bindings for the i2c controller on the Loongson-2K Soc
>>>> or Loongosn LS7A bridge.
>>> It's a v3 which is for the first time sent to DT maintainers...
>> Sorry, it was my mistake, I didn't double check the mail recipients in
>> my .git/config.
>>> Subject: drop second, redundant "bindings for".
>> Ok. I get it.
> Actually, sending bindings and patches for same devices is a waste of
> everyone's time:
> https://lore.kernel.org/all/20221117075938.23379-2-zhuyinbo@loongson.cn/
>
> Get your upstream process synchronized. Perform reviews on each other
> patches, check mailing lists.

Hi:

Through communication with Yinbo offline, I will continue the series of 
patch set submissions.

There is his reply:

https://lore.kernel.org/all/2e10ae64-3c91-ccf8-a970-eb6e3371b948@loongson.cn/

Thanks.

Binbin

>
> Best regards,
> Krzysztof

Re: [PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Krzysztof Kozlowski]

On 25/11/2022 09:54, Binbin Zhou wrote:
> Add device tree bindings for the i2c controller on the Loongson-2K Soc
> or Loongosn LS7A bridge.
> 

...


> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c@1fe21000 {
> +        compatible = "loongson,ls2k-i2c";
> +        reg = <0 0x1fe21000 0 0x8>;

Plus what the Rob's robot told you - code testing examples uses 1 for
address/size cells.

Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

Best regards,
Krzysztof

[PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

This I2C module is integrated into the Loongson-2K SoCs and Loongson
LS7A bridge chip.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/Kconfig    |  11 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ls2x.c | 415 ++++++++++++++++++++++++++++++++++
 3 files changed, 427 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e50f9603d189..0e342cd5b079 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -888,6 +888,17 @@ config I2C_OWL
 	  Say Y here if you want to use the I2C bus controller on
 	  the Actions Semiconductor Owl SoC's.
 
+config I2C_LS2X
+	tristate "Loongson LS2X I2C adapter"
+	depends on MACH_LOONGSON64 || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge
+	  chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-ls2x.
+
 config I2C_PASEMI
 	tristate "PA Semi SMBus interface"
 	depends on PPC_PASEMI && PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e73cdb1d2b5a..df332ec3c489 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
 obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
+obj-$(CONFIG_I2C_LS2X)		+= i2c-ls2x.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
new file mode 100644
index 000000000000..362dfc3f9103
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -0,0 +1,414 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Loongson-2K/Loongson LS7A I2C master mode driver
+ *
+ * Copyright (C) 2013 Loongson Technology Corporation Limited.
+ * Copyright (C) 2014-2017 Lemote, Inc.
+ * Copyright (C) 2018-2022 Loongson Technology Corporation Limited.
+ *
+ * Originally written by liushaozong
+ *
+ * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn>
+ */
+
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+/* I2C Registers */
+#define I2C_LS2X_PRER_LO	0x0 /* Freq Division Low Byte Register */
+#define I2C_LS2X_PRER_HI	0x1 /* Freq Division High Byte Register */
+#define I2C_LS2X_CTR		0x2 /* Control Register */
+#define I2C_LS2X_TXR		0x3 /* Transport Data Register */
+#define I2C_LS2X_RXR		0x3 /* Receive Data Register */
+#define I2C_LS2X_CR		0x4 /* Command Control Register */
+#define I2C_LS2X_SR		0x4 /* State Register */
+
+/* Command Control Register Bit */
+#define LS2X_CR_START		BIT(7)
+#define LS2X_CR_STOP		BIT(6)
+#define LS2X_CR_READ		BIT(5)
+#define LS2X_CR_WRITE		BIT(4)
+#define LS2X_CR_ACK		BIT(3)
+#define LS2X_CR_IACK		BIT(0)
+
+/* State Register Bit */
+#define LS2X_SR_NOACK		BIT(7)
+#define LS2X_SR_BUSY		BIT(6)
+#define LS2X_SR_AL		BIT(5)
+#define LS2X_SR_TIP		BIT(1)
+#define LS2X_SR_IF		BIT(0)
+
+#define LS2X_I2C_MAX_RETRIES	5
+
+/* LS2X I2C clock frequency 50M */
+#define HZ_PER_MHZ		(50 * 1000000)
+
+struct ls2x_i2c_dev {
+	struct device		*dev;
+	void __iomem		*base;
+	int			irq;
+	u32			bus_clk_rate;
+	struct completion	cmd_complete;
+	struct i2c_adapter	adapter;
+	unsigned int		suspended:1;
+};
+
+static int ls2x_i2c_xfer_byte(struct i2c_adapter *adap, u8 txdata,
+				  u8 *rxdatap)
+{
+	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
+	unsigned long time_left;
+	u8 rxdata;
+
+	writeb(txdata, dev->base + I2C_LS2X_CR);
+
+	time_left = wait_for_completion_timeout(&dev->cmd_complete,
+						adap->timeout);
+	if (unlikely(!time_left)) {
+		dev_err(&adap->dev, "transaction timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	rxdata = readb(dev->base + I2C_LS2X_SR);
+	if (rxdatap)
+		*rxdatap = rxdata;
+
+	return 0;
+}
+
+static int ls2x_i2c_send_byte(struct i2c_adapter *adap, u8 txdata)
+{
+	int ret;
+	u8 rxdata;
+
+	ret = ls2x_i2c_xfer_byte(adap, txdata, &rxdata);
+	if (ret)
+		return ret;
+
+	if (unlikely(rxdata & LS2X_SR_NOACK))
+		return -ENXIO;
+
+	return 0;
+}
+
+static int ls2x_i2c_stop(struct i2c_adapter *adap)
+{
+	return ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
+}
+
+static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
+{
+	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
+	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
+
+	reinit_completion(&dev->cmd_complete);
+
+	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
+	writeb(addr, dev->base + I2C_LS2X_TXR);
+
+	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
+}
+
+static int ls2x_i2c_rx(struct i2c_adapter *adap, u8 *buf, u16 len)
+{
+	unsigned long time_left;
+	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
+	int cmd = LS2X_CR_READ;
+
+	while (len--) {
+		if (len == 0)
+			cmd |= LS2X_CR_ACK;
+
+		writeb(cmd, dev->base + I2C_LS2X_CR);
+		time_left = wait_for_completion_timeout(&dev->cmd_complete,
+							adap->timeout);
+		if (unlikely(!time_left)) {
+			dev_err(dev->dev, "transaction timeout\n");
+			return -ETIMEDOUT;
+		}
+
+		*buf++ = readb(dev->base + I2C_LS2X_RXR);
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_tx(struct i2c_adapter *adap, u8 *buf, u16 len)
+{
+	int ret;
+	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	while (len--) {
+		writeb(*buf++, dev->base + I2C_LS2X_TXR);
+
+		ret = ls2x_i2c_send_byte(adap, LS2X_CR_WRITE);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_xfer_one(struct i2c_adapter *adap,
+			struct i2c_msg *msg, bool stop)
+{
+	int ret, ret2;
+	bool is_read = msg->flags & I2C_M_RD;
+
+	ret = ls2x_i2c_start(adap, msg);
+	if (ret)
+		return ret;
+
+	if (is_read)
+		ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
+	else
+		ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
+
+	/* could not acquire bus. bail out without STOP */
+	if (ret == -EAGAIN)
+		return ret;
+
+	if (stop)
+		ret2 = ls2x_i2c_stop(adap);
+
+	return ret;
+}
+
+static int ls2x_i2c_doxfer(struct i2c_adapter *adap,
+			struct i2c_msg *msgs, int num)
+{
+	int ret;
+	struct i2c_msg *msg, *emsg = msgs + num;
+
+	for (msg = msgs; msg < emsg; msg++) {
+		/* Emit STOP if it is the last message or I2C_M_STOP is set. */
+		bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP);
+
+		ret = ls2x_i2c_xfer_one(adap, msg, stop);
+		if (ret)
+			return ret;
+	}
+
+	return num;
+}
+
+static int ls2x_i2c_xfer(struct i2c_adapter *adap,
+			struct i2c_msg *msgs, int num)
+{
+	int ret, retry;
+	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	for (retry = 0; retry < adap->retries; retry++) {
+
+		ret = ls2x_i2c_doxfer(adap, msgs, num);
+		if (ret != -EAGAIN)
+			return ret;
+
+		dev_dbg(dev->dev, "Retrying transmission (%d)\n", retry);
+		udelay(100);
+	}
+
+	return -EREMOTEIO;
+}
+
+static unsigned int ls2x_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ls2x_i2c_algo = {
+	.master_xfer	= ls2x_i2c_xfer,
+	.functionality	= ls2x_i2c_func,
+};
+
+/*
+ * Interrupt service routine.
+ * This gets called whenever an I2C interrupt occurs.
+ */
+static irqreturn_t ls2x_i2c_isr(int this_irq, void *dev_id)
+{
+	unsigned char iflag;
+	struct ls2x_i2c_dev *dev = dev_id;
+
+	iflag = readb(dev->base + I2C_LS2X_SR);
+
+	if (iflag & LS2X_SR_IF) {
+		writeb(LS2X_CR_IACK, dev->base + I2C_LS2X_CR);
+		complete(&dev->cmd_complete);
+	} else
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static void ls2x_i2c_reginit(struct ls2x_i2c_dev *dev)
+{
+	u8 data;
+	/* default value of I2C divider latch register */
+	u16 val = 0x12c;
+
+	/* Enable operations about frequency divider register */
+	data = readb(dev->base + I2C_LS2X_CTR);
+	writeb(data & ~0x80, dev->base + I2C_LS2X_CTR);
+
+	if (dev->bus_clk_rate)
+		val = HZ_PER_MHZ / (5 * dev->bus_clk_rate) - 1;
+
+	/* Set LS2x I2C frequency */
+	writeb(val, dev->base + I2C_LS2X_PRER_LO);
+	writeb((val & 0xff00) >> 8, dev->base + I2C_LS2X_PRER_HI);
+
+	/* Set to normal I2C operating mode and enable interrupts */
+	data = readb(dev->base + I2C_LS2X_CTR);
+	writeb(data | 0xe0, dev->base + I2C_LS2X_CTR);
+}
+
+static int ls2x_i2c_probe(struct platform_device *pdev)
+{
+	int r;
+	struct ls2x_i2c_dev *dev;
+	struct i2c_adapter *adap;
+
+	dev = devm_kzalloc(&pdev->dev,
+			sizeof(struct ls2x_i2c_dev), GFP_KERNEL);
+	if (unlikely(!dev))
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, dev);
+	init_completion(&dev->cmd_complete);
+	dev->dev = &pdev->dev;
+
+	/* Map hardware registers */
+	dev->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dev->base))
+		return PTR_ERR(dev->base);
+
+	dev->irq = platform_get_irq(pdev, 0);
+	if (unlikely(dev->irq <= 0))
+		return -ENODEV;
+
+	r = devm_request_irq(&pdev->dev, dev->irq, ls2x_i2c_isr,
+			      IRQF_SHARED, "ls2x-i2c", dev);
+	if (unlikely(r)) {
+		dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
+		return r;
+	}
+
+	/*
+	 * The I2C controller has a fixed I2C bus frequency by default, but to
+	 * be compatible with more client devices, we can obtain the set I2C
+	 * bus frequency from ACPI or FDT.
+	 */
+	dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);
+	if (!dev->bus_clk_rate)
+		device_property_read_u32(&pdev->dev, "clock-frequency",
+					&dev->bus_clk_rate);
+
+	ls2x_i2c_reginit(dev);
+
+	/* Add the i2c adapter */
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	adap->nr = pdev->id;
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	adap->retries = LS2X_I2C_MAX_RETRIES;
+	adap->algo = &ls2x_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
+
+	/* i2c device drivers may be active on return from add_adapter() */
+	r = i2c_add_adapter(adap);
+	if (r) {
+		dev_err(dev->dev, "failure adding adapter\n");
+		return r;
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_remove(struct platform_device *pdev)
+{
+	struct ls2x_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+	return 0;
+}
+
+static int __maybe_unused ls2x_i2c_suspend_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_dev->suspended = 1;
+
+	return 0;
+}
+
+static int __maybe_unused ls2x_i2c_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_dev->suspended = 0;
+	ls2x_i2c_reginit(i2c_dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops ls2x_i2c_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend_noirq, ls2x_i2c_resume)
+};
+
+static const struct of_device_id ls2x_i2c_id_table[] = {
+	{ .compatible = "loongson,ls2k-i2c" },
+	{ .compatible = "loongson,ls7a-i2c" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ls2x_i2c_id_table);
+
+static const struct acpi_device_id ls2x_i2c_acpi_match[] = {
+	{ "LOON0004", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, ls2x_i2c_acpi_match);
+
+static struct platform_driver ls2x_i2c_driver = {
+	.probe		= ls2x_i2c_probe,
+	.remove		= ls2x_i2c_remove,
+	.driver		= {
+		.name	= "ls2x-i2c",
+		.owner	= THIS_MODULE,
+		.pm	= &ls2x_i2c_dev_pm_ops,
+		.of_match_table = ls2x_i2c_id_table,
+		.acpi_match_table = ls2x_i2c_acpi_match,
+	},
+};
+
+static int __init ls2x_i2c_init_driver(void)
+{
+	return platform_driver_register(&ls2x_i2c_driver);
+}
+subsys_initcall(ls2x_i2c_init_driver);
+
+static void __exit ls2x_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&ls2x_i2c_driver);
+}
+module_exit(ls2x_i2c_exit_driver);
+
+MODULE_DESCRIPTION("Loongson LS2X I2C Bus driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ls2x-i2c");
-- 
2.31.1

Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko]

On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.

...

Missing bits.h.

> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

> +#include <linux/of_device.h>

There is no user of this header.
Why?

> +#include <linux/platform_device.h>

...

> +/* LS2X I2C clock frequency 50M */
> +#define HZ_PER_MHZ		(50 * 1000000)

units.h ?

...

> +struct ls2x_i2c_dev {
> +	struct device		*dev;
> +	void __iomem		*base;
> +	int			irq;
> +	u32			bus_clk_rate;
> +	struct completion	cmd_complete;

> +	struct i2c_adapter	adapter;

Check if moving this to be the first field makes code generation better
(bloat-o-meter is your friend).

> +	unsigned int		suspended:1;
> +};

> +	return ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
> +}

...

> +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
> +{
> +	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
> +	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
> +
> +	reinit_completion(&dev->cmd_complete);

> +	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;

Why is this needed?

> +	writeb(addr, dev->base + I2C_LS2X_TXR);
> +
> +	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
> +}

...

> +	while (len--) {

> +		if (len == 0)
> +			cmd |= LS2X_CR_ACK;
> +
> +		writeb(cmd, dev->base + I2C_LS2X_CR);

Can be written as

		writeb(cmd | (len ? 0 : LS2X_CR_ACK), dev->base + I2C_LS2X_CR);

but it's up to you.

> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
> +							adap->timeout);
> +		if (unlikely(!time_left)) {
> +			dev_err(dev->dev, "transaction timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		*buf++ = readb(dev->base + I2C_LS2X_RXR);
> +	}

...

> +	for (retry = 0; retry < adap->retries; retry++) {

> +

Unneeded blank line.

> +		ret = ls2x_i2c_doxfer(adap, msgs, num);
> +		if (ret != -EAGAIN)
> +			return ret;
> +
> +		dev_dbg(dev->dev, "Retrying transmission (%d)\n", retry);
> +		udelay(100);
> +	}

Can something from iopoll.h be utilized here?

...

> +	if (iflag & LS2X_SR_IF) {
> +		writeb(LS2X_CR_IACK, dev->base + I2C_LS2X_CR);
> +		complete(&dev->cmd_complete);
> +	} else
> +		return IRQ_NONE;


Use usual pattern: checking for error condition first.

	if (!(iflag & LS2X_SR_IF))
		return IRQ_NONE;

	writeb(LS2X_CR_IACK, dev->base + I2C_LS2X_CR);
	complete(&dev->cmd_complete);

> +	return IRQ_HANDLED;

...

> +	writeb((val & 0xff00) >> 8, dev->base + I2C_LS2X_PRER_HI);


What ' & 0xff00' part is for?

...

> +	dev = devm_kzalloc(&pdev->dev,
> +			sizeof(struct ls2x_i2c_dev), GFP_KERNEL);

sizeof(*dev) and make it one line.

> +	if (unlikely(!dev))

Why unlikely()?

> +		return -ENOMEM;

...

> +	dev->irq = platform_get_irq(pdev, 0);
> +	if (unlikely(dev->irq <= 0))

Why 'unlikely()'? Why == 0 is here?

> +		return -ENODEV;

...

> +	r = devm_request_irq(&pdev->dev, dev->irq, ls2x_i2c_isr,
> +			      IRQF_SHARED, "ls2x-i2c", dev);

> +	if (unlikely(r)) {

Why 'unlikely()'? You must explain all likely() / unlikely() use in the code.

> +		dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
> +		return r;

	return dev_err_probe(...);

> +	}

...

> +	/*
> +	 * The I2C controller has a fixed I2C bus frequency by default, but to
> +	 * be compatible with more client devices, we can obtain the set I2C
> +	 * bus frequency from ACPI or FDT.
> +	 */
> +	dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);

> +	if (!dev->bus_clk_rate)
> +		device_property_read_u32(&pdev->dev, "clock-frequency",
> +					&dev->bus_clk_rate);

This should be done via

        i2c_parse_fw_timings(&pdev->dev, ...);

no?

...

> +	adap->dev.of_node = pdev->dev.of_node;
> +	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));

device_set_node()

...

> +	/* i2c device drivers may be active on return from add_adapter() */
> +	r = i2c_add_adapter(adap);
> +	if (r) {
> +		dev_err(dev->dev, "failure adding adapter\n");
> +		return r;

	return dev_err_probe(...);

> +	}

...

> +static int __maybe_unused ls2x_i2c_suspend_noirq(struct device *dev)

No __maybe_unused, use proper PM macros and definitions.
(look for pm_ptr() / pm_sleep_ptr() and corresponding defines)

> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +	i2c_dev->suspended = 1;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ls2x_i2c_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +	i2c_dev->suspended = 0;
> +	ls2x_i2c_reginit(i2c_dev);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ls2x_i2c_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend_noirq, ls2x_i2c_resume)
> +};

As per above.

...

> +static const struct of_device_id ls2x_i2c_id_table[] = {
> +	{ .compatible = "loongson,ls2k-i2c" },
> +	{ .compatible = "loongson,ls7a-i2c" },
> +	{ /* sentinel */ },

No comma for terminator entry.

> +};

...

> +	{ "LOON0004", 0 },

', 0' is redundant.

...

> +static struct platform_driver ls2x_i2c_driver = {
> +	.probe		= ls2x_i2c_probe,
> +	.remove		= ls2x_i2c_remove,
> +	.driver		= {
> +		.name	= "ls2x-i2c",

> +		.owner	= THIS_MODULE,

Why?

> +		.pm	= &ls2x_i2c_dev_pm_ops,
> +		.of_match_table = ls2x_i2c_id_table,
> +		.acpi_match_table = ls2x_i2c_acpi_match,
> +	},
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

Hi Andy:

Firstly, thanks for your careful review.

在 2022/11/25 18:41, Andy Shevchenko 写道:
> On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:
>> This I2C module is integrated into the Loongson-2K SoCs and Loongson
>> LS7A bridge chip.
> ...
>
> Missing bits.h.

Is it needed? I found it already included in I2c.h.

>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
> There is no user of this header.
> Why?
>
>> +#include <linux/platform_device.h>
> ...
>
>> +/* LS2X I2C clock frequency 50M */
>> +#define HZ_PER_MHZ		(50 * 1000000)
> units.h ?
I misunderstood your previous comment, and  the HZ_PER_MHZ in units.h 
will be used.
>
> ...
>
>> +struct ls2x_i2c_dev {
>> +	struct device		*dev;
>> +	void __iomem		*base;
>> +	int			irq;
>> +	u32			bus_clk_rate;
>> +	struct completion	cmd_complete;
>> +	struct i2c_adapter	adapter;
> Check if moving this to be the first field makes code generation better
> (bloat-o-meter is your friend).

vmlinux.old: original order

vmlinux:  adapter to be the first field

[zhoubinbin@kernelserver github]$ scripts/bloat-o-meter vmlinux.old vmlinux
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-8 (-8)
Function                                     old     new   delta
ls2x_i2c_remove                               36      32      -4
ls2x_i2c_probe                               424     420      -4

Total: Before=19302026, After=19302018, chg -0.00%


>
>> +	unsigned int		suspended:1;
>> +};
>> +	return ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
>> +}
> ...
>
>> +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
>> +{
>> +	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
>> +	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
>> +
>> +	reinit_completion(&dev->cmd_complete);
>> +	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
> Why is this needed?
In the ls2x I2C controller, the bit 0 of TXR indicates the read/write 
status when transferring the address.
>
>> +	writeb(addr, dev->base + I2C_LS2X_TXR);
>> +
>> +	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
>> +}
> ...
>
>> +	while (len--) {
>> +		if (len == 0)
>> +			cmd |= LS2X_CR_ACK;
>> +
>> +		writeb(cmd, dev->base + I2C_LS2X_CR);
> Can be written as
>
> 		writeb(cmd | (len ? 0 : LS2X_CR_ACK), dev->base + I2C_LS2X_CR);
>
> but it's up to you.
>
>> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
>> +							adap->timeout);
>> +		if (unlikely(!time_left)) {
>> +			dev_err(dev->dev, "transaction timeout\n");
>> +			return -ETIMEDOUT;
>> +		}
>> +
>> +		*buf++ = readb(dev->base + I2C_LS2X_RXR);
>> +	}
> ...
>
>> +	for (retry = 0; retry < adap->retries; retry++) {
>> +
> Unneeded blank line.
>
>> +		ret = ls2x_i2c_doxfer(adap, msgs, num);
>> +		if (ret != -EAGAIN)
>> +			return ret;
>> +
>> +		dev_dbg(dev->dev, "Retrying transmission (%d)\n", retry);
>> +		udelay(100);
>> +	}
> Can something from iopoll.h be utilized here?
I think udelay() should be suitable because it is just the time interval 
between two retry.
>
> ...
>
>> +	if (iflag & LS2X_SR_IF) {
>> +		writeb(LS2X_CR_IACK, dev->base + I2C_LS2X_CR);
>> +		complete(&dev->cmd_complete);
>> +	} else
>> +		return IRQ_NONE;
>
> Use usual pattern: checking for error condition first.
>
> 	if (!(iflag & LS2X_SR_IF))
> 		return IRQ_NONE;
>
> 	writeb(LS2X_CR_IACK, dev->base + I2C_LS2X_CR);
> 	complete(&dev->cmd_complete);
>
>> +	return IRQ_HANDLED;
> ...
>
>> +	writeb((val & 0xff00) >> 8, dev->base + I2C_LS2X_PRER_HI);
>
> What ' & 0xff00' part is for?
Emm... I'll use  writel(val, priv->base + I2C_LS2X_PRER_LO); instead.
> ...
>
>> +	dev = devm_kzalloc(&pdev->dev,
>> +			sizeof(struct ls2x_i2c_dev), GFP_KERNEL);
> sizeof(*dev) and make it one line.
>
>> +	if (unlikely(!dev))
> Why unlikely()?
>
>> +		return -ENOMEM;
> ...
>
>> +	dev->irq = platform_get_irq(pdev, 0);
>> +	if (unlikely(dev->irq <= 0))
> Why 'unlikely()'? Why == 0 is here?
>
>> +		return -ENODEV;
> ...
>
>> +	r = devm_request_irq(&pdev->dev, dev->irq, ls2x_i2c_isr,
>> +			      IRQF_SHARED, "ls2x-i2c", dev);
>> +	if (unlikely(r)) {
> Why 'unlikely()'? You must explain all likely() / unlikely() use in the code.
These 'unlikely()' may not be quite right, at that time I just thought 
these anomalies were infrequent.
>
>> +		dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
>> +		return r;
> 	return dev_err_probe(...);
>
>> +	}
> ...
>
>> +	/*
>> +	 * The I2C controller has a fixed I2C bus frequency by default, but to
>> +	 * be compatible with more client devices, we can obtain the set I2C
>> +	 * bus frequency from ACPI or FDT.
>> +	 */
>> +	dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);
>> +	if (!dev->bus_clk_rate)
>> +		device_property_read_u32(&pdev->dev, "clock-frequency",
>> +					&dev->bus_clk_rate);
> This should be done via
>
>          i2c_parse_fw_timings(&pdev->dev, ...);
>
> no?

Yes, I get it，and the i2c_ls2x_adjust_bus_speed() function will be 
introduced to calculate i2c bus_freq_hz.

>
> ...
>
>> +	adap->dev.of_node = pdev->dev.of_node;
>> +	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> device_set_node()
>
> ...
>
>> +	/* i2c device drivers may be active on return from add_adapter() */
>> +	r = i2c_add_adapter(adap);
>> +	if (r) {
>> +		dev_err(dev->dev, "failure adding adapter\n");
>> +		return r;
> 	return dev_err_probe(...);
>
>> +	}
> ...
>
>> +static int __maybe_unused ls2x_i2c_suspend_noirq(struct device *dev)
> No __maybe_unused, use proper PM macros and definitions.
> (look for pm_ptr() / pm_sleep_ptr() and corresponding defines)
>
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
>> +
>> +	i2c_dev->suspended = 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused ls2x_i2c_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ls2x_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
>> +
>> +	i2c_dev->suspended = 0;
>> +	ls2x_i2c_reginit(i2c_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops ls2x_i2c_dev_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(ls2x_i2c_suspend_noirq, ls2x_i2c_resume)
>> +};
> As per above.

The pm_sleep_ptr(&ls2x_i2c_dev_pm_ops) will be used in ls2x_i2c_driver 
and drop __maybe_unused.

Binbin

> ...
>
>> +static const struct of_device_id ls2x_i2c_id_table[] = {
>> +	{ .compatible = "loongson,ls2k-i2c" },
>> +	{ .compatible = "loongson,ls7a-i2c" },
>> +	{ /* sentinel */ },
> No comma for terminator entry.
>
>> +};
> ...
>
>> +	{ "LOON0004", 0 },
> ', 0' is redundant.
>
> ...
>
>> +static struct platform_driver ls2x_i2c_driver = {
>> +	.probe		= ls2x_i2c_probe,
>> +	.remove		= ls2x_i2c_remove,
>> +	.driver		= {
>> +		.name	= "ls2x-i2c",
>> +		.owner	= THIS_MODULE,
> Why?
>
>> +		.pm	= &ls2x_i2c_dev_pm_ops,
>> +		.of_match_table = ls2x_i2c_id_table,
>> +		.acpi_match_table = ls2x_i2c_acpi_match,
>> +	},
>> +};

Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 08:03:40PM +0800, Binbin Zhou wrote:
> 在 2022/11/25 18:41, Andy Shevchenko 写道:
> > On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:

...

> > Missing bits.h.
> 
> Is it needed? I found it already included in I2c.h.

The rule of thumb is to include headers we are the direct user of.
Exceptions are the headers that are guaranteed to be included by
others. i2c.h doesn't guarantee this and many other inclusions
while using them for itself or simply included them wrongly.

...

> > > +struct ls2x_i2c_dev {
> > > +	struct device		*dev;
> > > +	void __iomem		*base;
> > > +	int			irq;
> > > +	u32			bus_clk_rate;
> > > +	struct completion	cmd_complete;
> > > +	struct i2c_adapter	adapter;
> > Check if moving this to be the first field makes code generation better
> > (bloat-o-meter is your friend).
> 
> vmlinux.old: original order
> 
> vmlinux:  adapter to be the first field
> 
> [zhoubinbin@kernelserver github]$ scripts/bloat-o-meter vmlinux.old vmlinux
> add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-8 (-8)
> Function                                     old     new   delta
> ls2x_i2c_remove                               36      32      -4
> ls2x_i2c_probe                               424     420      -4
> 
> Total: Before=19302026, After=19302018, chg -0.00%

Good, up to you (personally I find usually better to have embedded structures,
which are parent objects in terms of OOP, to be first members).

> > > +	unsigned int		suspended:1;
> > > +};
> > > +	return ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
> > > +}

...

> > > +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
> > > +{
> > > +	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
> > > +	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
> > > +
> > > +	reinit_completion(&dev->cmd_complete);
> > > +	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
> > Why is this needed?
> In the ls2x I2C controller, the bit 0 of TXR indicates the read/write status
> when transferring the address.

Yes, I understand this. I don't understand why do you need this twice.

> > > +	writeb(addr, dev->base + I2C_LS2X_TXR);
> > > +
> > > +	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
> > > +}

...

> > > +	for (retry = 0; retry < adap->retries; retry++) {
> > > +
> > Unneeded blank line.
> > 
> > > +		ret = ls2x_i2c_doxfer(adap, msgs, num);
> > > +		if (ret != -EAGAIN)
> > > +			return ret;
> > > +
> > > +		dev_dbg(dev->dev, "Retrying transmission (%d)\n", retry);
> > > +		udelay(100);
> > > +	}
> > Can something from iopoll.h be utilized here?
> I think udelay() should be suitable because it is just the time interval
> between two retry.

Read again my comment. Also thanks for pointing out that this is atomic. Is
this really needs to be atomic?

...

> > > +	/*
> > > +	 * The I2C controller has a fixed I2C bus frequency by default, but to
> > > +	 * be compatible with more client devices, we can obtain the set I2C
> > > +	 * bus frequency from ACPI or FDT.
> > > +	 */
> > > +	dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);
> > > +	if (!dev->bus_clk_rate)
> > > +		device_property_read_u32(&pdev->dev, "clock-frequency",
> > > +					&dev->bus_clk_rate);
> > This should be done via

> >          i2c_parse_fw_timings(&pdev->dev, ...);

> > no?
> 
> Yes, I get it，and the i2c_ls2x_adjust_bus_speed() function will be
> introduced to calculate i2c bus_freq_hz.

Yep, depends on your needs. It might be that some of the drivers are using
the code that you may reuse (by moving to i2c-core-acpi.c).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

Hi Andy:

在 2022/11/28 21:24, Andy Shevchenko 写道:
> On Mon, Nov 28, 2022 at 08:03:40PM +0800, Binbin Zhou wrote:
>> 在 2022/11/25 18:41, Andy Shevchenko 写道:
>>> On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:
> ...
>
>>> Missing bits.h.
>> Is it needed? I found it already included in I2c.h.
> The rule of thumb is to include headers we are the direct user of.
> Exceptions are the headers that are guaranteed to be included by
> others. i2c.h doesn't guarantee this and many other inclusions
> while using them for itself or simply included them wrongly.
OK, I get it.
>
> ...
>
>>>> +struct ls2x_i2c_dev {
>>>> +	struct device		*dev;
>>>> +	void __iomem		*base;
>>>> +	int			irq;
>>>> +	u32			bus_clk_rate;
>>>> +	struct completion	cmd_complete;
>>>> +	struct i2c_adapter	adapter;
>>> Check if moving this to be the first field makes code generation better
>>> (bloat-o-meter is your friend).
>> vmlinux.old: original order
>>
>> vmlinux:  adapter to be the first field
>>
>> [zhoubinbin@kernelserver github]$ scripts/bloat-o-meter vmlinux.old vmlinux
>> add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-8 (-8)
>> Function                                     old     new   delta
>> ls2x_i2c_remove                               36      32      -4
>> ls2x_i2c_probe                               424     420      -4
>>
>> Total: Before=19302026, After=19302018, chg -0.00%
> Good, up to you (personally I find usually better to have embedded structures,
> which are parent objects in terms of OOP, to be first members).
>
>>>> +	unsigned int		suspended:1;
>>>> +};
>>>> +	return ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
>>>> +}
> ...
>
>>>> +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
>>>> +{
>>>> +	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
>>>> +	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
>>>> +
>>>> +	reinit_completion(&dev->cmd_complete);
>>>> +	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
>>> Why is this needed?
>> In the ls2x I2C controller, the bit 0 of TXR indicates the read/write status
>> when transferring the address.
> Yes, I understand this. I don't understand why do you need this twice.

Are you saying that the "is_read" variable in ls2x_i2c_xfer_one() 
already indicates the read/write state of data transfer?

I just didn't think it was necessary to take "is_read" as an argument to 
ls2x_i2c_start() at the time, since we could get it from "msg".

Thanks.

Binbin

>
>>>> +	writeb(addr, dev->base + I2C_LS2X_TXR);
>>>> +
>>>> +	return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
>>>> +}
> ...
>
>>>> +	for (retry = 0; retry < adap->retries; retry++) {
>>>> +
>>> Unneeded blank line.
>>>
>>>> +		ret = ls2x_i2c_doxfer(adap, msgs, num);
>>>> +		if (ret != -EAGAIN)
>>>> +			return ret;
>>>> +
>>>> +		dev_dbg(dev->dev, "Retrying transmission (%d)\n", retry);
>>>> +		udelay(100);
>>>> +	}
>>> Can something from iopoll.h be utilized here?
>> I think udelay() should be suitable because it is just the time interval
>> between two retry.
> Read again my comment. Also thanks for pointing out that this is atomic. Is
> this really needs to be atomic?
>
> ...
>
>>>> +	/*
>>>> +	 * The I2C controller has a fixed I2C bus frequency by default, but to
>>>> +	 * be compatible with more client devices, we can obtain the set I2C
>>>> +	 * bus frequency from ACPI or FDT.
>>>> +	 */
>>>> +	dev->bus_clk_rate = i2c_acpi_find_bus_speed(&pdev->dev);
>>>> +	if (!dev->bus_clk_rate)
>>>> +		device_property_read_u32(&pdev->dev, "clock-frequency",
>>>> +					&dev->bus_clk_rate);
>>> This should be done via
>>>           i2c_parse_fw_timings(&pdev->dev, ...);
>>> no?
>> Yes, I get it，and the i2c_ls2x_adjust_bus_speed() function will be
>> introduced to calculate i2c bus_freq_hz.
> Yep, depends on your needs. It might be that some of the drivers are using
> the code that you may reuse (by moving to i2c-core-acpi.c).
>

Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko]

On Tue, Nov 29, 2022 at 07:34:58PM +0800, Binbin Zhou wrote:
> 在 2022/11/28 21:24, Andy Shevchenko 写道:
> > On Mon, Nov 28, 2022 at 08:03:40PM +0800, Binbin Zhou wrote:
> > > 在 2022/11/25 18:41, Andy Shevchenko 写道:
> > > > On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:

...

> > > > > +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
> > > > > +{
> > > > > +	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
> > > > > +	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
> > > > > +
> > > > > +	reinit_completion(&dev->cmd_complete);
> > > > > +	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
> > > > Why is this needed?
> > > In the ls2x I2C controller, the bit 0 of TXR indicates the read/write status
> > > when transferring the address.
> > Yes, I understand this. I don't understand why do you need this twice.
> 
> Are you saying that the "is_read" variable in ls2x_i2c_xfer_one() already
> indicates the read/write state of data transfer?
> 
> I just didn't think it was necessary to take "is_read" as an argument to
> ls2x_i2c_start() at the time, since we could get it from "msg".

Have you checked what i2c_8bit_addr_from_msg() is doing?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 4/5] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Binbin Zhou]

在 2022/11/29 20:53, Andy Shevchenko 写道:
> On Tue, Nov 29, 2022 at 07:34:58PM +0800, Binbin Zhou wrote:
>> 在 2022/11/28 21:24, Andy Shevchenko 写道:
>>> On Mon, Nov 28, 2022 at 08:03:40PM +0800, Binbin Zhou wrote:
>>>> 在 2022/11/25 18:41, Andy Shevchenko 写道:
>>>>> On Fri, Nov 25, 2022 at 04:55:20PM +0800, Binbin Zhou wrote:
> ...
>
>>>>>> +static int ls2x_i2c_start(struct i2c_adapter *adap, struct i2c_msg *msgs)
>>>>>> +{
>>>>>> +	struct ls2x_i2c_dev *dev = i2c_get_adapdata(adap);
>>>>>> +	unsigned char addr = i2c_8bit_addr_from_msg(msgs);
>>>>>> +
>>>>>> +	reinit_completion(&dev->cmd_complete);
>>>>>> +	addr |= (msgs->flags & I2C_M_RD) ? 1 : 0;
>>>>> Why is this needed?
>>>> In the ls2x I2C controller, the bit 0 of TXR indicates the read/write status
>>>> when transferring the address.
>>> Yes, I understand this. I don't understand why do you need this twice.
>> Are you saying that the "is_read" variable in ls2x_i2c_xfer_one() already
>> indicates the read/write state of data transfer?
>>
>> I just didn't think it was necessary to take "is_read" as an argument to
>> ls2x_i2c_start() at the time, since we could get it from "msg".
> Have you checked what i2c_8bit_addr_from_msg() is doing?

Emm... Sorry, it was a cheap mistake on my part, I will drop it.

Then I will send the V4 patchset, with all the relevant changes.

Thanks.

Binbin

>

[PATCH V3 5/5] LoongArch: Enable LS2X I2C in loongson3_defconfig

[Binbin Zhou]

This is now supported, enable for Loongson-3 systems.
Other systems are unaffected.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 arch/loongarch/configs/loongson3_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/loongarch/configs/loongson3_defconfig b/arch/loongarch/configs/loongson3_defconfig
index cd8e003d885e..017eb0e36738 100644
--- a/arch/loongarch/configs/loongson3_defconfig
+++ b/arch/loongarch/configs/loongson3_defconfig
@@ -600,6 +600,7 @@ CONFIG_HW_RANDOM_VIRTIO=m
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_PIIX4=y
 CONFIG_I2C_GPIO=y
+CONFIG_I2C_LS2X=y
 CONFIG_SPI=y
 CONFIG_GPIO_SYSFS=y
 CONFIG_GPIO_LOONGSON=y
-- 
2.31.1

Re: [PATCH V3 3/5] dt-bindings: i2c: add bindings for Loongson LS2X I2C

[Rob Herring]

On Fri, 25 Nov 2022 16:54:13 +0800, Binbin Zhou wrote:
> Add device tree bindings for the i2c controller on the Loongson-2K Soc
> or Loongosn LS7A bridge.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../bindings/i2c/loongson,ls2x-i2c.yaml       | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.example.dtb: i2c@1fe21000: reg: [[0, 534908928], [0, 8]] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.example.dtb: i2c@1fe21000: Unevaluated properties are not allowed ('reg' was unexpected)
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/57339e73b6c0bfe446e19a7f55a48b7ca640b9ec.1669359515.git.zhoubinbin@loongson.cn

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command.