| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20181222144118.GB15439@nishad>
Date: Sat, 22 Dec 2018 20:11:19 +0530
From: Nishad Kamdar <nishadkamdar@...il.com>
To: Johan Hovold <johan@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Alex Elder <elder@...nel.org>,
Rui Miguel Silva <rmfrfs@...il.com>,
greybus-dev@...ts.linaro.org, devel@...verdev.osuosl.org,
linux-kernel@...r.kernel.org,
Nishad Kamdar <nishadkamdar@...il.com>
Subject: Re: [PATCH v3 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the
gpio descriptor interface
On Tue, Dec 18, 2018 at 12:35:40PM +0100, Johan Hovold wrote:
> On Thu, Nov 22, 2018 at 10:38:18PM +0530, Nishad Kamdar wrote:
> > Use the gpiod interface instead of the deprecated old non-descriptor
> > interface.
> >
> > Signed-off-by: Nishad Kamdar <nishadkamdar@...il.com>
> > ---
> > Changes in v2:
> > - Resolved compilation errors.
> > ---
> > drivers/staging/greybus/arche-apb-ctrl.c | 159 +++++++++--------------
> > 1 file changed, 65 insertions(+), 94 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
> > index be5ffed90bcf..e887f6aee20b 100644
> > --- a/drivers/staging/greybus/arche-apb-ctrl.c
> > +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> > @@ -8,9 +8,8 @@
> >
> > #include <linux/clk.h>
> > #include <linux/delay.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/interrupt.h>
> > -#include <linux/of_gpio.h>
> > #include <linux/of_irq.h>
> > #include <linux/module.h>
> > #include <linux/pinctrl/consumer.h>
> > @@ -24,12 +23,12 @@ static void apb_bootret_deassert(struct device *dev);
> >
> > struct arche_apb_ctrl_drvdata {
> > /* Control GPIO signals to and from AP <=> AP Bridges */
> > - int resetn_gpio;
> > - int boot_ret_gpio;
> > - int pwroff_gpio;
> > - int wake_in_gpio;
> > - int wake_out_gpio;
> > - int pwrdn_gpio;
> > + struct gpio_desc *resetn;
> > + struct gpio_desc *boot_ret;
> > + struct gpio_desc *pwroff;
> > + struct gpio_desc *wake_in;
> > + struct gpio_desc *wake_out;
> > + struct gpio_desc *pwrdn;
> >
> > enum arche_platform_state state;
> > bool init_disabled;
> > @@ -37,28 +36,28 @@ struct arche_apb_ctrl_drvdata {
> > struct regulator *vcore;
> > struct regulator *vio;
> >
> > - int clk_en_gpio;
> > + struct gpio_desc *clk_en;
> > struct clk *clk;
> >
> > struct pinctrl *pinctrl;
> > struct pinctrl_state *pin_default;
> >
> > /* V2: SPI Bus control */
> > - int spi_en_gpio;
> > + struct gpio_desc *spi_en;
> > bool spi_en_polarity_high;
> > };
> >
> > /*
> > * Note that these low level api's are active high
> > */
> > -static inline void deassert_reset(unsigned int gpio)
> > +static inline void deassert_reset(struct gpio_desc *gpio)
> > {
> > - gpio_set_value(gpio, 1);
> > + gpiod_set_value(gpio, 1);
> > }
> >
> > -static inline void assert_reset(unsigned int gpio)
> > +static inline void assert_reset(struct gpio_desc *gpio)
> > {
> > - gpio_set_value(gpio, 0);
> > + gpiod_set_value(gpio, 0);
> > }
>
> As the comment above deassert_reset() suggests, this change will
> actually change the semantics of these calls by taking any gpio flags
> into account (e.g. ACTIVE_LOW which will invert the signals).
>
> Perhaps you should just use gpiod_set_raw_value() for now, and this can
> be addressed later. Alternatively, drop the comment and invert the
> polarity here and any users will need to update their device trees.
>
> Either way, mention this in the commit message.
>
Ok, I'll use the gpiod_set_raw_value().
> > /*
> > @@ -75,11 +74,11 @@ 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);
> > + apb->spi_en)
>
> No need to break the line any more.
>
Ok.
> > + devm_gpiod_put(dev, apb->spi_en);
> >
> > /* Enable power to APB */
> > if (!IS_ERR(apb->vcore)) {
> > @@ -101,13 +100,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;
> >
> > @@ -119,6 +118,7 @@ static int fw_flashing_seq(struct platform_device *pdev)
> > struct device *dev = &pdev->dev;
> > struct arche_apb_ctrl_drvdata *apb = platform_get_drvdata(pdev);
> > int ret;
> > + unsigned long flags;
> >
> > if (apb->init_disabled ||
> > apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING)
> > @@ -136,25 +136,20 @@ static int fw_flashing_seq(struct platform_device *pdev)
> > return ret;
> > }
> >
> > - if (gpio_is_valid(apb->spi_en_gpio)) {
>
> spi_en_gpio is currently optional, so cannot just drop the check.
>
Ok, I'll restore it.
> > - unsigned long flags;
> > + if (apb->spi_en_polarity_high)
> > + flags = GPIOD_OUT_HIGH;
> > + else
> > + flags = GPIOD_OUT_LOW;
>
> This should probably also be converted to honouring the gpio flags, but
> perhaps better to do in a later patch.
>
Ok.
> >
> > - if (apb->spi_en_polarity_high)
> > - flags = GPIOF_OUT_INIT_HIGH;
> > - else
> > - flags = GPIOF_OUT_INIT_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);
> > - return ret;
> > - }
> > + apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);
> > + if (IS_ERR(apb->spi_en)) {
> > + ret = PTR_ERR(apb->spi_en);
> > + dev_err(dev, "Failed requesting SPI bus en GPIO: %d\n", ret);
> > + return ret;
> > }
> >
> > /* for flashing device should be in reset state */
> > - assert_reset(apb->resetn_gpio);
> > + assert_reset(apb->resetn);
> > apb->state = ARCHE_PLATFORM_STATE_FW_FLASHING;
> >
> > return 0;
> > @@ -177,8 +172,8 @@ static int standby_boot_seq(struct platform_device *pdev)
> > return 0;
> >
> > if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> > - gpio_is_valid(apb->spi_en_gpio))
> > - devm_gpio_free(dev, apb->spi_en_gpio);
> > + apb->spi_en)
>
> No line break. Check this throughout.
>
Yes.
> > + devm_gpiod_put(dev, apb->spi_en);
> >
> > /*
> > * As per WDM spec, do nothing
>
> > @@ -404,12 +379,8 @@ static int apb_ctrl_get_devtree_data(struct platform_device *pdev,
> > }
> >
> > /* Only applicable for platform >= V2 */
> > - apb->spi_en_gpio = of_get_named_gpio(np, "spi-en-gpio", 0);
> > - if (apb->spi_en_gpio >= 0) {
> > - if (of_property_read_bool(pdev->dev.of_node,
> > - "spi-en-active-high"))
> > - apb->spi_en_polarity_high = true;
> > - }
> > + if (of_property_read_bool(pdev->dev.of_node, "gb,spi-en-active-high"))
> > + apb->spi_en_polarity_high = true;
>
> Guess it's fine to drop the spi_en check here though.
>
Ok.
Thanks for the review.
regards,
Nishad
> Johan
Powered by blists - more mailing lists