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

[Andy Shevchenko]

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