lkml.org 
[lkml]   [2022]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v12 net-next 9/9] mfd: ocelot: add support for the vsc7512 chip via spi
    On Fri, Jul 1, 2022 at 9:26 PM 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.

    ...

    > +config MFD_OCELOT
    > + tristate "Microsemi Ocelot External Control Support"
    > + depends on SPI_MASTER
    > + select MFD_CORE
    > + select REGMAP_SPI
    > + help
    > + Ocelot is a family of networking chips that support multiple ethernet
    > + and fibre interfaces. In addition to networking, they contain several
    > + other functions, including pinctrl, MDIO, and communication with
    > + external chips. While some chips have an internal processor capable of
    > + running an OS, others don't. All chips can be controlled externally
    > + through different interfaces, including SPI, I2C, and PCIe.
    > +
    > + Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
    > + VSC7513, VSC7514) controlled externally.
    > +
    > + If unsure, say N.

    What will be the module name?

    ...

    It misses a few inclusions, like kernel.h for ARRAY_SIZE() and types.h
    for booleans.

    > +#include <linux/mfd/core.h>
    > +#include <linux/mfd/ocelot.h>
    > +#include <linux/module.h>
    > +#include <linux/regmap.h>

    + blank line?

    > +#include <soc/mscc/ocelot.h>

    ...

    > +#define GCB_SOFT_RST 0x0008
    > +
    > +#define SOFT_CHIP_RST 0x1

    It's not clear what these values are: register offsets? Bit fields of
    the hardware registers? Commands to some IPC?

    > +#define VSC7512_GCB_RST_SLEEP 100
    > +#define VSC7512_GCB_RST_TIMEOUT 100000

    Missed units in both cases.

    ...


    > +static int ocelot_gcb_chip_rst_status(struct ocelot_ddata *ddata)
    > +{
    > + int val, err;
    > +
    > + err = regmap_read(ddata->gcb_regmap, GCB_SOFT_RST, &val);
    > + if (err)

    > + val = -1;

    Can be returned directly. Why not a proper error code, btw?

    > + return val;
    > +}

    ...

    > +#include <linux/iopoll.h>

    What for? Maybe it should be ioport.h ?


    ...

    > + static const u8 dummy_buf[16] = {0};

    On stack for DMA?! Hmm...
    ...

    > + err = spi_setup(spi);
    > + if (err < 0) {
    > + return dev_err_probe(&spi->dev, err,
    > + "Error performing SPI setup\n");
    > + }

    {} are not needed.

    ...

    > + err = ocelot_spi_initialize(dev);
    > + if (err) {
    > + return dev_err_probe(dev, err,
    > + "Error initializing SPI bus after reset\n");
    > + }

    {} are not needed.

    > + err = ocelot_core_init(dev);
    > + if (err < 0) {

    Ditto.

    > + return dev_err_probe(dev, err,
    > + "Error initializing Ocelot core\n");

    > + return err;

    Dead code.

    > + }

    ...

    > +#include <asm/byteorder.h>

    You missed a lot of forward declarations that are used in this file.

    Like

    struct spi_device;

    --
    With Best Regards,
    Andy Shevchenko

    \
     
     \ /
      Last update: 2022-07-01 22:39    [W:4.110 / U:0.804 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site