Received-SPF: pass (google.com: domain of linux-kernel+bounces-81362-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 26 Feb 2024 13:27:01 +0100
From: Marco Felsch <m.felsch@pengutronix.de>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: gregkh@linuxfoundation.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	linux@roeck-us.net, jun.li@nxp.com, devicetree@vger.kernel.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel@pengutronix.de
Subject: Re: [PATCH v3 4/4] usb: typec: tcpci: add support to set connector
 orientation
Message-ID: <20240226122701.inqpodm6mdfxwjo2@pengutronix.de>
References: <20240222210903.208901-1-m.felsch@pengutronix.de>
 <20240222210903.208901-5-m.felsch@pengutronix.de>
 <ZdxII9W/CBx76Xai@kuha.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZdxII9W/CBx76Xai@kuha.fi.intel.com>

Hi,

On 24-02-26, Heikki Krogerus wrote:
> On Thu, Feb 22, 2024 at 10:09:03PM +0100, Marco Felsch wrote:
> > This add the support to set the optional connector orientation bit which
> > is part of the optional CONFIG_STANDARD_OUTPUT register 0x18 [1]. This
> > allows system designers to connect the tcpc orientation pin directly to
> > the 2:1 ss-mux.
> > 
> > [1] https://www.usb.org/sites/default/files/documents/usb-port_controller_specification_rev2.0_v1.0_0.pdf
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v3:
> > - no changes
> > v2:
> > - Make use of fallthrough 
> > 
> >  drivers/usb/typec/tcpm/tcpci.c | 44 ++++++++++++++++++++++++++++++++++
> >  include/linux/usb/tcpci.h      |  8 +++++++
> >  2 files changed, 52 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index 7118551827f6..73a52e7f95c2 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -67,6 +67,18 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
> >  	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
> >  }
> >  
> > +static bool tcpci_check_std_output_cap(struct regmap *regmap, u8 mask)
> > +{
> > +	unsigned int reg;
> > +	int ret;
> > +
> > +	ret = regmap_read(regmap, TCPC_STD_OUTPUT_CAP, &reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return (reg & mask) == mask;
> > +}
> > +
> >  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
> >  {
> >  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > @@ -301,6 +313,28 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> >  			   TCPC_TCPC_CTRL_ORIENTATION : 0);
> >  }
> >  
> > +static int tcpci_set_orientation(struct tcpc_dev *tcpc,
> > +				 enum typec_orientation orientation)
> > +{
> > +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > +	unsigned int reg;
> > +
> > +	switch (orientation) {
> > +	case TYPEC_ORIENTATION_NONE:
> > +		/* We can't put a single output into high impedance */
> > +		fallthrough;
> > +	case TYPEC_ORIENTATION_NORMAL:
> > +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL;
> > +		break;
> > +	case TYPEC_ORIENTATION_REVERSE:
> > +		reg = TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED;
> > +		break;
> > +	}
> > +
> > +	return regmap_update_bits(tcpci->regmap, TCPC_CONFIG_STD_OUTPUT,
> > +				  TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK, reg);
> > +}
> > +
> >  static void tcpci_set_partner_usb_comm_capable(struct tcpc_dev *tcpc, bool capable)
> >  {
> >  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > @@ -808,6 +842,9 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
> >  	if (tcpci->data->vbus_vsafe0v)
> >  		tcpci->tcpc.is_vbus_vsafe0v = tcpci_is_vbus_vsafe0v;
> >  
> > +	if (tcpci->data->set_orientation)
> > +		tcpci->tcpc.set_orientation = tcpci_set_orientation;
> 
> I don't think that flag is needed - not yet at least. Please just call
> tcpci_check_std_output_cap() directly from here.

The reason for having it this way was to not break exsisting user like:
tcpci_rt1711h, tcpci_mt6370, tcpci_maxim which may or may not implement
the TCPC_STD_OUTPUT_CAP_ORIENTATION. This way the users of
tcpci_register_port() can decide by on its own if they do have this
feature or not and how this is checked. I'm fine with your proposal if
you still think that we can check this unconditional.

Regards,
  Marco

> >  	err = tcpci_parse_config(tcpci);
> >  	if (err < 0)
> >  		return ERR_PTR(err);
> > @@ -851,6 +888,13 @@ static int tcpci_probe(struct i2c_client *client)
> >  	if (err < 0)
> >  		return err;
> >  
> > +	err = tcpci_check_std_output_cap(chip->data.regmap,
> > +					 TCPC_STD_OUTPUT_CAP_ORIENTATION);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	chip->data.set_orientation = err;
> > +
> >  	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> >  	if (IS_ERR(chip->tcpci))
> >  		return PTR_ERR(chip->tcpci);
> > diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
> > index 467e8045e9f8..f2bfb4250366 100644
> > --- a/include/linux/usb/tcpci.h
> > +++ b/include/linux/usb/tcpci.h
> > @@ -47,6 +47,9 @@
> >  #define TCPC_SINK_FAST_ROLE_SWAP	BIT(0)
> >  
> >  #define TCPC_CONFIG_STD_OUTPUT		0x18
> > +#define TCPC_CONFIG_STD_OUTPUT_ORIENTATION_MASK		BIT(0)
> > +#define TCPC_CONFIG_STD_OUTPUT_ORIENTATION_NORMAL	0
> > +#define TCPC_CONFIG_STD_OUTPUT_ORIENTATION_FLIPPED	1
> >  
> >  #define TCPC_TCPC_CTRL			0x19
> >  #define TCPC_TCPC_CTRL_ORIENTATION	BIT(0)
> > @@ -127,6 +130,7 @@
> >  #define TCPC_DEV_CAP_2			0x26
> >  #define TCPC_STD_INPUT_CAP		0x28
> >  #define TCPC_STD_OUTPUT_CAP		0x29
> > +#define TCPC_STD_OUTPUT_CAP_ORIENTATION	BIT(0)
> >  
> >  #define TCPC_MSG_HDR_INFO		0x2e
> >  #define TCPC_MSG_HDR_INFO_DATA_ROLE	BIT(3)
> > @@ -198,12 +202,16 @@ struct tcpci;
> >   *		Chip level drivers are expected to check for contaminant and call
> >   *		tcpm_clean_port when the port is clean to put the port back into
> >   *		toggling state.
> > + * @set_orientation:
> > + *		Optional; Enable setting the connector orientation
> > + *		CONFIG_STANDARD_OUTPUT (0x18) bit0.
> >   */
> >  struct tcpci_data {
> >  	struct regmap *regmap;
> >  	unsigned char TX_BUF_BYTE_x_hidden:1;
> >  	unsigned char auto_discharge_disconnect:1;
> >  	unsigned char vbus_vsafe0v:1;
> > +	unsigned char set_orientation:1;
> >  
> >  	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> >  	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> > -- 
> > 2.39.2
> 
> -- 
> heikki
>