Re: [PATCH v4 3/4] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

Johan Hovold
2019-01-09T11:30:44Z

On Sat, Dec 22, 2018 at 08:22:09PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
>
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
> Changes in v4:
> - Use gpiod_set_raw_value() for deassert_reset() and
> assert_reset() as gpiod_set_value() will change the
> sematics of these calls by taking any gpio flags
> into account.

Please also mention this in the commit message (i.e. that we continue to
ignore gpio flags from device tree for now).

> - Remove some unnecesssary line breaks.
> - Restore 'spi_en' gpio check in fw_flashing_seq()
> as it is currently optional.
> Changes in v3:
> - Add this patch in a patchset.
> Changes in v2:
> - Resolved compilation errors.
> ---

> @@ -75,11 +74,10 @@ static int coldboot_seq(struct platform_device *pdev)
> return 0;
>
> /* Hold APB in reset state */
> - assert_reset(apb->resetn_gpio);
> + assert_reset(apb->resetn);
>
> - if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> - gpio_is_valid(apb->spi_en_gpio))
> - devm_gpio_free(dev, apb->spi_en_gpio);
> + if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING && apb->spi_en)
> + devm_gpiod_put(dev, apb->spi_en);
>
> /* Enable power to APB */
> if (!IS_ERR(apb->vcore)) {
> @@ -101,13 +99,13 @@ static int coldboot_seq(struct platform_device *pdev)
> apb_bootret_deassert(dev);
>
> /* On DB3 clock was not mandatory */
> - if (gpio_is_valid(apb->clk_en_gpio))
> - gpio_set_value(apb->clk_en_gpio, 1);
> + if (apb->clk_en)
> + gpiod_set_value(apb->clk_en, 1);
>
> usleep_range(100, 200);
>
> /* deassert reset to APB : Active-low signal */
> - deassert_reset(apb->resetn_gpio);
> + deassert_reset(apb->resetn);
>
> apb->state = ARCHE_PLATFORM_STATE_ACTIVE;
>
> @@ -136,25 +134,25 @@ static int fw_flashing_seq(struct platform_device *pdev)
> return ret;
> }
>
> - if (gpio_is_valid(apb->spi_en_gpio)) {
> + if (apb->spi_en) {
> unsigned long flags;
>
> if (apb->spi_en_polarity_high)
> - flags = GPIOF_OUT_INIT_HIGH;
> + flags = GPIOD_OUT_HIGH;
> else
> - flags = GPIOF_OUT_INIT_LOW;
> + flags = GPIOD_OUT_LOW;
>
> - ret = devm_gpio_request_one(dev, apb->spi_en_gpio,
> - flags, "apb_spi_en");
> - if (ret) {
> - dev_err(dev, "Failed requesting SPI bus en gpio %d\n",
> - apb->spi_en_gpio);
> + apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);

I just noticed that you change the name of the device-tree property here
(and later in apb_ctrl_get_devtree_data()). How is that expected to
work without breaking current systems? This will be unavoidable at some
point, but must not be snuck into a patch like this without any comment.
Please keep the current names for now.

I do think you need to drop the "-gpio" suffix when requesting the gpio
though. Please double check to make sure.

Johan