[PATCH 0/2] usb: typec: tcpm: Fix logic and documentation of tcpc_dev->init

[PATCH 0/2] usb: typec: tcpm: Fix logic and documentation of tcpc_dev->init

[Bryan O'Donoghue]

The tcpm code treats the init callback as optional when registering a port
but as required when doing a tcpm_init(). In addition we uniquely don't
document init in struct tcpc_dev {}.

This series fixes both situations calling tcpc_dev->init() only if the
implementing driver provides an init and documenting init in the tcpc_dev
data-structure for consistency.

Bryan O'Donoghue (2):
  usb: typec: tcpm: Call init callback only when provided
  usb: typec: tcpm: Add a description for the init callback

 drivers/usb/typec/tcpm/tcpm.c | 3 ++-
 include/linux/usb/tcpm.h      | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.30.1

[PATCH 1/2] usb: typec: tcpm: Call init callback only when provided

[Bryan O'Donoghue]

The tcpc_dev structure lists a number of callbacks as required when
implementing a TCPM driver. tcpc_dev->init() is not listed as required.

Currently tcpc_dev->init() is called irrespective of whether or not the
callback is set. Let's conditionally call init() as with other non-required
callbacks such as get_current_limit() or set_current_limit().

Fixes: f0690a25a140b ("staging: typec: USB Type-C Port Manager (tcpm)")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/typec/tcpm/tcpm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index c4fdc00a3bc8..355067e6d420 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -5657,7 +5657,8 @@ static void tcpm_init(struct tcpm_port *port)
 {
 	enum typec_cc_status cc1, cc2;
 
-	port->tcpc->init(port->tcpc);
+	if (port->tcpc->init)
+		port->tcpc->init(port->tcpc);
 
 	tcpm_reset_port(port);
 
-- 
2.30.1

[PATCH 2/2] usb: typec: tcpm: Add a description for the init callback

[Bryan O'Donoghue]

The init callback is the only callback in the tcpc_dev structure which
doesn't have a description. The code treats the callback as optional but,
we don't document that.

Let's add a description making clear the callback is optional.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/linux/usb/tcpm.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 42fcfbe10590..452a0bb9ec50 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -58,6 +58,8 @@ enum tcpm_transmit_type {
 /**
  * struct tcpc_dev - Port configuration and callback functions
  * @fwnode:	Pointer to port fwnode
+ * @init:	Optional; Called by tcpm_port_register() and tcpm_tcpc_reset()
+ *		to set the TCPM driver into a known initial state.
  * @get_vbus:	Called to read current VBUS state
  * @get_current_limit:
  *		Optional; called by the tcpm core when configured as a snk
-- 
2.30.1

Re: [PATCH 1/2] usb: typec: tcpm: Call init callback only when provided

[Guenter Roeck]

On 5/10/21 7:22 PM, Bryan O'Donoghue wrote:
> The tcpc_dev structure lists a number of callbacks as required when
> implementing a TCPM driver. tcpc_dev->init() is not listed as required.
> 
> Currently tcpc_dev->init() is called irrespective of whether or not the
> callback is set. Let's conditionally call init() as with other non-required
> callbacks such as get_current_limit() or set_current_limit().
> 
> Fixes: f0690a25a140b ("staging: typec: USB Type-C Port Manager (tcpm)")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Are we going to see a driver with no init function ?
If not, I would suggest to make the call officially mandatory.

Guenter

> ---
>   drivers/usb/typec/tcpm/tcpm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index c4fdc00a3bc8..355067e6d420 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -5657,7 +5657,8 @@ static void tcpm_init(struct tcpm_port *port)
>   {
>   	enum typec_cc_status cc1, cc2;
>   
> -	port->tcpc->init(port->tcpc);
> +	if (port->tcpc->init)
> +		port->tcpc->init(port->tcpc);
>   
>   	tcpm_reset_port(port);
>   
> 

Re: [PATCH 1/2] usb: typec: tcpm: Call init callback only when provided

[Bryan O'Donoghue]

On 11/05/2021 03:44, Guenter Roeck wrote:
> Are we going to see a driver with no init function ?
> If not, I would suggest to make the call officially mandatory.
> 
> Guenter

again in plaintext..

I have some tpcm code that doesn't need to put anything into the init 
function which is how I found this.

Its very much up to yourself on making init() mandatory though. I'm just 
as happy to redo the patch and add a check to the add port routine

Re: [PATCH 1/2] usb: typec: tcpm: Call init callback only when provided

[Guenter Roeck]

On 5/11/21 2:21 AM, Bryan O'Donoghue wrote:
> On 11/05/2021 03:44, Guenter Roeck wrote:
>> Are we going to see a driver with no init function ?
>> If not, I would suggest to make the call officially mandatory.
>>
>> Guenter
> 
> again in plaintext..
> 
> I have some tpcm code that doesn't need to put anything into the init function which is how I found this.
> 
> Its very much up to yourself on making init() mandatory though. I'm just as happy to redo the patch and add a check to the add port routine


No need, but maybe submit the code that doesn't need it together
with the change making it optional so we can see the actual use case.

Thanks,
Guenter

Re: [PATCH 1/2] usb: typec: tcpm: Call init callback only when provided

[Bryan O'Donoghue]

On 11/05/2021 20:03, Guenter Roeck wrote:
> No need, but maybe submit the code that doesn't need it together
> with the change making it optional so we can see the actual use case.

Fair enough