Guenter Roeck [off-list ref] 於 2020年8月27日 週四 下午9:46寫道：

On 8/27/20 4:32 AM, 啟原黃 wrote:

quoted

Chunfeng Yun [off-list ref] 於 2020年8月27日 週四 下午4:05寫道：

quoted

On Thu, 2020-08-27 at 12:50 +0800, cy_huang wrote:

quoted

From: ChiYuan Huang <redacted>

Mediatek MT6360 is a multi-functional IC that includes USB Type-C.
It works with Type-C Port Controller Manager to provide USB PD
and USB Type-C functionalities.

Add fix to Prevent the race condition from interrupt and tcpci port unregister
during module remove.

Signed-off-by: ChiYuan Huang <redacted>
---
 drivers/usb/typec/tcpm/Kconfig        |   8 ++
 drivers/usb/typec/tcpm/Makefile       |   1 +
 drivers/usb/typec/tcpm/tcpci_mt6360.c | 213 ++++++++++++++++++++++++++++++++++

Can you avoid using special SoC name in file name?
It's not clear if you later support new SoC in the driver, e.g. mt63xx?

Okay, I will rename it to mt636x. From our SubPMIC generation,
currently, naming will be 6360/62/67, etc..

What if 6361 or 6365 or 6369 will require a different driver ?
What if 6371 will, in the future, be supported by the same driver ?
Or, for that matter, 7360 ? 6537 ?

We usually try to avoid "x" in driver names because it can never be
guaranteed that it will apply to x={0..9}. The current file name is
just fine; it is customary to name drivers after the first chip they
support.

Guenter

Thanks. it means the current file name and description is okay....
I'll start to revert the rename work.

quoted

quoted

From our SOC roadmap, one generation can be used for one to two more years.

I think the name MT636x is enough.
For the next one, I can submit the patch to make it compatible with this driver.
Thanks for you comment.

quoted

quoted

 3 files changed, 222 insertions(+)
 create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6360.c

diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
index fa3f393..58a64e1 100644
--- a/drivers/usb/typec/tcpm/Kconfig
+++ b/drivers/usb/typec/tcpm/Kconfig

@@ -27,6 +27,14 @@ config TYPEC_RT1711H
        Type-C Port Controller Manager to provide USB PD and USB
        Type-C functionalities.

+config TYPEC_MT6360
+     tristate "Mediatek MT6360 Type-C driver"
+     depends on MFD_MT6360
+     help
+       Mediatek MT6360 is a multi-functional IC that includes
+       USB Type-C. It works with Type-C Port Controller Manager
+       to provide USB PD and USB Type-C functionalities.
+
 endif # TYPEC_TCPCI

 config TYPEC_FUSB302

diff --git a/drivers/usb/typec/tcpm/Makefile b/drivers/usb/typec/tcpm/Makefile
index a5ff6c8..7592ccb 100644
--- a/drivers/usb/typec/tcpm/Makefile
+++ b/drivers/usb/typec/tcpm/Makefile

@@ -5,3 +5,4 @@ obj-$(CONFIG_TYPEC_WCOVE)     += typec_wcove.o
 typec_wcove-y                        := wcove.o
 obj-$(CONFIG_TYPEC_TCPCI)    += tcpci.o
 obj-$(CONFIG_TYPEC_RT1711H)  += tcpci_rt1711h.o
+obj-$(CONFIG_TYPEC_MT6360)   += tcpci_mt6360.o

diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
new file mode 100644
index 00000000..a381b5d
--- /dev/null
+++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c

@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2020 MediaTek Inc.
+//
+// Author: ChiYuan Huang <cy_huang@richtek.com>

Use /* */ except SPDX?

Yes, sure.

quoted

quoted

