Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 10 Jun 2019 16:59:19 +0100
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Linus Walleij <linus.walleij@linaro.org>
CC:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Richard Fitzgerald <rf@opensource.cirrus.com>,
        <patches@opensource.cirrus.com>
Subject: Re: [PATCH] regulator: wm831x: Convert to use GPIO descriptors
Message-ID: <20190610155919.GQ28362@ediswmail.ad.cirrus.com>
References: <20190609210025.29224-1-linus.walleij@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20190609210025.29224-1-linus.walleij@linaro.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jun 09, 2019 at 11:00:25PM +0200, Linus Walleij wrote:
> This converts the Wolfson Micro WM831x DCDC converter to use
> a GPIO descriptor for the GPIO driving the DVS pin.
> 
> There is just one (non-DT) machine in the kernel using this, and
> that is the Wolfson Micro (now Cirrus) Cragganmore 6410 so we
> patch this board to pass a descriptor table and fix up the driver
> accordingly.
> 
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Cc: Richard Fitzgerald <rf@opensource.cirrus.com>
> Cc: patches@opensource.cirrus.com
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> +/*
> + * VDDARM is eventually ending up as a regulator hanging on the MFD cell device
> + * "wm831x-buckv.1" spawn from drivers/mfd/wm831x-core.c.
> + *
> + * From the note on the platform data we can see that this is clearly DVS1
> + * and assigned as dcdc1 resource to the MFD core which sets .id of the cell
> + * spawning the DVS1 platform device to 1, resulting in the device name
> + * "wm831x-buckv.1".
> + */
> +static struct gpiod_lookup_table crag_pmic_gpiod_table = {
> +	.dev_id = "wm831x-buckv.1",

This is not correct, the mfd_add_devices in wm831x-core passes an
ID to the MFD core. Which is calculated from wm831x_num  * 10,
taken from the pdata. So this should be "wm831x-buckv.11".

> +	.table = {
> +		GPIO_LOOKUP("GPIOK", 0, "dvs", GPIO_ACTIVE_HIGH),
> +		{ },
> +	},
> +};
> +

<snip>

> -	ret = devm_gpio_request_one(&pdev->dev, pdata->dvs_gpio,
> -				    dcdc->dvs_gpio_state ? GPIOF_INIT_HIGH : 0,
> -				    "DCDC DVS");
> -	if (ret < 0) {
> -		dev_err(wm831x->dev, "Failed to get %s DVS GPIO: %d\n",
> -			dcdc->name, ret);
> +	dcdc->dvs_gpiod = devm_gpiod_get_optional(&pdev->dev, "dvs",
> +			dcdc->dvs_gpio_state ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> +	if (IS_ERR(dcdc->dvs_gpiod)) {
> +		dev_err(wm831x->dev, "Failed to get %s DVS GPIO: %ld\n",
> +			dcdc->name, PTR_ERR(dcdc->dvs_gpiod));
>  		return;
>  	}
>  

Whats the thinking behind using get_optional here? A plain
devm_gpiod_get looks like it would be closer to the original
code. Previously if there was no GPIO we would log an error and
not execute the rest of the function, this is I think no longer
the case after this change.

Thanks,
Charles