linux-kernel - Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

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: <1611041874.12761.13.camel@mhfsdcap03>
Date:   Tue, 19 Jan 2021 15:37:54 +0800
From:   Chunfeng Yun <chunfeng.yun@...iatek.com>
To:     ChiYuan Huang <u0084500@...il.com>
CC:     Guenter Roeck <linux@...ck-us.net>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        <matthias.bgg@...il.com>, Rob Herring <robh+dt@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        lkml <linux-kernel@...r.kernel.org>,
        cy_huang <cy_huang@...htek.com>, <gene_chen@...htek.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and
 external vbus supply control

On Mon, 2021-01-18 at 16:28 +0800, ChiYuan Huang wrote:
> Guenter Roeck <linux@...ck-us.net> 於 2021年1月18日 週一 上午1:43寫道：
> >
> > On 1/15/21 6:13 AM, cy_huang wrote:
> > > From: ChiYuan Huang <cy_huang@...htek.com>
> > >
> > > MT6360 not support for TCPC command to control source and sink.
> >
> > does not
> >
> Ack
> > > Uses external 5V vbus regulator as the vbus source control.
> > >
> > Use
> >
> Ack
> > > Also adds the capability to report vsafe0v.
> > >
> > add
> >
> Ack
> > So far this driver works without regulator. Unless I am missing something,
> > this patch makes regulator support mandatory, meaning existing code will fail.
> > I am not sure if that is appropriate/acceptable. Can we be sure that this will
> > work for existing users of this driver ?
> >
> Yes, I already checked all the src/snk functionality based on  the
> latest typec code.
> It'll be common for our TCPC. It didn't support for TCPC command.
> From the recent patches, actually, I have the local change to test the
> src capability.
> But I didn't submit it. It's almost the same to add set_vbus callback.
> That's why I submit this change after tcpci 'set_vbus callback' is added.
> 
> > Thanks,
> > Guenter
> >
> > > Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > index f1bd9e0..0edf4b6 100644
> > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > >  #include <linux/usb/tcpm.h>
> > >
> > >  #include "tcpci.h"
> > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > >       struct tcpci_data tdata;
> > >       struct tcpci *tcpci;
> > >       struct device *dev;
> > > +     struct regulator *vbus;
> > >       int irq;
> > >  };
> > >
> > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
> > >       return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > >  }
> > >
> > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> > > +{
> > > +     struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> > > +     int ret;
> > > +
> > > +     /* To correctly handle the already enabled vbus and disable its supply first */
> > > +     if (regulator_is_enabled(mti->vbus)) {
> > > +             ret = regulator_disable(mti->vbus);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> >
> > Is it really a good idea to disable vbus if it happens to be already enabled
> > and there is (another ?) request to enable it ?
> >
> Yes, for  the state change from src_attach_wait to src_attach,
> It need to meet the requirement that  the vbus is at vsafe0v.
> So to disable it first is needed.
> And to prevent other users from enabling/disabling external vbus
> regulator in any case.
> I think we may change regulator_get  to 'regulator_get_exclusive'.
> From the design, 5v regulator only can be controlled via typec framework.
> If other user touch it, it'll affect the typec state transition.
How about to process the case that even switch usb controller to device
mode, platform also need to keep vbus on? e.g. Iphone Carplay


> > > +
> > > +     if (src) {
> > > +             ret = regulator_enable(mti->vbus);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> > >  {
> > >       struct regmap *regmap = tdata->regmap;
> > > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
> > >       if (mti->irq < 0)
> > >               return mti->irq;
> > >
> > > +     mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > > +     if (IS_ERR(mti->vbus))
> > > +             return PTR_ERR(mti->vbus);
> > > +
> > >       mti->tdata.init = mt6360_tcpc_init;
> > > +     mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > > +     mti->tdata.vbus_vsafe0v = 1;
> > >       mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > >       if (IS_ERR(mti->tcpci)) {
> > >               dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > >
> >