| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75Ve-pqgb56punEL=p=PnEtjRnqTBSqgs+vVn1Zv8F94g9Q@mail.gmail.com>
Date: Mon, 25 Jul 2022 22:00:42 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Colin Foster <colin.foster@...advantage.com>
Cc: linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
netdev <netdev@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Terry Bowman <terry.bowman@....com>,
Vladimir Oltean <vladimir.oltean@....com>,
Wolfram Sang <wsa@...nel.org>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
Steen Hegelund <Steen.Hegelund@...rochip.com>,
Lars Povlsen <lars.povlsen@...rochip.com>,
Linus Walleij <linus.walleij@...aro.org>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>,
Russell King <linux@...linux.org.uk>,
Heiner Kallweit <hkallweit1@...il.com>,
Andrew Lunn <andrew@...n.ch>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Lee Jones <lee.jones@...aro.org>, katie.morris@...advantage.com
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@...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
Powered by blists - more mailing lists