Re: [PATCH] gpio: samsung: add devicetree init for s3c24xx arches

["Heiko Stübner" <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>]

Hi Thomas,

thanks for your review:

Am Montag, 27. August 2012, 06:20:49 schrieb Thomas Abraham:
> Hi Heiko,
> 
> On 26 August 2012 03:23, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> > Until now the Exynos-SoC was the only Samsung-SoC supporting the GPIOs
> > via the device tree. This patch implements dt-support for the
> > s3c24xx arches.
> > 
> > The controllers contain only 3 cells, as the underlying gpio controller
> > does not support controlling the drive strength on a gpio level.
> > 
> > Tested with the gpio-keys driver on a s3c2416 based machine.
> > 
> > Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> > ---
> > 
> >  .../devicetree/bindings/gpio/gpio-samsung.txt      |   38 ++++++++++++
> >  drivers/gpio/gpio-samsung.c                        |   63
> >  ++++++++++++++++++++ 2 files changed, 101 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> > b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt index
> > 5375625..ce6a7d4 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-samsung.txt
> > 
> > @@ -39,3 +39,41 @@ Example:
> >                 #gpio-cells = <4>;
> >                 gpio-controller;
> >         
> >         };
> > 
> > +
> > +
> > +Samsung S3C24XX GPIO Controller
> > +
> > +Required properties:
> > +- compatible: Compatible property value should be
> > "samsung,s3c24xx-gpio".
> 
> This is debatable, but I would choose to be specific here and use
> "samsing,s3c2416-gpio". For instance, SoC's in s3c24xx family have
> differing value for pull none/down/up.

Hmmm, but the logic to drive the gpio controller is the same for all arches. 
The only difference is the number to use for the different pull up/down 
settings. So I think the "samsung,s3c24xx-gpio" is ok but the documentation 
should simply reflect the different pull settings.


> > +
> > +- reg: Physical base address of the controller and length of memory
> > mapped +  region.
> > +
> > +- #gpio-cells: Should be 3. The syntax of the gpio specifier used by
> > client nodes +  should be the following with values derived from the SoC
> > user manual. +     <[phandle of the gpio controller node]
> > +      [pin number within the gpio controller]
> > +      [mux function]
> > +      [flags and pull up/down]
> > +
> > +  Values for gpio specifier:
> > +  - Pin number: depending on the controller a number from 0 up to 15.
> > +  - Flags and Pull Up/Down: 0 - Pull Up/Down Disabled.
> > +                            1 - Pull Down Enabled.
> > +                            3 - Pull Up Enabled.
> 
> As per s3c2416 user manual, 2 is used for Pull Up and 3 is listed as
> reserved value. Any particular reason to use 3 here for pull up and
> not 2 as per the user manual?

Sorry, copy and paste error :-)


> > +          Bit 16 (0x00010000) - Input is active low.
> > +
> > +- gpio-controller: Specifies that the node is a gpio controller.
> > +- #address-cells: should be 1.
> > +- #size-cells: should be 1.
> 
> It would be informative to add information about the 'mux function'
> cell here as well. Specifically, on how to handle the banks that have
> an extended GPxSEL register that allow additional pin function
> selection.

Will add a mux function description.

Until now I've never realised the existence of the GPxSEL registers, so thanks 
for the pointer :-).

But is this used in the driver at all? All the setcfg/getcfg functions in 
gpio-samsung.c only handle the GPxCON registers - I haven't found code to 
handle the GPxSEL registers at all. So my guess is that this was never 
implemented - or that I'm blind ;-) .


