Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 14 Feb 2018 12:56:41 +0200
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     ShuFanLee <leechu729@gmail.com>
Cc:     linux@roeck-us.net, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, cy_huang@richtek.com,
        shufan_lee@richtek.com
Subject: Re: [PATCH] staging: typec: handle vendor defined part and modify
 drp toggling flow
Message-ID: <20180214105641.GE1480@kuha.fi.intel.com>
References: <1518600244-29949-1-git-send-email-leechu729@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1518600244-29949-1-git-send-email-leechu729@gmail.com>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 14, 2018 at 05:24:04PM +0800, ShuFanLee wrote:
> From: ShuFanLee <shufan_lee@richtek.com>
> 
> Handle vendor defined behavior in tcpci_init and tcpci_irq.
> More operations can be extended in tcpci_vendor_data if needed.
> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
> TCPC shall not start DRP toggling until subsequently the TCPM
> writes to the COMMAND register to start DRP toggling.
> DRP toggling flow is chagned as following:
>   - Write DRP = 0 & Rd/Rd
>   - Write DRP = 1
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
> ---
>  drivers/staging/typec/tcpci.c | 98 ++++++++++++++++++++++++++++++++++++-------
>  drivers/staging/typec/tcpci.h | 15 +++++++
>  2 files changed, 97 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..b3a97b3 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -30,6 +30,7 @@ struct tcpci {
>  	bool controls_vbus;
>  
>  	struct tcpc_dev tcpc;
> +	struct tcpci_vendor_data *vdata;
>  };
>  
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -37,16 +38,29 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
>  	return container_of(tcpc, struct tcpci, tcpc);
>  }
>  
> -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> -			u16 *val)
> +int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
>  {
>  	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
>  }
> +EXPORT_SYMBOL_GPL(tcpci_read16);
>  
> -static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
> +int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
>  {
>  	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
>  }
> +EXPORT_SYMBOL_GPL(tcpci_write16);
> +
> +int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val)
> +{
> +	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u8));
> +}
> +EXPORT_SYMBOL_GPL(tcpci_read8);
> +
> +int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val)
> +{
> +	return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u8));
> +}
> +EXPORT_SYMBOL_GPL(tcpci_write8);

I don't think there is any need to export those wrappers. You can
always expect the glue driver to supply the regmap as member of that
struct tcpci_vendor_data. See below..

>  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  {
> @@ -98,8 +112,10 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>  				    enum typec_cc_status cc)
>  {
> +	int ret;
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> -	unsigned int reg = TCPC_ROLE_CTRL_DRP;
> +	unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +			   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
>  
>  	switch (cc) {
>  	default:
> @@ -116,8 +132,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>  			TCPC_ROLE_CTRL_RP_VAL_SHIFT);
>  		break;
>  	}
> -
> -	return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +	ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(500, 1000);
> +	reg |= TCPC_ROLE_CTRL_DRP;
> +	ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> +			   TCPC_CMD_LOOK4CONNECTION);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
>  }
>  
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -323,6 +350,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>  	if (time_after(jiffies, timeout))
>  		return -ETIMEDOUT;
>  
> +	/* Handle vendor init */
> +	if (tcpci->vdata) {
> +		if (tcpci->vdata->init) {
> +			ret = (*tcpci->vdata->init)(tcpci, tcpci->vdata);

ret = tcpci->vdata->init(...

> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
>  	/* Clear all events */
>  	ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff);
>  	if (ret < 0)
> @@ -351,6 +387,13 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>  
>  	tcpci_read16(tcpci, TCPC_ALERT, &status);
>  
> +	/* Handle vendor defined interrupt */
> +	if (tcpci->vdata) {
> +		if (tcpci->vdata->irq_handler)
> +			(*tcpci->vdata->irq_handler)(tcpci, tcpci->vdata,
> +						     &status);

Ditto.

> +	}
> +
>  	/*
>  	 * Clear alert status for everything except RX_STATUS, which shouldn't
>  	 * be cleared until we have successfully retrieved message.
> @@ -417,7 +460,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>  	.reg_bits = 8,
>  	.val_bits = 8,
>  
> -	.max_register = 0x7F, /* 0x80 .. 0xFF are vendor defined */
> +	.max_register = 0xFF, /* 0x80 .. 0xFF are vendor defined */
>  };
>  
>  static const struct tcpc_config tcpci_tcpc_config = {
> @@ -435,22 +478,22 @@ static int tcpci_parse_config(struct tcpci *tcpci)
>  	return 0;
>  }
>  
> -static int tcpci_probe(struct i2c_client *client,
> -		       const struct i2c_device_id *i2c_id)
> +struct tcpci *tcpci_register_port(struct i2c_client *client,
> +				  struct tcpci_vendor_data *vdata)

struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_...)
{
...

>  {
>  	struct tcpci *tcpci;
>  	int err;
>  
>  	tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL);
>  	if (!tcpci)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	tcpci->client = client;

struct tcpci does not need that "client" member.

>  	tcpci->dev = &client->dev;

tcpci->dev = dev;

> -	i2c_set_clientdata(client, tcpci);
> +	tcpci->vdata = vdata;
>  	tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);

You would now do that in your glue driver, and here just:

tcpci->regmap = data->regmap;

>  	tcpci->tcpc.init = tcpci_init;
>  	tcpci->tcpc.get_vbus = tcpci_get_vbus;
> @@ -467,7 +510,7 @@ static int tcpci_probe(struct i2c_client *client,
>  
>  	err = tcpci_parse_config(tcpci);
>  	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
>  
>  	/* Disable chip interrupts */
>  	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
> @@ -477,17 +520,40 @@ static int tcpci_probe(struct i2c_client *client,
>  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>  					dev_name(tcpci->dev), tcpci);

The interrupt could also be requested in the glue driver. Then from
the irq routine in your glue driver, you call tcpci_irq(). That would
mean you export tcpci_irq() of course.

>  	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
>  
>  	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> -	return PTR_ERR_OR_ZERO(tcpci->port);
> +	if (PTR_ERR_OR_ZERO(tcpci->port))
> +		return ERR_CAST(tcpci->port);
> +
> +	return tcpci;
> +}
> +EXPORT_SYMBOL_GPL(tcpci_register_port);
> +
> +void tcpci_unregister_port(struct tcpci *tcpci)
> +{
> +	tcpm_unregister_port(tcpci->port);
> +}
> +EXPORT_SYMBOL_GPL(tcpci_unregister_port);
> +
> +static int tcpci_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *i2c_id)
> +{
> +	struct tcpci *tcpci;
> +
> +	tcpci = tcpci_register_port(client, NULL);
> +	if (PTR_ERR_OR_ZERO(tcpci))
> +		return PTR_ERR(tcpci);
> +
> +	i2c_set_clientdata(client, tcpci);
> +	return 0;
>  }

That would then look something like this:

static int tcpci_probe(struct i2c_client *client...
{
        struct tcpci_data data; /* s/tcpci_vendor_data/tcpci_data/ */
        struct tcpci *tcpci;

        data.regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
        if (IS_ERR(tcpci->regmap))
                return PTR_ERR(tcpci->regmap);

        /*
         * NOTE: everything in struct tcpci_data should be copied in
         * tcpci_register_port().
         */
        tcpci = tcpci_register_port(&client->dev, &data);
        if (IS_ERR(tcpci))
                return PTR_ERR(tcpci);

        devm_request_threaded_irq(&client->dev, client->irq, NULL,
                                  tcpci_irq,
                                  ...

        i2c_set_clientdata(client, tcpci);

        return 0;
}


Br,

-- 
heikki