* [PATCH v3 0/3] Add support for HiSilicon I2C controller
@ 2021-03-22 11:10 Yicong Yang
2021-03-22 11:10 ` [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters Yicong Yang
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Yicong Yang @ 2021-03-22 11:10 UTC (permalink / raw)
To: wsa, linux-i2c
Cc: andriy.shevchenko, digetx, treding, jarkko.nikula, rmk+kernel,
song.bao.hua, john.garry, yangyicong, prime.zeng, linuxarm
Add driver and MAINTAINERS for HiSilicon I2C controller on Kunpeng SoC. Also
provide the devm_*() variants for adding the I2C adapters.
Change since v2:
- handle -EPROBE_DEFER case when get irq number by platform_get_irq()
Link: https://lore.kernel.org/linux-i2c/1615296137-14558-1-git-send-email-yangyicong@hisilicon.com/
Change since v1:
- fix compile test error on 32bit arch, reported by intel lkp robot:
64 bit division without using kernel wrapper in probe function.
Link:https://lore.kernel.org/linux-i2c/1615016946-55670-1-git-send-email-yangyicong@hisilicon.com/
Yicong Yang (3):
i2c: core: add managed function for adding i2c adapters
i2c: add support for HiSilicon I2C controller
MAINTAINERS: Add maintainer for HiSilicon I2C driver
MAINTAINERS | 7 +
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-hisi.c | 525 ++++++++++++++++++++++++++++++++++++++++++
drivers/i2c/i2c-core-base.c | 39 ++++
include/linux/i2c.h | 1 +
6 files changed, 583 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-hisi.c
--
2.8.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters 2021-03-22 11:10 [PATCH v3 0/3] Add support for HiSilicon I2C controller Yicong Yang @ 2021-03-22 11:10 ` Yicong Yang 2021-03-22 16:35 ` Andy Shevchenko 2021-03-22 16:45 ` Dmitry Osipenko 2021-03-22 11:10 ` [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller Yicong Yang 2021-03-22 11:10 ` [PATCH v3 3/3] MAINTAINERS: Add maintainer for HiSilicon I2C driver Yicong Yang 2 siblings, 2 replies; 19+ messages in thread From: Yicong Yang @ 2021-03-22 11:10 UTC (permalink / raw) To: wsa, linux-i2c Cc: andriy.shevchenko, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, yangyicong, prime.zeng, linuxarm Some I2C controller drivers will only unregister the I2C adapter in their .remove() callback, which can be done by simply using a managed variant to add the I2C adapter. So add the managed functions for adding the I2C adapter. Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/i2c/i2c-core-base.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 1 + 2 files changed, 40 insertions(+) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 63ebf72..61486dc 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1550,6 +1550,38 @@ int i2c_add_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL(i2c_add_adapter); +static void devm_i2c_del_adapter(struct device *dev, void *ptr); + +/** + * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter() + * @dev: managing device for adding this I2C adapter + * @adapter: the adapter to add + * Context: can sleep + * + * Add adapter with dynamic bus number, same with i2c_add_adapter() + * but the adapter will be auto deleted on driver detach. + */ +int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter) +{ + struct i2c_adapter **ptr; + int ret; + + ptr = devres_alloc(devm_i2c_del_adapter, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + ret = i2c_add_adapter(adapter); + if (!ret) { + *ptr = adapter; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return ret; +} +EXPORT_SYMBOL(devm_i2c_add_adapter); + /** * i2c_add_numbered_adapter - declare i2c adapter, use static bus number * @adap: the adapter to register (with adap->nr initialized) @@ -1703,6 +1735,13 @@ void i2c_del_adapter(struct i2c_adapter *adap) } EXPORT_SYMBOL(i2c_del_adapter); +static void devm_i2c_del_adapter(struct device *dev, void *ptr) +{ + struct i2c_adapter *adapter = *((struct i2c_adapter **)ptr); + + i2c_del_adapter(adapter); +} + static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p, u32 def_val, bool use_def) { diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 5662265..10bd0b0 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -844,6 +844,7 @@ static inline void i2c_mark_adapter_resumed(struct i2c_adapter *adap) */ #if IS_ENABLED(CONFIG_I2C) int i2c_add_adapter(struct i2c_adapter *adap); +int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter); void i2c_del_adapter(struct i2c_adapter *adap); int i2c_add_numbered_adapter(struct i2c_adapter *adap); -- 2.8.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters 2021-03-22 11:10 ` [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters Yicong Yang @ 2021-03-22 16:35 ` Andy Shevchenko 2021-03-24 8:26 ` Yicong Yang 2021-03-22 16:45 ` Dmitry Osipenko 1 sibling, 1 reply; 19+ messages in thread From: Andy Shevchenko @ 2021-03-22 16:35 UTC (permalink / raw) To: Yicong Yang Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On Mon, Mar 22, 2021 at 07:10:11PM +0800, Yicong Yang wrote: > Some I2C controller drivers will only unregister the I2C > adapter in their .remove() callback, which can be done > by simply using a managed variant to add the I2C adapter. > > So add the managed functions for adding the I2C adapter. ... > +static void devm_i2c_del_adapter(struct device *dev, void *ptr); Can we instead of forward declaration simply move below to be after devm_i2c_del_adapter()? > +/** > + * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter() > + * @dev: managing device for adding this I2C adapter > + * @adapter: the adapter to add > + * Context: can sleep > + * > + * Add adapter with dynamic bus number, same with i2c_add_adapter() > + * but the adapter will be auto deleted on driver detach. > + */ > +int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter) > +{ > + struct i2c_adapter **ptr; > + int ret; > + > + ptr = devres_alloc(devm_i2c_del_adapter, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return -ENOMEM; > + > + ret = i2c_add_adapter(adapter); > + if (!ret) { > + *ptr = adapter; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } > + > + return ret; > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters 2021-03-22 16:35 ` Andy Shevchenko @ 2021-03-24 8:26 ` Yicong Yang 2021-03-24 11:05 ` Andy Shevchenko 0 siblings, 1 reply; 19+ messages in thread From: Yicong Yang @ 2021-03-24 8:26 UTC (permalink / raw) To: Andy Shevchenko Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On 2021/3/23 0:35, Andy Shevchenko wrote: > On Mon, Mar 22, 2021 at 07:10:11PM +0800, Yicong Yang wrote: >> Some I2C controller drivers will only unregister the I2C >> adapter in their .remove() callback, which can be done >> by simply using a managed variant to add the I2C adapter. >> >> So add the managed functions for adding the I2C adapter. > > ... > >> +static void devm_i2c_del_adapter(struct device *dev, void *ptr); > > Can we instead of forward declaration simply move below to be after > devm_i2c_del_adapter()? i just want to put the devm variant near i2c_add_adapter, i'll address this if it's unnecessary. Thanks, Yicong > >> +/** >> + * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter() >> + * @dev: managing device for adding this I2C adapter >> + * @adapter: the adapter to add >> + * Context: can sleep >> + * >> + * Add adapter with dynamic bus number, same with i2c_add_adapter() >> + * but the adapter will be auto deleted on driver detach. >> + */ >> +int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter) >> +{ >> + struct i2c_adapter **ptr; >> + int ret; >> + >> + ptr = devres_alloc(devm_i2c_del_adapter, sizeof(*ptr), GFP_KERNEL); >> + if (!ptr) >> + return -ENOMEM; >> + >> + ret = i2c_add_adapter(adapter); >> + if (!ret) { >> + *ptr = adapter; >> + devres_add(dev, ptr); >> + } else { >> + devres_free(ptr); >> + } >> + >> + return ret; >> +} > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters 2021-03-24 8:26 ` Yicong Yang @ 2021-03-24 11:05 ` Andy Shevchenko 0 siblings, 0 replies; 19+ messages in thread From: Andy Shevchenko @ 2021-03-24 11:05 UTC (permalink / raw) To: Yicong Yang Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On Wed, Mar 24, 2021 at 04:26:54PM +0800, Yicong Yang wrote: > On 2021/3/23 0:35, Andy Shevchenko wrote: > > On Mon, Mar 22, 2021 at 07:10:11PM +0800, Yicong Yang wrote: > >> Some I2C controller drivers will only unregister the I2C > >> adapter in their .remove() callback, which can be done > >> by simply using a managed variant to add the I2C adapter. > >> > >> So add the managed functions for adding the I2C adapter. > > > > ... > > > >> +static void devm_i2c_del_adapter(struct device *dev, void *ptr); > > > > Can we instead of forward declaration simply move below to be after > > devm_i2c_del_adapter()? > > i just want to put the devm variant near i2c_add_adapter, i'll address > this if it's unnecessary. Please, address. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters 2021-03-22 11:10 ` [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters Yicong Yang 2021-03-22 16:35 ` Andy Shevchenko @ 2021-03-22 16:45 ` Dmitry Osipenko 2021-03-24 8:29 ` Yicong Yang 1 sibling, 1 reply; 19+ messages in thread From: Dmitry Osipenko @ 2021-03-22 16:45 UTC (permalink / raw) To: Yicong Yang, wsa, linux-i2c Cc: andriy.shevchenko, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm 22.03.2021 14:10, Yicong Yang пишет: > Some I2C controller drivers will only unregister the I2C > adapter in their .remove() callback, which can be done > by simply using a managed variant to add the I2C adapter. > > So add the managed functions for adding the I2C adapter. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/i2c/i2c-core-base.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 1 + > 2 files changed, 40 insertions(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 63ebf72..61486dc 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -1550,6 +1550,38 @@ int i2c_add_adapter(struct i2c_adapter *adapter) > } > EXPORT_SYMBOL(i2c_add_adapter); > > +static void devm_i2c_del_adapter(struct device *dev, void *ptr); > + > +/** > + * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter() > + * @dev: managing device for adding this I2C adapter > + * @adapter: the adapter to add > + * Context: can sleep > + * > + * Add adapter with dynamic bus number, same with i2c_add_adapter() > + * but the adapter will be auto deleted on driver detach. > + */ > +int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter) > +{ > + struct i2c_adapter **ptr; > + int ret; > + > + ptr = devres_alloc(devm_i2c_del_adapter, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return -ENOMEM; > + > + ret = i2c_add_adapter(adapter); > + if (!ret) { > + *ptr = adapter; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } This could be simplified using devm_add_action_or_reset(). > + return ret; > +} > +EXPORT_SYMBOL(devm_i2c_add_adapter); EXPORT_SYMBOL_GPL ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters 2021-03-22 16:45 ` Dmitry Osipenko @ 2021-03-24 8:29 ` Yicong Yang 2021-03-24 11:06 ` Andy Shevchenko 0 siblings, 1 reply; 19+ messages in thread From: Yicong Yang @ 2021-03-24 8:29 UTC (permalink / raw) To: Dmitry Osipenko, wsa, linux-i2c Cc: andriy.shevchenko, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On 2021/3/23 0:45, Dmitry Osipenko wrote: > 22.03.2021 14:10, Yicong Yang пишет: >> Some I2C controller drivers will only unregister the I2C >> adapter in their .remove() callback, which can be done >> by simply using a managed variant to add the I2C adapter. >> >> So add the managed functions for adding the I2C adapter. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> drivers/i2c/i2c-core-base.c | 39 +++++++++++++++++++++++++++++++++++++++ >> include/linux/i2c.h | 1 + >> 2 files changed, 40 insertions(+) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 63ebf72..61486dc 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -1550,6 +1550,38 @@ int i2c_add_adapter(struct i2c_adapter *adapter) >> } >> EXPORT_SYMBOL(i2c_add_adapter); >> >> +static void devm_i2c_del_adapter(struct device *dev, void *ptr); >> + >> +/** >> + * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter() >> + * @dev: managing device for adding this I2C adapter >> + * @adapter: the adapter to add >> + * Context: can sleep >> + * >> + * Add adapter with dynamic bus number, same with i2c_add_adapter() >> + * but the adapter will be auto deleted on driver detach. >> + */ >> +int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter) >> +{ >> + struct i2c_adapter **ptr; >> + int ret; >> + >> + ptr = devres_alloc(devm_i2c_del_adapter, sizeof(*ptr), GFP_KERNEL); >> + if (!ptr) >> + return -ENOMEM; >> + >> + ret = i2c_add_adapter(adapter); >> + if (!ret) { >> + *ptr = adapter; >> + devres_add(dev, ptr); >> + } else { >> + devres_free(ptr); >> + } > > This could be simplified using devm_add_action_or_reset(). thanks for the hint. i'll see whether i can change to that. > >> + return ret; >> +} >> +EXPORT_SYMBOL(devm_i2c_add_adapter); > > EXPORT_SYMBOL_GPL considering i2c_add_adapter exported with no GPL, i think the variant should keep the same. Thanks, Yicong > > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters 2021-03-24 8:29 ` Yicong Yang @ 2021-03-24 11:06 ` Andy Shevchenko 0 siblings, 0 replies; 19+ messages in thread From: Andy Shevchenko @ 2021-03-24 11:06 UTC (permalink / raw) To: Yicong Yang Cc: Dmitry Osipenko, wsa, linux-i2c, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On Wed, Mar 24, 2021 at 04:29:26PM +0800, Yicong Yang wrote: > On 2021/3/23 0:45, Dmitry Osipenko wrote: > > 22.03.2021 14:10, Yicong Yang пишет: ... > >> +EXPORT_SYMBOL(devm_i2c_add_adapter); > > > > EXPORT_SYMBOL_GPL > > considering i2c_add_adapter exported with no GPL, i think the variant should > keep the same. Nope. You should follow other devm_*() APIs. Hint: they are using struct device which is GPL. Please, fix. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-22 11:10 [PATCH v3 0/3] Add support for HiSilicon I2C controller Yicong Yang 2021-03-22 11:10 ` [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters Yicong Yang @ 2021-03-22 11:10 ` Yicong Yang 2021-03-22 15:21 ` Dmitry Osipenko ` (2 more replies) 2021-03-22 11:10 ` [PATCH v3 3/3] MAINTAINERS: Add maintainer for HiSilicon I2C driver Yicong Yang 2 siblings, 3 replies; 19+ messages in thread From: Yicong Yang @ 2021-03-22 11:10 UTC (permalink / raw) To: wsa, linux-i2c Cc: andriy.shevchenko, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, yangyicong, prime.zeng, linuxarm Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides the access to the i2c busses, which connects to the eeprom, rtc, etc. The driver works with IRQ mode, and supports basic I2C features and 10bit address. The DMA is not supported. Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-hisi.c | 525 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 536 insertions(+) create mode 100644 drivers/i2c/busses/i2c-hisi.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 05ebf75..f6b8b71 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -645,6 +645,16 @@ config I2C_HIGHLANDER This driver can also be built as a module. If so, the module will be called i2c-highlander. +config I2C_HISI + tristate "HiSilicon I2C controller" + depends on (ARM64 && ACPI) || COMPILE_TEST + help + Say Y here if you want to have Hisilicon I2C controller support + available on the Kunpeng Server. + + This driver can also be built as a module. If so, the module + will be called i2c-hisi. + config I2C_IBM_IIC tristate "IBM PPC 4xx on-chip I2C interface" depends on 4xx diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 615f35e..e1c9292 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_EMEV2) += i2c-emev2.o obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o +obj-$(CONFIG_I2C_HISI) += i2c-hisi.o obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o obj-$(CONFIG_I2C_IMG) += i2c-img-scb.o diff --git a/drivers/i2c/busses/i2c-hisi.c b/drivers/i2c/busses/i2c-hisi.c new file mode 100644 index 0000000..8656f71 --- /dev/null +++ b/drivers/i2c/busses/i2c-hisi.c @@ -0,0 +1,525 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * HiSilicon I2C Controller Driver for Kunpeng SoC + * + * Copyright (c) 2021 HiSilicon Limited. + */ + +#include <linux/acpi.h> +#include <linux/bits.h> +#include <linux/bitfield.h> +#include <linux/completion.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> + +#define HISI_I2C_FRAME_CTRL 0x0000 +#define HISI_I2C_FRAME_CTRL_SPEED_MODE GENMASK(1, 0) +#define HISI_I2C_FRAME_CTRL_ADDR_TEN BIT(2) +#define HISI_I2C_SLV_ADDR 0x0004 +#define HISI_I2C_SLV_ADDR_VAL GENMASK(9, 0) +#define HISI_I2C_SLV_ADDR_GC_S_MODE BIT(10) +#define HISI_I2C_SLV_ADDR_GC_S_EN BIT(11) +#define HISI_I2C_CMD_TXDATA 0x0008 +#define HISI_I2C_CMD_TXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_CMD_TXDATA_RW BIT(8) +#define HISI_I2C_CMD_TXDATA_P_EN BIT(9) +#define HISI_I2C_CMD_TXDATA_SR_EN BIT(10) +#define HISI_I2C_RXDATA 0x000c +#define HISI_I2C_RXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_SS_SCL_HCNT 0x0010 +#define HISI_I2C_SS_SCL_LCNT 0x0014 +#define HISI_I2C_FS_SCL_HCNT 0x0018 +#define HISI_I2C_FS_SCL_LCNT 0x001c +#define HISI_I2C_HS_SCL_HCNT 0x0020 +#define HISI_I2C_HS_SCL_LCNT 0x0024 +#define HISI_I2C_FIFO_CTRL 0x0028 +#define HISI_I2C_FIFO_RX_CLR BIT(0) +#define HISI_I2C_FIFO_TX_CLR BIT(1) +#define HISI_I2C_FIFO_RX_AF_THRESH GENMASK(7, 2) +#define HISI_I2C_FIFO_TX_AE_THRESH GENMASK(13, 8) +#define HISI_I2C_FIFO_STATE 0x002c +#define HISI_I2C_FIFO_STATE_RX_RERR BIT(0) +#define HISI_I2C_FIFO_STATE_RX_WERR BIT(1) +#define HISI_I2C_FIFO_STATE_RX_EMPTY BIT(3) +#define HISI_I2C_FIFO_STATE_TX_RERR BIT(6) +#define HISI_I2C_FIFO_STATE_TX_WERR BIT(7) +#define HISI_I2C_FIFO_STATE_TX_FULL BIT(11) +#define HISI_I2C_SDA_HOLD 0x0030 +#define HISI_I2C_SDA_HOLD_TX GENMASK(15, 0) +#define HISI_I2C_SDA_HOLD_RX GENMASK(23, 16) +#define HISI_I2C_FS_SPK_LEN 0x0038 +#define HISI_I2C_FS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_HS_SPK_LEN 0x003c +#define HISI_I2C_HS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_INT_MSTAT 0x0044 +#define HISI_I2C_INT_CLR 0x0048 +#define HISI_I2C_INT_MASK 0x004C +#define HISI_I2C_TRANS_STATE 0x0050 +#define HISI_I2C_TRANS_ERR 0x0054 +#define HISI_I2C_VERSION 0x0058 + +#define HISI_I2C_INT_ALL 0x1f +#define HISI_I2C_INT_TRANS_CPLT BIT(0) +#define HISI_I2C_INT_TRANS_ERR BIT(1) +#define HISI_I2C_INT_FIFO_ERR BIT(2) +#define HISI_I2C_INT_RX_FULL BIT(3) +#define HISI_I2C_INT_TX_EMPTY BIT(4) +#define HISI_I2C_INT_ERR (HISI_I2C_INT_TRANS_ERR | \ + HISI_I2C_INT_FIFO_ERR) + +#define HISI_I2C_STD_SPEED_MODE 0x0 +#define HISI_I2C_FAST_SPEED_MODE 0x1 +#define HISI_I2C_HIGH_SPEED_MODE 0x2 + +#define HISI_I2C_TX_FIFO_DEPTH 64 +#define HISI_I2C_RX_FIFO_DEPTH 64 +#define HISI_I2C_TX_F_AE_THRESH 1 +#define HISI_I2C_RX_F_AF_THRESH 60 + +#define NSEC_TO_CYCLES(ns, clk_rate_khz) (DIV_ROUND_UP_ULL((clk_rate_khz) * (ns), NSEC_PER_MSEC)) + +struct hisi_i2c_controller { + struct device *dev; + struct i2c_adapter adapter; + void __iomem *iobase; + int irq; + + /* Intermediates for recording the transfer process */ + struct completion *completion; + struct i2c_msg *msgs; + int msg_num; + int msg_tx_idx; + int buf_tx_idx; + int msg_rx_idx; + int buf_rx_idx; + u16 tar_addr; + u32 xfer_err; + + /* I2C bus configuration */ + u32 scl_fall_time; + u32 scl_rise_time; + u32 sda_hold_time; + u64 clk_rate_khz; + u32 bus_freq_hz; + u32 spk_len; +}; + +static const char *hisi_i2c_speed_string(u32 bus_freq_hz) +{ + switch (bus_freq_hz) { + case I2C_MAX_STANDARD_MODE_FREQ: + return "100K"; + case I2C_MAX_FAST_MODE_FREQ: + return "400K"; + case I2C_MAX_HIGH_SPEED_MODE_FREQ: + return "3.4M"; + default: + return "unknown"; + } +} + +static void hisi_i2c_enable_int(struct hisi_i2c_controller *ctlr, u32 mask) +{ + writel(mask, ctlr->iobase + HISI_I2C_INT_MASK); +} + +static void hisi_i2c_disable_int(struct hisi_i2c_controller *ctlr, u32 mask) +{ + writel((~mask) & HISI_I2C_INT_ALL, ctlr->iobase + HISI_I2C_INT_MASK); +} + +static void hisi_i2c_clear_int(struct hisi_i2c_controller *ctlr, u32 mask) +{ + writel(mask, ctlr->iobase + HISI_I2C_INT_CLR); +} + +static void hisi_i2c_handle_errors(struct hisi_i2c_controller *ctlr) +{ + u32 int_err = ctlr->xfer_err, reg; + + if (int_err & HISI_I2C_INT_FIFO_ERR) { + reg = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); + + if (reg & HISI_I2C_FIFO_STATE_RX_RERR) + dev_err(ctlr->dev, "rx fifo error read.\n"); + + if (reg & HISI_I2C_FIFO_STATE_RX_WERR) + dev_err(ctlr->dev, "rx fifo error write.\n"); + + if (reg & HISI_I2C_FIFO_STATE_TX_RERR) + dev_err(ctlr->dev, "tx fifo error read.\n"); + + if (reg & HISI_I2C_FIFO_STATE_TX_WERR) + dev_err(ctlr->dev, "tx fifo error write.\n"); + } +} + +static int hisi_i2c_start_xfer(struct hisi_i2c_controller *ctlr) +{ + struct i2c_msg *msg = ctlr->msgs; + u32 reg; + + reg = readl(ctlr->iobase + HISI_I2C_FRAME_CTRL); + reg &= ~HISI_I2C_FRAME_CTRL_ADDR_TEN; + if (msg->flags & I2C_M_TEN) + reg |= HISI_I2C_FRAME_CTRL_ADDR_TEN; + writel(reg, ctlr->iobase + HISI_I2C_FRAME_CTRL); + + reg = readl(ctlr->iobase + HISI_I2C_SLV_ADDR); + reg &= ~HISI_I2C_SLV_ADDR_VAL; + reg |= FIELD_PREP(HISI_I2C_SLV_ADDR_VAL, msg->addr); + writel(reg, ctlr->iobase + HISI_I2C_SLV_ADDR); + + reg = readl(ctlr->iobase + HISI_I2C_FIFO_CTRL); + reg |= HISI_I2C_FIFO_RX_CLR | HISI_I2C_FIFO_TX_CLR; + writel(reg, ctlr->iobase + HISI_I2C_FIFO_CTRL); + reg &= ~(HISI_I2C_FIFO_RX_CLR | HISI_I2C_FIFO_TX_CLR); + writel(reg, ctlr->iobase + HISI_I2C_FIFO_CTRL); + + hisi_i2c_clear_int(ctlr, HISI_I2C_INT_ALL); + hisi_i2c_enable_int(ctlr, HISI_I2C_INT_ALL); + + return 0; +} + +static void hisi_i2c_reset_xfer(struct hisi_i2c_controller *ctlr) +{ + ctlr->msg_num = 0; + ctlr->xfer_err = 0; + ctlr->msg_tx_idx = 0; + ctlr->msg_rx_idx = 0; + ctlr->buf_tx_idx = 0; + ctlr->buf_rx_idx = 0; +} + +/* + * Initialize the transfer information and start the I2C bus transfer. + * We only configure the transfer and do some pre/post works here, and + * wait for the transfer done. The major transfer process is performed + * in the IRQ handler. + */ +static int hisi_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, + int num) +{ + struct hisi_i2c_controller *ctlr = i2c_get_adapdata(adap); + DECLARE_COMPLETION_ONSTACK(done); + int ret = num; + + hisi_i2c_reset_xfer(ctlr); + ctlr->completion = &done; + ctlr->msg_num = num; + ctlr->msgs = msgs; + + hisi_i2c_start_xfer(ctlr); + + if (!wait_for_completion_timeout(ctlr->completion, adap->timeout)) { + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); + i2c_recover_bus(&ctlr->adapter); + dev_err(ctlr->dev, "bus transfer timeout\n"); + ret = -EIO; + } + + if (ctlr->xfer_err) { + hisi_i2c_handle_errors(ctlr); + ret = -EIO; + } + + hisi_i2c_reset_xfer(ctlr); + ctlr->completion = NULL; + + return ret; +} + +static u32 hisi_i2c_functionality(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL; +} + +static const struct i2c_algorithm hisi_i2c_algo = { + .master_xfer = hisi_i2c_master_xfer, + .functionality = hisi_i2c_functionality, +}; + +static int hisi_i2c_read_rx_fifo(struct hisi_i2c_controller *ctlr) +{ + struct i2c_msg *cur_msg; + u32 fifo_state; + + while (ctlr->msg_rx_idx < ctlr->msg_num) { + cur_msg = ctlr->msgs + ctlr->msg_rx_idx; + + if (!(cur_msg->flags & I2C_M_RD)) { + ctlr->msg_rx_idx++; + continue; + } + + fifo_state = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); + while (!(fifo_state & HISI_I2C_FIFO_STATE_RX_EMPTY) && + ctlr->buf_rx_idx < cur_msg->len) { + cur_msg->buf[ctlr->buf_rx_idx++] = readl(ctlr->iobase + HISI_I2C_RXDATA); + fifo_state = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); + } + + if (ctlr->buf_rx_idx == cur_msg->len) { + ctlr->buf_rx_idx = 0; + ctlr->msg_rx_idx++; + } + + if (fifo_state & HISI_I2C_FIFO_STATE_RX_EMPTY) + break; + } + + return 0; +} + +static void hisi_i2c_xfer_msg(struct hisi_i2c_controller *ctlr) +{ + int max_write = HISI_I2C_TX_FIFO_DEPTH; + bool need_restart = false, last_msg; + struct i2c_msg *cur_msg; + u32 cmd, fifo_state; + + while (ctlr->msg_tx_idx < ctlr->msg_num) { + cur_msg = ctlr->msgs + ctlr->msg_tx_idx; + last_msg = (ctlr->msg_tx_idx == ctlr->msg_num - 1); + + /* Signal the SR bit when we start transferring a new message */ + if (ctlr->msg_tx_idx && !ctlr->buf_tx_idx) + need_restart = true; + + fifo_state = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); + while (!(fifo_state & HISI_I2C_FIFO_STATE_TX_FULL) && + ctlr->buf_tx_idx < cur_msg->len && + max_write) { + cmd = 0; + + if (need_restart) { + cmd |= HISI_I2C_CMD_TXDATA_SR_EN; + need_restart = false; + } + + /* Signal the STOP bit at the last frame of the last message */ + if (ctlr->buf_tx_idx == cur_msg->len - 1 && + last_msg) + cmd |= HISI_I2C_CMD_TXDATA_P_EN; + + if (cur_msg->flags & I2C_M_RD) + cmd |= HISI_I2C_CMD_TXDATA_RW; + else + cmd |= FIELD_PREP(HISI_I2C_CMD_TXDATA_DATA, + cur_msg->buf[ctlr->buf_tx_idx]); + + writel(cmd, ctlr->iobase + HISI_I2C_CMD_TXDATA); + ctlr->buf_tx_idx++; + max_write--; + + fifo_state = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); + } + + /* Update the transfer index after per message transfer is done. */ + if (ctlr->buf_tx_idx == cur_msg->len) { + ctlr->buf_tx_idx = 0; + ctlr->msg_tx_idx++; + } + + if ((fifo_state & HISI_I2C_FIFO_STATE_TX_FULL) || + max_write == 0) + break; + } +} + +static irqreturn_t hisi_i2c_irq(int irq, void *context) +{ + struct hisi_i2c_controller *ctlr = context; + u32 int_stat; + + int_stat = readl(ctlr->iobase + HISI_I2C_INT_MSTAT); + hisi_i2c_clear_int(ctlr, int_stat); + if (!(int_stat & HISI_I2C_INT_ALL)) + return IRQ_NONE; + + if (int_stat & HISI_I2C_INT_TX_EMPTY) + hisi_i2c_xfer_msg(ctlr); + + if (int_stat & HISI_I2C_INT_ERR) { + ctlr->xfer_err = int_stat; + goto out; + } + + /* Drain the rx fifo before finish the transfer */ + if (int_stat & (HISI_I2C_INT_TRANS_CPLT | HISI_I2C_INT_RX_FULL)) + hisi_i2c_read_rx_fifo(ctlr); + +out: + if (int_stat & HISI_I2C_INT_TRANS_CPLT || ctlr->xfer_err) { + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); + hisi_i2c_clear_int(ctlr, HISI_I2C_INT_ALL); + complete(ctlr->completion); + } + + return IRQ_HANDLED; +} + +/* + * Helper function for calculating and configuring the HIGH and LOW + * periods of SCL clock. The caller will pass the ratio of the + * counts (divide / divisor) according to the target speed mode, + * and the target registers. + */ +static void hisi_i2c_set_scl(struct hisi_i2c_controller *ctlr, + u32 divide, u32 divisor, + u32 reg_hcnt, u32 reg_lcnt) +{ + u32 total_cnt, t_scl_hcnt, t_scl_lcnt, scl_fall_cnt, scl_rise_cnt; + u32 scl_hcnt, scl_lcnt; + + /* Total SCL clock cycles per speed period */ + total_cnt = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz * 1000, ctlr->bus_freq_hz); + /* Total HIGH level SCL clock cycles including edges */ + t_scl_hcnt = DIV_ROUND_UP_ULL(total_cnt * divide, divisor); + /* Total LOW level SCL clock cycles including edges */ + t_scl_lcnt = total_cnt - t_scl_hcnt; + /* Fall edge SCL clock cycles */ + scl_fall_cnt = NSEC_TO_CYCLES(ctlr->scl_fall_time, ctlr->clk_rate_khz); + /* Rise edge SCL clock cycles */ + scl_rise_cnt = NSEC_TO_CYCLES(ctlr->scl_rise_time, ctlr->clk_rate_khz); + + /* Calculated HIGH and LOW periods of SCL clock */ + scl_hcnt = t_scl_hcnt - ctlr->spk_len - 7 - scl_fall_cnt; + scl_lcnt = t_scl_lcnt - 1 - scl_rise_cnt; + + writel(scl_hcnt, ctlr->iobase + reg_hcnt); + writel(scl_lcnt, ctlr->iobase + reg_lcnt); +} + +static void hisi_i2c_configure_bus(struct hisi_i2c_controller *ctlr) +{ + u32 reg, sda_hold_cnt, speed_mode; + struct i2c_timings t; + + i2c_parse_fw_timings(ctlr->dev, &t, true); + ctlr->bus_freq_hz = t.bus_freq_hz; + ctlr->spk_len = NSEC_TO_CYCLES(t.digital_filter_width_ns, ctlr->clk_rate_khz); + ctlr->scl_fall_time = t.scl_fall_ns; + ctlr->scl_rise_time = t.scl_rise_ns; + ctlr->sda_hold_time = t.sda_hold_ns; + + switch (ctlr->bus_freq_hz) { + case I2C_MAX_FAST_MODE_FREQ: + speed_mode = HISI_I2C_FAST_SPEED_MODE; + hisi_i2c_set_scl(ctlr, 26, 76, HISI_I2C_FS_SCL_HCNT, HISI_I2C_FS_SCL_LCNT); + break; + case I2C_MAX_HIGH_SPEED_MODE_FREQ: + speed_mode = HISI_I2C_HIGH_SPEED_MODE; + hisi_i2c_set_scl(ctlr, 6, 22, HISI_I2C_HS_SCL_HCNT, HISI_I2C_HS_SCL_LCNT); + break; + case I2C_MAX_STANDARD_MODE_FREQ: + default: + speed_mode = HISI_I2C_STD_SPEED_MODE; + + /* For default condition force the bus speed to standard mode. */ + ctlr->bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ; + hisi_i2c_set_scl(ctlr, 40, 87, HISI_I2C_SS_SCL_HCNT, HISI_I2C_SS_SCL_LCNT); + break; + } + + reg = readl(ctlr->iobase + HISI_I2C_FRAME_CTRL); + reg &= ~HISI_I2C_FRAME_CTRL_SPEED_MODE; + reg |= FIELD_PREP(HISI_I2C_FRAME_CTRL_SPEED_MODE, speed_mode); + writel(reg, ctlr->iobase + HISI_I2C_FRAME_CTRL); + + sda_hold_cnt = NSEC_TO_CYCLES(ctlr->sda_hold_time, ctlr->clk_rate_khz); + + reg = FIELD_PREP(HISI_I2C_SDA_HOLD_TX, sda_hold_cnt); + writel(reg, ctlr->iobase + HISI_I2C_SDA_HOLD); + + writel(ctlr->spk_len, ctlr->iobase + HISI_I2C_FS_SPK_LEN); + + reg = FIELD_PREP(HISI_I2C_FIFO_RX_AF_THRESH, HISI_I2C_RX_F_AF_THRESH); + reg |= FIELD_PREP(HISI_I2C_FIFO_TX_AE_THRESH, HISI_I2C_TX_F_AE_THRESH); + writel(reg, ctlr->iobase + HISI_I2C_FIFO_CTRL); +} + +static int hisi_i2c_probe(struct platform_device *pdev) +{ + struct hisi_i2c_controller *ctlr; + struct device *dev = &pdev->dev; + struct i2c_adapter *adapter; + u32 hw_version; + int ret; + + ctlr = devm_kzalloc(dev, sizeof(*ctlr), GFP_KERNEL); + if (!ctlr) + return -ENOMEM; + + ctlr->dev = dev; + ctlr->iobase = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(ctlr->iobase)) + return PTR_ERR(ctlr->iobase); + + ctlr->irq = platform_get_irq(pdev, 0); + if (ctlr->irq < 0) + return ctlr->irq; + + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); + + ret = devm_request_irq(dev, ctlr->irq, hisi_i2c_irq, + 0, "hisi-i2c", ctlr); + if (ret) { + dev_err(dev, "failed to request irq handler, ret = %d.\n", ret); + return ret; + } + + ret = device_property_read_u64(dev, "clk_rate", &ctlr->clk_rate_khz); + if (ret) { + dev_err(dev, "failed to get clock frequency, ret = %d.\n", ret); + return ret; + } + ctlr->clk_rate_khz = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz, 1000); + + hisi_i2c_configure_bus(ctlr); + + adapter = &ctlr->adapter; + snprintf(adapter->name, sizeof(adapter->name), + "HiSilicon I2C Controller %s", dev_name(dev)); + adapter->owner = THIS_MODULE; + adapter->algo = &hisi_i2c_algo; + adapter->dev.parent = dev; + i2c_set_adapdata(adapter, ctlr); + + ret = devm_i2c_add_adapter(dev, adapter); + if (ret) { + dev_err(dev, "failed to add i2c adapter, ret = %d.\n", ret); + return ret; + } + + hw_version = readl(ctlr->iobase + HISI_I2C_VERSION); + dev_info(ctlr->dev, "speed mode %s. hw version 0x%x.\n", + hisi_i2c_speed_string(ctlr->bus_freq_hz), hw_version); + + return 0; +} + +static const struct acpi_device_id hisi_i2c_acpi_ids[] = { + { "HISI03D1", 0 }, + { } +}; +MODULE_DEVICE_TABLE(acpi, hisi_i2c_acpi_ids); +MODULE_ALIAS("platform:hisi-i2c"); + +static struct platform_driver hisi_i2c_driver = { + .probe = hisi_i2c_probe, + .driver = { + .name = "hisi-i2c", + .acpi_match_table = hisi_i2c_acpi_ids, + }, +}; + +module_platform_driver(hisi_i2c_driver); + +MODULE_AUTHOR("Yicong Yang <yangyicong@hisilicon.com>"); +MODULE_DESCRIPTION("HiSilicon I2C Controller Driver"); +MODULE_LICENSE("GPL"); -- 2.8.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-22 11:10 ` [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller Yicong Yang @ 2021-03-22 15:21 ` Dmitry Osipenko 2021-03-24 9:30 ` Yicong Yang 2021-03-22 16:59 ` Andy Shevchenko 2021-03-22 17:04 ` Andy Shevchenko 2 siblings, 1 reply; 19+ messages in thread From: Dmitry Osipenko @ 2021-03-22 15:21 UTC (permalink / raw) To: Yicong Yang, wsa, linux-i2c Cc: andriy.shevchenko, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm Hello Yicong, 22.03.2021 14:10, Yicong Yang пишет: > Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides > the access to the i2c busses, which connects to the eeprom, rtc, etc. > > The driver works with IRQ mode, and supports basic I2C features and 10bit > address. The DMA is not supported. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/i2c/busses/Kconfig | 10 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-hisi.c | 525 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 536 insertions(+) > create mode 100644 drivers/i2c/busses/i2c-hisi.c ... > + > +#define NSEC_TO_CYCLES(ns, clk_rate_khz) (DIV_ROUND_UP_ULL((clk_rate_khz) * (ns), NSEC_PER_MSEC)) This is a very long line, you should split it into two. Parens around DIV_ROUND_UP_ULL aren't needed. ... > +static void hisi_i2c_enable_int(struct hisi_i2c_controller *ctlr, u32 mask) > +{ > + writel(mask, ctlr->iobase + HISI_I2C_INT_MASK); Why you don't use relaxed versions of readl/writel? Do you really need to insert memory barriers? > +} > + > +static void hisi_i2c_disable_int(struct hisi_i2c_controller *ctlr, u32 mask) > +{ > + writel((~mask) & HISI_I2C_INT_ALL, ctlr->iobase + HISI_I2C_INT_MASK); > +} > + > +static void hisi_i2c_clear_int(struct hisi_i2c_controller *ctlr, u32 mask) > +{ > + writel(mask, ctlr->iobase + HISI_I2C_INT_CLR); > +} > + > +static void hisi_i2c_handle_errors(struct hisi_i2c_controller *ctlr) > +{ > + u32 int_err = ctlr->xfer_err, reg; > + > + if (int_err & HISI_I2C_INT_FIFO_ERR) { > + reg = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); > + > + if (reg & HISI_I2C_FIFO_STATE_RX_RERR) > + dev_err(ctlr->dev, "rx fifo error read.\n"); The dot "." in the end of error messages is unnecessary. ... > +/* > + * Initialize the transfer information and start the I2C bus transfer. > + * We only configure the transfer and do some pre/post works here, and > + * wait for the transfer done. The major transfer process is performed > + * in the IRQ handler. > + */ > +static int hisi_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, > + int num) > +{ > + struct hisi_i2c_controller *ctlr = i2c_get_adapdata(adap); > + DECLARE_COMPLETION_ONSTACK(done); > + int ret = num; > + > + hisi_i2c_reset_xfer(ctlr); > + ctlr->completion = &done; > + ctlr->msg_num = num; > + ctlr->msgs = msgs; > + > + hisi_i2c_start_xfer(ctlr); > + > + if (!wait_for_completion_timeout(ctlr->completion, adap->timeout)) { > + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); This doesn't save you from racing with the interrupt handler. It looks like you need to enable/disable IRQ around the completion, similarly to what NVIDIA Tegra I2C driver does. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-22 15:21 ` Dmitry Osipenko @ 2021-03-24 9:30 ` Yicong Yang 2021-03-24 12:26 ` Dmitry Osipenko 0 siblings, 1 reply; 19+ messages in thread From: Yicong Yang @ 2021-03-24 9:30 UTC (permalink / raw) To: Dmitry Osipenko, wsa, linux-i2c Cc: andriy.shevchenko, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm Hi Dmitry, Thanks for reviewing this. On 2021/3/22 23:21, Dmitry Osipenko wrote: > Hello Yicong, > > 22.03.2021 14:10, Yicong Yang пишет: >> Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides >> the access to the i2c busses, which connects to the eeprom, rtc, etc. >> >> The driver works with IRQ mode, and supports basic I2C features and 10bit >> address. The DMA is not supported. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> drivers/i2c/busses/Kconfig | 10 + >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-hisi.c | 525 ++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 536 insertions(+) >> create mode 100644 drivers/i2c/busses/i2c-hisi.c > > ... >> + >> +#define NSEC_TO_CYCLES(ns, clk_rate_khz) (DIV_ROUND_UP_ULL((clk_rate_khz) * (ns), NSEC_PER_MSEC)) > > This is a very long line, you should split it into two. will split. > > Parens around DIV_ROUND_UP_ULL aren't needed. > will drop the parens. > ... >> +static void hisi_i2c_enable_int(struct hisi_i2c_controller *ctlr, u32 mask) >> +{ >> + writel(mask, ctlr->iobase + HISI_I2C_INT_MASK); > > Why you don't use relaxed versions of readl/writel? Do you really need > to insert memory barriers? > this will not be used during the transfer process, so a relaxed version of readl/writel will not have performance enhancement. the barriers are necessary as i want to make the operations in order to avoid potential problems caused by reordering. >> +} >> + >> +static void hisi_i2c_disable_int(struct hisi_i2c_controller *ctlr, u32 mask) >> +{ >> + writel((~mask) & HISI_I2C_INT_ALL, ctlr->iobase + HISI_I2C_INT_MASK); >> +} >> + >> +static void hisi_i2c_clear_int(struct hisi_i2c_controller *ctlr, u32 mask) >> +{ >> + writel(mask, ctlr->iobase + HISI_I2C_INT_CLR); >> +} >> + >> +static void hisi_i2c_handle_errors(struct hisi_i2c_controller *ctlr) >> +{ >> + u32 int_err = ctlr->xfer_err, reg; >> + >> + if (int_err & HISI_I2C_INT_FIFO_ERR) { >> + reg = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); >> + >> + if (reg & HISI_I2C_FIFO_STATE_RX_RERR) >> + dev_err(ctlr->dev, "rx fifo error read.\n"); > > The dot "." in the end of error messages is unnecessary. > i'd like to keep this, as i think this is rather driver specific and not violating any rules. > ... >> +/* >> + * Initialize the transfer information and start the I2C bus transfer. >> + * We only configure the transfer and do some pre/post works here, and >> + * wait for the transfer done. The major transfer process is performed >> + * in the IRQ handler. >> + */ >> +static int hisi_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, >> + int num) >> +{ >> + struct hisi_i2c_controller *ctlr = i2c_get_adapdata(adap); >> + DECLARE_COMPLETION_ONSTACK(done); >> + int ret = num; >> + >> + hisi_i2c_reset_xfer(ctlr); >> + ctlr->completion = &done; >> + ctlr->msg_num = num; >> + ctlr->msgs = msgs; >> + >> + hisi_i2c_start_xfer(ctlr); >> + >> + if (!wait_for_completion_timeout(ctlr->completion, adap->timeout)) { >> + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); > > This doesn't save you from racing with the interrupt handler. It looks > like you need to enable/disable IRQ around the completion, similarly to > what NVIDIA Tegra I2C driver does. > thanks for suggestion. the hardware between tegra and this one is a little different as we don't provide a way to reinit the controller. so {synchronize,disable}_irq() after mask the interrupt here will avoid the race. Thanks, Yicong > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-24 9:30 ` Yicong Yang @ 2021-03-24 12:26 ` Dmitry Osipenko 0 siblings, 0 replies; 19+ messages in thread From: Dmitry Osipenko @ 2021-03-24 12:26 UTC (permalink / raw) To: Yicong Yang, wsa, linux-i2c Cc: andriy.shevchenko, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm 24.03.2021 12:30, Yicong Yang пишет: ... >>> +static void hisi_i2c_enable_int(struct hisi_i2c_controller *ctlr, u32 mask) >>> +{ >>> + writel(mask, ctlr->iobase + HISI_I2C_INT_MASK); >> >> Why you don't use relaxed versions of readl/writel? Do you really need >> to insert memory barriers? >> > > this will not be used during the transfer process, so a relaxed version of readl/writel > will not have performance enhancement. > > the barriers are necessary as i want to make the operations in order to avoid potential > problems caused by reordering. The iomap is strongly ordered, hence register accesses are always ordered. The barrier ensures that CPU memory accesses are finished before h/w registers are touched. Looks like you don't need to worry about the memory barrier in the case of this driver. >>> +} >>> + >>> +static void hisi_i2c_disable_int(struct hisi_i2c_controller *ctlr, u32 mask) >>> +{ >>> + writel((~mask) & HISI_I2C_INT_ALL, ctlr->iobase + HISI_I2C_INT_MASK); >>> +} >>> + >>> +static void hisi_i2c_clear_int(struct hisi_i2c_controller *ctlr, u32 mask) >>> +{ >>> + writel(mask, ctlr->iobase + HISI_I2C_INT_CLR); >>> +} >>> + >>> +static void hisi_i2c_handle_errors(struct hisi_i2c_controller *ctlr) >>> +{ >>> + u32 int_err = ctlr->xfer_err, reg; >>> + >>> + if (int_err & HISI_I2C_INT_FIFO_ERR) { >>> + reg = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); >>> + >>> + if (reg & HISI_I2C_FIFO_STATE_RX_RERR) >>> + dev_err(ctlr->dev, "rx fifo error read.\n"); >> >> The dot "." in the end of error messages is unnecessary. >> > > i'd like to keep this, as i think this is rather driver specific and not > violating any rules. The common kernel style is *not* to have the dot + some other messages in this driver already don't have it. Should be better if you could remove it. >>> +/* >>> + * Initialize the transfer information and start the I2C bus transfer. >>> + * We only configure the transfer and do some pre/post works here, and >>> + * wait for the transfer done. The major transfer process is performed >>> + * in the IRQ handler. >>> + */ >>> +static int hisi_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, >>> + int num) >>> +{ >>> + struct hisi_i2c_controller *ctlr = i2c_get_adapdata(adap); >>> + DECLARE_COMPLETION_ONSTACK(done); >>> + int ret = num; >>> + >>> + hisi_i2c_reset_xfer(ctlr); >>> + ctlr->completion = &done; >>> + ctlr->msg_num = num; >>> + ctlr->msgs = msgs; >>> + >>> + hisi_i2c_start_xfer(ctlr); >>> + >>> + if (!wait_for_completion_timeout(ctlr->completion, adap->timeout)) { >>> + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); >> >> This doesn't save you from racing with the interrupt handler. It looks >> like you need to enable/disable IRQ around the completion, similarly to >> what NVIDIA Tegra I2C driver does. >> > > thanks for suggestion. > > the hardware between tegra and this one is a little different as we don't provide > a way to reinit the controller. so {synchronize,disable}_irq() after mask > the interrupt here will avoid the race. The disable/enable will be ideal, but synchronize should be good enough as well. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-22 11:10 ` [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller Yicong Yang 2021-03-22 15:21 ` Dmitry Osipenko @ 2021-03-22 16:59 ` Andy Shevchenko 2021-03-24 10:07 ` Yicong Yang 2021-03-22 17:04 ` Andy Shevchenko 2 siblings, 1 reply; 19+ messages in thread From: Andy Shevchenko @ 2021-03-22 16:59 UTC (permalink / raw) To: Yicong Yang Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On Mon, Mar 22, 2021 at 07:10:12PM +0800, Yicong Yang wrote: > Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides > the access to the i2c busses, which connects to the eeprom, rtc, etc. > > The driver works with IRQ mode, and supports basic I2C features and 10bit > address. The DMA is not supported. ... > +#include <linux/acpi.h> Hadn't noticed how you are using this header. > +#include <linux/bits.h> > +#include <linux/bitfield.h> > +#include <linux/completion.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> Missed mod_devicetable.h. Probably missed property.h, but not sure. > +#include <linux/platform_device.h> ... > +#define HISI_I2C_INT_ALL 0x1f GENMASK() ? ... > +#define HISI_I2C_INT_ERR (HISI_I2C_INT_TRANS_ERR | \ > + HISI_I2C_INT_FIFO_ERR) Either one line, or it will look better like #define HISI_I2C_INT_ERR \ (HISI_I2C_INT_TRANS_ERR | HISI_I2C_INT_FIFO_ERR) ... > +#define HISI_I2C_STD_SPEED_MODE 0x0 > +#define HISI_I2C_FAST_SPEED_MODE 0x1 > +#define HISI_I2C_HIGH_SPEED_MODE 0x2 Why not plain decimal numbers? ... > +struct hisi_i2c_controller { > + struct device *dev; > + struct i2c_adapter adapter; If you put this as a first member, the container_of() become a no-op for this. But I dunno if it's used against this structure. > + void __iomem *iobase; > + int irq; > + > + /* Intermediates for recording the transfer process */ > + struct completion *completion; > + struct i2c_msg *msgs; > + int msg_num; > + int msg_tx_idx; > + int buf_tx_idx; > + int msg_rx_idx; > + int buf_rx_idx; > + u16 tar_addr; > + u32 xfer_err; > + > + /* I2C bus configuration */ > + u32 scl_fall_time; > + u32 scl_rise_time; > + u32 sda_hold_time; > + u64 clk_rate_khz; > + u32 bus_freq_hz; > + u32 spk_len; > +}; ... > + struct i2c_timings t; > + > + i2c_parse_fw_timings(ctlr->dev, &t, true); > + ctlr->bus_freq_hz = t.bus_freq_hz; > + ctlr->scl_fall_time = t.scl_fall_ns; > + ctlr->scl_rise_time = t.scl_rise_ns; > + ctlr->sda_hold_time = t.sda_hold_ns; Why not simply to have the timings structure embedded into hisi_i2c_controller one? ... > + ctlr->dev = dev; Would it make sense to assign aster getting IRQ resource... > + ctlr->iobase = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(ctlr->iobase)) > + return PTR_ERR(ctlr->iobase); > + > + ctlr->irq = platform_get_irq(pdev, 0); > + if (ctlr->irq < 0) > + return ctlr->irq; ...somewhere here? > + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); ... > + ctlr->clk_rate_khz = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz, 1000); HZ_PER_KHZ ? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-22 16:59 ` Andy Shevchenko @ 2021-03-24 10:07 ` Yicong Yang 2021-03-24 11:15 ` Andy Shevchenko 0 siblings, 1 reply; 19+ messages in thread From: Yicong Yang @ 2021-03-24 10:07 UTC (permalink / raw) To: Andy Shevchenko Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm Hi Andy, Thanks for reviewing this. On 2021/3/23 0:59, Andy Shevchenko wrote: > On Mon, Mar 22, 2021 at 07:10:12PM +0800, Yicong Yang wrote: >> Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides >> the access to the i2c busses, which connects to the eeprom, rtc, etc. >> >> The driver works with IRQ mode, and supports basic I2C features and 10bit >> address. The DMA is not supported. > > ... > >> +#include <linux/acpi.h> > > Hadn't noticed how you are using this header. will remove this. > >> +#include <linux/bits.h> >> +#include <linux/bitfield.h> >> +#include <linux/completion.h> >> +#include <linux/i2c.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/module.h> > > Missed mod_devicetable.h. > Probably missed property.h, but not sure.> i think this has been included implicitly as the module compilation worked well. >> +#include <linux/platform_device.h> > > ... > >> +#define HISI_I2C_INT_ALL 0x1f > > GENMASK() ? will change to that. > > ... > >> +#define HISI_I2C_INT_ERR (HISI_I2C_INT_TRANS_ERR | \ >> + HISI_I2C_INT_FIFO_ERR) > > Either one line, or it will look better like > #define HISI_I2C_INT_ERR \ > (HISI_I2C_INT_TRANS_ERR | HISI_I2C_INT_FIFO_ERR) > sure. that looks better. > ... > >> +#define HISI_I2C_STD_SPEED_MODE 0x0 >> +#define HISI_I2C_FAST_SPEED_MODE 0x1 >> +#define HISI_I2C_HIGH_SPEED_MODE 0x2 > > Why not plain decimal numbers? > it's something will be written to the register, so i make them hexadecimal to better corresponding to the register fields. > ... > >> +struct hisi_i2c_controller { >> + struct device *dev; > >> + struct i2c_adapter adapter; > > If you put this as a first member, the container_of() become a no-op for this. > But I dunno if it's used against this structure. > the container_of() is not used in this drivers. but it's fine to swap these two fields. thanks for the information. >> + void __iomem *iobase; >> + int irq; >> + >> + /* Intermediates for recording the transfer process */ >> + struct completion *completion; >> + struct i2c_msg *msgs; >> + int msg_num; >> + int msg_tx_idx; >> + int buf_tx_idx; >> + int msg_rx_idx; >> + int buf_rx_idx; >> + u16 tar_addr; >> + u32 xfer_err; >> + >> + /* I2C bus configuration */ >> + u32 scl_fall_time; >> + u32 scl_rise_time; >> + u32 sda_hold_time; >> + u64 clk_rate_khz; >> + u32 bus_freq_hz; >> + u32 spk_len; >> +}; > > ... > >> + struct i2c_timings t; >> + >> + i2c_parse_fw_timings(ctlr->dev, &t, true); >> + ctlr->bus_freq_hz = t.bus_freq_hz; > >> + ctlr->scl_fall_time = t.scl_fall_ns; >> + ctlr->scl_rise_time = t.scl_rise_ns; >> + ctlr->sda_hold_time = t.sda_hold_ns; > > Why not simply to have the timings structure embedded into hisi_i2c_controller > one? > not all the fields of the timing structures are needed to be stored, only three of them are necessary. > ... > >> + ctlr->dev = dev; > > Would it make sense to assign aster getting IRQ resource... > >> + ctlr->iobase = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(ctlr->iobase)) >> + return PTR_ERR(ctlr->iobase); >> + >> + ctlr->irq = platform_get_irq(pdev, 0); >> + if (ctlr->irq < 0) >> + return ctlr->irq; > > ...somewhere here? i cannot see any differences and some other drivers also assign the 'dev' before getting IRQ resources. are there any considerations on this? > >> + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); > > ... > >> + ctlr->clk_rate_khz = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz, 1000); > > HZ_PER_KHZ ? the macro is not public. seems it's redundant to have this macro which will only be used once. Thanks, Yicong > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-24 10:07 ` Yicong Yang @ 2021-03-24 11:15 ` Andy Shevchenko 0 siblings, 0 replies; 19+ messages in thread From: Andy Shevchenko @ 2021-03-24 11:15 UTC (permalink / raw) To: Yicong Yang Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On Wed, Mar 24, 2021 at 06:07:17PM +0800, Yicong Yang wrote: > On 2021/3/23 0:59, Andy Shevchenko wrote: > > On Mon, Mar 22, 2021 at 07:10:12PM +0800, Yicong Yang wrote: ... > > Missed mod_devicetable.h. > > Probably missed property.h, but not sure.> > > i think this has been included implicitly as the module compilation worked well. The rule of thumb is to include headers you are direct user of. The implicit inclusion is allowed if and only if there is a guarantee by explicit inclusion that an implicit one will be included no matter what. I don't remember we have such guarantee for mod_devicetable.h. On top of that, it's performance wise to explicitly include, otherwise it's an additional burden for compiler and thus on electrical plant and hence not environment friendly. And all this for peanuts. ... > >> +#define HISI_I2C_STD_SPEED_MODE 0x0 > >> +#define HISI_I2C_FAST_SPEED_MODE 0x1 > >> +#define HISI_I2C_HIGH_SPEED_MODE 0x2 > > > > Why not plain decimal numbers? > > it's something will be written to the register, so i make them > hexadecimal to better corresponding to the register fields. Are they bits? No. Why hex numbers? Please, give a better justification. ... > >> + ctlr->bus_freq_hz = t.bus_freq_hz; > > > >> + ctlr->scl_fall_time = t.scl_fall_ns; > >> + ctlr->scl_rise_time = t.scl_rise_ns; > >> + ctlr->sda_hold_time = t.sda_hold_ns; > > > > Why not simply to have the timings structure embedded into hisi_i2c_controller > > one? > > > > not all the fields of the timing structures are needed to be stored, only three > of them are necessary. Four as far as I see. But for the sake of standardization I would keep a whole structure. It will be easier to grep and find users of it (looking into data structures only). ... > >> + ctlr->dev = dev; > > > > Would it make sense to assign aster getting IRQ resource... > > > >> + ctlr->iobase = devm_platform_ioremap_resource(pdev, 0); > >> + if (IS_ERR(ctlr->iobase)) > >> + return PTR_ERR(ctlr->iobase); > >> + > >> + ctlr->irq = platform_get_irq(pdev, 0); > >> + if (ctlr->irq < 0) > >> + return ctlr->irq; > > > > ...somewhere here? > > i cannot see any differences and some other drivers also assign the 'dev' before getting IRQ > resources. > > are there any considerations on this? Of course. The rule of thumb: be lazy. Why do you assign earlier if a) there are no users; b) in between it may bail out with error ? Give a better justification why it should be there. ... > >> + ctlr->clk_rate_khz = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz, 1000); > > > > HZ_PER_KHZ ? > > the macro is not public. seems it's redundant to have this macro which > will only be used once. It will be easier to see how many users of it we have. It's not needed to make it public right now, but keeping something like #define HZ_PER_KHZ 1000L in this file will be helpful. Give a better justification why it should not be there. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-22 11:10 ` [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller Yicong Yang 2021-03-22 15:21 ` Dmitry Osipenko 2021-03-22 16:59 ` Andy Shevchenko @ 2021-03-22 17:04 ` Andy Shevchenko 2021-03-24 10:21 ` Yicong Yang 2 siblings, 1 reply; 19+ messages in thread From: Andy Shevchenko @ 2021-03-22 17:04 UTC (permalink / raw) To: Yicong Yang Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On Mon, Mar 22, 2021 at 07:10:12PM +0800, Yicong Yang wrote: > Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides > the access to the i2c busses, which connects to the eeprom, rtc, etc. > > The driver works with IRQ mode, and supports basic I2C features and 10bit > address. The DMA is not supported. ... > +static const char *hisi_i2c_speed_string(u32 bus_freq_hz) > +{ > + switch (bus_freq_hz) { > + case I2C_MAX_STANDARD_MODE_FREQ: > + return "100K"; > + case I2C_MAX_FAST_MODE_FREQ: > + return "400K"; > + case I2C_MAX_HIGH_SPEED_MODE_FREQ: > + return "3.4M"; > + default: > + return "unknown"; > + } > +} Just realized that if you print the name of the mode (and maybe frequency value) then it can be moved to generic I²C code and other will benefit out of this (DesignWare is the first in my mind). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-22 17:04 ` Andy Shevchenko @ 2021-03-24 10:21 ` Yicong Yang 2021-03-24 11:16 ` Andy Shevchenko 0 siblings, 1 reply; 19+ messages in thread From: Yicong Yang @ 2021-03-24 10:21 UTC (permalink / raw) To: Andy Shevchenko Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On 2021/3/23 1:04, Andy Shevchenko wrote: > On Mon, Mar 22, 2021 at 07:10:12PM +0800, Yicong Yang wrote: >> Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides >> the access to the i2c busses, which connects to the eeprom, rtc, etc. >> >> The driver works with IRQ mode, and supports basic I2C features and 10bit >> address. The DMA is not supported. > > ... > >> +static const char *hisi_i2c_speed_string(u32 bus_freq_hz) >> +{ >> + switch (bus_freq_hz) { >> + case I2C_MAX_STANDARD_MODE_FREQ: >> + return "100K"; >> + case I2C_MAX_FAST_MODE_FREQ: >> + return "400K"; >> + case I2C_MAX_HIGH_SPEED_MODE_FREQ: >> + return "3.4M"; >> + default: >> + return "unknown"; >> + } >> +} > > Just realized that if you print the name of the mode (and maybe frequency > value) then it can be moved to generic I²C code and other will benefit out of > this (DesignWare is the first in my mind). sure, that's good. but the i2c core doesn't make use of the speed mode information so maybe print of this information is rather driver depended. > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller 2021-03-24 10:21 ` Yicong Yang @ 2021-03-24 11:16 ` Andy Shevchenko 0 siblings, 0 replies; 19+ messages in thread From: Andy Shevchenko @ 2021-03-24 11:16 UTC (permalink / raw) To: Yicong Yang Cc: wsa, linux-i2c, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, prime.zeng, linuxarm On Wed, Mar 24, 2021 at 06:21:36PM +0800, Yicong Yang wrote: > On 2021/3/23 1:04, Andy Shevchenko wrote: > > On Mon, Mar 22, 2021 at 07:10:12PM +0800, Yicong Yang wrote: ... > >> +static const char *hisi_i2c_speed_string(u32 bus_freq_hz) > >> +{ > >> + switch (bus_freq_hz) { > >> + case I2C_MAX_STANDARD_MODE_FREQ: > >> + return "100K"; > >> + case I2C_MAX_FAST_MODE_FREQ: > >> + return "400K"; > >> + case I2C_MAX_HIGH_SPEED_MODE_FREQ: > >> + return "3.4M"; > >> + default: > >> + return "unknown"; > >> + } > >> +} > > > > Just realized that if you print the name of the mode (and maybe frequency > > value) then it can be moved to generic I²C code and other will benefit out of > > this (DesignWare is the first in my mind). > > sure, that's good. but the i2c core doesn't make use of the speed mode > information so maybe print of this information is rather driver depended. Yes, but it's useful. And since we will have at least two users of this it is a good justification to use I²C core to keep and provide this API. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 3/3] MAINTAINERS: Add maintainer for HiSilicon I2C driver 2021-03-22 11:10 [PATCH v3 0/3] Add support for HiSilicon I2C controller Yicong Yang 2021-03-22 11:10 ` [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters Yicong Yang 2021-03-22 11:10 ` [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller Yicong Yang @ 2021-03-22 11:10 ` Yicong Yang 2 siblings, 0 replies; 19+ messages in thread From: Yicong Yang @ 2021-03-22 11:10 UTC (permalink / raw) To: wsa, linux-i2c Cc: andriy.shevchenko, digetx, treding, jarkko.nikula, rmk+kernel, song.bao.hua, john.garry, yangyicong, prime.zeng, linuxarm Add maintainer for HiSilicon I2C driver. Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index aa84121..10aaa9a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8040,6 +8040,13 @@ F: drivers/crypto/hisilicon/hpre/hpre.h F: drivers/crypto/hisilicon/hpre/hpre_crypto.c F: drivers/crypto/hisilicon/hpre/hpre_main.c +HISILICON I2C CONTROLLER DRIVER +M: Yicong Yang <yangyicong@hisilicon.com> +L: linux-i2c@vger.kernel.org +S: Maintained +W: https://www.hisilicon.com +F: drivers/i2c/busses/i2c-hisi.c + HISILICON LPC BUS DRIVER M: john.garry@huawei.com S: Maintained -- 2.8.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-03-24 12:26 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-22 11:10 [PATCH v3 0/3] Add support for HiSilicon I2C controller Yicong Yang 2021-03-22 11:10 ` [PATCH v3 1/3] i2c: core: add managed function for adding i2c adapters Yicong Yang 2021-03-22 16:35 ` Andy Shevchenko 2021-03-24 8:26 ` Yicong Yang 2021-03-24 11:05 ` Andy Shevchenko 2021-03-22 16:45 ` Dmitry Osipenko 2021-03-24 8:29 ` Yicong Yang 2021-03-24 11:06 ` Andy Shevchenko 2021-03-22 11:10 ` [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller Yicong Yang 2021-03-22 15:21 ` Dmitry Osipenko 2021-03-24 9:30 ` Yicong Yang 2021-03-24 12:26 ` Dmitry Osipenko 2021-03-22 16:59 ` Andy Shevchenko 2021-03-24 10:07 ` Yicong Yang 2021-03-24 11:15 ` Andy Shevchenko 2021-03-22 17:04 ` Andy Shevchenko 2021-03-24 10:21 ` Yicong Yang 2021-03-24 11:16 ` Andy Shevchenko 2021-03-22 11:10 ` [PATCH v3 3/3] MAINTAINERS: Add maintainer for HiSilicon I2C driver Yicong Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).