On Fri, Jul 22, 2022 at 6:06 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.
...
+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
