| 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: <6a66d52d-5f30-5982-e14c-12d81863f684@roeck-us.net>
Date: Fri, 28 Aug 2020 07:48:56 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: ChiYuan Huang <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 v3 1/3] usb typec: mt6360: Add support for mt6360 Type-C
driver
On 8/28/20 7:33 AM, ChiYuan Huang wrote:
> Chunfeng Yun <chunfeng.yun@...iatek.com> 於 2020年8月28日 週五 下午8:11寫道：
>>
>> On Thu, 2020-08-27 at 19:18 +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.
>>>
>>> v1 to v2
>>> 1. Add fix to Prevent the race condition from interrupt and tcpci port
>>> unregister during module remove.
>>>
>>> v2 to v3
>>> 1. Change comment style for the head of source code.
>>> 2. No need to print error for platform_get_irq_byname.
>>> 3. Fix tcpci_register_port check from IS_ERR_OR_NULL to IS_ERR.
>>> 4. Rename driver/Kconfig/Makefile form mt6360 to mt636x.
>>> 5. Rename DT binding documents from mt6360 to mt636x.
>>>
>>> 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 | 212 ++++++++++++++++++++++++++++++++++
>>> 3 files changed, 221 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..f1bd9e0
>>> --- /dev/null
>>> +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
>>> @@ -0,0 +1,212 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (C) 2020 MediaTek Inc.
>>> + *
>>> + * Author: ChiYuan Huang <cy_huang@...htek.com>
>>> + */
>>> +
>>> +#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);
>> Wonder why access some register by 2bytes, and others 4bytes?
>> can we access them all by 4bytes?
>
> Actually, the access bus is still I2C.
> Each register is still one byte only.
> Register address like as TCPC_ALERT_MASK are continuous two byte used
> as IRQ MASK.
> That's why it's used wirte16 function to access.
>
> Actually, you also can divided into two bytes write into one byte
> access by two times.
> For I2C bus efficiency, it seems bad.
For MT6360_REG_DRPCTRL2, it might also be racy, since the two i2c addresses
are really one register containing a 10-bit value. I would probably not matter
here (afaics the value is changed from the default 327 to 330), but in general
it seems safer to write such a value with a single 16-bit i2c write.
Guenter
Powered by blists - more mailing lists