2018-06-18 17:15:07

by Matthias Reichl

[permalink] [raw]
Subject: Regression in 4.18-rc1 caused by regulator: arizona-ldo1: Look up a descriptor and pass to the core

Commit e1739e86f0cb9c48e8745a610e6981a4e24cadad breaks reading
the wlf,ldoena property from device tree. This causes ldo1 to
stay off and thus arizona device detection to fail:

[ 4.495958] of_get_named_gpiod_flags: parsed 'wlf,reset' property of node '/s
oc/spi@7e204000/wm5102@1[0]' - status (0)
[ 4.509756] arizona-ldo1 arizona-ldo1: GPIO lookup for consumer wlf,ldoena
[ 4.511339] arizona-ldo1 arizona-ldo1: using lookup tables for GPIO lookup
[ 4.511351] arizona-ldo1 arizona-ldo1: No GPIO consumer wlf,ldoena found
[ 4.511413] LDO1: supplied by RPi-Cirrus 1v8
[ 4.549164] arizona spi0.1: Unknown device ID: 0

With this commit reverted the ldoena GPIO is properly found and
asserted high and the arizona device type is read successfully:

[ 4.630445] of_get_named_gpiod_flags: parsed 'wlf,reset' property of node '/s
oc/spi@7e204000/wm5102@1[0]' - status (0)
[ 4.647622] of_get_named_gpiod_flags: parsed 'wlf,ldoena' property of node '/soc/spi@7e204000/wm5102@1[0]' - status (0)
[ 4.666100] arizona spi0.1: WM5102 revision C

I've tested this on a Raspberry Pi 3 with upstream 4.18-rc1,
the Cirrus Logic Audio Card from Element 14 (using a WM5102
connected via SPI) and the downstream card driver added as
a module.

Both wlf,reset and wlf,ldoena properties are defined in
the wm5102 devicetree node:

wm5102@1{
compatible = "wlf,wm5102";
reg = <1>;
...
wlf,reset = <&gpio 17 0>;
wlf,ldoena = <&gpio 22 0>;
...
};

If I understand the code correctly the cuplrit seems to be that
with this patch the gpio is looked up from the arizona-ldo1 dev,
which has a NULL of_node:

config.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, "wlf,ldoena", ...

The old version did an of lookup in the parent (arizona) dev, which
contains the of_node with wlf,ldoena:

arizona_ldo1_common_init:
struct device *parent_dev = pdev->dev.parent;
struct regulator_config config = { };
...
config.dev = parent_dev;
...
arizona_ldo1_of_get_pdata(pdata, config, ...

arizona_ldo1_of_get_pdata:
struct device_node *np = config->dev->of_node;
...
pdata->ldoena = of_get_named_gpio(np, "wlf,ldoena", 0);

so long,

Hias


2018-06-19 08:53:35

by Charles Keepax

[permalink] [raw]
Subject: Re: Regression in 4.18-rc1 caused by regulator: arizona-ldo1: Look up a descriptor and pass to the core

On Mon, Jun 18, 2018 at 07:14:10PM +0200, Matthias Reichl wrote:
> Commit e1739e86f0cb9c48e8745a610e6981a4e24cadad breaks reading
> the wlf,ldoena property from device tree. This causes ldo1 to
> stay off and thus arizona device detection to fail:
>
> [ 4.495958] of_get_named_gpiod_flags: parsed 'wlf,reset' property of node '/s
> oc/spi@7e204000/wm5102@1[0]' - status (0)
> [ 4.509756] arizona-ldo1 arizona-ldo1: GPIO lookup for consumer wlf,ldoena
> [ 4.511339] arizona-ldo1 arizona-ldo1: using lookup tables for GPIO lookup
> [ 4.511351] arizona-ldo1 arizona-ldo1: No GPIO consumer wlf,ldoena found
> [ 4.511413] LDO1: supplied by RPi-Cirrus 1v8
> [ 4.549164] arizona spi0.1: Unknown device ID: 0
>
> With this commit reverted the ldoena GPIO is properly found and
> asserted high and the arizona device type is read successfully:
>
> [ 4.630445] of_get_named_gpiod_flags: parsed 'wlf,reset' property of node '/s
> oc/spi@7e204000/wm5102@1[0]' - status (0)
> [ 4.647622] of_get_named_gpiod_flags: parsed 'wlf,ldoena' property of node '/soc/spi@7e204000/wm5102@1[0]' - status (0)
> [ 4.666100] arizona spi0.1: WM5102 revision C
>
> I've tested this on a Raspberry Pi 3 with upstream 4.18-rc1,
> the Cirrus Logic Audio Card from Element 14 (using a WM5102
> connected via SPI) and the downstream card driver added as
> a module.
>
> Both wlf,reset and wlf,ldoena properties are defined in
> the wm5102 devicetree node:
>
> wm5102@1{
> compatible = "wlf,wm5102";
> reg = <1>;
> ...
> wlf,reset = <&gpio 17 0>;
> wlf,ldoena = <&gpio 22 0>;
> ...
> };
>
> If I understand the code correctly the cuplrit seems to be that
> with this patch the gpio is looked up from the arizona-ldo1 dev,
> which has a NULL of_node:
>
> config.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, "wlf,ldoena", ...
>
> The old version did an of lookup in the parent (arizona) dev, which
> contains the of_node with wlf,ldoena:
>
> arizona_ldo1_common_init:
> struct device *parent_dev = pdev->dev.parent;
> struct regulator_config config = { };
> ...
> config.dev = parent_dev;
> ...
> arizona_ldo1_of_get_pdata(pdata, config, ...
>
> arizona_ldo1_of_get_pdata:
> struct device_node *np = config->dev->of_node;
> ...
> pdata->ldoena = of_get_named_gpio(np, "wlf,ldoena", 0);
>

Urgh.. sorry about that. I did test that patch but it seems I
messed it up. On our boards the LDOENA defaults to on so I missed
that it wasn't getting configured anymore.

I think we just need to not use the devm version of the
get_optional and pass the correct device. I will get a patch
ready this morning and ping it through for testing.

Thanks,
Charles