Re: [RFC v8 net-next 08/16] mfd: ocelot: add support for the vsc7512 chip via spi

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sun, May 8, 2022 at 8:53 PM Colin Foster
<colin.foster@xxxxxxxxxxxxxxxx> 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.

...

> +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.

...

> +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)?

> +       .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.

...

> +       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).

...

> +/*
> + * 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.

> +#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