Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 25 Jul 2022 22:00:42 +0200 | Subject | Re: [PATCH v14 mfd 9/9] mfd: ocelot: add support for the vsc7512 chip via spi |
| |
On Fri, Jul 22, 2022 at 6:06 AM 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.
...
+bits.h
> +#include <linux/kernel.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/ocelot.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/types.h>
...
> +#define VSC7512_MIIM_RES_SIZE 0x24 > +#define VSC7512_PHY_RES_SIZE 0x4
Can you make _SIZEs to be fixed width? Like 0x004 here.
> +#define VSC7512_GPIO_RES_SIZE 0x6c
> +#define VSC7512_SIO_CTRL_RES_SIZE 0x100
...
> + ret = readx_poll_timeout(ocelot_gcb_chip_rst_status, ddata, val, !val, > + VSC7512_GCB_RST_SLEEP_US, VSC7512_GCB_RST_TIMEOUT_US); > + return ret;
return readx_poll_timeou(...);
...
> +static int ocelot_spi_regmap_bus_read(void *context, const void *reg, size_t reg_size, > + void *val, size_t val_size) > +{ > + struct spi_transfer tx, padding, rx; > + struct device *dev = context; > + struct ocelot_ddata *ddata; > + struct spi_device *spi; > + struct spi_message msg; > + > + ddata = dev_get_drvdata(dev); > + spi = to_spi_device(dev); > + > + spi_message_init(&msg); > + > + memset(&tx, 0, sizeof(tx)); > + > + tx.tx_buf = reg; > + tx.len = reg_size; > + > + spi_message_add_tail(&tx, &msg); > + > + if (ddata->spi_padding_bytes) { > + memset(&padding, 0, sizeof(padding)); > + > + padding.len = ddata->spi_padding_bytes; > + padding.tx_buf = ddata->dummy_buf; > + padding.dummy_data = 1; > + > + spi_message_add_tail(&padding, &msg); > + } > + > + memset(&rx, 0, sizeof(rx)); > + rx.rx_buf = val; > + rx.len = val_size; > + > + spi_message_add_tail(&rx, &msg);
I'm wondering if you can use in both cases spi_message_init_with_transfers(). (Note you may explicitly as SPI core to toggle CS if needed)
> + return spi_sync(spi, &msg); > +}
...
> +static int ocelot_spi_regmap_bus_write(void *context, const void *data, size_t count) > +{ > + struct device *dev = context; > + struct spi_device *spi; > + > + spi = to_spi_device(dev);
Can be unified with definition block above to save 2 LoCs.
> + return spi_write(spi, data, count); > +}
...
> + ddata->spi_padding_bytes = 1 + (spi->max_speed_hz / 1000000 + 2) / 8;
HZ_PER_MHZ ?
...
> + /* > + * A chip reset will clear the SPI configuration, so it needs to be done > + * again before we can access any registers
Missed period.
> + */
...
> + err = ocelot_core_init(dev); > + if (err < 0)
Does ' < 0' part here and everywhere else bring any value?
> + return dev_err_probe(dev, err, "Error initializing Ocelot core\n");
...
> + * struct ocelot_ddata - Private data for an external Ocelot chip > + *
No need for this and blank lines between field descriptions..
> + * @gcb_regmap: General Configuration Block regmap. Used for > + * operations like chip reset. > + * > + * @cpuorg_regmap: CPU Device Origin Block regmap. Used for operations > + * like SPI bus configuration. > + * > + * @spi_padding_bytes: Number of padding bytes that must be thrown out before > + * read data gets returned. This is calculated during > + * initialization based on bus speed. > + * > + * @dummy_buf: Zero-filled buffer of spi_padding_bytes size. The dummy > + * bytes that will be sent out between the address and > + * data of a SPI read operation.
-- With Best Regards, Andy Shevchenko
| |