Date:   Mon, 9 May 2022 16:15:00 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
        netdev@...r.kernel.org, Terry Bowman <terry.bowman@....com>,
        Wolfram Sang <wsa@...nel.org>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Russell King <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>, UNGLinuxDriver@...rochip.com,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Lee Jones <lee.jones@...aro.org>
Subject: Re: [RFC v8 net-next 08/16] mfd: ocelot: add support for the vsc7512
 chip via spi

Hi Andy,

Thanks for all the feedback (on this and the other patches). They all
seem straightforward for me to implement. 

On Mon, May 09, 2022 at 11:02:42AM +0200, Andy Shevchenko wrote:
> On Sun, May 8, 2022 at 8:53 PM Colin Foster
> <colin.foster@...advantage.com> wrote:
> >
> > The VSC7512 is a networking chip that contains several peripherals. Many of
> > these peripherals are currently supported by the VSC7513 and VSC7514 chips,
> > but those run on an internal CPU. The VSC7512 lacks this CPU, and must be
> > controlled externally.
> >
> > Utilize the existing drivers by referencing the chip as an MFD. Add support
> > for the two MDIO buses, the internal phys, pinctrl, and serial GPIO.
> 
> ...
> 
> > +         If unsure, say N
> 
> Seems like an abrupt sentence.

It seems to match a common theme in Kconfigs (1149 hits)... although
I notice they all have a '.' at the end, which I'll add.

> 
> ...
> 
> > +EXPORT_SYMBOL(ocelot_chip_reset);
> 
> Please, switch to the namespace (_NS) variant of the exported-imported
> symbols for these drivers.
> 
> ...
> 
> > +int ocelot_core_init(struct device *dev)
> > +{
> > +       int ret;
> > +
> > +       ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> > +                                  ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> 
> Isn't it simple
> 
>   return devm_mfd_add_devices(...);
> 
> ?
> 
> > +}
> 
> ...
> 
> > +#include <linux/of.h>
> 
> Do you really use this? (See also below).
> 
> ...
> 
> > +#define VSC7512_CPUORG_RES_START       0x71000000
> > +#define VSC7512_CPUORG_RES_SIZE                0x2ff
> 
> Doesn't look right.
> I'm expecting to see 0x300 here and -1 where it's needed in the code.

I see what you're saying. I can do that. Also, this number is larger
than it needs to be - the max defined register in this block is 0x34.
Thanks for pointing this out!

> 
> ...
> 
> > +static const struct regmap_config ocelot_spi_regmap_config = {
> > +       .reg_bits = 24,
> > +       .reg_stride = 4,
> > +       .reg_downshift = 2,
> > +       .val_bits = 32,
> > +
> > +       .write_flag_mask = 0x80,
> 
> > +       .max_register = 0xffffffff,
> 
> Is it for real?! Have you considered what happens if someone actually
> tries to read all these registers (and for the record it's not a
> theoretical case, since the user may do it via debugfs)?

You had me worried for a second there. This is a useless assignment,
since the max_register gets calculated when the regmap is actually
initialized, based on the struct resoruce. I'll remove this.

> 
> > +       .use_single_write = true,
> > +       .can_multi_write = false,
> > +
> > +       .reg_format_endian = REGMAP_ENDIAN_BIG,
> > +       .val_format_endian = REGMAP_ENDIAN_NATIVE,
> > +};
> 
> ...
> 
> > +       if (ddata->spi_padding_bytes > 0) {
> 
> ' > 0' part is redundant.
> 
> > +               memset(&padding, 0, sizeof(padding));
> > +
> > +               padding.len = ddata->spi_padding_bytes;
> > +               padding.tx_buf = dummy_buf;
> > +               padding.dummy_data = 1;
> > +
> > +               spi_message_add_tail(&padding, &msg);
> > +       }
> 
> ...
> 
> > +       memcpy(&regmap_config, &ocelot_spi_regmap_config,
> > +              sizeof(ocelot_spi_regmap_config));
> 
> sizeof(regmap_config) is a bit safer (in case somebody changes the
> types of the src and dst).
> 
> ...
> 
> > +       err = spi_setup(spi);
> > +       if (err < 0) {
> > +               dev_err(&spi->dev, "Error %d initializing SPI\n", err);
> > +               return err;
> 
> return dev_err_probe(...);
> 
> > +       }
> ...
> 
> > +       ddata->cpuorg_regmap =
> > +               ocelot_spi_devm_init_regmap(dev, dev,
> > +                                           &vsc7512_dev_cpuorg_resource);
> 
> It's easier to read when it's a longer line. At least the last two can
> be on one line.
> 
> ...
> 
> > +       ddata->gcb_regmap = ocelot_spi_devm_init_regmap(dev, dev,
> > +                                                       &vsc7512_gcb_resource);
> 
> Do you have different cases for two first parameters? If not, drop duplication.

Yes. The thought here is the first "dev" is everything needed to
communicate with the chip. SPI bus, frequency, padding, etc.

The second "dev" is child device, to which the regmap gets
devm-attached. That should allow modules of the child devices to be
loaded / unloaded.

> 
> ...
> 
> > +       if (err) {
> > +               dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
> > +               return err;
> 
> return dev_err_probe(...);
> 
> And everywhere else where it's appropriate.
> 
> > +       }
> 
> ...
> 
> > +const struct of_device_id ocelot_spi_of_match[] = {
> > +       { .compatible = "mscc,vsc7512_mfd_spi" },
> > +       { },
> 
> No comma for terminator entry.
> 
> > +};
> 
> ...
> 
> > +               .of_match_table = of_match_ptr(ocelot_spi_of_match),
> 
> of_match_ptr() is rather harmful than useful. We have a lot of
> compiler warnings due to misuse of this. Besides that it prevents to
> use driver in non-OF environments (if it is / will be the case).

I used drivers/mfd/madera* as my template, since it seemed the closest
to what I was trying to achieve. Are you saying just to omit the
of_match_ptr wrapper (like in drivers/mfd/sprd-sc27xx-spi.c?)

> 
> ...
> 
> > +/*
> > + * Copyright 2021 Innovative Advantage Inc.
> > + */
> 
> One line.
> 
> ...
> 
> > +#include <linux/regmap.h>
> 
> I don't see the users of this. Use forward declarations (many of them
> are actually missed).
> 
> Also, seems kconfig.h, err.h and errno.h missed.

Thanks for pointing this out. I'll check these.

> 
> > +#include <asm/byteorder.h>
> 
> > +struct ocelot_ddata {
> > +       struct device *dev;
> > +       struct regmap *gcb_regmap;
> > +       struct regmap *cpuorg_regmap;
> > +       int spi_padding_bytes;
> > +       struct spi_device *spi;
> > +};
> 
> ...
> 
> > +#if IS_ENABLED(CONFIG_MFD_OCELOT)
> > +struct regmap *ocelot_init_regmap_from_resource(struct device *child,
> > +                                               const struct resource *res);
> > +#else
> >  static inline struct regmap *
> >  ocelot_init_regmap_from_resource(struct device *child,
> >                                  const struct resource *res)
> >  {
> >         return ERR_PTR(-EOPNOTSUPP);
> >  }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Powered by blists - more mailing lists