Re: [PATCH v3 2/7] usb: typec: tcpci_rt1711h: Fix vendor setting when set vconn

[Gene Chen]

Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> 於 2022年8月2日 週二 下午3:57寫道：
>
> Hi Gene,
>
> On Mon, Aug 01, 2022 at 06:14:42PM +0800, Gene Chen wrote:
> > From: Gene Chen <gene_chen@xxxxxxxxxxx>
> >
> > replace overwrite whole register with update bits
> >
> > Signed-off-by: Gene Chen <gene_chen@xxxxxxxxxxx>
> > ---
> >  drivers/usb/typec/tcpm/tcpci_rt1711h.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > index b56a0880a044..6197d9a05d36 100644
> > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > @@ -5,13 +5,15 @@
> >   * Richtek RT1711H Type-C Chip Driver
> >   */
> >
> > -#include <linux/kernel.h>
> > -#include <linux/module.h>
> > +#include <linux/bits.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/gpio/consumer.h>
> > -#include <linux/usb/tcpm.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> >  #include <linux/regmap.h>
> > +#include <linux/usb/tcpm.h>
>
> That header reshuffling is not necessary for this change - at least you
> are not giving any reason for it in your commit message.
>
> If there is no real need for that in this patch, then please leave the
> headers as they are. You can propose changing the order of the headers
> in a separate patch. Though, I would not bother with it unless there
> is some real benefit in doing so, and I'm pretty sure there isn't any.
>

ACK, I will remove reshuffling.
It was suggested coding style by other reviewer in other driver.

> >  #include "tcpci.h"
> >
> >  #define RT1711H_VID          0x29CF
> > @@ -23,6 +25,7 @@
> >  #define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
> >                           (((ck300) << 7) | ((ship_off) << 5) | \
> >                           ((auto_idle) << 3) | ((tout) & 0x07))
> > +#define RT1711H_AUTOIDLEEN   BIT(3)
> >
> >  #define RT1711H_RTCTRL11     0x9E
> >
> > @@ -109,8 +112,8 @@ static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
> >  {
> >       struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
> >
> > -     return rt1711h_write8(chip, RT1711H_RTCTRL8,
> > -                           RT1711H_RTCTRL8_SET(0, 1, !enable, 2));
> > +     return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL8,
> > +                               RT1711H_AUTOIDLEEN, enable ? 0 : RT1711H_AUTOIDLEEN);
> >  }
> >
> >  static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
> > --
> > 2.25.1
>
> thanks,
>
> --
> heikki