linux-kernel - Re: [PATCH v2 1/2] can: tcan4x5x: add option for selecting nWKRQ voltage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMZ6Rq++_yecNY-nNL7NK48ZsNPqH0KDRuqvCCGhUur24+7KGA@mail.gmail.com>
Date: Thu, 14 Nov 2024 13:53:34 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Sean Nyekjaer <sean@...nix.com>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/2] can: tcan4x5x: add option for selecting nWKRQ voltage

On Mon. 11 Nov. 2024 at 17:55, Sean Nyekjaer <sean@...nix.com> wrote:
> nWKRQ supports an output voltage of either the internal reference voltage
> (3.6V) or the reference voltage of the digital interface 0 - 6V.
> Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> between them.
> Default is kept as the internal reference voltage.
>
> Signed-off-by: Sean Nyekjaer <sean@...nix.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/net/can/m_can/tcan4x5x.h      |  2 ++
>  2 files changed, 37 insertions(+)
>
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 2f73bf3abad889c222f15c39a3d43de1a1cf5fbb..264bba830be50033347056da994102f8b614e51b 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -92,6 +92,8 @@
>  #define TCAN4X5X_MODE_STANDBY BIT(6)
>  #define TCAN4X5X_MODE_NORMAL BIT(7)
>
> +#define TCAN4X5X_NWKRQ_VOLTAGE_MASK BIT(19)
> +
>  #define TCAN4X5X_DISABLE_WAKE_MSK      (BIT(31) | BIT(30))
>  #define TCAN4X5X_DISABLE_INH_MSK       BIT(9)
>
> @@ -267,6 +269,11 @@ static int tcan4x5x_init(struct m_can_classdev *cdev)
>         if (ret)
>                 return ret;
>
> +       ret = regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
> +                                TCAN4X5X_NWKRQ_VOLTAGE_MASK, tcan4x5x->nwkrq_voltage);
> +       if (ret)
> +               return ret;
> +
>         return ret;
>  }
>
> @@ -318,6 +325,28 @@ static const struct tcan4x5x_version_info
>         return &tcan4x5x_versions[TCAN4X5X];
>  }
>
> +static int tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
> +{
> +       struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> +       struct device_node *np = cdev->dev->of_node;
> +       u8 prop;
> +       int ret;
> +
> +       ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
> +       if (!ret) {
> +               if (prop <= 1)
> +                       tcan4x5x->nwkrq_voltage = prop;
> +               else
> +                       dev_warn(cdev->dev,
> +                                "nwkrq-voltage-sel have invalid option: %u\n",
> +                                prop);
> +       } else {
> +               tcan4x5x->nwkrq_voltage = 0;
> +       }

If the

  if (prop <= 1)

condition fails, you print a warning, but you are not assigning a
value to tcan4x5x->nwkrq_voltage. Is this intentional?

What about:

        tcan4x5x->nwkrq_voltage = 0;
        ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
        if (!ret) {
                if (prop <= 1)
                        tcan4x5x->nwkrq_voltage = prop;
                else
                        dev_warn(cdev->dev,
                                 "nwkrq-voltage-sel have invalid option: %u\n",
                                 prop);
        }

so that you make sure that tcan4x5x->nwkrq_voltage always gets a
default zero value? Else, if you can make sure that tcan4x5x is always
zero initialized, you can just drop the

        tcan4x5x->nwkrq_voltage = 0;

thing.

> +       return 0;
> +}
> +
>  static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
>                               const struct tcan4x5x_version_info *version_info)
>  {
> @@ -453,6 +482,12 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>                 goto out_power;
>         }
>
> +       ret = tcan4x5x_get_dt_data(mcan_class);
> +       if (ret) {
> +               dev_err(&spi->dev, "Getting dt data failed %pe\n", ERR_PTR(ret));
> +               goto out_power;
> +       }
> +
>         tcan4x5x_check_wake(priv);
>
>         ret = tcan4x5x_write_tcan_reg(mcan_class, TCAN4X5X_INT_EN, 0);
> diff --git a/drivers/net/can/m_can/tcan4x5x.h b/drivers/net/can/m_can/tcan4x5x.h
> index e62c030d3e1e5a713c997e7c8ecad4a44aff4e6a..04ebe5c64f4f7056a62e72e717cb85dd3817ab9c 100644
> --- a/drivers/net/can/m_can/tcan4x5x.h
> +++ b/drivers/net/can/m_can/tcan4x5x.h
> @@ -42,6 +42,8 @@ struct tcan4x5x_priv {
>
>         struct tcan4x5x_map_buf map_buf_rx;
>         struct tcan4x5x_map_buf map_buf_tx;
> +
> +       u8 nwkrq_voltage;
>  };
>
>  static inline void
>
> --
> 2.46.2
>
>