On 27 August 2012 13:31, Heiko Stübner <heiko@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> >
>> > .../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.
Ok.
>
>
>> > +
>> > +- 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 :-)
Ok.
>
>
>> > + 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 ;-) .
You are right, there is no code that is handling the GPxCON registers.
Thanks,
Thomas.
>
>
>> > +
>> > +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@xxxxxxxxxx>
>
>
> Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
