[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kernel-janitors
Subject:    AW: [PATCH 2/2] usb: typec: stusb160x: fix some signedness bugs
From:       Walter Harms <wharms () bfs ! de>
Date:       2020-10-29 8:21:57
Message-ID: f843e2866d6f421d83b4455582809a95 () bfs ! de
[Download RAW message or body]

this is much better to read
________________________________________
Von: Amelie DELAUNAY [amelie.delaunay@st.com]
Gesendet: Mittwoch, 28. Oktober 2020 13:26
An: Dan Carpenter; Heikki Krogerus
Cc: Greg Kroah-Hartman; linux-usb@vger.kernel.org; kernel-janitors@vger.kernel.org
Betreff: Re: [PATCH 2/2] usb: typec: stusb160x: fix some signedness bugs

Hi Dan,

On 10/23/20 1:24 PM, Dan Carpenter wrote:
> These variables are enums but in this situation GCC will treat them as
> unsigned so the conditions are never true.
>
> Fixes: da0cb6310094 ("usb: typec: add support for STUSB160x Type-C controller family")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/usb/typec/stusb160x.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/typec/stusb160x.c b/drivers/usb/typec/stusb160x.c
> index f7369e371dd4..da7f1957bcb3 100644
> --- a/drivers/usb/typec/stusb160x.c
> +++ b/drivers/usb/typec/stusb160x.c
> @@ -545,7 +545,7 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
>       ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
>       if (!ret) {
>               chip->port_type = typec_find_port_power_role(cap_str);
> -             if (chip->port_type < 0) {
> +             if ((int)chip->port_type < 0) {
>                       ret = chip->port_type;
>                       return ret;
>               }

I was preparing a patch for this one, and it uses the ret instead of the
cast:
        ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
        if (!ret) {
                ret = typec_find_port_power_role(cap_str);
                if (ret < 0)
                        return ret;
                chip->port_type = ret;

        }

> @@ -567,9 +567,10 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
>       if (!ret) {
>               chip->pwr_opmode = typec_find_pwr_opmode(cap_str);
>               /* Power delivery not yet supported */
> -             if (chip->pwr_opmode < 0 ||
> +             if ((int)chip->pwr_opmode < 0 ||
>                   chip->pwr_opmode == TYPEC_PWR_MODE_PD) {
> -                     ret = chip->pwr_opmode < 0 ? chip->pwr_opmode : -EINVAL;
> +                     ret = (int)chip->pwr_opmode < 0 ? chip->pwr_opmode :
> +                                                       -EINVAL;
>                       dev_err(chip->dev, "bad power operation mode: %d\n",
>                               chip->pwr_opmode);
>                       return ret;
>

        if (!ret) {
                ret = typec_find_pwr_opmode(cap_str);
                /* Power delivery not yet supported */
                if (ret < 0 || ret == TYPEC_PWR_MODE_PD) {
                        dev_err(chip->dev, "bad power operation mode: %d\n", ret);
                        return -EINVAL;
                }
                chip->pwr_opmode = ret;
        }


So, which fix sounds better ? IMHO using ret make the code more readable.

Regards,
Amelie
[prev in list] [next in list] [prev in thread] [next in thread]