| Message ID | 20210131151832.215931-4-kyletso@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | common SVDM version and VDO from dt | expand |
On 1/31/21 7:18 AM, Kyle Tso wrote: > Commit a079973f462a ("usb: typec: tcpm: Remove tcpc_config > configuration mechanism") removed the tcpc_config which includes the > Sink VDO and it is not yet added back with fwnode. Add it now. > > Signed-off-by: Kyle Tso <kyletso@google.com> > --- > Changes since v1: > - updated the commit message > > drivers/usb/typec/tcpm/tcpm.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 403a483645dd..84c8a52f8af1 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -5677,6 +5677,18 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, > port->new_source_frs_current = frs_current; > } > > + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", NULL, 0); fwnode_property_count_u32(), maybe ? > + if (ret <= 0 && ret != -EINVAL) { > + return -EINVAL; Why return any error except -EINVAL (including return values of 0) as -EINVAL, and -EINVAL as no error ? > + } else if (ret > 0) { > + port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS); > + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", > + port->snk_vdo, > + port->nr_snk_vdo); > + if (ret < 0) > + return -EINVAL; static analyzer code used to complain about overriding error codes. Not sure if that is still true. Either case, why not return the original error ? Thanks, Guenter > + } > + > return 0; > } > >
On Mon, Feb 1, 2021 at 12:02 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 1/31/21 7:18 AM, Kyle Tso wrote: > > Commit a079973f462a ("usb: typec: tcpm: Remove tcpc_config > > configuration mechanism") removed the tcpc_config which includes the > > Sink VDO and it is not yet added back with fwnode. Add it now. > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > --- > > Changes since v1: > > - updated the commit message > > > > drivers/usb/typec/tcpm/tcpm.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 403a483645dd..84c8a52f8af1 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -5677,6 +5677,18 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, > > port->new_source_frs_current = frs_current; > > } > > > > + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", NULL, 0); > > fwnode_property_count_u32(), maybe ? > That's the same and looks like fwnode_property_count_u32 is better to read. I will revise it in the next version. > > + if (ret <= 0 && ret != -EINVAL) { > > + return -EINVAL; > > Why return any error except -EINVAL (including return values of 0) as -EINVAL, > and -EINVAL as no error ? > sink-vdos is not a mandatory property which means -EINVAL is acceptable. If the return < 0 and the value is not -EINVAL, it means that the error is other than "not present" in the device tree. If the return == 0, it means that the sink-vdos is present in the device tree but no value inside it. Both of the above situations are not acceptable. > > + } else if (ret > 0) { > > + port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS); > > + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", > > + port->snk_vdo, > > + port->nr_snk_vdo); > > + if (ret < 0) > > + return -EINVAL; > > static analyzer code used to complain about overriding error codes. > Not sure if that is still true. Either case, why not return the > original error ? > Returning the original error codes is good. I just followed the return value of other error handling in this function. will revise it in the next version. Thanks, Kyle > Thanks, > Guenter > > > + } > > + > > return 0; > > } > > > > >
On 1/31/21 9:11 PM, Kyle Tso wrote: > On Mon, Feb 1, 2021 at 12:02 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 1/31/21 7:18 AM, Kyle Tso wrote: >>> Commit a079973f462a ("usb: typec: tcpm: Remove tcpc_config >>> configuration mechanism") removed the tcpc_config which includes the >>> Sink VDO and it is not yet added back with fwnode. Add it now. >>> >>> Signed-off-by: Kyle Tso <kyletso@google.com> >>> --- >>> Changes since v1: >>> - updated the commit message >>> >>> drivers/usb/typec/tcpm/tcpm.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >>> index 403a483645dd..84c8a52f8af1 100644 >>> --- a/drivers/usb/typec/tcpm/tcpm.c >>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>> @@ -5677,6 +5677,18 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, >>> port->new_source_frs_current = frs_current; >>> } >>> >>> + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", NULL, 0); >> >> fwnode_property_count_u32(), maybe ? >> > That's the same and looks like fwnode_property_count_u32 is better to read. > I will revise it in the next version. > >>> + if (ret <= 0 && ret != -EINVAL) { >>> + return -EINVAL; >> >> Why return any error except -EINVAL (including return values of 0) as -EINVAL, >> and -EINVAL as no error ? >> > sink-vdos is not a mandatory property which means -EINVAL is acceptable. > > If the return < 0 and the value is not -EINVAL, it means that the > error is other than "not present" in the device tree. > If the return == 0, it means that the sink-vdos is present in the > device tree but no value inside it. I think that should return -ENODATA. Not sure if/when it would actually return 0. > Both of the above situations are not acceptable. > Personally I would prefer that a bit more explicit in the code, ie handle errors first and drop the else statement below. But maybe that is just me. >>> + } else if (ret > 0) { >>> + port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS); >>> + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", >>> + port->snk_vdo, >>> + port->nr_snk_vdo); >>> + if (ret < 0) >>> + return -EINVAL; >> >> static analyzer code used to complain about overriding error codes. >> Not sure if that is still true. Either case, why not return the >> original error ? >> > Returning the original error codes is good. I just followed the return > value of other error handling in this function. > will revise it in the next version. > Never mind, with the rest of the code being similar I guess that either static analyzers gave up complaining or they already have a field day anyway. Thanks, Guenter > Thanks, > Kyle > > > >> Thanks, >> Guenter >> >>> + } >>> + >>> return 0; >>> } >>> >>> >>
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 403a483645dd..84c8a52f8af1 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -5677,6 +5677,18 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, port->new_source_frs_current = frs_current; } + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", NULL, 0); + if (ret <= 0 && ret != -EINVAL) { + return -EINVAL; + } else if (ret > 0) { + port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS); + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", + port->snk_vdo, + port->nr_snk_vdo); + if (ret < 0) + return -EINVAL; + } + return 0; }
Commit a079973f462a ("usb: typec: tcpm: Remove tcpc_config configuration mechanism") removed the tcpc_config which includes the Sink VDO and it is not yet added back with fwnode. Add it now. Signed-off-by: Kyle Tso <kyletso@google.com> --- Changes since v1: - updated the commit message drivers/usb/typec/tcpm/tcpm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)