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 <[email protected]>
---
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);
@@ -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
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 <[email protected]>
> ---
> 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,
--
heikki
On Wed, Nov 29, 2023 at 4:26 PM Heikki Krogerus
<[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>> ---
>> 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
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,
--
heikki
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,
>
--
cheers,
-roger
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,
--
heikki
+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.
--
cheers,
-roger
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