| Message ID | 20231123210021.463122-1-alex@shruggie.ro (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: typec: tps6598x: use device 'type' field to identify devices | expand |
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) ... > @@ -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,
On Wed, Nov 29, 2023 at 4:26 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi, > > Sorry to keep you waiting. No worries. I know how this works: maintainers do what they can, when they can. > > 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) Sure. This version also works. I've been more accustomed to the enum version. Will update. As a broader context, this patch came along from trying to backport some patches back to 5.15. device_is_compatible() is not present i 5.15 of_device_is_compatible is present there. It seemed like doing a neutral version (between 5.15 and latest kernel) looks like a good idea. > ... > > > @@ -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, > > -- > heikki
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, >
Hi Roger, > > 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(). No. Functions like that isolate cd321x specific functionality into an actual "function" just like they should. Quirk flags mean that if something breaks, it will almost always break for everybody (there is no real isolation with quirk flags), and when things are fixed and when features are added, we are forced to always "dance" around those quirk flags - you always have to consider them. Platform/device type checks are just as bad IMO, but in one way they are better than quirk flags. There is no question about what a platform check is checking, but quirk flags can so easily become incomprehensible (just what exactly does it mean when you say NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document those quirks, which is waste of effort, and in reality nobody will do). In case of tipd/code.c, it should be converted into a library that only has the common/shared functionality. CD321, TPS2579x, TPS6598x and what ever there is, then will have a glue driver that handles everything that specific for their controller type. Before this driver is reorganised like that (any volunteers?), we'll have the PD controller type checks, but quirk flags we will not have. In general, you should only use quirk flags if there is no other way to move forward - they are the last resort. They are dangerous, and even in the best case they reduce the maintenability of the code. thanks,
Hi Heikki, On 30/11/2023 12:54, Heikki Krogerus wrote: > Hi Roger, > >>> 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(). > > No. Functions like that isolate cd321x specific functionality into an > actual "function" just like they should. > > Quirk flags mean that if something breaks, it will almost always break > for everybody (there is no real isolation with quirk flags), and when > things are fixed and when features are added, we are forced to always > "dance" around those quirk flags - you always have to consider them. > > Platform/device type checks are just as bad IMO, but in one way they > are better than quirk flags. There is no question about what a > platform check is checking, but quirk flags can so easily become > incomprehensible (just what exactly does it mean when you say > NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document > those quirks, which is waste of effort, and in reality nobody will do). > > In case of tipd/code.c, it should be converted into a library that > only has the common/shared functionality. CD321, TPS2579x, TPS6598x > and what ever there is, then will have a glue driver that handles > everything that specific for their controller type. Do you mean that you want to treat the 3 devices as different incompatible devices so each one has a separate driver which warrants for a different DT binding for each and also Kconfig symbol? > > Before this driver is reorganised like that (any volunteers?), we'll > have the PD controller type checks, but quirk flags we will not have. > > In general, you should only use quirk flags if there is no other > way to move forward - they are the last resort. They are dangerous, > and even in the best case they reduce the maintenability of the code. > > thanks, >
Hi Roger, On Thu, Nov 30, 2023 at 03:30:54PM +0200, Roger Quadros wrote: > Hi Heikki, > > On 30/11/2023 12:54, Heikki Krogerus wrote: > > Hi Roger, > > > >>> 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(). > > > > No. Functions like that isolate cd321x specific functionality into an > > actual "function" just like they should. > > > > Quirk flags mean that if something breaks, it will almost always break > > for everybody (there is no real isolation with quirk flags), and when > > things are fixed and when features are added, we are forced to always > > "dance" around those quirk flags - you always have to consider them. > > > > Platform/device type checks are just as bad IMO, but in one way they > > are better than quirk flags. There is no question about what a > > platform check is checking, but quirk flags can so easily become > > incomprehensible (just what exactly does it mean when you say > > NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document > > those quirks, which is waste of effort, and in reality nobody will do). > > > > In case of tipd/code.c, it should be converted into a library that > > only has the common/shared functionality. CD321, TPS2579x, TPS6598x > > and what ever there is, then will have a glue driver that handles > > everything that specific for their controller type. > > Do you mean that you want to treat the 3 devices as different incompatible devices > so each one has a separate driver which warrants for a different DT binding > for each and also Kconfig symbol? I did not consider that, I was thinking that we would still continue with just one probe driver for all of these, but now that you mentioned this, maybe it would actually make sense to have separate full fledged probing drivers for all of these. Do you think it would be better like that? Would it be a problem to split the bindings? thanks,
+Rob & Krzysztof On 01/12/2023 10:10, Heikki Krogerus wrote: > Hi Roger, > > On Thu, Nov 30, 2023 at 03:30:54PM +0200, Roger Quadros wrote: >> Hi Heikki, >> >> On 30/11/2023 12:54, Heikki Krogerus wrote: >>> Hi Roger, >>> >>>>> 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(). >>> >>> No. Functions like that isolate cd321x specific functionality into an >>> actual "function" just like they should. >>> >>> Quirk flags mean that if something breaks, it will almost always break >>> for everybody (there is no real isolation with quirk flags), and when >>> things are fixed and when features are added, we are forced to always >>> "dance" around those quirk flags - you always have to consider them. >>> >>> Platform/device type checks are just as bad IMO, but in one way they >>> are better than quirk flags. There is no question about what a >>> platform check is checking, but quirk flags can so easily become >>> incomprehensible (just what exactly does it mean when you say >>> NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document >>> those quirks, which is waste of effort, and in reality nobody will do). >>> >>> In case of tipd/code.c, it should be converted into a library that >>> only has the common/shared functionality. CD321, TPS2579x, TPS6598x >>> and what ever there is, then will have a glue driver that handles >>> everything that specific for their controller type. >> >> Do you mean that you want to treat the 3 devices as different incompatible devices >> so each one has a separate driver which warrants for a different DT binding >> for each and also Kconfig symbol? > > I did not consider that, I was thinking that we would still continue > with just one probe driver for all of these, but now that you > mentioned this, maybe it would actually make sense to have separate > full fledged probing drivers for all of these. Do you think it would > be better like that? Would it be a problem to split the bindings? I'm no DT expert but looks like an overkill to me.
On 01/12/2023 11:57, Roger Quadros wrote: > +Rob & Krzysztof > > On 01/12/2023 10:10, Heikki Krogerus wrote: >> Hi Roger, >> >> On Thu, Nov 30, 2023 at 03:30:54PM +0200, Roger Quadros wrote: >>> Hi Heikki, >>> >>> On 30/11/2023 12:54, Heikki Krogerus wrote: >>>> Hi Roger, >>>> >>>>>> 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(). >>>> >>>> No. Functions like that isolate cd321x specific functionality into an >>>> actual "function" just like they should. >>>> >>>> Quirk flags mean that if something breaks, it will almost always break >>>> for everybody (there is no real isolation with quirk flags), and when >>>> things are fixed and when features are added, we are forced to always >>>> "dance" around those quirk flags - you always have to consider them. >>>> >>>> Platform/device type checks are just as bad IMO, but in one way they >>>> are better than quirk flags. There is no question about what a >>>> platform check is checking, but quirk flags can so easily become >>>> incomprehensible (just what exactly does it mean when you say >>>> NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document >>>> those quirks, which is waste of effort, and in reality nobody will do). >>>> >>>> In case of tipd/code.c, it should be converted into a library that >>>> only has the common/shared functionality. CD321, TPS2579x, TPS6598x >>>> and what ever there is, then will have a glue driver that handles >>>> everything that specific for their controller type. >>> >>> Do you mean that you want to treat the 3 devices as different incompatible devices >>> so each one has a separate driver which warrants for a different DT binding >>> for each and also Kconfig symbol? >> >> I did not consider that, I was thinking that we would still continue >> with just one probe driver for all of these, but now that you >> mentioned this, maybe it would actually make sense to have separate >> full fledged probing drivers for all of these. Do you think it would >> be better like that? Would it be a problem to split the bindings? > > I'm no DT expert but looks like an overkill to me. Splitting or not splitting drivers has nothing to do with bindings. Does the hardware suddenly change if you decide to do something with drivers? No, it does not. Best regards, Krzysztof
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); @@ -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,
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(-)