[net-next,v4,5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration
Commit Message
Explicitely zero-ize the local mdio_regmap_config data, and explicitely
set the .autoscan parameter, as we only have a PCS on this bus.
Fixes: 5d1f3fe7d2d5 ("net: stmmac: dwmac-sogfpga: use the lynx pcs driver")
Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V3->V4 : Move pcs_regmap_cfg into a more local block, and zeroize mrc
V2->V3 : New patch
.../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Comments
On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> struct mdio_regmap_config mrc;
> struct regmap *pcs_regmap;
> struct mii_bus *pcs_bus;
>
...
> + memset(&mrc, 0, sizeof(mrc));
...
> mrc.parent = &pdev->dev;
> mrc.valid_addr = 0x0;
> + mrc.autoscan = false;
Isn't this covered by the memset() ?
On Wed, Jun 07, 2023 at 04:54:09PM +0200, Maxime Chevallier wrote:
> On Wed, 7 Jun 2023 13:28:03 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>
> > On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> > > @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> > > struct mdio_regmap_config mrc;
> > > struct regmap *pcs_regmap;
> > > struct mii_bus *pcs_bus;
> > >
> > ...
> > > + memset(&mrc, 0, sizeof(mrc));
> > ...
> > > mrc.parent = &pdev->dev;
> > > mrc.valid_addr = 0x0;
> > > + mrc.autoscan = false;
> >
> > Isn't this covered by the memset() ?
>
> I have the same answer as for the above. It's redundant, but I don't
> think there's any harm having it set explicitely ?
No harm, just redundant. I don't think this is a good enough reason not
to merge it.
On Wed, 7 Jun 2023 13:28:03 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> > @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> > struct mdio_regmap_config mrc;
> > struct regmap *pcs_regmap;
> > struct mii_bus *pcs_bus;
> >
> ...
> > + memset(&mrc, 0, sizeof(mrc));
> ...
> > mrc.parent = &pdev->dev;
> > mrc.valid_addr = 0x0;
> > + mrc.autoscan = false;
>
> Isn't this covered by the memset() ?
I have the same answer as for the above. It's redundant, but I don't
think there's any harm having it set explicitely ?
Maxime
On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> Explicitely zero-ize the local mdio_regmap_config data, and explicitely
nit: Explicitely -> explicitly
@@ -389,7 +389,6 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
struct net_device *ndev;
struct stmmac_priv *stpriv;
const struct socfpga_dwmac_ops *ops;
- struct regmap_config pcs_regmap_cfg;
ops = device_get_match_data(&pdev->dev);
if (!ops) {
@@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
if (ret)
goto err_dvr_remove;
- memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
- pcs_regmap_cfg.reg_bits = 16;
- pcs_regmap_cfg.val_bits = 16;
- pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
-
/* Create a regmap for the PCS so that it can be used by the PCS driver,
* if we have such a PCS
*/
if (dwmac->tse_pcs_base) {
+ struct regmap_config pcs_regmap_cfg;
struct mdio_regmap_config mrc;
struct regmap *pcs_regmap;
struct mii_bus *pcs_bus;
+ memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
+ memset(&mrc, 0, sizeof(mrc));
+
+ pcs_regmap_cfg.reg_bits = 16;
+ pcs_regmap_cfg.val_bits = 16;
+ pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
+
pcs_regmap = devm_regmap_init_mmio(&pdev->dev, dwmac->tse_pcs_base,
&pcs_regmap_cfg);
if (IS_ERR(pcs_regmap)) {
@@ -470,6 +472,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
mrc.regmap = pcs_regmap;
mrc.parent = &pdev->dev;
mrc.valid_addr = 0x0;
+ mrc.autoscan = false;
snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);