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
