On 28.08.2024 17:16:24, Simon Horman wrote:
> On Wed, Aug 28, 2024 at 08:19:02AM -0500, Rob Herring (Arm) wrote:
> > Use of the typed property accessors is preferred over of_get_property().
> > The existing code doesn't work on little endian systems either. Replace
> > the of_get_property() calls with of_property_read_bool() and
> > of_property_read_u32().
> >
> > Signed-off-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
>
> ...
>
> > diff --git a/drivers/net/can/cc770/cc770_platform.c b/drivers/net/can/cc770/cc770_platform.c
> > index 13bcfba05f18..9993568154f8 100644
> > --- a/drivers/net/can/cc770/cc770_platform.c
> > +++ b/drivers/net/can/cc770/cc770_platform.c
> > @@ -71,16 +71,9 @@ static int cc770_get_of_node_data(struct platform_device *pdev,
> > struct cc770_priv *priv)
> > {
> > struct device_node *np = pdev->dev.of_node;
> > - const u32 *prop;
> > - int prop_size;
> > - u32 clkext;
> > -
> > - prop = of_get_property(np, "bosch,external-clock-frequency",
> > - &prop_size);
> > - if (prop && (prop_size == sizeof(u32)))
> > - clkext = *prop;
> > - else
> > - clkext = CC770_PLATFORM_CAN_CLOCK; /* default */
> > + u32 clkext = CC770_PLATFORM_CAN_CLOCK, clkout = 0;
>
> Marc,
>
> Could you clarify if reverse xmas tree ordering - longest line to shortest
> - of local variables is desired for can code? If so, I'm flagging that the
> above now doesn't follow that scheme.
If you touch the code, and noting speaks against it, please make it
reverse xmas.
>
> > +
> > + of_property_read_u32(np, "bosch,external-clock-frequency", &clkext);
> > priv->can.clock.freq = clkext;
> >
> > /* The system clock may not exceed 10 MHz */
>
> ...
>
> > @@ -109,20 +102,16 @@ static int cc770_get_of_node_data(struct platform_device *pdev,
> > if (of_property_read_bool(np, "bosch,polarity-dominant"))
> > priv->bus_config |= BUSCFG_POL;
> >
> > - prop = of_get_property(np, "bosch,clock-out-frequency", &prop_size);
> > - if (prop && (prop_size == sizeof(u32)) && *prop > 0) {
> > - u32 cdv = clkext / *prop;
> > - int slew;
> > + of_property_read_u32(np, "bosch,clock-out-frequency", &clkout);
> > + if (clkout > 0) {
> > + u32 cdv = clkext / clkout;
> > + u32 slew;
> >
> > if (cdv > 0 && cdv < 16) {
> > priv->cpu_interface |= CPUIF_CEN;
> > priv->clkout |= (cdv - 1) & CLKOUT_CD_MASK;
> >
> > - prop = of_get_property(np, "bosch,slew-rate",
> > - &prop_size);
> > - if (prop && (prop_size == sizeof(u32))) {
> > - slew = *prop;
> > - } else {
> > + if (of_property_read_u32(np, "bosch,slew-rate", &slew)) {
> > /* Determine default slew rate */
> > slew = (CLKOUT_SL_MASK >>
> > CLKOUT_SL_SHIFT) -
>
> Rob,
>
> The next few lines look like this:
>
> ((cdv * clkext - 1) / 8000000);
> if (slew < 0)
> slew = 0;
>
> But slew is now unsigned, so this check will always be false.
>
> Flagged by Smatch and Coccinelle.
Good finding.
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
