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

[Sander Vanheule]

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@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> 
> 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