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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <283c9452-996d-d1ce-90a0-28fbd55c291d@roeck-us.net>
Date:   Thu, 27 Aug 2020 06:46:44 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     啟原黃 <u0084500@...il.com>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>
Cc:     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

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

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

Powered by Openwall GNU/*/Linux Powered by OpenVZ