| 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: <CAHp75VdnFSP9-D=O3h5L80O19xK7ct6ax6kXGfHEiKe9niktYA@mail.gmail.com>
Date: Mon, 9 May 2022 11:02:42 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Colin Foster <colin.foster@...advantage.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
netdev@...r.kernel.org, Terry Bowman <terry.bowman@....com>,
Wolfram Sang <wsa@...nel.org>,
Steen Hegelund <Steen.Hegelund@...rochip.com>,
Lars Povlsen <lars.povlsen@...rochip.com>,
Linus Walleij <linus.walleij@...aro.org>,
Russell King <linux@...linux.org.uk>,
Heiner Kallweit <hkallweit1@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>,
Florian Fainelli <f.fainelli@...il.com>,
Vivien Didelot <vivien.didelot@...il.com>,
Andrew Lunn <andrew@...n.ch>, UNGLinuxDriver@...rochip.com,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Claudiu Manoil <claudiu.manoil@....com>,
Vladimir Oltean <vladimir.oltean@....com>,
Lee Jones <lee.jones@...aro.org>
Subject: Re: [RFC v8 net-next 08/16] mfd: ocelot: add support for the vsc7512
chip via spi
On Sun, May 8, 2022 at 8:53 PM 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.
...
> + If unsure, say N
Seems like an abrupt sentence.
...
> +EXPORT_SYMBOL(ocelot_chip_reset);
Please, switch to the namespace (_NS) variant of the exported-imported
symbols for these drivers.
...
> +int ocelot_core_init(struct device *dev)
> +{
> + int ret;
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> + ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> + if (ret) {
> + dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
Isn't it simple
return devm_mfd_add_devices(...);
?
> +}
...
> +#include <linux/of.h>
Do you really use this? (See also below).
...
> +#define VSC7512_CPUORG_RES_START 0x71000000
> +#define VSC7512_CPUORG_RES_SIZE 0x2ff
Doesn't look right.
I'm expecting to see 0x300 here and -1 where it's needed in the code.
...
> +static const struct regmap_config ocelot_spi_regmap_config = {
> + .reg_bits = 24,
> + .reg_stride = 4,
> + .reg_downshift = 2,
> + .val_bits = 32,
> +
> + .write_flag_mask = 0x80,
> + .max_register = 0xffffffff,
Is it for real?! Have you considered what happens if someone actually
tries to read all these registers (and for the record it's not a
theoretical case, since the user may do it via debugfs)?
> + .use_single_write = true,
> + .can_multi_write = false,
> +
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .val_format_endian = REGMAP_ENDIAN_NATIVE,
> +};
...
> + if (ddata->spi_padding_bytes > 0) {
' > 0' part is redundant.
> + memset(&padding, 0, sizeof(padding));
> +
> + padding.len = ddata->spi_padding_bytes;
> + padding.tx_buf = dummy_buf;
> + padding.dummy_data = 1;
> +
> + spi_message_add_tail(&padding, &msg);
> + }
...
> + memcpy(®map_config, &ocelot_spi_regmap_config,
> + sizeof(ocelot_spi_regmap_config));
sizeof(regmap_config) is a bit safer (in case somebody changes the
types of the src and dst).
...
> + err = spi_setup(spi);
> + if (err < 0) {
> + dev_err(&spi->dev, "Error %d initializing SPI\n", err);
> + return err;
return dev_err_probe(...);
> + }
...
> + ddata->cpuorg_regmap =
> + ocelot_spi_devm_init_regmap(dev, dev,
> + &vsc7512_dev_cpuorg_resource);
It's easier to read when it's a longer line. At least the last two can
be on one line.
...
> + ddata->gcb_regmap = ocelot_spi_devm_init_regmap(dev, dev,
> + &vsc7512_gcb_resource);
Do you have different cases for two first parameters? If not, drop duplication.
...
> + if (err) {
> + dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
> + return err;
return dev_err_probe(...);
And everywhere else where it's appropriate.
> + }
...
> +const struct of_device_id ocelot_spi_of_match[] = {
> + { .compatible = "mscc,vsc7512_mfd_spi" },
> + { },
No comma for terminator entry.
> +};
...
> + .of_match_table = of_match_ptr(ocelot_spi_of_match),
of_match_ptr() is rather harmful than useful. We have a lot of
compiler warnings due to misuse of this. Besides that it prevents to
use driver in non-OF environments (if it is / will be the case).
...
> +/*
> + * Copyright 2021 Innovative Advantage Inc.
> + */
One line.
...
> +#include <linux/regmap.h>
I don't see the users of this. Use forward declarations (many of them
are actually missed).
Also, seems kconfig.h, err.h and errno.h missed.
> +#include <asm/byteorder.h>
> +struct ocelot_ddata {
> + struct device *dev;
> + struct regmap *gcb_regmap;
> + struct regmap *cpuorg_regmap;
> + int spi_padding_bytes;
> + struct spi_device *spi;
> +};
...
> +#if IS_ENABLED(CONFIG_MFD_OCELOT)
> +struct regmap *ocelot_init_regmap_from_resource(struct device *child,
> + const struct resource *res);
> +#else
> static inline struct regmap *
> ocelot_init_regmap_from_resource(struct device *child,
> const struct resource *res)
> {
> return ERR_PTR(-EOPNOTSUPP);
> }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists