[PATCH 0/3] usb: typec: tipd: improve probe diagnostics and POWER_STATUS handling

[PATCH 0/3] usb: typec: tipd: improve probe diagnostics and POWER_STATUS handling

[Radhey Shyam Pandey]

This series tightens TI TIPD (TPS6598x family) bring-up ergonomics and
cleans up register bit definitions:

- Log vendor ID read failures with I2C return code and register value so
  boards with wiring or address mistakes are easier to debug at probe
  time.

- Treat operation without an interrupt line as a normal polling setup and
  log at info instead of warning.

- Name TPS_REG_POWER_STATUS connection/source-sink bit masks and use
  them consistently with TPS_FIELD_GET (no behavior change).

Radhey Shyam Pandey (3):
  usb: typec: tipd: add error message for vendor ID read failure
  usb: typec: tipd: demote missing IRQ log to info for polling mode
  usb: typec: tipd: name TPS_REG_POWER_STATUS field masks

 drivers/usb/typec/tipd/core.c     |  9 ++++++---
 drivers/usb/typec/tipd/tps6598x.h | 10 +++++++---
 2 files changed, 13 insertions(+), 6 deletions(-)


base-commit: 1f63dd8ca0dc05a8272bb8155f643c691d29bb11
-- 
2.44.4

[PATCH 1/3] usb: typec: tipd: add error message for vendor ID read failure

[Radhey Shyam Pandey]

Log when the vendor ID read fails or returns zero, including the I2C error
code and register value, to ease probe diagnostics.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/usb/typec/tipd/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 43faec794b95..b282366b5326 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1744,7 +1744,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	struct tps6598x *tps;
 	struct fwnode_handle *fwnode;
 	u32 status;
-	u32 vid;
+	u32 vid = 0;
 	int ret;
 
 	data = i2c_get_match_data(client);
@@ -1772,8 +1772,11 @@ static int tps6598x_probe(struct i2c_client *client)
 
 	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
 		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
