On Sun, May 8, 2022 at 8:53 PM Colin Foster
[off-list ref] 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel