Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 2 Aug 2022 10:57:53 +0300
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Gene Chen <gene.chen.richtek@gmail.com>
Cc:     linux@roeck-us.net, gregkh@linuxfoundation.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        gene_chen@richtek.com, cy_huang@richtek.com
Subject: Re: [PATCH v3 2/7] usb: typec: tcpci_rt1711h: Fix vendor setting
 when set vconn
Message-ID: <YujZATquqh0srgHm@kuha.fi.intel.com>
References: <20220801101447.86207-1-gene.chen.richtek@gmail.com>
 <20220801101447.86207-3-gene.chen.richtek@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220801101447.86207-3-gene.chen.richtek@gmail.com>
Precedence: bulk

Hi Gene,

On Mon, Aug 01, 2022 at 06:14:42PM +0800, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> replace overwrite whole register with update bits
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>  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.

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