On Wed, Jan 09, 2019 at 12:30:43PM +0100, Johan Hovold wrote:
> 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@xxxxxxxxx>
> > ---
> > 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).
>
Ok, I'll do that.
> > - 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.
>
Ok, I'll do that.
> I do think you need to drop the "-gpio" suffix when requesting the gpio
> though. Please double check to make sure.
>
Ok, I'll check it.
> Johan
Thanks for the review.
Regards,
Nishad
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
