Re: [PATCH v15 mfd 9/9] mfd: ocelot: add support for the vsc7512 chip via spi

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Wed, Aug 03, 2022 at 01:45:04PM +0200, Andy Shevchenko wrote:
> On Wed, Aug 3, 2022 at 7:48 AM 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.
> 
> 
> ...
> 
> > +#include <asm/byteorder.h>
> 
> Not sure I see the user of this header.

Interesting. And I think you uncovered one more issue.

I'd used byteorder at one time to modify the SPI payload (addr is always
big-endian, payload should always be native).

When I migrated to using the spi_bus_read() instead of spi_reg_read(),
this became handled much more elegantly in regmap itself. So this isn't
needed anymore.

I also have byteorder in include/linux/mfd/ocelot.h, where it also isn't
needed. It is checking:

#ifdef __LITTLE_ENDIAN
#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_LE
#else
#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_BE
#endif


That file should be replaced with
#include <linux/kconfig.h>

> 
> ...
> 
> > +struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource *res)
> > +{
> > +       struct regmap_config regmap_config;
> > +
> > +       memcpy(&regmap_config, &ocelot_spi_regmap_config, sizeof(regmap_config));
> > +
> > +       regmap_config.name = res->name;
> 
> > +       regmap_config.max_register = res->end - res->start;
> 
> Hmm... First of all, resource_size() is for that (with - 1 to the
> result). But don't you need to use stride in the calculations?

DEFINE_RES_NAMED populates the resource .end with (_start) + (_size) - 1
so I don't think resource_size is correct to use here.

reg_stride gets handled at the top of regmap_read(), so I don't think
that's really needed either.


For reference:

#define VSC7512_DEVCPU_ORG_RES_START    0x71000000
#define VSC7512_DEVCPU_ORG_RES_SIZE     0x38


# pwd
/sys/kernel/debug/regmap/spi0.0-devcpu_org
# cat range
0-34
# cat registers
00: 00000000
04: 02000001
08: 00000001
0c: 00000000
10: 00000fff
14: 00000000
18: 00000000
1c: 00000000
20: 00000000
24: 00000000
28: 00000001
2c: 00000004
30: 00000001
34: 00000004


> 
> > +       regmap_config.reg_base = res->start;
> > +
> > +       return devm_regmap_init(dev, &ocelot_spi_regmap_bus, dev, &regmap_config);
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko