Re: [PATCH net-next 07/11] net: stmmac: platform: provide stmmac_pltfr_remove_no_dt()

[Bartosz Golaszewski]

On Thu, Jun 22, 2023 at 2:15 PM Simon Horman <simon.horman@xxxxxxxxxxxx> wrote:
>
> On Wed, Jun 21, 2023 at 05:36:46PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > Add a variant of stmmac_pltfr_remove() that only frees resources
> > allocated by stmmac_pltfr_probe() and - unlike stmmac_pltfr_remove() -
> > does not call stmmac_remove_config_dt().
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---
> >  .../ethernet/stmicro/stmmac/stmmac_platform.c | 20 +++++++++++++++++--
> >  .../ethernet/stmicro/stmmac/stmmac_platform.h |  1 +
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
>
> Hi Bartosz,
>
> some minor feedback from my side as it looks like there will be a v2 anyway.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index df417cdab8c1..58d5c5cc2269 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -762,6 +762,23 @@ int stmmac_pltfr_probe(struct platform_device *pdev,
> >  }
> >  EXPORT_SYMBOL_GPL(stmmac_pltfr_probe);
> >
> > +/**
> > + * stmmac_pltfr_remove_no_dt
> > + * @pdev: pointer to the platform device
> > + * Description: This undoes the effects of stmmac_pltfr_probe() by removing the
> > + * driver and calling the platform's exit() callback.
> > + */
> > +void stmmac_pltfr_remove_no_dt(struct platform_device *pdev)
> > +{
> > +     struct net_device *ndev = platform_get_drvdata(pdev);
> > +     struct stmmac_priv *priv = netdev_priv(ndev);
> > +     struct plat_stmmacenet_data *plat = priv->plat;
>
> nit: please use reverse xmas tree - longest line to shortest - for
>      new Networking code.
>
>      e.g.:
>
>         struct net_device *ndev = platform_get_drvdata(pdev);
>         struct stmmac_priv *priv = netdev_priv(ndev);
>         struct plat_stmmacenet_data *plat = plat;
>
>         plat = priv->plat;
>

I normally stick to this convention but here, you need 5 lines for the
same effect and you make it more confusing by initializing some of the
variables at their declaration and some not. In other places in this
driver the same approach is used i.e. not adhering to reverse xmas
tree when all variables are initialized when declared.

Bart

> > +
> > +     stmmac_dvr_remove(&pdev->dev);
> > +     stmmac_pltfr_exit(pdev, plat);
> > +}
> > +EXPORT_SYMBOL_GPL(stmmac_pltfr_remove_no_dt);
> > +
> >  /**
> >   * stmmac_pltfr_remove
> >   * @pdev: platform device pointer
>
> ...