Re: [PATCH 3/7] phy: qcom-qmp-combo: Introduce orientation variable

[Bjorn Andersson]

On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote:
> On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote:
> > In multiple places throughout the driver code has been written in
> > prepration for handling of orientation switching.
> > 
> > Introduce a typec_orientation in qmp_combo and fill out the various
> > "placeholders" with the associated logic. By initializing the
> > orientation to "normal" this change has no functional impact, but
> > reduces the size of the upcoming introduction of dynamic orientation
> > switching.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++----------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > index 7280f7141961..6748f31da7a3 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/reset.h>
> >  #include <linux/slab.h>
> > +#include <linux/usb/typec.h>
> >  
> >  #include <dt-bindings/phy/phy-qcom-qmp.h>
> >  
> > @@ -63,6 +64,10 @@
> >  /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
> >  #define CLAMP_EN				BIT(0) /* enables i/o clamp_n */
> >  
> > +/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */
> > +#define SW_PORTSELECT_VAL			BIT(0)
> > +#define SW_PORTSELECT_MUX			BIT(1)
> > +
> >  #define PHY_INIT_COMPLETE_TIMEOUT		10000
> >  
> >  struct qmp_phy_init_tbl {
> > @@ -1323,6 +1328,8 @@ struct qmp_combo {
> >  	struct clk_fixed_rate pipe_clk_fixed;
> >  	struct clk_hw dp_link_hw;
> >  	struct clk_hw dp_pixel_hw;
> > +
> > +	enum typec_orientation orientation;
> >  };
> >  
> >  static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
> > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
> >  static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> >  {
> >  	u32 val;
> > -	bool reverse = false;
> > +	bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
> 
> Adding parentheses around the right-hand side should make this a little
> easier to parse.
> 
> It also looks like these callbacks end up being called without holding
> the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a
> concurrent switch notification and dp phy power-on but it's not that
> obvious.
> 

It seems we're arriving here from hpd_event_thread(), while
phy_power_on() and phy_power_off() will be called in some other context.
I've not been able to convince myself if DP driver ensures ordering, or
if we have an existing race here...

Unless you insist, I would prefer to follow up with an additional patch
once we've landed this series. The fix will depend on the phy_mutex
shuffling patch anyways...

> > +	const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
> 
> Also could you add these before u32 val to maintain an approximation of
> reverse xmas style?
> 

I'd be happy to do so :)

Regards,
Bjorn

> And similar below.
> 
> Johan