linux-kernel - Re: [PATCH anybus v2 1/5] misc: support the Arcx anybus bridge.

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACRpkdai-6iNbz4EGuHNHkQYyR+yhTEJdRXP4G6fUKpA+XApjQ@mail.gmail.com>
Date:   Thu, 1 Nov 2018 00:15:25 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Sven Van Asbroeck <thesven73@...il.com>,
        Arnd Bergmann <arnd@...db.de>
Cc:     svendev@...x.com, Lee Jones <lee.jones@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Andreas Färber <afaerber@...e.de>,
        Thierry Reding <treding@...dia.com>,
        David Lechner <david@...hnology.com>,
        Noralf Trønnes <noralf@...nnes.org>,
        Johan Hovold <johan@...nel.org>,
        Michal Simek <monstr@...str.eu>, michal.vokac@...ft.com,
        Greg KH <gregkh@...uxfoundation.org>, john.garry@...wei.com,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Robin Murphy <robin.murphy@....com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Sebastien Bourdelin <sebastien.bourdelin@...oirfairelinux.com>,
        Icenowy Zheng <icenowy@...c.io>,
        Stuart Yoder <stuyoder@...il.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH anybus v2 1/5] misc: support the Arcx anybus bridge.

Hi Sven,

On Wed, Oct 31, 2018 at 8:44 PM <thesven73@...il.com> wrote:

> From: Sven Van Asbroeck <svendev@...x.com>
>
> Add a driver for the Arcx anybus bridge.
>
> This chip embeds up to two Anybus-S application connectors
> (slots), and connects to the SoC via a parallel memory bus.
> There is also a CAN power readout, unrelated to the Anybus.
>
> Signed-off-by: Sven Van Asbroeck <svendev@...x.com>

This is fun :)

>  drivers/misc/Kconfig         |   9 ++
>  drivers/misc/Makefile        |   1 +
>  drivers/misc/anybus-bridge.c | 301 +++++++++++++++++++++++++++++++++++

I would put this also in drivers/bus, why not. Just two files
there. It's a bus bridge for sure, we keep them there.
drivers/reset if it is mostly about resetting stuff.

> +config ARCX_ANYBUS_BRIDGE
> +       tristate "Arcx Anybus-S Bridge"
> +       depends on OF

depends on GPIOLIB

> +       help
> +         Select this to get support for the Arcx Anybus bridge.
> +         It connects to the SoC via a parallel memory bus, and
> +         embeds up to two Anybus-S application connectors (slots).
> +         There is also a CAN power readout, unrelated to the Anybus.

(...)
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>

Don't use these please. Juse use
#include <linux/gpio/consumer.h>

> +struct bridge_priv {
> +       struct device *class_dev;
> +       struct reset_controller_dev rcdev;
> +       bool common_reset;
> +       int reset_gpio;

struct gpio_desc *reset_gpiod;

> +       void __iomem *cpld_base;
> +       spinlock_t regs_lock;
> +       u8 control_reg;
> +       char version[3];
> +       u16 design_no;
(...)

> +static ssize_t version_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct bridge_priv *cd = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%s\n", cd->version);
> +}
> +static DEVICE_ATTR_RO(version);

Do you need this in userspace really?

> +static ssize_t design_number_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct bridge_priv *cd = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%d\n", cd->design_no);
> +}
> +static DEVICE_ATTR_RO(design_number);

And this?

> +static ssize_t can_power_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct bridge_priv *cd = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%d\n",
> +               !(readb(cd->cpld_base + CPLD_STATUS1) &
> +                                       CPLD_STATUS1_CAN_POWER));
> +}
> +static DEVICE_ATTR_RO(can_power);

This should certainly be reflected as a fixed-voltage regulator
and not a random integer in sysfs.

> +static int bridge_probe(struct platform_device *pdev)
> +{
> +       struct bridge_priv *cd;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       int err, id;
> +       struct resource *res;
> +       u8 status1, cap;
> +
> +       cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
> +       if (!cd)
> +               return -ENOMEM;
> +       dev_set_drvdata(dev, cd);
> +       spin_lock_init(&cd->regs_lock);

This:

> +       cd->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> +       if (!gpio_is_valid(cd->reset_gpio)) {
> +               dev_err(dev, "reset-gpios not found\n");
> +               return -EINVAL;
> +       }
> +       devm_gpio_request(dev, cd->reset_gpio, NULL);
> +       gpio_direction_output(cd->reset_gpio, 0);

Should be:

cd->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(cd->reset_gpiod))
    return PTR_ERR(cd->reset_gpiod);

You can turn it to input as you do in the .remove() function to let
a pull-up resistor pull it high, but isn't it better to actively just drive
it high?

Yours,
Linus Walleij