-		if (ret < 0 || !vid)
+		if (ret < 0 || !vid) {
+			dev_err(tps->dev, "failed to read vendor ID: %d, vid: %#x\n",
+				ret, vid);
 			return -ENODEV;
+		}
 	}
 
 	/*
-- 
2.44.4

Re: [PATCH 1/3] usb: typec: tipd: add error message for vendor ID read failure

[Heikki Krogerus]

On Wed, May 13, 2026 at 11:58:48PM +0530, Radhey Shyam Pandey wrote:
> Log when the vendor ID read fails or returns zero, including the I2C error
> code and register value, to ease probe diagnostics.
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
>  drivers/usb/typec/tipd/core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 43faec794b95..b282366b5326 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1744,7 +1744,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	struct tps6598x *tps;
>  	struct fwnode_handle *fwnode;
>  	u32 status;
> -	u32 vid;
> +	u32 vid = 0;

Why is this necessary?

>  	int ret;
>  
>  	data = i2c_get_match_data(client);
> @@ -1772,8 +1772,11 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
>  		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> -		if (ret < 0 || !vid)
> +		if (ret < 0 || !vid) {
> +			dev_err(tps->dev, "failed to read vendor ID: %d, vid: %#x\n",
> +				ret, vid);
>  			return -ENODEV;
> +		}
>  	}
>  
>  	/*
> -- 
> 2.44.4

-- 
heikki

Re: [PATCH 1/3] usb: typec: tipd: add error message for vendor ID read failure

[Pandey, Radhey Shyam]

On 5/18/2026 3:49 PM, Heikki Krogerus wrote:
> On Wed, May 13, 2026 at 11:58:48PM +0530, Radhey Shyam Pandey wrote:
>> Log when the vendor ID read fails or returns zero, including the I2C error
>> code and register value, to ease probe diagnostics.
>>
>> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
>> ---
>>   drivers/usb/typec/tipd/core.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index 43faec794b95..b282366b5326 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -1744,7 +1744,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>   	struct tps6598x *tps;
>>   	struct fwnode_handle *fwnode;
>>   	u32 status;
>> -	u32 vid;
>> +	u32 vid = 0;
> 
> Why is this necessary?

Thanks for the review.

When ret < 0, tps6598x_read32() → tps6598x_block_read() returns on
error before writing *val. So vid is never set; passing it to
dev_err with %#x would read an uninitialized u32(random log noise).

ret captures if I2C/regmap path reported an error and vid tells what
came back when the transport layer did not fail. Hope that clarifies.

Thanks,
Radhey
> 
>>   	int ret;
>>   
>>   	data = i2c_get_match_data(client);
>> @@ -1772,8 +1772,11 @@ static int tps6598x_probe(struct i2c_client *client)
>>   
>>   	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
>>   		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
>> -		if (ret < 0 || !vid)
>> +		if (ret < 0 || !vid) {
>> +			dev_err(tps->dev, "failed to read vendor ID: %d, vid: %#x\n",
>> +				ret, vid);
>>   			return -ENODEV;
>> +		}
>>   	}
>>   
>>   	/*
>> -- 
>> 2.44.4
> 

Re: [PATCH 1/3] usb: typec: tipd: add error message for vendor ID read failure

[Heikki Krogerus]

On Mon, May 18, 2026 at 09:23:58PM +0530, Pandey, Radhey Shyam wrote:
> On 5/18/2026 3:49 PM, Heikki Krogerus wrote:
> > On Wed, May 13, 2026 at 11:58:48PM +0530, Radhey Shyam Pandey wrote:
> > > Log when the vendor ID read fails or returns zero, including the I2C error
> > > code and register value, to ease probe diagnostics.
> > > 
> > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > > ---
> > >   drivers/usb/typec/tipd/core.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > > index 43faec794b95..b282366b5326 100644
> > > --- a/drivers/usb/typec/tipd/core.c
> > > +++ b/drivers/usb/typec/tipd/core.c
> > > @@ -1744,7 +1744,7 @@ static int tps6598x_probe(struct i2c_client *client)
> > >   	struct tps6598x *tps;
> > >   	struct fwnode_handle *fwnode;
> > >   	u32 status;
> > > -	u32 vid;
> > > +	u32 vid = 0;
> > 
> > Why is this necessary?
> 
> Thanks for the review.
> 
> When ret < 0, tps6598x_read32() → tps6598x_block_read() returns on
> error before writing *val. So vid is never set; passing it to
> dev_err with %#x would read an uninitialized u32(random log noise).
> 
> ret captures if I2C/regmap path reported an error and vid tells what
> came back when the transport layer did not fail. Hope that clarifies.

Okay, got it.

Thanks,

> Thanks,
> Radhey
> > 
> > >   	int ret;
> > >   	data = i2c_get_match_data(client);
> > > @@ -1772,8 +1772,11 @@ static int tps6598x_probe(struct i2c_client *client)
> > >   	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
> > >   		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> > > -		if (ret < 0 || !vid)
> > > +		if (ret < 0 || !vid) {
> > > +			dev_err(tps->dev, "failed to read vendor ID: %d, vid: %#x\n",
> > > +				ret, vid);
> > >   			return -ENODEV;
> > > +		}
> > >   	}
> > >   	/*
> > > -- 
> > > 2.44.4
> > 

-- 
heikki

Re: [PATCH 1/3] usb: typec: tipd: add error message for vendor ID read failure

[Pandey, Radhey Shyam]

> On Mon, May 18, 2026 at 09:23:58PM +0530, Pandey, Radhey Shyam wrote:
>> On 5/18/2026 3:49 PM, Heikki Krogerus wrote:
>>> On Wed, May 13, 2026 at 11:58:48PM +0530, Radhey Shyam Pandey wrote:
>>>> Log when the vendor ID read fails or returns zero, including the I2C error
>>>> code and register value, to ease probe diagnostics.
>>>>
>>>> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
>>>> ---
>>>>    drivers/usb/typec/tipd/core.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>>> index 43faec794b95..b282366b5326 100644
>>>> --- a/drivers/usb/typec/tipd/core.c
>>>> +++ b/drivers/usb/typec/tipd/core.c
>>>> @@ -1744,7 +1744,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>    	struct tps6598x *tps;
>>>>    	struct fwnode_handle *fwnode;
>>>>    	u32 status;
>>>> -	u32 vid;
>>>> +	u32 vid = 0;
>>>
>>> Why is this necessary?
>>
>> Thanks for the review.
>>
>> When ret < 0, tps6598x_read32() → tps6598x_block_read() returns on
>> error before writing *val. So vid is never set; passing it to
>> dev_err with %#x would read an uninitialized u32(random log noise).
>>
>> ret captures if I2C/regmap path reported an error and vid tells what
>> came back when the transport layer did not fail. Hope that clarifies.
> 
> Okay, got it.
> 

Thanks , will also capture this justification in v2 commit message.
> Thanks,
> 
>> Thanks,
>> Radhey
>>>
>>>>    	int ret;
>>>>    	data = i2c_get_match_data(client);
>>>> @@ -1772,8 +1772,11 @@ static int tps6598x_probe(struct i2c_client *client)
>>>>    	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
>>>>    		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
>>>> -		if (ret < 0 || !vid)
>>>> +		if (ret < 0 || !vid) {
>>>> +			dev_err(tps->dev, "failed to read vendor ID: %d, vid: %#x\n",
>>>> +				ret, vid);
>>>>    			return -ENODEV;
>>>> +		}
>>>>    	}
>>>>    	/*
>>>> -- 
>>>> 2.44.4
>>>
> 

[PATCH 2/3] usb: typec: tipd: demote missing IRQ log to info for polling mode

[Radhey Shyam Pandey]

Operating without an interrupt line and using the driver's polling path
is valid. Log at info level instead of warning.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/usb/typec/tipd/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index b282366b5326..fcd56bcffab8 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1853,7 +1853,7 @@ static int tps6598x_probe(struct i2c_client *client)
 						IRQF_SHARED | IRQF_ONESHOT,
 						dev_name(&client->dev), tps);
 	} else {
-		dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n");
+		dev_info(tps->dev, "no IRQ specified, using polling mode\n");
 		INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
 		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
 				   msecs_to_jiffies(POLL_INTERVAL));
-- 
2.44.4

Re: [PATCH 2/3] usb: typec: tipd: demote missing IRQ log to info for polling mode

[Heikki Krogerus]

On Wed, May 13, 2026 at 11:58:49PM +0530, Radhey Shyam Pandey wrote:
> Operating without an interrupt line and using the driver's polling path
> is valid. Log at info level instead of warning.
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
>  drivers/usb/typec/tipd/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index b282366b5326..fcd56bcffab8 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1853,7 +1853,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  						IRQF_SHARED | IRQF_ONESHOT,
>  						dev_name(&client->dev), tps);
>  	} else {
> -		dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n");
> +		dev_info(tps->dev, "no IRQ specified, using polling mode\n");

If this is a valid case, then the driver should not make any noise.
Let's use deb_dbg() instead.

>  		INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
>  		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>  				   msecs_to_jiffies(POLL_INTERVAL));

thanks,

-- 
heikki

Re: [PATCH 2/3] usb: typec: tipd: demote missing IRQ log to info for polling mode

[Pandey, Radhey Shyam]

On 5/18/2026 3:56 PM, Heikki Krogerus wrote:
> On Wed, May 13, 2026 at 11:58:49PM +0530, Radhey Shyam Pandey wrote:
>> Operating without an interrupt line and using the driver's polling path
>> is valid. Log at info level instead of warning.
>>
>> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
>> ---
>>   drivers/usb/typec/tipd/core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index b282366b5326..fcd56bcffab8 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -1853,7 +1853,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>   						IRQF_SHARED | IRQF_ONESHOT,
>>   						dev_name(&client->dev), tps);
>>   	} else {
>> -		dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n");
>> +		dev_info(tps->dev, "no IRQ specified, using polling mode\n");
> 
> If this is a valid case, then the driver should not make any noise.
> Let's use deb_dbg() instead.

Sure , will make it dev_dbg in v2.
> 
>>   		INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
>>   		queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>>   				   msecs_to_jiffies(POLL_INTERVAL));
> 
> thanks,
> 

[PATCH 3/3] usb: typec: tipd: name TPS_REG_POWER_STATUS field masks

[Radhey Shyam Pandey]

Define named masks for Power Status fields (connection and source/sink)
and reuse them consistently for both field extraction and value
construction. This avoids raw bit usage, keeps the definitions aligned.
No functional change.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/usb/typec/tipd/tps6598x.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 03edbb77bbd6..d4140f4da5bb 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -142,9 +142,13 @@
 #define TPS_SYSTEM_POWER_STATE_S4	0x04
 #define TPS_SYSTEM_POWER_STATE_S5	0x05
 
-/* TPS_REG_POWER_STATUS bits */
-#define TPS_POWER_STATUS_CONNECTION(x)  TPS_FIELD_GET(BIT(0), (x))
-#define TPS_POWER_STATUS_SOURCESINK(x)	TPS_FIELD_GET(BIT(1), (x))
+/* TPS_REG_POWER_STATUS bits (masks shared by TPS_FIELD_GET accessors and FIELD_PREP) */
+#define TPS_POWER_STATUS_CONNECTION_MASK	BIT(0)
+#define TPS_POWER_STATUS_SOURCESINK_MASK	BIT(1)
+#define TPS_POWER_STATUS_CONNECTION(x) \
+	TPS_FIELD_GET(TPS_POWER_STATUS_CONNECTION_MASK, (x))
+#define TPS_POWER_STATUS_SOURCESINK(x) \
+	TPS_FIELD_GET(TPS_POWER_STATUS_SOURCESINK_MASK, (x))
 #define TPS_POWER_STATUS_BC12_DET(x)	TPS_FIELD_GET(BIT(2), (x))
 
 #define TPS_POWER_STATUS_TYPEC_CURRENT_MASK GENMASK(3, 2)
