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

[Sean Nyekjaer]

Hi Vincent,

On Thu, Nov 14, 2024 at 01:53:34PM +0100, Vincent Mailhol wrote:
> On Mon. 11 Nov. 2024 at 17:55, Sean Nyekjaer <sean@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  drivers/net/can/m_can/tcan4x5x-core.c | 35 +++++++++++++++++++++++++++++++++++
> >  drivers/net/can/m_can/tcan4x5x.h      |  2 ++
> >  2 files changed, 37 insertions(+)
> >

[...]

> >
> > +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.

Thanks for the review.
You are right, so I reworked this for v3:
https://lore.kernel.org/r/20241112-tcan-wkrqv-v3-0-c66423fba26d@xxxxxxxxxx

> 
> > +       return 0;
> > +}
> > +

[...]

> >
> >

/Sean