On Tue, 20 Feb 2018, Charles Keepax wrote:

> Now GPIOD has support for both pdata systems and for non-standard DT
> bindings the Arizona reset GPIO can be converted to use it.
> 
> Signed-off-by: Charles Keepax <ckee...@opensource.cirrus.com>
> ---
> 
> Changes since v2:
>  - Kept null check in arizona_enable_reset, although
>    gpiod_set_value_cansleep does its own null check it will also issue
>    a fat WARN we don't want if gpiolib is not built in.
>  - Added a check for ENOSYS in arizona_of_get_core_pdata, just to
>    silence the extra error that would be printed here if gpiolib is not
>    built in.
> 
> Thanks,
> Charles
> 
>  drivers/mfd/arizona-core.c        | 50 
> ++++++++++++++++++++++++++-------------
>  include/linux/mfd/arizona/pdata.h |  3 ++-
>  2 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 77875250abe5..9558c4d9c8ca 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -279,7 +279,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
>  static inline void arizona_enable_reset(struct arizona *arizona)
>  {
>       if (arizona->pdata.reset)
> -             gpio_set_value_cansleep(arizona->pdata.reset, 0);
> +             gpiod_set_value_cansleep(arizona->pdata.reset, 0);
>  }
>  
>  static void arizona_disable_reset(struct arizona *arizona)
> @@ -295,7 +295,7 @@ static void arizona_disable_reset(struct arizona *arizona)
>                       break;
>               }
>  
> -             gpio_set_value_cansleep(arizona->pdata.reset, 1);
> +             gpiod_set_value_cansleep(arizona->pdata.reset, 1);
>               usleep_range(1000, 5000);
>       }
>  }
> @@ -799,14 +799,26 @@ static int arizona_of_get_core_pdata(struct arizona 
> *arizona)
>       struct arizona_pdata *pdata = &arizona->pdata;
>       int ret, i;
>  
> -     pdata->reset = of_get_named_gpio(arizona->dev->of_node, "wlf,reset", 0);
> -     if (pdata->reset == -EPROBE_DEFER) {
> -             return pdata->reset;
> -     } else if (pdata->reset < 0) {
> -             dev_err(arizona->dev, "Reset GPIO missing/malformed: %d\n",
> -                     pdata->reset);
> +     pdata->reset = devm_gpiod_get_from_of_node(arizona->dev,
> +                                                arizona->dev->of_node,
> +                                                "wlf,reset", 0,
> +                                                GPIOD_OUT_LOW,
> +                                                "arizona /RESET");
> +     if (IS_ERR(pdata->reset)) {
> +             ret = PTR_ERR(pdata->reset);
> +             switch (ret) {
> +             case -ENOENT:
> +             case -ENOSYS:
> +                     break;
> +             case -EPROBE_DEFER:
> +                     return ret;
> +             default:
> +                     dev_err(arizona->dev, "Reset GPIO malformed: %d\n",
> +                             ret);
> +                     break;
> +             }

I haven't seen a switch statement used in the error handling path
before.  There is probably good reason for that.

What is the 'default' case?  -EINVAL?

> -             pdata->reset = 0;
> +             pdata->reset = NULL;
>       }
>  
>       ret = of_property_read_u32_array(arizona->dev->of_node,
> @@ -1050,14 +1062,20 @@ int arizona_dev_init(struct arizona *arizona)
>               goto err_early;
>       }
>  
> -     if (arizona->pdata.reset) {
> +     if (!arizona->pdata.reset) {
>               /* Start out with /RESET low to put the chip into reset */
> -             ret = devm_gpio_request_one(arizona->dev, arizona->pdata.reset,
> -                                         GPIOF_DIR_OUT | GPIOF_INIT_LOW,
> -                                         "arizona /RESET");
> -             if (ret != 0) {
> -                     dev_err(dev, "Failed to request /RESET: %d\n", ret);
> -                     goto err_dcvdd;
> +             arizona->pdata.reset = devm_gpiod_get(arizona->dev, "reset",
> +                                                   GPIOD_OUT_LOW);
> +             if (IS_ERR(arizona->pdata.reset)) {
> +                     ret = PTR_ERR(arizona->pdata.reset);
> +                     if (ret == -EPROBE_DEFER)
> +                             goto err_dcvdd;
> +                     else
> +                             dev_err(arizona->dev,
> +                                     "Reset GPIO missing/malformed: %d\n",
> +                                     ret);

You don't need the else.

        arizona->pdata.reset =
                devm_gpiod_get(arizona->dev, "reset", GPIOD_OUT_LOW);

        ret = PTR_ERR(arizona->pdata.reset);
        if (ret == -EPROBE_DEFER)
                goto err_dcvdd;

        if (ret) {
                dev_err(arizona->dev,
                        "Reset GPIO missing/malformed: %d\n", ret);
                arizona->pdata.reset = NULL;
        }

> +                     arizona->pdata.reset = NULL;
>               }
>       }
>  
> diff --git a/include/linux/mfd/arizona/pdata.h 
> b/include/linux/mfd/arizona/pdata.h
> index f72dc53848d7..0013075d4cda 100644
> --- a/include/linux/mfd/arizona/pdata.h
> +++ b/include/linux/mfd/arizona/pdata.h
> @@ -56,6 +56,7 @@
>  #define ARIZONA_MAX_PDM_SPK 2
>  
>  struct regulator_init_data;
> +struct gpio_desc;
>  
>  struct arizona_micbias {
>       int mV;                    /** Regulated voltage */
> @@ -77,7 +78,7 @@ struct arizona_micd_range {
>  };
>  
>  struct arizona_pdata {
> -     int reset;      /** GPIO controlling /RESET, if any */
> +     struct gpio_desc *reset;      /** GPIO controlling /RESET, if any */
>  
>       /** Regulator configuration for MICVDD */
>       struct arizona_micsupp_pdata micvdd;

I know it doesn't have much to do with this patch, but it's probably
better to use a Kerneldoc header for this struct.

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Reply via email to