[PATCH next v9 1/4] i2c: hisi: Add initial device tree support

[PATCH next v9 1/4] i2c: hisi: Add initial device tree support

[Weilong Chen]

The HiSilicon I2C controller can be used on embedded platform, which
boot from devicetree.

Signed-off-by: Weilong Chen <chenweilong@huawei.com>
Acked-by: Yicong Yang <yangyicong@hisilicon.com>
---
Change since v8:
- Change hisilicon,i2c-ascend910 to hisilicon,ascend910-i2c
  as the normal convention is: vendor,soc-ipblock
Link: https://lore.kernel.org/lkml/20221024015151.342651-1-chenweilong@huawei.com/

 drivers/i2c/busses/Kconfig    |  2 +-
 drivers/i2c/busses/i2c-hisi.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e50f9603d189..a7bfddf08fa7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -673,7 +673,7 @@ config I2C_HIGHLANDER
 
 config I2C_HISI
 	tristate "HiSilicon I2C controller"
-	depends on (ARM64 && ACPI) || COMPILE_TEST
+	depends on ARM64 || COMPILE_TEST
 	help
 	  Say Y here if you want to have Hisilicon I2C controller support
 	  available on the Kunpeng Server.
diff --git a/drivers/i2c/busses/i2c-hisi.c b/drivers/i2c/busses/i2c-hisi.c
index 76c3d8f6fc3c..0fc9400e9e92 100644
--- a/drivers/i2c/busses/i2c-hisi.c
+++ b/drivers/i2c/busses/i2c-hisi.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2021 HiSilicon Technologies Co., Ltd.
  */
 
+#include <linux/acpi.h>
 #include <linux/bits.h>
 #include <linux/bitfield.h>
 #include <linux/completion.h>
