Re: [PATCH 2/2] i2c: Add Spreadtrum I2C controller driver
Hi Andy,
Sorry for late reply due to my business trip.
On ε
, 6ζ 17, 2017 at 08:18:49δΈε +0300, Andy Shevchenko wrote:
> On Thu, Jun 15, 2017 at 2:30 PM, Baolin Wang
> wrote:
> > This patch adds the I2C controller driver for Spreadtrum platform.
>
>
> > + i2c_dev->irq = platform_get_irq(pdev, 0);
> > + if (i2c_dev->irq < 0) {
> > + dev_err(&pdev->dev, "failed to get irq resource\n");
>
> > + return -ENXIO;
>
> Why shadow actual error?
Sorry for missing this and will fix in next version.
>
> > + }
>
>
> > + if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
> > + i2c_dev->bus_freq = prop;
> > +
> > + sprd_i2c_clk_init(i2c_dev);
> > + platform_set_drvdata(pdev, i2c_dev);
> > +
> > + pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);
> > + pm_runtime_use_autosuspend(i2c_dev->dev);
> > + pm_runtime_set_active(i2c_dev->dev);
> > + pm_runtime_enable(i2c_dev->dev);
> > +
> > + ret = pm_runtime_get_sync(i2c_dev->dev);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "i2c%d pm runtime resume failed!\n",
> > + pdev->id);
>
> > + return ret;
>
> goto error;
Yes, will fix it.
>
> > + }
> > +
> > + ret = devm_request_threaded_irq(dev, i2c_dev->irq,
> > + sprd_i2c_isr, sprd_i2c_isr_thread,
> > + IRQF_NO_SUSPEND | IRQF_ONESHOT,
> > + pdev->name, i2c_dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to request irq %d\n",
> > i2c_dev->irq);
> > + goto error;
> > + }
> > +
> > + ret = i2c_add_numbered_adapter(&i2c_dev->adap);
> > + if (ret) {
> > + dev_err(&pdev->dev, "add adapter failed\n");
> > + goto error;
> > + }
> > +
> > + pm_runtime_mark_last_busy(i2c_dev->dev);
> > + pm_runtime_put_autosuspend(i2c_dev->dev);
> > + return 0;
> > +
> > +error:
> > + pm_runtime_put_noidle(i2c_dev->dev);
> > + pm_runtime_disable(i2c_dev->dev);
> > + return ret;
> > +}
> > +
> > +static int sprd_i2c_remove(struct platform_device *pdev)
> > +{
> > + struct sprd_i2c *i2c_dev = platform_get_drvdata(pdev);
> > + int ret;
> > +
>
> > + ret = pm_runtime_get_sync(i2c_dev->dev);
> > + if (ret < 0)
> > + return ret;
>
> Does it make any sense? Doesn't device core power on the device before
> calling ->remove() ?
Yes, you are right. We need power on device in probe() function.
>
> > +
> > + i2c_del_adapter(&i2c_dev->adap);
> > +
> > + if (!IS_ERR_OR_NULL(i2c_dev->clk))
>
> _OR_NULL looks suspicious.
Since our ->clk can be NULL as one optional connfig in case we test I2C
driver on FPGA platform which does not support clock operation.
>
> > + clk_unprepare(i2c_dev->clk);
> > +
> > + pm_runtime_put_noidle(i2c_dev->dev);
> > + pm_runtime_disable(i2c_dev->dev);
> > +
> > + return 0;
> > +}
> > +
>
> > +#ifdef CONFIG_PM_SLEEP
>
> __maybe_unused instead?
OK.
>
> > +static int sprd_i2c_suspend_noirq(struct device *pdev)
>
> > +static int sprd_i2c_resume_noirq(struct device *pdev)
>
> > +#endif /* CONFIG_PM_SLEEP */
>
> > +#ifdef CONFIG_PM
>
> Ditto.
OK.
>
> > +static int sprd_i2c_runtime_suspend(struct device *pdev)
>
> > +}
>
> > +static int sprd_i2c_runtime_resume(struct device *pdev)
> > +{
>
> > + clk_prepare_enable(i2c_dev->clk);
>
> This might fail.
Yes, will check return value.
>
>
> > +}
> > +#endif /* CONFIG_PM */
>
>
> > +static struct platform_driver sprd_i2c_driver = {
> > + .probe = sprd_i2c_probe,
> > + .remove = sprd_i2c_remove,
> > + .driver = {
> > + .name = "sprd-i2c",
>
> > + .of_match_table = of_match_ptr(sprd_i2c_of_match),
>
> of_match_ptr seems redundant.
OK.
>
> > + .pm = &sprd_i2c_pm_ops,
> > + },
> > +};
> > +
>
> > +static int __init sprd_i2c_init(void)
> > +{
> > + return platform_driver_register(&sprd_i2c_driver);
> > +}
>
> > +arch_initcall_sync(sprd_i2c_init);
>
> Why?
IN our Spreadtrum platform, our regulator driver will depend on I2C driver
and the regulator driver uses subsys_initcall() level to initialize. Moreover
some other drivers like GPU, they will depend on regulator to set voltage and
they also need initialization much earlier. Thanks for your comments.
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH 2/2] i2c: Add Spreadtrum I2C controller driver
On Thu, Jun 15, 2017 at 2:30 PM, Baolin Wang wrote:
> This patch adds the I2C controller driver for Spreadtrum platform.
> + i2c_dev->irq = platform_get_irq(pdev, 0);
> + if (i2c_dev->irq < 0) {
> + dev_err(&pdev->dev, "failed to get irq resource\n");
> + return -ENXIO;
Why shadow actual error?
> + }
> + if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
> + i2c_dev->bus_freq = prop;
> +
> + sprd_i2c_clk_init(i2c_dev);
> + platform_set_drvdata(pdev, i2c_dev);
> +
> + pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);
> + pm_runtime_use_autosuspend(i2c_dev->dev);
> + pm_runtime_set_active(i2c_dev->dev);
> + pm_runtime_enable(i2c_dev->dev);
> +
> + ret = pm_runtime_get_sync(i2c_dev->dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "i2c%d pm runtime resume failed!\n",
> + pdev->id);
> + return ret;
goto error;
> + }
> +
> + ret = devm_request_threaded_irq(dev, i2c_dev->irq,
> + sprd_i2c_isr, sprd_i2c_isr_thread,
> + IRQF_NO_SUSPEND | IRQF_ONESHOT,
> + pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request irq %d\n",
> i2c_dev->irq);
> + goto error;
> + }
> +
> + ret = i2c_add_numbered_adapter(&i2c_dev->adap);
> + if (ret) {
> + dev_err(&pdev->dev, "add adapter failed\n");
> + goto error;
> + }
> +
> + pm_runtime_mark_last_busy(i2c_dev->dev);
> + pm_runtime_put_autosuspend(i2c_dev->dev);
> + return 0;
> +
> +error:
> + pm_runtime_put_noidle(i2c_dev->dev);
> + pm_runtime_disable(i2c_dev->dev);
> + return ret;
> +}
> +
> +static int sprd_i2c_remove(struct platform_device *pdev)
> +{
> + struct sprd_i2c *i2c_dev = platform_get_drvdata(pdev);
> + int ret;
> +
> + ret = pm_runtime_get_sync(i2c_dev->dev);
> + if (ret < 0)
> + return ret;
Does it make any sense? Doesn't device core power on the device before
calling ->remove() ?
> +
> + i2c_del_adapter(&i2c_dev->adap);
> +
> + if (!IS_ERR_OR_NULL(i2c_dev->clk))
_OR_NULL looks suspicious.
> + clk_unprepare(i2c_dev->clk);
> +
> + pm_runtime_put_noidle(i2c_dev->dev);
> + pm_runtime_disable(i2c_dev->dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
__maybe_unused instead?
> +static int sprd_i2c_suspend_noirq(struct device *pdev)
> +static int sprd_i2c_resume_noirq(struct device *pdev)
> +#endif /* CONFIG_PM_SLEEP */
> +#ifdef CONFIG_PM
Ditto.
> +static int sprd_i2c_runtime_suspend(struct device *pdev)
> +}
> +static int sprd_i2c_runtime_resume(struct device *pdev)
> +{
> + clk_prepare_enable(i2c_dev->clk);
This might fail.
> +}
> +#endif /* CONFIG_PM */
> +static struct platform_driver sprd_i2c_driver = {
> + .probe = sprd_i2c_probe,
> + .remove = sprd_i2c_remove,
> + .driver = {
> + .name = "sprd-i2c",
> + .of_match_table = of_match_ptr(sprd_i2c_of_match),
of_match_ptr seems redundant.
> + .pm = &sprd_i2c_pm_ops,
> + },
> +};
> +
> +static int __init sprd_i2c_init(void)
> +{
> + return platform_driver_register(&sprd_i2c_driver);
> +}
> +arch_initcall_sync(sprd_i2c_init);
Why?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/2] i2c: Add Spreadtrum I2C controller driver
Hi Baolin,
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.12-rc5 next-20170616]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Baolin-Wang/dt-bindings-i2c-Add-Spreadtrum-I2C-controller-documentation/20170616-072616
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
i2c/for-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All error/warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-sprd.c:681:1: warning: data definition has no type or
>> storage class
arch_initcall_sync(sprd_i2c_init);
^~
>> drivers/i2c/busses/i2c-sprd.c:681:1: error: type defaults to 'int' in
>> declaration of 'arch_initcall_sync' [-Werror=implicit-int]
>> drivers/i2c/busses/i2c-sprd.c:681:1: warning: parameter names (without
>> types) in function declaration
drivers/i2c/busses/i2c-sprd.c:677:19: warning: 'sprd_i2c_init' defined but
not used [-Wunused-function]
static int __init sprd_i2c_init(void)
^
cc1: some warnings being treated as errors
vim +681 drivers/i2c/busses/i2c-sprd.c
675 };
676
677 static int __init sprd_i2c_init(void)
678 {
679 return platform_driver_register(&sprd_i2c_driver);
680 }
> 681 arch_initcall_sync(sprd_i2c_init);
682
683 static void __exit sprd_i2c_exit(void)
684 {
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
[PATCH 2/2] i2c: Add Spreadtrum I2C controller driver
This patch adds the I2C controller driver for Spreadtrum platform.
Signed-off-by: Baolin Wang
---
drivers/i2c/busses/Kconfig| 10 +
drivers/i2c/busses/Makefile |1 +
drivers/i2c/busses/i2c-sprd.c | 691 +
3 files changed, 702 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-sprd.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8adc0f1..dfbc301 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -877,6 +877,16 @@ config I2C_SIRF
This driver can also be built as a module. If so, the module
will be called i2c-sirf.
+config I2C_SPRD
+ tristate "Spreadtrum I2C interface"
+ depends on ARCH_SPRD
+ help
+ If you say yes to this option, support will be included for the
+ Spreadtrum I2C interface.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-sprd.
+
config I2C_ST
tristate "STMicroelectronics SSC I2C support"
depends on ARCH_STI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 30b6085..dd46c21 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_SH7760) += i2c-sh7760.o
obj-$(CONFIG_I2C_SH_MOBILE)+= i2c-sh_mobile.o
obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o
+obj-$(CONFIG_I2C_SPRD) += i2c-sprd.o
obj-$(CONFIG_I2C_ST) += i2c-st.o
obj-$(CONFIG_I2C_STM32F4) += i2c-stm32f4.o
obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
new file mode 100644
index 000..16d3e2c
--- /dev/null
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -0,0 +1,691 @@
+/*
+ * Copyright (C) 2017 Spreadtrum Communications Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define I2C_CTL0x0
+#define I2C_ADDR_CFG 0x4
+#define I2C_COUNT 0x8
+#define I2C_RX 0xC
+#define I2C_TX 0x10
+#define I2C_STATUS 0x14
+#define I2C_HSMODE_CFG 0x18
+#define I2C_VERSION0x1C
+#define ADDR_DVD0 0x20
+#define ADDR_DVD1 0x24
+#define ADDR_STA0_DVD 0x28
+#define ADDR_RST 0x2C
+
+/* I2C_CTL */
+#define STP_EN BIT(20)
+#define FIFO_AF_LVL16
+#define FIFO_AE_LVL12
+#define I2C_DMA_EN BIT(11)
+#define FULL_INTEN BIT(10)
+#define EMPTY_INTENBIT(9)
+#define I2C_DVD_OPTBIT(8)
+#define I2C_OUT_OPTBIT(7)
+#define I2C_TRIM_OPT BIT(6)
+#define I2C_HS_MODEBIT(4)
+#define I2C_MODE BIT(3)
+#define I2C_EN BIT(2)
+#define I2C_INT_EN BIT(1)
+#define I2C_START BIT(0)
+
+/* I2C_STATUS */
+#define SDA_IN BIT(21)
+#define SCL_IN BIT(20)
+#define FIFO_FULL BIT(4)
+#define FIFO_EMPTY BIT(3)
+#define I2C_INTBIT(2)
+#define I2C_RX_ACK BIT(1)
+#define I2C_BUSY BIT(0)
+
+/* ADDR_RST */
+#define I2C_RSTBIT(0)
+
+#define I2C_FIFO_DEEP 12
+#define I2C_FIFO_FULL_THLD 15
+#define I2C_FIFO_EMPTY_THLD4
+#define I2C_DATA_STEP 8
+#define SPRD_I2C_TIMEOUT (msecs_to_jiffies(1000))
+
+/* timeout (ms) for pm runtime autosuspend */
+#define SPRD_I2C_PM_TIMEOUT1000
+
+/* SPRD i2c data structure */
+struct sprd_i2c {
+ struct i2c_adapter adap;
+ struct device *dev;
+ void __iomem *base;
+ struct i2c_msg *msg;
+ struct clk *clk;
+ unsigned int src_clk;
+ unsigned int bus_freq;
+ struct completion complete;
+ u8 *buf;
+ u16 count;
+ int irq;
+ int err;
+};
+
+static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)
+{
+ dev_err(&i2c_dev->adap.dev, ": ==dump i2c-%d reg===\n",
+ i2c_dev->adap.nr);
+ dev_err(&i2c_dev->adap.dev, ": I2C_CTRL:0x%x\n",
+ readl(i2c_dev->base + I2C_CTL));
+ dev_err(&i2c_dev->adap.dev, ": I2C_ADDR_CFG:0x%x\n",
+ readl(i2c_dev->base + I2C_ADDR_CFG));
+ dev_err(&i2c_dev->adap.dev, ": I2C_COUNT:0x%x\n",
+