+
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/usb/tcpm.h>
+
+#include "tcpci.h"
+
+#define MT6360_REG_VCONNCTRL1        0x8C
+#define MT6360_REG_MODECTRL2 0x8F
+#define MT6360_REG_SWRESET   0xA0
+#define MT6360_REG_DEBCTRL1  0xA1
+#define MT6360_REG_DRPCTRL1  0xA2
+#define MT6360_REG_DRPCTRL2  0xA3
+#define MT6360_REG_I2CTORST  0xBF
+#define MT6360_REG_RXCTRL2   0xCF
+#define MT6360_REG_CTDCTRL2  0xEC
+
+/* MT6360_REG_VCONNCTRL1 */
+#define MT6360_VCONNCL_ENABLE        BIT(0)
+/* MT6360_REG_RXCTRL2 */
+#define MT6360_OPEN40M_ENABLE        BIT(7)
+/* MT6360_REG_CTDCTRL2 */
+#define MT6360_RPONESHOT_ENABLE      BIT(6)
+
+struct mt6360_tcpc_info {
+     struct tcpci_data tdata;
+     struct tcpci *tcpci;
+     struct device *dev;
+     int irq;
+};
+
+static inline int mt6360_tcpc_read16(struct regmap *regmap,
+                                  unsigned int reg, u16 *val)
+{
+     return regmap_raw_read(regmap, reg, val, sizeof(u16));
+}
+
+static inline int mt6360_tcpc_write16(struct regmap *regmap,
+                                   unsigned int reg, u16 val)
+{
+     return regmap_raw_write(regmap, reg, &val, sizeof(u16));
+}
+
+static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
+{
+     struct regmap *regmap = tdata->regmap;
+     int ret;
+
+     ret = regmap_write(regmap, MT6360_REG_SWRESET, 0x01);
+     if (ret)
+             return ret;
+
+     /* after reset command, wait 1~2ms to wait IC action */
+     usleep_range(1000, 2000);
+
+     /* write all alert to masked */
+     ret = mt6360_tcpc_write16(regmap, TCPC_ALERT_MASK, 0);
+     if (ret)
+             return ret;
+
+     /* config I2C timeout reset enable , and timeout to 200ms */
+     ret = regmap_write(regmap, MT6360_REG_I2CTORST, 0x8F);
+     if (ret)
+             return ret;
+
+     /* config CC Detect Debounce : 26.7*val us */
+     ret = regmap_write(regmap, MT6360_REG_DEBCTRL1, 0x10);
+     if (ret)
+             return ret;
+
+     /* DRP Toggle Cycle : 51.2 + 6.4*val ms */
+     ret = regmap_write(regmap, MT6360_REG_DRPCTRL1, 4);
+     if (ret)
+             return ret;
+
+     /* DRP Duyt Ctrl : dcSRC: /1024 */
+     ret = mt6360_tcpc_write16(regmap, MT6360_REG_DRPCTRL2, 330);
+     if (ret)
+             return ret;
+
+     /* Enable VCONN Current Limit function */
+     ret = regmap_update_bits(regmap, MT6360_REG_VCONNCTRL1, MT6360_VCONNCL_ENABLE,
+                              MT6360_VCONNCL_ENABLE);
+     if (ret)
+             return ret;
+
+     /* Enable cc open 40ms when pmic send vsysuv signal */
+     ret = regmap_update_bits(regmap, MT6360_REG_RXCTRL2, MT6360_OPEN40M_ENABLE,
+                              MT6360_OPEN40M_ENABLE);
+     if (ret)
+             return ret;
+
+     /* Enable Rpdet oneshot detection */
+     ret = regmap_update_bits(regmap, MT6360_REG_CTDCTRL2, MT6360_RPONESHOT_ENABLE,
+                              MT6360_RPONESHOT_ENABLE);
+     if (ret)
+             return ret;
+
+     /* Set shipping mode off, AUTOIDLE on */
+     return regmap_write(regmap, MT6360_REG_MODECTRL2, 0x7A);
+}
+
+static irqreturn_t mt6360_irq(int irq, void *dev_id)
+{
+     struct mt6360_tcpc_info *mti = dev_id;
+
+     return tcpci_irq(mti->tcpci);
+}
+
+static int mt6360_tcpc_probe(struct platform_device *pdev)
+{
+     struct mt6360_tcpc_info *mti;
+     int ret;
+
+     mti = devm_kzalloc(&pdev->dev, sizeof(*mti), GFP_KERNEL);
+     if (!mti)
+             return -ENOMEM;
+
+     mti->dev = &pdev->dev;
+
+     mti->tdata.regmap = dev_get_regmap(pdev->dev.parent, NULL);
+     if (!mti->tdata.regmap) {
+             dev_err(&pdev->dev, "Failed to get parent regmap\n");
+             return -ENODEV;
+     }
+
+     mti->irq = platform_get_irq_byname(pdev, "PD_IRQB");
+     if (mti->irq < 0) {
+             dev_err(&pdev->dev, "Failed to get PD_IRQB irq\n");

No need add error log, platform_get_irq_byname will print it

I'll remove it in the next revision. Thanks

quoted

quoted

+             return mti->irq;
+     }
+
+     mti->tdata.init = mt6360_tcpc_init;
+     mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
+     if (IS_ERR_OR_NULL(mti->tcpci)) {

Use IS_ERR()? it seems not return NULL

quoted

+             dev_err(&pdev->dev, "Failed to register tcpci port\n");
+             return PTR_ERR(mti->tcpci);

If return NULL, this may return 0?

Remove it later. you're right.

quoted

quoted

+     }
+
+     ret = devm_request_threaded_irq(mti->dev, mti->irq, NULL, mt6360_irq, IRQF_ONESHOT,
+                                     dev_name(&pdev->dev), mti);
+     if (ret) {
+             dev_err(mti->dev, "Failed to register irq\n");
+             tcpci_unregister_port(mti->tcpci);
+             return ret;
+     }
+
+     device_init_wakeup(&pdev->dev, true);
+     platform_set_drvdata(pdev, mti);
+
+     return 0;
+}
+
+static int mt6360_tcpc_remove(struct platform_device *pdev)
+{
+     struct mt6360_tcpc_info *mti = platform_get_drvdata(pdev);
+
+     disable_irq(mti->irq);

need sync?

Yes, sync must be considered to wait interrupt is really finished.
And disable_irq already the sync version.

quoted

quoted

+     tcpci_unregister_port(mti->tcpci);
+     return 0;
+}
+
+static int __maybe_unused mt6360_tcpc_suspend(struct device *dev)
+{
+     struct mt6360_tcpc_info *mti = dev_get_drvdata(dev);
+
+     if (device_may_wakeup(dev))
+             enable_irq_wake(mti->irq);
+
+     return 0;
+}
+
+static int __maybe_unused mt6360_tcpc_resume(struct device *dev)
+{
+     struct mt6360_tcpc_info *mti = dev_get_drvdata(dev);
+
+     if (device_may_wakeup(dev))
+             disable_irq_wake(mti->irq);
+
+     return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(mt6360_tcpc_pm_ops, mt6360_tcpc_suspend, mt6360_tcpc_resume);
+
+static const struct of_device_id __maybe_unused mt6360_tcpc_of_id[] = {
+     { .compatible = "mediatek,mt6360-tcpc", },
+     {},
+};
+MODULE_DEVICE_TABLE(of, mt6360_tcpc_of_id);
+
+static struct platform_driver mt6360_tcpc_driver = {
+     .driver = {
+             .name = "mt6360-tcpc",
+             .pm = &mt6360_tcpc_pm_ops,
+             .of_match_table = mt6360_tcpc_of_id,
+     },
+     .probe = mt6360_tcpc_probe,
+     .remove = mt6360_tcpc_remove,
+};
+module_platform_driver(mt6360_tcpc_driver);
+
+MODULE_AUTHOR("ChiYuan Huang [off-list ref]");
+MODULE_DESCRIPTION("MT6360 USB Type-C Port Controller Interface Driver");
+MODULE_LICENSE("GPL v2");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel