Message-ID: <CAJq09z5TE_VSGmyWdNfb+o7ymg2qsfGhjky-AXY+Pv5_0V7RLw@mail.gmail.com>
Date: Sun, 28 Jan 2024 21:09:38 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: netdev@...r.kernel.org, linus.walleij@...aro.org, alsi@...g-olufsen.dk, 
	andrew@...n.ch, f.fainelli@...il.com, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	arinc.unal@...nc9.com, ansuelsmth@...il.com
Subject: Re: [PATCH net-next v4 05/11] net: dsa: realtek: common rtl83xx module

> On Tue, Jan 23, 2024 at 06:55:57PM -0300, Luiz Angelo Daros de Luca wrote:
> > Some code can be shared between both interface modules (MDIO and SMI)
> > and among variants. These interface functions migrated to a common
> > module:
> >
> > - rtl83xx_lock
> > - rtl83xx_unlock
> > - rtl83xx_probe
> > - rtl83xx_register_switch
>
> For API uniformity, I would also introduce rtl83xx_unregister_switch()
> to make it clear that there is a teardown step associated with this.
>
> > - rtl83xx_remove
>
> No rtl83xx_shutdown()?

I was avoiding one-line functions. However, I'll add those functions
for API uniformity. In the end, it will naturally untangle some other
problems you mentioned in other patches.

> You could centralize the comments from the mdio and smi interfaces into
> the rtl83xx common layer.

I'll use a more generic kdoc for each interface and a detailed one in common.

>
> >
> > The reset during probe was moved to the end of the common probe. This way,
> > we avoid a reset if anything else fails.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> > ---
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 57bd5d8814c2..26b8371ecc87 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -303,3 +194,5 @@ EXPORT_SYMBOL_NS_GPL(realtek_mdio_shutdown, REALTEK_DSA);
> >  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> >  MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
> >  MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(REALTEK_DSA);
> > +
>
> Stray blank line at the end of the file.

OK. I'll remove all of them.

> > diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> > new file mode 100644
> > index 000000000000..57d185226b03
> > --- /dev/null
> > +++ b/drivers/net/dsa/realtek/rtl83xx.c
> > @@ -0,0 +1,201 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <linux/module.h>
> > +
> > +#include "realtek.h"
> > +#include "rtl83xx.h"
> > +
> > +/**
> > + * rtl83xx_lock() - Locks the mutex used by regmaps
> > + * @ctx: realtek_priv pointer
> > + *
> > + * This function is passed to regmap to be used as the lock function.
> > + * It is also used externally to block regmap before executing multiple
> > + * operations that must happen in sequence (which will use
> > + * realtek_priv.map_nolock instead).
> > + *
> > + * Context: Any context. Holds priv->map_lock lock.
>
> Not "any context", but "sleepable context". You cannot acquire a mutex
> in atomic context. Actually this applies across the board in this patch
> set. The entry points into DSA also take mutex_lock(&dsa2_mutex), so
> this also applies to its callers' context.

Yes. I'll update all kdocs that might use a lock. I guess just the
unlock that is actually "any context".
I believe it will be much easier to write a kdoc when all called
symbols also have a kdoc with a Context: field. It would avoid all
kdoc writers to dig down the stack with every called function. It's a
good move to require kdoc for all exported symbols.

> > + * Return: nothing
> > + */
> > +void rtl83xx_lock(void *ctx)
> > +{
> > +     struct realtek_priv *priv = ctx;
> > +
> > +     mutex_lock(&priv->map_lock);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(rtl83xx_lock, REALTEK_DSA);
> > +
> > +/**
> > + * rtl83xx_unlock() - Unlocks the mutex used by regmaps
> > + * @ctx: realtek_priv pointer
> > + *
> > + * This function unlocks the lock acquired by rtl83xx_lock.
> > + *
> > + * Context: Any context. Releases priv->map_lock lock
> > + * Return: nothing
> > + */
> > +void rtl83xx_unlock(void *ctx)
> > +{
> > +     struct realtek_priv *priv = ctx;
> > +
> > +     mutex_unlock(&priv->map_lock);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
> > +
> > +/**
> > + * rtl83xx_probe() - probe a Realtek switch
> > + * @dev: the device being probed
> > + * @interface_info: reg read/write methods for a specific management interface.
>
> Leave this description more open-ended, otherwise it will have to be
> modified when struct realtek_interface_info gains more fields.

OK, it makes sense. However I'm not sure management interfaces might
differ in anything but the reg read/write.

>
> > + *
> > + * This function initializes realtek_priv and read data from the device-tree
>
> s/read/reads/
> s/device-tree/device tree/

OK

> > + * node. The switch is hard resetted if a method is provided.
> > + *
> > + * Context: Any context.
> > + * Return: Pointer to the realtek_priv or ERR_PTR() in case of failure.
> > + *
> > + * The realtek_priv pointer does not need to be freed as it is controlled by
> > + * devres.
> > + *
> > + */
> > +struct realtek_priv *
> > +rtl83xx_probe(struct device *dev,
> > +           const struct realtek_interface_info *interface_info)
> > +{
> > +     const struct realtek_variant *var;
> > +     struct realtek_priv *priv;
> > +     struct regmap_config rc = {
> > +                     .reg_bits = 10, /* A4..A0 R4..R0 */
> > +                     .val_bits = 16,
> > +                     .reg_stride = 1,
> > +                     .max_register = 0xffff,
> > +                     .reg_format_endian = REGMAP_ENDIAN_BIG,
> > +                     .reg_read = interface_info->reg_read,
> > +                     .reg_write = interface_info->reg_write,
> > +                     .cache_type = REGCACHE_NONE,
> > +                     .lock = rtl83xx_lock,
> > +                     .unlock = rtl83xx_unlock,
>
> Too many levels of indentation.

OK

> > +/**
> > + * rtl83xx_remove() - Cleanup a realtek switch driver
> > + * @ctx: realtek_priv pointer
>
> s/ctx/priv/

I was missing some kernel warnings with too much debug output (V=s1c).

> > --- /dev/null
> > +++ b/drivers/net/dsa/realtek/rtl83xx.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +
> > +#ifndef _RTL83XX_H
> > +#define _RTL83XX_H
> > +
> > +#include <linux/regmap.h>
>
> I don't think anything from this header needs regmap.h?
> For testing what is needed, you can create a dummy.c file which includes
> only rtl83xx.h, and add just the necessary headers so that dummy.c
> builds cleanly.
>

It is a leftover from the time probe was receiving a regmap_config. It
is, indeed, missing in rtl83xx.c.

Regards,

Luiz

Powered by blists - more mailing lists