| Message ID | 20230320100711.3708-4-frank.wang@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some defects related Type-C TCPM | expand |
Hi, On Mon, Mar 20, 2023 at 06:07:11PM +0800, Frank Wang wrote: > Traverse fixed pdos to calculate the maximum power that the charger > can provide, and it can be get by POWER_SUPPLY_PROP_INPUT_POWER_LIMIT > property. > > Signed-off-by: Frank Wang <frank.wang@rock-chips.com> > --- > drivers/usb/typec/tcpm/tcpm.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 13830b5e2d09f..d6ad3cdf9e4af 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -6320,6 +6320,27 @@ static int tcpm_psy_get_current_now(struct tcpm_port *port, > return 0; > } > > +static int tcpm_psy_get_input_power_limit(struct tcpm_port *port, > + union power_supply_propval *val) > +{ > + unsigned int src_mv, src_ma, max_src_mw = 0; > + unsigned int i, tmp; > + > + for (i = 0; i < port->nr_source_caps; i++) { > + u32 pdo = port->source_caps[i]; > + > + if (pdo_type(pdo) == PDO_TYPE_FIXED) { > + src_mv = pdo_fixed_voltage(pdo); > + src_ma = pdo_max_current(pdo); > + tmp = src_mv * src_ma / 1000; > + max_src_mw = tmp > max_src_mw ? tmp : max_src_mw; > + } > + } > + > + val->intval = max_src_mw; The power-supply subsystem expects Microwatts and not Milliwatts. -- Sebastian > + return 0; > +} > + > static int tcpm_psy_get_prop(struct power_supply *psy, > enum power_supply_property psp, > union power_supply_propval *val) > @@ -6349,6 +6370,9 @@ static int tcpm_psy_get_prop(struct power_supply *psy, > case POWER_SUPPLY_PROP_CURRENT_NOW: > ret = tcpm_psy_get_current_now(port, val); > break; > + case POWER_SUPPLY_PROP_INPUT_POWER_LIMIT: > + tcpm_psy_get_input_power_limit(port, val); > + break; > default: > ret = -EINVAL; > break; > -- > 2.17.1 >
Hi Sebastian, On 2023/3/21 4:31, Sebastian Reichel wrote: > Hi, > > On Mon, Mar 20, 2023 at 06:07:11PM +0800, Frank Wang wrote: >> Traverse fixed pdos to calculate the maximum power that the charger >> can provide, and it can be get by POWER_SUPPLY_PROP_INPUT_POWER_LIMIT >> property. >> >> Signed-off-by: Frank Wang <frank.wang@rock-chips.com> >> --- >> drivers/usb/typec/tcpm/tcpm.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >> index 13830b5e2d09f..d6ad3cdf9e4af 100644 >> --- a/drivers/usb/typec/tcpm/tcpm.c >> +++ b/drivers/usb/typec/tcpm/tcpm.c >> @@ -6320,6 +6320,27 @@ static int tcpm_psy_get_current_now(struct tcpm_port *port, >> return 0; >> } >> >> +static int tcpm_psy_get_input_power_limit(struct tcpm_port *port, >> + union power_supply_propval *val) >> +{ >> + unsigned int src_mv, src_ma, max_src_mw = 0; >> + unsigned int i, tmp; >> + >> + for (i = 0; i < port->nr_source_caps; i++) { >> + u32 pdo = port->source_caps[i]; >> + >> + if (pdo_type(pdo) == PDO_TYPE_FIXED) { >> + src_mv = pdo_fixed_voltage(pdo); >> + src_ma = pdo_max_current(pdo); >> + tmp = src_mv * src_ma / 1000; >> + max_src_mw = tmp > max_src_mw ? tmp : max_src_mw; >> + } >> + } >> + >> + val->intval = max_src_mw; > The power-supply subsystem expects Microwatts and not Milliwatts. Yes, but I see the 'power_supply_propval' member 'intval' is an integer type, I worry about it may be overflowed that uses Microwatts. BR. Frank > -- Sebastian > >> + return 0; >> +} >> + >> static int tcpm_psy_get_prop(struct power_supply *psy, >> enum power_supply_property psp, >> union power_supply_propval *val) >> @@ -6349,6 +6370,9 @@ static int tcpm_psy_get_prop(struct power_supply *psy, >> case POWER_SUPPLY_PROP_CURRENT_NOW: >> ret = tcpm_psy_get_current_now(port, val); >> break; >> + case POWER_SUPPLY_PROP_INPUT_POWER_LIMIT: >> + tcpm_psy_get_input_power_limit(port, val); >> + break; >> default: >> ret = -EINVAL; >> break; >> -- >> 2.17.1 >>
Hi, On Tue, Mar 21, 2023 at 09:32:53AM +0800, Frank Wang wrote: > On 2023/3/21 4:31, Sebastian Reichel wrote: > > On Mon, Mar 20, 2023 at 06:07:11PM +0800, Frank Wang wrote: > > > Traverse fixed pdos to calculate the maximum power that the charger > > > can provide, and it can be get by POWER_SUPPLY_PROP_INPUT_POWER_LIMIT > > > property. > > > > > > Signed-off-by: Frank Wang <frank.wang@rock-chips.com> > > > --- > > > drivers/usb/typec/tcpm/tcpm.c | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > index 13830b5e2d09f..d6ad3cdf9e4af 100644 > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > @@ -6320,6 +6320,27 @@ static int tcpm_psy_get_current_now(struct tcpm_port *port, > > > return 0; > > > } > > > +static int tcpm_psy_get_input_power_limit(struct tcpm_port *port, > > > + union power_supply_propval *val) > > > +{ > > > + unsigned int src_mv, src_ma, max_src_mw = 0; > > > + unsigned int i, tmp; > > > + > > > + for (i = 0; i < port->nr_source_caps; i++) { > > > + u32 pdo = port->source_caps[i]; > > > + > > > + if (pdo_type(pdo) == PDO_TYPE_FIXED) { > > > + src_mv = pdo_fixed_voltage(pdo); > > > + src_ma = pdo_max_current(pdo); > > > + tmp = src_mv * src_ma / 1000; > > > + max_src_mw = tmp > max_src_mw ? tmp : max_src_mw; > > > + } > > > + } > > > + > > > + val->intval = max_src_mw; > > The power-supply subsystem expects Microwatts and not Milliwatts. > > Yes, but I see the 'power_supply_propval' member 'intval' is an integer > type, I worry about it may be overflowed that uses Microwatts. Data being encoded in Microwatts is part of the ABI. The data you are supplying will be interpreted in µW. If you submit your data in mW it is basically always wrong even without an overflow. Now regarding the overflow: A signed int can store 2^31 bit, so 2,147,483,648 µW = 2147 W. Looking at your code you effectively calculate Microwatts in an unsigned int and then divide by 1000. Since the intermediate value (before dividing by 1000) needs to be stored you gain only one bit. That raises the question: Why do you expect data to be between 2147 W and 4294 W when the latest released USB PD spec allows 5A@48V = 240W? -- Sebastian
Hi Sebastian, On 2023/3/21 13:00, Sebastian Reichel wrote: > Hi, > > On Tue, Mar 21, 2023 at 09:32:53AM +0800, Frank Wang wrote: >> On 2023/3/21 4:31, Sebastian Reichel wrote: >>> On Mon, Mar 20, 2023 at 06:07:11PM +0800, Frank Wang wrote: >>>> Traverse fixed pdos to calculate the maximum power that the charger >>>> can provide, and it can be get by POWER_SUPPLY_PROP_INPUT_POWER_LIMIT >>>> property. >>>> >>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com> >>>> --- >>>> drivers/usb/typec/tcpm/tcpm.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >>>> index 13830b5e2d09f..d6ad3cdf9e4af 100644 >>>> --- a/drivers/usb/typec/tcpm/tcpm.c >>>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>>> @@ -6320,6 +6320,27 @@ static int tcpm_psy_get_current_now(struct tcpm_port *port, >>>> return 0; >>>> } >>>> +static int tcpm_psy_get_input_power_limit(struct tcpm_port *port, >>>> + union power_supply_propval *val) >>>> +{ >>>> + unsigned int src_mv, src_ma, max_src_mw = 0; >>>> + unsigned int i, tmp; >>>> + >>>> + for (i = 0; i < port->nr_source_caps; i++) { >>>> + u32 pdo = port->source_caps[i]; >>>> + >>>> + if (pdo_type(pdo) == PDO_TYPE_FIXED) { >>>> + src_mv = pdo_fixed_voltage(pdo); >>>> + src_ma = pdo_max_current(pdo); >>>> + tmp = src_mv * src_ma / 1000; >>>> + max_src_mw = tmp > max_src_mw ? tmp : max_src_mw; >>>> + } >>>> + } >>>> + >>>> + val->intval = max_src_mw; >>> The power-supply subsystem expects Microwatts and not Milliwatts. >> Yes, but I see the 'power_supply_propval' member 'intval' is an integer >> type, I worry about it may be overflowed that uses Microwatts. > Data being encoded in Microwatts is part of the ABI. The data > you are supplying will be interpreted in µW. If you submit your > data in mW it is basically always wrong even without an overflow. > > Now regarding the overflow: A signed int can store 2^31 bit, so > 2,147,483,648 µW = 2147 W. Looking at your code you effectively > calculate Microwatts in an unsigned int and then divide by 1000. > Since the intermediate value (before dividing by 1000) needs to be > stored you gain only one bit. That raises the question: Why do you > expect data to be between 2147 W and 4294 W when the latest released > USB PD spec allows 5A@48V = 240W? > > -- Sebastian Okay, got it, I shall delete conversion codes ( divide by 1000 ) in the next version. BR. Frank
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 13830b5e2d09f..d6ad3cdf9e4af 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -6320,6 +6320,27 @@ static int tcpm_psy_get_current_now(struct tcpm_port *port, return 0; } +static int tcpm_psy_get_input_power_limit(struct tcpm_port *port, + union power_supply_propval *val) +{ + unsigned int src_mv, src_ma, max_src_mw = 0; + unsigned int i, tmp; + + for (i = 0; i < port->nr_source_caps; i++) { + u32 pdo = port->source_caps[i]; + + if (pdo_type(pdo) == PDO_TYPE_FIXED) { + src_mv = pdo_fixed_voltage(pdo); + src_ma = pdo_max_current(pdo); + tmp = src_mv * src_ma / 1000; + max_src_mw = tmp > max_src_mw ? tmp : max_src_mw; + } + } + + val->intval = max_src_mw; + return 0; +} + static int tcpm_psy_get_prop(struct power_supply *psy, enum power_supply_property psp, union power_supply_propval *val) @@ -6349,6 +6370,9 @@ static int tcpm_psy_get_prop(struct power_supply *psy, case POWER_SUPPLY_PROP_CURRENT_NOW: ret = tcpm_psy_get_current_now(port, val); break; + case POWER_SUPPLY_PROP_INPUT_POWER_LIMIT: + tcpm_psy_get_input_power_limit(port, val); + break; default: ret = -EINVAL; break;
Traverse fixed pdos to calculate the maximum power that the charger can provide, and it can be get by POWER_SUPPLY_PROP_INPUT_POWER_LIMIT property. Signed-off-by: Frank Wang <frank.wang@rock-chips.com> --- drivers/usb/typec/tcpm/tcpm.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)