-- 
2.44.4

Re: [PATCH 3/3] usb: typec: tipd: name TPS_REG_POWER_STATUS field masks

[Heikki Krogerus]

On Wed, May 13, 2026 at 11:58:50PM +0530, Radhey Shyam Pandey wrote:
> Define named masks for Power Status fields (connection and source/sink)
> and reuse them consistently for both field extraction and value
> construction. This avoids raw bit usage, keeps the definitions aligned.
> No functional change.
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/tps6598x.h | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
> index 03edbb77bbd6..d4140f4da5bb 100644
> --- a/drivers/usb/typec/tipd/tps6598x.h
> +++ b/drivers/usb/typec/tipd/tps6598x.h
> @@ -142,9 +142,13 @@
>  #define TPS_SYSTEM_POWER_STATE_S4	0x04
>  #define TPS_SYSTEM_POWER_STATE_S5	0x05
>  
> -/* TPS_REG_POWER_STATUS bits */
> -#define TPS_POWER_STATUS_CONNECTION(x)  TPS_FIELD_GET(BIT(0), (x))
> -#define TPS_POWER_STATUS_SOURCESINK(x)	TPS_FIELD_GET(BIT(1), (x))
> +/* TPS_REG_POWER_STATUS bits (masks shared by TPS_FIELD_GET accessors and FIELD_PREP) */
> +#define TPS_POWER_STATUS_CONNECTION_MASK	BIT(0)
> +#define TPS_POWER_STATUS_SOURCESINK_MASK	BIT(1)
> +#define TPS_POWER_STATUS_CONNECTION(x) \
> +	TPS_FIELD_GET(TPS_POWER_STATUS_CONNECTION_MASK, (x))
> +#define TPS_POWER_STATUS_SOURCESINK(x) \
> +	TPS_FIELD_GET(TPS_POWER_STATUS_SOURCESINK_MASK, (x))
>  #define TPS_POWER_STATUS_BC12_DET(x)	TPS_FIELD_GET(BIT(2), (x))
>  
>  #define TPS_POWER_STATUS_TYPEC_CURRENT_MASK GENMASK(3, 2)
> -- 
> 2.44.4

-- 
heikki