| Message ID | 20210121091005.74417-1-suyanjun218@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] can: mcp251xfd: replace sizeof(u32) with val_bytes in regmap | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On Thu, Jan 21, 2021 at 05:10:05PM +0800, Su Yanjun wrote: Please describe why you change this. > No functional effect. Not quite: scripts/bloat-o-meter shows: add/remove: 0/0 grow/shrink: 3/0 up/down: 104/0 (104) Function old new delta mcp251xfd_handle_tefif 980 1028 +48 mcp251xfd_irq 3716 3756 +40 mcp251xfd_handle_rxif_ring 964 980 +16 Total: Before=20832, After=20936, chg +0.50% > > Signed-off-by: Su Yanjun <suyanjun218@gmail.com> > --- > drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c > index f07e8b737d31..b15bfd50b863 100644 > --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c > +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c > @@ -181,6 +181,12 @@ static int mcp251xfd_clks_and_vdd_disable(const struct mcp251xfd_priv *priv) > return 0; > } > > +static inline int > +mcp251xfd_get_val_bytes(const struct mcp251xfd_priv *priv) > +{ > + return regmap_get_val_bytes(priv->map_reg); You're always using the "map_reg" here > +} > + > static inline u8 > mcp251xfd_cmd_prepare_write_reg(const struct mcp251xfd_priv *priv, > union mcp251xfd_write_reg_buf *write_reg_buf, > @@ -1308,6 +1314,7 @@ mcp251xfd_tef_obj_read(const struct mcp251xfd_priv *priv, > const u8 offset, const u8 len) > { > const struct mcp251xfd_tx_ring *tx_ring = priv->tx; > + int val_bytes = mcp251xfd_get_val_bytes(priv); > > if (IS_ENABLED(CONFIG_CAN_MCP251XFD_SANITY) && > (offset > tx_ring->obj_num || > @@ -1322,7 +1329,7 @@ mcp251xfd_tef_obj_read(const struct mcp251xfd_priv *priv, > return regmap_bulk_read(priv->map_rx, But this works on map_rx. > mcp251xfd_get_tef_obj_addr(offset), > hw_tef_obj, > - sizeof(*hw_tef_obj) / sizeof(u32) * len); > + sizeof(*hw_tef_obj) / val_bytes * len); > } > > static int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv) > @@ -1511,11 +1518,12 @@ mcp251xfd_rx_obj_read(const struct mcp251xfd_priv *priv, > const u8 offset, const u8 len) > { > int err; > + int val_bytes = mcp251xfd_get_val_bytes(priv); > > err = regmap_bulk_read(priv->map_rx, Same here > mcp251xfd_get_rx_obj_addr(ring, offset), > hw_rx_obj, > - len * ring->obj_size / sizeof(u32)); > + len * ring->obj_size / val_bytes); > > return err; > } > @@ -2139,6 +2147,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id) > struct mcp251xfd_priv *priv = dev_id; > irqreturn_t handled = IRQ_NONE; > int err; > + int val_bytes = mcp251xfd_get_val_bytes(priv); > > if (priv->rx_int) > do { > @@ -2162,7 +2171,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id) > err = regmap_bulk_read(priv->map_reg, MCP251XFD_REG_INT, Here it's map_reg. > &priv->regs_status, > sizeof(priv->regs_status) / > - sizeof(u32)); > + val_bytes); > if (err) > goto out_fail; > > -- > 2.25.1 Marc
The sizeof(u32) is hardcoded. IMO it's better to use the config value in regmap. Thanks 在 2021/1/21 下午5:21, Marc Kleine-Budde 写道: > On Thu, Jan 21, 2021 at 05:10:05PM +0800, Su Yanjun wrote: > > Please describe why you change this. > >> No functional effect. > Not quite: > > scripts/bloat-o-meter shows: > > add/remove: 0/0 grow/shrink: 3/0 up/down: 104/0 (104) > Function old new delta > mcp251xfd_handle_tefif 980 1028 +48 > mcp251xfd_irq 3716 3756 +40 > mcp251xfd_handle_rxif_ring 964 980 +16 > Total: Before=20832, After=20936, chg +0.50% > >> Signed-off-by: Su Yanjun <suyanjun218@gmail.com> >> --- >> drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c >> index f07e8b737d31..b15bfd50b863 100644 >> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c >> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c >> @@ -181,6 +181,12 @@ static int mcp251xfd_clks_and_vdd_disable(const struct mcp251xfd_priv *priv) >> return 0; >> } >> >> +static inline int >> +mcp251xfd_get_val_bytes(const struct mcp251xfd_priv *priv) >> +{ >> + return regmap_get_val_bytes(priv->map_reg); > You're always using the "map_reg" here > >> +} >> + >> static inline u8 >> mcp251xfd_cmd_prepare_write_reg(const struct mcp251xfd_priv *priv, >> union mcp251xfd_write_reg_buf *write_reg_buf, >> @@ -1308,6 +1314,7 @@ mcp251xfd_tef_obj_read(const struct mcp251xfd_priv *priv, >> const u8 offset, const u8 len) >> { >> const struct mcp251xfd_tx_ring *tx_ring = priv->tx; >> + int val_bytes = mcp251xfd_get_val_bytes(priv); >> >> if (IS_ENABLED(CONFIG_CAN_MCP251XFD_SANITY) && >> (offset > tx_ring->obj_num || >> @@ -1322,7 +1329,7 @@ mcp251xfd_tef_obj_read(const struct mcp251xfd_priv *priv, >> return regmap_bulk_read(priv->map_rx, > But this works on map_rx. > >> mcp251xfd_get_tef_obj_addr(offset), >> hw_tef_obj, >> - sizeof(*hw_tef_obj) / sizeof(u32) * len); >> + sizeof(*hw_tef_obj) / val_bytes * len); >> } >> >> static int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv) >> @@ -1511,11 +1518,12 @@ mcp251xfd_rx_obj_read(const struct mcp251xfd_priv *priv, >> const u8 offset, const u8 len) >> { >> int err; >> + int val_bytes = mcp251xfd_get_val_bytes(priv); >> >> err = regmap_bulk_read(priv->map_rx, > Same here > >> mcp251xfd_get_rx_obj_addr(ring, offset), >> hw_rx_obj, >> - len * ring->obj_size / sizeof(u32)); >> + len * ring->obj_size / val_bytes); >> >> return err; >> } >> @@ -2139,6 +2147,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id) >> struct mcp251xfd_priv *priv = dev_id; >> irqreturn_t handled = IRQ_NONE; >> int err; >> + int val_bytes = mcp251xfd_get_val_bytes(priv); >> >> if (priv->rx_int) >> do { >> @@ -2162,7 +2171,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id) >> err = regmap_bulk_read(priv->map_reg, MCP251XFD_REG_INT, > Here it's map_reg. > >> &priv->regs_status, >> sizeof(priv->regs_status) / >> - sizeof(u32)); >> + val_bytes); >> if (err) >> goto out_fail; >> >> -- >> 2.25.1 > Marc >
On Thu, Jan 21, 2021 at 05:33:40PM +0800, Su wrote: > The sizeof(u32) is hardcoded. IMO it's better to use the config value in > regmap. I got why you want to change this. Please update the patch description, comment on the increase of the object size and address the other issues I pointed out. I think it makes no sense to have the function mcp251xfd_get_val_bytes() as you have to use several regmaps anyways. > > > No functional effect. > > Not quite: > > > > scripts/bloat-o-meter shows: > > > > add/remove: 0/0 grow/shrink: 3/0 up/down: 104/0 (104) > > Function old new delta > > mcp251xfd_handle_tefif 980 1028 +48 > > mcp251xfd_irq 3716 3756 +40 > > mcp251xfd_handle_rxif_ring 964 980 +16 > > Total: Before=20832, After=20936, chg +0.50% Marc
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index f07e8b737d31..b15bfd50b863 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c @@ -181,6 +181,12 @@ static int mcp251xfd_clks_and_vdd_disable(const struct mcp251xfd_priv *priv) return 0; } +static inline int +mcp251xfd_get_val_bytes(const struct mcp251xfd_priv *priv) +{ + return regmap_get_val_bytes(priv->map_reg); +} + static inline u8 mcp251xfd_cmd_prepare_write_reg(const struct mcp251xfd_priv *priv, union mcp251xfd_write_reg_buf *write_reg_buf, @@ -1308,6 +1314,7 @@ mcp251xfd_tef_obj_read(const struct mcp251xfd_priv *priv, const u8 offset, const u8 len) { const struct mcp251xfd_tx_ring *tx_ring = priv->tx; + int val_bytes = mcp251xfd_get_val_bytes(priv); if (IS_ENABLED(CONFIG_CAN_MCP251XFD_SANITY) && (offset > tx_ring->obj_num || @@ -1322,7 +1329,7 @@ mcp251xfd_tef_obj_read(const struct mcp251xfd_priv *priv, return regmap_bulk_read(priv->map_rx, mcp251xfd_get_tef_obj_addr(offset), hw_tef_obj, - sizeof(*hw_tef_obj) / sizeof(u32) * len); + sizeof(*hw_tef_obj) / val_bytes * len); } static int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv) @@ -1511,11 +1518,12 @@ mcp251xfd_rx_obj_read(const struct mcp251xfd_priv *priv, const u8 offset, const u8 len) { int err; + int val_bytes = mcp251xfd_get_val_bytes(priv); err = regmap_bulk_read(priv->map_rx, mcp251xfd_get_rx_obj_addr(ring, offset), hw_rx_obj, - len * ring->obj_size / sizeof(u32)); + len * ring->obj_size / val_bytes); return err; } @@ -2139,6 +2147,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id) struct mcp251xfd_priv *priv = dev_id; irqreturn_t handled = IRQ_NONE; int err; + int val_bytes = mcp251xfd_get_val_bytes(priv); if (priv->rx_int) do { @@ -2162,7 +2171,7 @@ static irqreturn_t mcp251xfd_irq(int irq, void *dev_id) err = regmap_bulk_read(priv->map_reg, MCP251XFD_REG_INT, &priv->regs_status, sizeof(priv->regs_status) / - sizeof(u32)); + val_bytes); if (err) goto out_fail;
No functional effect. Signed-off-by: Su Yanjun <suyanjun218@gmail.com> --- drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)