[1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[Hans de Goede]

The 2 callers of fusb302_set_cc_polarity both call fusb302_set_cc_pull
directly before calling fusb302_set_cc_polarity, this is not ideal for
2 reasons:

1) fusb302_set_cc_pull uses the cached polarity when applying pull-ups,
which maybe changed immediately afterwards, to fix this set_cc_polarity
already does the pull-up setting.

2) Both touch the SWITCHES0 register in a r-w-modify cycle, this leads to
read reg, write reg, read reg, write reg. If we fold the setting of
the pull-downs into fusb302_set_cc_polarity then not only can we avoid
doing the reads / writes twice, at this point we set all bits, so we
can skip the read, turning 4 (slowish) i2c-transfers into 1.

Doing this also avoids the need to cache the pull_up state in
struct fusb302_chip.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 48 +++++++++++---------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index c25d98fe24ac..3ad92d68b6af 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -99,7 +99,6 @@ struct fusb302_chip {
 	bool intr_comp_chng;
 
 	/* port status */
-	bool pull_up;
 	bool vconn_on;
 	bool vbus_on;
 	bool charge_on;
@@ -540,7 +539,6 @@ static int fusb302_set_cc_pull(struct fusb302_chip *chip,
 				     mask, data);
 	if (ret < 0)
 		return ret;
-	chip->pull_up = pull_up;
 
 	return ret;
 }
@@ -1231,38 +1229,36 @@ static const char * const cc_polarity_name[] = {
 	[TYPEC_POLARITY_CC2]	= "Polarity_CC2",
 };
 
