| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADiBU39L+zxhMa+Vam8S=j6zSROOebUDGn7dh9v6cbpqqFj3hQ@mail.gmail.com>
Date: Thu, 27 Aug 2020 22:34:39 +0800
From: ChiYuan Huang <u0084500@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Chunfeng Yun <chunfeng.yun@...iatek.com>,
Greg KH <gregkh@...uxfoundation.org>, robh+dt@...nel.org,
matthias.bgg@...il.com,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
cy_huang <cy_huang@...htek.com>, gene_chen@...htek.com,
linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] usb typec: mt6360: Add support for mt6360 Type-C driver
Guenter Roeck <linux@...ck-us.net> 於 2020年8月27日 週四 下午9:46寫道：
>
> On 8/27/20 4:32 AM, 啟原黃 wrote:
> > Chunfeng Yun <chunfeng.yun@...iatek.com> 於 2020年8月27日 週四 下午4:05寫道：
> >>
> >> On Thu, 2020-08-27 at 12:50 +0800, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@...htek.com>
> >>>
> >>> 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 <cy_huang@...htek.com>
> >>> ---
> >>> 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.
>
> >>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.
> >>
> >>> 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@...htek.com>
> >> Use /* */ except SPDX?
> > Yes, sure.
> >>
> >>> +
> >>> +#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
> >>
> >>> + 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
> >>> + 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.
> >>
> >>> + }
> >>> +
> >>> + 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.
> >>
> >>> + 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 <cy_huang@...htek.com>");
> >>> +MODULE_DESCRIPTION("MT6360 USB Type-C Port Controller Interface Driver");
> >>> +MODULE_LICENSE("GPL v2");
> >>
>
Powered by blists - more mailing lists