> > +
> > +Example:
> > +
> > +       gpa: gpio-controller@56000000 {
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               compatible = "samsung,s3c24xx-gpio";
> > +               reg = <0x56000000 0x10>;
> > +               #gpio-cells = <3>;
> > +               gpio-controller;
> > +       };
> > diff --git a/drivers/gpio/gpio-samsung.c b/drivers/gpio/gpio-samsung.c
> > index a150d2e..80a2817 100644
> > --- a/drivers/gpio/gpio-samsung.c
> > +++ b/drivers/gpio/gpio-samsung.c
> > @@ -938,6 +938,67 @@ static void __init samsung_gpiolib_add(struct
> > samsung_gpio_chip *chip)
> > 
> >                 s3c_gpiolib_track(chip);
> >  
> >  }
> > 
> > +#if defined(CONFIG_PLAT_S3C24XX) && defined(CONFIG_OF)
> > +static int s3c24xx_gpio_xlate(struct gpio_chip *gc,
> > +                       const struct of_phandle_args *gpiospec, u32
> > *flags) +{
> > +       unsigned int pin;
> > +
> > +       if (WARN_ON(gc->of_gpio_n_cells < 3))
> > +               return -EINVAL;
> > +
> > +       if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> > +               return -EINVAL;
> > +
> > +       if (gpiospec->args[0] > gc->ngpio)
> > +               return -EINVAL;
> > +
> > +       pin = gc->base + gpiospec->args[0];
> > +
> > +       if (s3c_gpio_cfgpin(pin, S3C_GPIO_SFN(gpiospec->args[1])))
> > +               pr_warn("gpio_xlate: failed to set pin function\n");
> > +       if (s3c_gpio_setpull(pin, gpiospec->args[2] & 0xffff))
> > +               pr_warn("gpio_xlate: failed to set pin pull up/down\n");
> > +
> > +       if (flags)
> > +               *flags = gpiospec->args[2] >> 16;
> > +
> > +       return gpiospec->args[0];
> > +}
> > +
> > +static const struct of_device_id s3c24xx_gpio_dt_match[] __initdata = {
> > +       { .compatible = "samsung,s3c24xx-gpio", },
> > +       {}
> > +};
> > +
> > +static __init void s3c24xx_gpiolib_attach_ofnode(struct
> > samsung_gpio_chip *chip, +                                              
> >  u64 base, u64 offset) +{
> > +       struct gpio_chip *gc =  &chip->chip;
> > +       u64 address;
> > +
> > +       if (!of_have_populated_dt())
> > +               return;
> > +
> > +       address = chip->base ? base + ((u32)chip->base & 0xfff) : base +
> > offset; +       gc->of_node = of_find_matching_node_by_address(NULL,
> > +                       s3c24xx_gpio_dt_match, address);
> > +       if (!gc->of_node) {
> > +               pr_info("gpio: device tree node not found for gpio
> > controller" +                       " with base address %08llx\n",
> > address); +               return;
> > +       }
> > +       gc->of_gpio_n_cells = 3;
> > +       gc->of_xlate = s3c24xx_gpio_xlate;
> > +}
> > +#elif defined(CONFIG_PLAT_S3C24XX)
> > +static __init void s3c24xx_gpiolib_attach_ofnode(struct
> > samsung_gpio_chip *chip, +                                              
> >  u64 base, u64 offset) +{
> > +       return;
> > +}
> > +#endif /* defined(CONFIG_PLAT_S3C24XX) && defined(CONFIG_OF) */
> > +
> > 
> >  static void __init s3c24xx_gpiolib_add_chips(struct samsung_gpio_chip
> >  *chip,
> >  
> >                                              int nr_chips, void __iomem
> >                                              *base)
> >  
> >  {
> > 
> > @@ -962,6 +1023,8 @@ static void __init s3c24xx_gpiolib_add_chips(struct
> > samsung_gpio_chip *chip,
> > 
> >                         gc->direction_output =
> >                         samsung_gpiolib_2bit_output;
> >                 
> >                 samsung_gpiolib_add(chip);
> > 
> > +
> > +               s3c24xx_gpiolib_attach_ofnode(chip, S3C24XX_PA_GPIO, i *
> > 0x10);
> > 
> >         }
> >  
> >  }
> > 
> > --
> > 1.7.2.3
> 
> Overall, this is patch looks fine.
> 
> Reviewed-by: Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>


Heiko