diff mbox series

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

Message ID 20221029115937.179788-1-chenweilong@huawei.com
State New
Headers show
Series [next,v9,1/4] i2c: hisi: Add initial device tree support | expand

Commit Message

Weilong Chen Oct. 29, 2022, 11:59 a.m. UTC
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(-)

Comments

Andy Shevchenko Oct. 30, 2022, 10:01 p.m. UTC | #1
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()?
Weilong Chen Oct. 31, 2022, 1:57 a.m. UTC | #2
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.

>
Weilong Chen Nov. 1, 2022, 7:23 a.m. UTC | #3
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.
diff mbox series

Patch

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);