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

Andy Shevchenko
2022-05-09T09:25:57Z

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

...

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