linux-kernel - Re: [PATCH v2 5/7] mfd: Add RTL8231 core device

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]

Date:   Sun, 23 May 2021 23:28:48 +0200
From:   Sander Vanheule <sander@...nheule.net>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 5/7] mfd: Add RTL8231 core device

Hi Andy,

I've implemented the minor remarks (redundant assignments, if/else code
structure...). Some extra details below.

On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@...nheule.net> wrote:
> > 
> > The RTL8231 is implemented as an MDIO device, and provides a regmap
> > interface for register access by the core and child devices.
> > 
> > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek.
> > Since kernel support for SMI is limited, and no real-world SMI
> > implementations have been encountered for this device, this is currently
> > unimplemented. The use of the regmap interface should make any future
> > support relatively straightforward.
> > 
> > After reset, all pins are muxed to GPIO inputs before the pin drivers
> > are enabled. This is done to prevent accidental system resets, when a
> > pin is connected to the parent SoC's reset line.
> 
> > [missing MDIO_BUS dependency, provided via REGMAP_MDIO]
> > Reported-by: kernel test robot <lkp@...el.com>
> 
> What is the culprit? Shouldn't this have a Fixes tag?

But it doesn't actually fix an issue created by an existing commit, just
something that was wrong in the first version of the patch.  This patch is not
dedicated to fixing that single issue though, it's just a part of it. Hence the
note above the Reported-by tag.

> > 
> > +       mdiodev->reset_gpio = gpiod_get_optional(dev, "reset",
> > GPIOD_OUT_LOW);
> > +       device_property_read_u32(dev, "reset-assert-delay", &mdiodev-
> > >reset_assert_delay);
> > +       device_property_read_u32(dev, "reset-deassert-delay", &mdiodev-
> > >reset_deassert_delay);
> > +
> > +       err = rtl8231_init(dev, map);
> > +       if (err)
> 
> Resource leakage.

Replaced gpiod_get_optional by devm_gpiod_get_optional.

> 
> > +               return err;
> > +
> > +       /* LED_START enables power to output pins, and starts the LED engine
> > */
> > +       regmap_field_write(led_start, 1);
> 
> > +       return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, rtl8231_cells,
> > +               ARRAY_SIZE(rtl8231_cells), NULL, 0, NULL);
> 
> Ditto.
> 
> > +}
> 
> ...
> 
> > +#ifdef CONFIG_PM
> 
> Replace this with __maybe_unused attribute.

Done. I've also used a few extra macros from PM header to clean this part up a
bit more.



Best,
Sander