Re: [PATCH] USB: typec: tps6598x: use device 'type' field to identify devices

[Roger Quadros <rogerq@kernel.org>]

Hi,

On 29/11/2023 16:26, Heikki Krogerus wrote:
> Hi,
> 
> Sorry to keep you waiting.
> 
> On Thu, Nov 23, 2023 at 11:00:21PM +0200, Alexandru Ardelean wrote:
>> Using the {of_}device_is_compatible function(s) is not too expensive.
>> But since the driver already needs to match for the 'struct tipd_data'
>> device parameters (via device_get_match_data()), we can add a simple 'type'
>> field.
>>
>> This adds a minor optimization in certain operations, where we the check
>> for TPS25750 (or Apple CD321X) is a bit faster.
>>
>> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
>> ---
>>  drivers/usb/typec/tipd/core.c | 34 ++++++++++++++++++++++------------
>>  1 file changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index fbd23de5c5cb..69d3e11bb30c 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -105,7 +105,14 @@ static const char *const modes[] = {
>>  
>>  struct tps6598x;
>>  
>> +enum tipd_type {
>> +	TIPD_TYPE_TI_TPS6598X,
>> +	TIPD_TYPE_APPLE_CD321X,
>> +	TIPD_TYPE_TI_TPS25750X,
>> +};
>> +
>>  struct tipd_data {
>> +	enum tipd_type type;
>>  	irq_handler_t irq_handler;
>>  	int (*register_port)(struct tps6598x *tps, struct fwnode_handle *node);
>>  	void (*trace_power_status)(u16 status);
> 
> Why not just match against the structures themselves?
> 
>         if (tps->data == &tps25750_data)
>                 ...

Then you need to declare tps25750_data and friends at the top of the file?

A better approach might be to have type agnostic quirk flags for the special
behavior required for different types. This way, multiple devices can share
the same quirk if needed.

e.g.
NEEDS_POWER_UP instead of TIPD_TYPE_APPLE_CD321X
SKIP_VID_READ instead of TIPD_TYPE_TI_TPS25750X
INIT_ON_RESUME instead of TIPD_TYPE_TI_TPS25750X

Also rename cd321x_switch_power_state() to tps6598x_switch_power_state().

> 
>> @@ -1195,7 +1202,6 @@ tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>>  
>>  static int tps6598x_probe(struct i2c_client *client)
>>  {
>> -	struct device_node *np = client->dev.of_node;
>>  	struct tps6598x *tps;
>>  	struct fwnode_handle *fwnode;
>>  	u32 status;
>> @@ -1211,11 +1217,19 @@ static int tps6598x_probe(struct i2c_client *client)
>>  	mutex_init(&tps->lock);
>>  	tps->dev = &client->dev;
>>  
>> +	if (dev_fwnode(tps->dev))
>> +		tps->data = device_get_match_data(tps->dev);
>> +	else
>> +		tps->data = i2c_get_match_data(client);
>> +
>> +	if (!tps->data)
>> +		return -EINVAL;
>> +
>>  	tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config);
>>  	if (IS_ERR(tps->regmap))
>>  		return PTR_ERR(tps->regmap);
>>  
>> -	is_tps25750 = device_is_compatible(tps->dev, "ti,tps25750");
>> +	is_tps25750 = (tps->data->type == TIPD_TYPE_TI_TPS25750X);
>>  	if (!is_tps25750) {
>>  		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
>>  		if (ret < 0 || !vid)
>> @@ -1229,7 +1243,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>  	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>>  		tps->i2c_protocol = true;
>>  
>> -	if (np && of_device_is_compatible(np, "apple,cd321x")) {
>> +	if (tps->data->type == TIPD_TYPE_APPLE_CD321X) {
>>  		/* Switch CD321X chips to the correct system power state */
>>  		ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
>>  		if (ret)
>> @@ -1247,13 +1261,6 @@ static int tps6598x_probe(struct i2c_client *client)
>>  			TPS_REG_INT_PLUG_EVENT;
>>  	}
>>  
>> -	if (dev_fwnode(tps->dev))
>> -		tps->data = device_get_match_data(tps->dev);
>> -	else
>> -		tps->data = i2c_get_match_data(client);
>> -	if (!tps->data)
>> -		return -EINVAL;
>> -
>>  	/* Make sure the controller has application firmware running */
>>  	ret = tps6598x_check_mode(tps);
>>  	if (ret < 0)
>> @@ -1366,7 +1373,7 @@ static void tps6598x_remove(struct i2c_client *client)
>>  	usb_role_switch_put(tps->role_sw);
>>  
>>  	/* Reset PD controller to remove any applied patch */
>> -	if (device_is_compatible(tps->dev, "ti,tps25750"))
>> +	if (tps->data->type == TIPD_TYPE_TI_TPS25750X)
>>  		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>>  }
>>  
>> @@ -1396,7 +1403,7 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	if (device_is_compatible(tps->dev, "ti,tps25750") && ret == TPS_MODE_PTCH) {
>> +	if (tps->data->type == TIPD_TYPE_TI_TPS25750X && ret == TPS_MODE_PTCH) {
>>  		ret = tps25750_init(tps);
>>  		if (ret)
>>  			return ret;
>> @@ -1419,6 +1426,7 @@ static const struct dev_pm_ops tps6598x_pm_ops = {
>>  };
>>  
>>  static const struct tipd_data cd321x_data = {
>> +	.type = TIPD_TYPE_APPLE_CD321X,
>>  	.irq_handler = cd321x_interrupt,
>>  	.register_port = tps6598x_register_port,
>>  	.trace_power_status = trace_tps6598x_power_status,
>> @@ -1426,6 +1434,7 @@ static const struct tipd_data cd321x_data = {
>>  };
>>  
>>  static const struct tipd_data tps6598x_data = {
>> +	.type = TIPD_TYPE_TI_TPS6598X,
>>  	.irq_handler = tps6598x_interrupt,
>>  	.register_port = tps6598x_register_port,
>>  	.trace_power_status = trace_tps6598x_power_status,
>> @@ -1433,6 +1442,7 @@ static const struct tipd_data tps6598x_data = {
>>  };
>>  
>>  static const struct tipd_data tps25750_data = {
>> +	.type = TIPD_TYPE_TI_TPS25750X,
>>  	.irq_handler = tps25750_interrupt,
>>  	.register_port = tps25750_register_port,
>>  	.trace_power_status = trace_tps25750_power_status,
>> -- 
>> 2.42.1
> 
> thanks,
> 

-- 
cheers,
-roger