| Message ID | 20240828131902.3632167-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: can: cc770: Simplify parsing DT properties | expand |
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@kernel.org> ... > 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. > + > + 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.
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@kernel.org> > > ... > > > 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
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; + + of_property_read_u32(np, "bosch,external-clock-frequency", &clkext); priv->can.clock.freq = clkext; /* The system clock may not exceed 10 MHz */ @@ -98,7 +91,7 @@ static int cc770_get_of_node_data(struct platform_device *pdev, if (of_property_read_bool(np, "bosch,iso-low-speed-mux")) priv->cpu_interface |= CPUIF_MUX; - if (!of_get_property(np, "bosch,no-comperator-bypass", NULL)) + if (!of_property_read_bool(np, "bosch,no-comperator-bypass")) priv->bus_config |= BUSCFG_CBY; if (of_property_read_bool(np, "bosch,disconnect-rx0-input")) priv->bus_config |= BUSCFG_DR0; @@ -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) -
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@kernel.org> --- drivers/net/can/cc770/cc770_platform.c | 29 ++++++++------------------ 1 file changed, 9 insertions(+), 20 deletions(-)