@@ -13,6 +14,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/units.h>
@@ -483,17 +485,28 @@ static int hisi_i2c_probe(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
 static const struct acpi_device_id hisi_i2c_acpi_ids[] = {
 	{ "HISI03D1", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, hisi_i2c_acpi_ids);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id hisi_i2c_dts_ids[] = {
+	{ .compatible = "hisilicon,ascend910-i2c", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, hisi_i2c_dts_ids);
+#endif
 
 static struct platform_driver hisi_i2c_driver = {
 	.probe		= hisi_i2c_probe,
 	.driver		= {
 		.name	= "hisi-i2c",
-		.acpi_match_table = hisi_i2c_acpi_ids,
+		.acpi_match_table = ACPI_PTR(hisi_i2c_acpi_ids),
+		.of_match_table = of_match_ptr(hisi_i2c_dts_ids),
 	},
 };
 module_platform_driver(hisi_i2c_driver);
-- 
2.31.GIT

[PATCH next v9 2/4] dt-bindings: i2c: add entry for hisilicon,ascend910-i2c

[Weilong Chen]

Add the new compatible for HiSilicon i2c.

Signed-off-by: Weilong Chen <chenweilong@huawei.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change since v8:
- Use vendor,soc-ipblock format.
- Drop quotes.
- Drop "Device Tree bindings".
- Description goes to top level description.
- Use defines for constants.
Link: https://lore.kernel.org/lkml/20221024015151.342651-2-chenweilong@huawei.com/

 .../bindings/i2c/hisilicon,ascend910-i2c.yaml | 73 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/hisilicon,ascend910-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/hisilicon,ascend910-i2c.yaml b/Documentation/devicetree/bindings/i2c/hisilicon,ascend910-i2c.yaml
new file mode 100644
index 000000000000..7d7a8de7bcd8
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/hisilicon,ascend910-i2c.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/hisilicon,ascend910-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon common I2C controller
+
+maintainers:
+  - Yicong Yang <yangyicong@hisilicon.com>
+
+description:
+  The HiSilicon common I2C controller can be used for many different
+  types of SoC such as Huawei Ascend AI series chips.
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    const: hisilicon,ascend910-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-frequency:
+    default: 400000
+
+  i2c-sda-falling-time-ns:
+    default: 343
+
+  i2c-scl-falling-time-ns:
+    default: 203
+
+  i2c-sda-hold-time-ns:
+    default: 830
+
+  i2c-scl-rising-time-ns:
+    default: 365
+
+  i2c-digital-filter-width-ns:
+    default: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i2c@38b0000 {
+      compatible = "hisilicon,ascend910-i2c";
+      reg = <0x38b0000 0x10000>;
+      interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+      i2c-sda-falling-time-ns = <56>;
+      i2c-scl-falling-time-ns = <56>;
+      i2c-sda-hold-time-ns = <56>;
+      i2c-scl-rising-time-ns = <56>;
+      i2c-digital-filter;
+      i2c-digital-filter-width-ns = <0x0>;
+      clocks = <&alg_clk>;
+      clock-frequency = <400000>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d1214d83c2df..d42e34d1e8e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9228,6 +9228,7 @@ M:	Yicong Yang <yangyicong@hisilicon.com>
 L:	linux-i2c@vger.kernel.org
 S:	Maintained
 W:	https://www.hisilicon.com
+F:	Documentation/devicetree/bindings/i2c/hisilicon,ascend910-i2c.yaml
 F:	drivers/i2c/busses/i2c-hisi.c
 
 HISILICON LPC BUS DRIVER
-- 
2.31.GIT

Re: [PATCH next v9 1/4] i2c: hisi: Add initial device tree support

[Andy Shevchenko]

On Sat, Oct 29, 2022 at 07:59:36PM +0800, Weilong Chen wrote:
> The HiSilicon I2C controller can be used on embedded platform, which
> boot from devicetree.

...

> +#include <linux/acpi.h>
> +#include <linux/of.h>

Why?

...

> +#ifdef CONFIG_ACPI

Why?

...

> +#ifdef CONFIG_OF

Why?

...

> -		.acpi_match_table = hisi_i2c_acpi_ids,
> +		.acpi_match_table = ACPI_PTR(hisi_i2c_acpi_ids),

Why?

...

> +		.of_match_table = of_match_ptr(hisi_i2c_dts_ids),

Why of_match_ptr()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v9 1/4] i2c: hisi: Add initial device tree support

[chenweilong]

On 2022/10/31 6:01, Andy Shevchenko wrote:
> On Sat, Oct 29, 2022 at 07:59:36PM +0800, Weilong Chen wrote:
>> The HiSilicon I2C controller can be used on embedded platform, which
>> boot from devicetree.
> ...
>
>> +#include <linux/acpi.h>
>> +#include <linux/of.h>
> Why?
>
> ...
>
>> +#ifdef CONFIG_ACPI
> Why?
>
> ...
>
>> +#ifdef CONFIG_OF
> Why?
>
> ...
>
>> -		.acpi_match_table = hisi_i2c_acpi_ids,
>> +		.acpi_match_table = ACPI_PTR(hisi_i2c_acpi_ids),
> Why?
>
> ...
>
>> +		.of_match_table = of_match_ptr(hisi_i2c_dts_ids),
> Why of_match_ptr()?

There's a lot of drivers use of_match_ptr/ACPI_PTR to protect the of_device_id and

have explicit headers file references to linux/acpi.h or linux/of.h, such as

drivers/media/platform/intel/pxa_camera.c,

bluetooth/hci_intel.c, 

platform/x86/intel/chtwc_int33fe.c,

platform/x86/intel/pmc/core.c and so on.


The acpi.h and of.h have a nice function or macro definition if CONFIG_OF/ACPI is not satisfy,

for example:

#define ACPI_PTR(_ptr)  (_ptr)  vs  #define ACPI_PTR(_ptr)  (NULL)

and also a lot of 'static inline' function there.


Seems a good idea to remove all of them, the codes your noted may look a bit verbose there ,

But I think it is valuable for a driver and device ,telling users it support acpi boot or is it just embedded.


Thanks for review.

Best Regards.

>

Re: [PATCH next v9 1/4] i2c: hisi: Add initial device tree support

[Andy Shevchenko]

On Mon, Oct 31, 2022 at 09:57:51AM +0800, chenweilong wrote:
> On 2022/10/31 6:01, Andy Shevchenko wrote:
> > On Sat, Oct 29, 2022 at 07:59:36PM +0800, Weilong Chen wrote:
> >> The HiSilicon I2C controller can be used on embedded platform, which
> >> boot from devicetree.
> > ...
> >
> >> +#include <linux/acpi.h>
> >> +#include <linux/of.h>
> > Why?
> >
> > ...
> >
> >> +#ifdef CONFIG_ACPI
> > Why?
> >
> > ...
> >
> >> +#ifdef CONFIG_OF
> > Why?
> >
> > ...
> >
> >> -		.acpi_match_table = hisi_i2c_acpi_ids,
> >> +		.acpi_match_table = ACPI_PTR(hisi_i2c_acpi_ids),
> > Why?
> >
> > ...
> >
> >> +		.of_match_table = of_match_ptr(hisi_i2c_dts_ids),
> > Why of_match_ptr()?
> 
> There's a lot of drivers use of_match_ptr/ACPI_PTR to protect the of_device_id and
> have explicit headers file references to linux/acpi.h or linux/of.h, such as
> drivers/media/platform/intel/pxa_camera.c,
> bluetooth/hci_intel.c, 
> platform/x86/intel/chtwc_int33fe.c,
> platform/x86/intel/pmc/core.c and so on.

We have a lot of the legacy or not-up-to-dated to all new kernel APIs code.
Does it justify not to use the new approach in the new contribution?

...

> The acpi.h and of.h have a nice function or macro definition if CONFIG_OF/ACPI is not satisfy,
> for example:
> 
> #define ACPI_PTR(_ptr)  (_ptr)  vs  #define ACPI_PTR(_ptr)  (NULL)
> 
> and also a lot of 'static inline' function there.

And why do you need it?

...

> Seems a good idea to remove all of them, the codes your noted may look a bit
> verbose there. But I think it is valuable for a driver and device ,telling
> users it support acpi boot or is it just embedded.

So, what do we gain here?

(Fill the "Advantages of your code" section below)

Disadvantages of your code:
- ugly ifdeffery which we usually do not appreciate
- in some cases it's good to have OF ID table on ACPI platforms (see what
  PRP0001 trick is)
- use old approach for the compiler on how to avoid warnings of the static
  variables being defined and not used (note, neither ACPI_PTR() nor
  of_match_ptr() provides a new approach on that, so you have to amend them
  first)
- as a side effect additional headers to be included that are used for 1% or
  less of their capacity and slow down the compilation

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v9 1/4] i2c: hisi: Add initial device tree support

[chenweilong]

On 2022/10/31 23:42, Andy Shevchenko wrote:
> On Mon, Oct 31, 2022 at 09:57:51AM +0800, chenweilong wrote:
>> On 2022/10/31 6:01, Andy Shevchenko wrote:
>>> On Sat, Oct 29, 2022 at 07:59:36PM +0800, Weilong Chen wrote:
>>>> The HiSilicon I2C controller can be used on embedded platform, which
>>>> boot from devicetree.
>>> ...
>>>
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/of.h>
>>> Why?
>>>
>>> ...
>>>
>>>> +#ifdef CONFIG_ACPI
>>> Why?
>>>
>>> ...
>>>
>>>> +#ifdef CONFIG_OF
>>> Why?
>>>
>>> ...
>>>
>>>> -		.acpi_match_table = hisi_i2c_acpi_ids,
>>>> +		.acpi_match_table = ACPI_PTR(hisi_i2c_acpi_ids),
>>> Why?
>>>
>>> ...
>>>
>>>> +		.of_match_table = of_match_ptr(hisi_i2c_dts_ids),
>>> Why of_match_ptr()?
>> There's a lot of drivers use of_match_ptr/ACPI_PTR to protect the of_device_id and
>> have explicit headers file references to linux/acpi.h or linux/of.h, such as
>> drivers/media/platform/intel/pxa_camera.c,
>> bluetooth/hci_intel.c, 
>> platform/x86/intel/chtwc_int33fe.c,
>> platform/x86/intel/pmc/core.c and so on.
> We have a lot of the legacy or not-up-to-dated to all new kernel APIs code.
> Does it justify not to use the new approach in the new contribution?
>
> ...
>
>> The acpi.h and of.h have a nice function or macro definition if CONFIG_OF/ACPI is not satisfy,
>> for example:
>>
>> #define ACPI_PTR(_ptr)  (_ptr)  vs  #define ACPI_PTR(_ptr)  (NULL)
>>
>> and also a lot of 'static inline' function there.
> And why do you need it?
>
> ...
>
>> Seems a good idea to remove all of them, the codes your noted may look a bit
>> verbose there. But I think it is valuable for a driver and device ,telling
>> users it support acpi boot or is it just embedded.
> So, what do we gain here?
>
> (Fill the "Advantages of your code" section below)
>
> Disadvantages of your code:
> - ugly ifdeffery which we usually do not appreciate
> - in some cases it's good to have OF ID table on ACPI platforms (see what
>   PRP0001 trick is)
> - use old approach for the compiler on how to avoid warnings of the static
>   variables being defined and not used (note, neither ACPI_PTR() nor
>   of_match_ptr() provides a new approach on that, so you have to amend them
>   first)
> - as a side effect additional headers to be included that are used for 1% or
>   less of their capacity and slow down the compilation

Thanks very much for your detailed explanation.

By the way,  is it valuable to make a cleanup for the legacy not-up-to-dated drivers?

There's lots of of_match_ptr or ACPI_PTR...


Best Regards.

Re: [PATCH next v9 1/4] i2c: hisi: Add initial device tree support

[Andy Shevchenko]

On Tue, Nov 01, 2022 at 03:23:29PM +0800, chenweilong wrote:
> On 2022/10/31 23:42, Andy Shevchenko wrote:

...

> Thanks very much for your detailed explanation.

You're welcome!

> By the way,  is it valuable to make a cleanup for the legacy not-up-to-dated drivers?
> 
> There's lots of of_match_ptr or ACPI_PTR...

Not on per se basis, only if there is a series which does something more useful
than that. E.g. enabling PRP0001 trick for discrete component drivers that may
be used on more than a single architecture.

-- 
With Best Regards,
Andy Shevchenko