-static int fusb302_set_cc_polarity(struct fusb302_chip *chip,
-				   enum typec_cc_polarity cc_polarity)
+static int fusb302_set_cc_polarity_and_pull(struct fusb302_chip *chip,
+					    enum typec_cc_polarity cc_polarity,
+					    bool pull_up, bool pull_down)
 {
 	int ret = 0;
-	u8 switches0_mask = FUSB_REG_SWITCHES0_CC1_PU_EN |
-			    FUSB_REG_SWITCHES0_CC2_PU_EN |
-			    FUSB_REG_SWITCHES0_VCONN_CC1 |
-			    FUSB_REG_SWITCHES0_VCONN_CC2 |
-			    FUSB_REG_SWITCHES0_MEAS_CC1 |
-			    FUSB_REG_SWITCHES0_MEAS_CC2;
 	u8 switches0_data = 0x00;
 	u8 switches1_mask = FUSB_REG_SWITCHES1_TXCC1_EN |
 			    FUSB_REG_SWITCHES1_TXCC2_EN;
 	u8 switches1_data = 0x00;
 
+	if (pull_down)
+		switches0_data |= FUSB_REG_SWITCHES0_CC1_PD_EN |
+				  FUSB_REG_SWITCHES0_CC2_PD_EN;
+
 	if (cc_polarity == TYPEC_POLARITY_CC1) {
-		switches0_data = FUSB_REG_SWITCHES0_MEAS_CC1;
+		switches0_data |= FUSB_REG_SWITCHES0_MEAS_CC1;
 		if (chip->vconn_on)
 			switches0_data |= FUSB_REG_SWITCHES0_VCONN_CC2;
-		if (chip->pull_up)
+		if (pull_up)
 			switches0_data |= FUSB_REG_SWITCHES0_CC1_PU_EN;
 		switches1_data = FUSB_REG_SWITCHES1_TXCC1_EN;
 	} else {
-		switches0_data = FUSB_REG_SWITCHES0_MEAS_CC2;
+		switches0_data |= FUSB_REG_SWITCHES0_MEAS_CC2;
 		if (chip->vconn_on)
 			switches0_data |= FUSB_REG_SWITCHES0_VCONN_CC1;
-		if (chip->pull_up)
+		if (pull_up)
 			switches0_data |= FUSB_REG_SWITCHES0_CC2_PU_EN;
 		switches1_data = FUSB_REG_SWITCHES1_TXCC2_EN;
 	}
-	ret = fusb302_i2c_mask_write(chip, FUSB_REG_SWITCHES0,
-				     switches0_mask, switches0_data);
+	ret = fusb302_i2c_write(chip, FUSB_REG_SWITCHES0, switches0_data);
 	if (ret < 0)
 		return ret;
 	ret = fusb302_i2c_mask_write(chip, FUSB_REG_SWITCHES1,
@@ -1283,16 +1279,10 @@ static int fusb302_handle_togdone_snk(struct fusb302_chip *chip,
 	enum typec_cc_polarity cc_polarity;
 	enum typec_cc_status cc_status_active, cc1, cc2;
 
-	/* set pull_up, pull_down */
-	ret = fusb302_set_cc_pull(chip, false, true);
-	if (ret < 0) {
-		fusb302_log(chip, "cannot set cc to pull down, ret=%d", ret);
-		return ret;
-	}
-	/* set polarity */
+	/* set polarity and pull_up, pull_down */
 	cc_polarity = (togdone_result == FUSB_REG_STATUS1A_TOGSS_SNK1) ?
 		      TYPEC_POLARITY_CC1 : TYPEC_POLARITY_CC2;
-	ret = fusb302_set_cc_polarity(chip, cc_polarity);
+	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, false, true);
 	if (ret < 0) {
 		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
 			    cc_polarity_name[cc_polarity], ret);
@@ -1359,16 +1349,10 @@ static int fusb302_handle_togdone_src(struct fusb302_chip *chip,
 	enum typec_cc_polarity cc_polarity;
 	enum typec_cc_status cc_status_active, cc1, cc2;
 
-	/* set pull_up, pull_down */
-	ret = fusb302_set_cc_pull(chip, true, false);
-	if (ret < 0) {
-		fusb302_log(chip, "cannot set cc to pull up, ret=%d", ret);
-		return ret;
-	}
-	/* set polarity */
+	/* set polarity and pull_up, pull_down */
 	cc_polarity = (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1) ?
 		      TYPEC_POLARITY_CC1 : TYPEC_POLARITY_CC2;
-	ret = fusb302_set_cc_polarity(chip, cc_polarity);
+	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, true, false);
 	if (ret < 0) {
 		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
 			    cc_polarity_name[cc_polarity], ret);

[1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[Greg Kroah-Hartman]

On Fri, Feb 22, 2019 at 03:11:58PM +0100, Hans de Goede wrote:
> The 2 callers of fusb302_set_cc_polarity both call fusb302_set_cc_pull
> directly before calling fusb302_set_cc_polarity, this is not ideal for
> 2 reasons:
> 
> 1) fusb302_set_cc_pull uses the cached polarity when applying pull-ups,
> which maybe changed immediately afterwards, to fix this set_cc_polarity
> already does the pull-up setting.
> 
> 2) Both touch the SWITCHES0 register in a r-w-modify cycle, this leads to
> read reg, write reg, read reg, write reg. If we fold the setting of
> the pull-downs into fusb302_set_cc_polarity then not only can we avoid
> doing the reads / writes twice, at this point we set all bits, so we
> can skip the read, turning 4 (slowish) i2c-transfers into 1.
> 
> Doing this also avoids the need to cache the pull_up state in
> struct fusb302_chip.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/fusb302.c | 48 +++++++++++---------------------
>  1 file changed, 16 insertions(+), 32 deletions(-)

It would be nice to get some people to review/ack this series before I
take it (hint...)

thanks,

greg k-h

Re: [PATCH 1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[Greg Kroah-Hartman]

On Fri, Feb 22, 2019 at 03:11:58PM +0100, Hans de Goede wrote:
> The 2 callers of fusb302_set_cc_polarity both call fusb302_set_cc_pull
> directly before calling fusb302_set_cc_polarity, this is not ideal for
> 2 reasons:
> 
> 1) fusb302_set_cc_pull uses the cached polarity when applying pull-ups,
> which maybe changed immediately afterwards, to fix this set_cc_polarity
> already does the pull-up setting.
> 
> 2) Both touch the SWITCHES0 register in a r-w-modify cycle, this leads to
> read reg, write reg, read reg, write reg. If we fold the setting of
> the pull-downs into fusb302_set_cc_polarity then not only can we avoid
> doing the reads / writes twice, at this point we set all bits, so we
> can skip the read, turning 4 (slowish) i2c-transfers into 1.
> 
> Doing this also avoids the need to cache the pull_up state in
> struct fusb302_chip.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/fusb302.c | 48 +++++++++++---------------------
>  1 file changed, 16 insertions(+), 32 deletions(-)

It would be nice to get some people to review/ack this series before I
take it (hint...)

thanks,

greg k-h

[1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[Heikki Krogerus]

On Tue, Apr 16, 2019 at 12:19:53PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Feb 22, 2019 at 03:11:58PM +0100, Hans de Goede wrote:
> > The 2 callers of fusb302_set_cc_polarity both call fusb302_set_cc_pull
> > directly before calling fusb302_set_cc_polarity, this is not ideal for
> > 2 reasons:
> > 
> > 1) fusb302_set_cc_pull uses the cached polarity when applying pull-ups,
> > which maybe changed immediately afterwards, to fix this set_cc_polarity
> > already does the pull-up setting.
> > 
> > 2) Both touch the SWITCHES0 register in a r-w-modify cycle, this leads to
> > read reg, write reg, read reg, write reg. If we fold the setting of
> > the pull-downs into fusb302_set_cc_polarity then not only can we avoid
> > doing the reads / writes twice, at this point we set all bits, so we
> > can skip the read, turning 4 (slowish) i2c-transfers into 1.
> > 
> > Doing this also avoids the need to cache the pull_up state in
> > struct fusb302_chip.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/usb/typec/tcpm/fusb302.c | 48 +++++++++++---------------------
> >  1 file changed, 16 insertions(+), 32 deletions(-)
> 
> It would be nice to get some people to review/ack this series before I
> take it (hint...)

Hans send second and third version of these. These are already in.

thanks,

Re: [PATCH 1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[Heikki Krogerus]

On Tue, Apr 16, 2019 at 12:19:53PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Feb 22, 2019 at 03:11:58PM +0100, Hans de Goede wrote:
> > The 2 callers of fusb302_set_cc_polarity both call fusb302_set_cc_pull
> > directly before calling fusb302_set_cc_polarity, this is not ideal for
> > 2 reasons:
> > 
> > 1) fusb302_set_cc_pull uses the cached polarity when applying pull-ups,
> > which maybe changed immediately afterwards, to fix this set_cc_polarity
> > already does the pull-up setting.
> > 
> > 2) Both touch the SWITCHES0 register in a r-w-modify cycle, this leads to
> > read reg, write reg, read reg, write reg. If we fold the setting of
> > the pull-downs into fusb302_set_cc_polarity then not only can we avoid
> > doing the reads / writes twice, at this point we set all bits, so we
> > can skip the read, turning 4 (slowish) i2c-transfers into 1.
> > 
> > Doing this also avoids the need to cache the pull_up state in
> > struct fusb302_chip.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/usb/typec/tcpm/fusb302.c | 48 +++++++++++---------------------
> >  1 file changed, 16 insertions(+), 32 deletions(-)
> 
> It would be nice to get some people to review/ack this series before I
> take it (hint...)

Hans send second and third version of these. These are already in.

thanks,

-- 
heikki

[1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[Greg Kroah-Hartman]

On Tue, Apr 16, 2019 at 04:44:39PM +0300, Heikki Krogerus wrote:
> On Tue, Apr 16, 2019 at 12:19:53PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Feb 22, 2019 at 03:11:58PM +0100, Hans de Goede wrote:
> > > The 2 callers of fusb302_set_cc_polarity both call fusb302_set_cc_pull
> > > directly before calling fusb302_set_cc_polarity, this is not ideal for
> > > 2 reasons:
> > > 
> > > 1) fusb302_set_cc_pull uses the cached polarity when applying pull-ups,
> > > which maybe changed immediately afterwards, to fix this set_cc_polarity
> > > already does the pull-up setting.
> > > 
> > > 2) Both touch the SWITCHES0 register in a r-w-modify cycle, this leads to
> > > read reg, write reg, read reg, write reg. If we fold the setting of
> > > the pull-downs into fusb302_set_cc_polarity then not only can we avoid
> > > doing the reads / writes twice, at this point we set all bits, so we
> > > can skip the read, turning 4 (slowish) i2c-transfers into 1.
> > > 
> > > Doing this also avoids the need to cache the pull_up state in
> > > struct fusb302_chip.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >  drivers/usb/typec/tcpm/fusb302.c | 48 +++++++++++---------------------
> > >  1 file changed, 16 insertions(+), 32 deletions(-)
> > 
> > It would be nice to get some people to review/ack this series before I
> > take it (hint...)
> 
> Hans send second and third version of these. These are already in.

Ah, nevermind then, now dropped :)

greg k-h

Re: [PATCH 1/8] usb: typec: fusb302: Make fusb302_set_cc_polarity also set pull ups / downs

[Greg Kroah-Hartman]

On Tue, Apr 16, 2019 at 04:44:39PM +0300, Heikki Krogerus wrote:
> On Tue, Apr 16, 2019 at 12:19:53PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Feb 22, 2019 at 03:11:58PM +0100, Hans de Goede wrote:
> > > The 2 callers of fusb302_set_cc_polarity both call fusb302_set_cc_pull
> > > directly before calling fusb302_set_cc_polarity, this is not ideal for
> > > 2 reasons:
> > > 
> > > 1) fusb302_set_cc_pull uses the cached polarity when applying pull-ups,
> > > which maybe changed immediately afterwards, to fix this set_cc_polarity
> > > already does the pull-up setting.
> > > 
> > > 2) Both touch the SWITCHES0 register in a r-w-modify cycle, this leads to
> > > read reg, write reg, read reg, write reg. If we fold the setting of
> > > the pull-downs into fusb302_set_cc_polarity then not only can we avoid
> > > doing the reads / writes twice, at this point we set all bits, so we
> > > can skip the read, turning 4 (slowish) i2c-transfers into 1.
> > > 
> > > Doing this also avoids the need to cache the pull_up state in
> > > struct fusb302_chip.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >  drivers/usb/typec/tcpm/fusb302.c | 48 +++++++++++---------------------
> > >  1 file changed, 16 insertions(+), 32 deletions(-)
> > 
> > It would be nice to get some people to review/ack this series before I
> > take it (hint...)
> 
> Hans send second and third version of these. These are already in.

Ah, nevermind then, now dropped :)

greg k-h