Re: [PATCH 2/2] ARM: EXYNOS5: Add gscalar device from DT

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Shaik,

On 07/11/2012 03:06 PM, AMEER BASHA SHAIK wrote:
>>> diff --git a/arch/arm/boot/dts/exynos5250.dtsi
>>> b/arch/arm/boot/dts/exynos5250.dtsi
>>> index 4272b29..b945c00 100644
>>> --- a/arch/arm/boot/dts/exynos5250.dtsi
>>> +++ b/arch/arm/boot/dts/exynos5250.dtsi
>>> @@ -23,6 +23,13 @@
>>>          compatible = "samsung,exynos5250";
>>>          interrupt-parent =<&gic>;
>>>
>>> +       aliases {
>>> +               gsc0 =&gsc_0;
>>> +               gsc1 =&gsc_1;
>>> +               gsc2 =&gsc_2;
>>> +               gsc3 =&gsc_3;
>>> +       };
>>> +
>>
>>
>> What are these aliases useful for ?
> 
> GScaler driver uses the "of_alias_get_id()" call to retrieve the device IDs, which
> internally uses these aliases...

Yeah, just figured it out already... I didn't know then it was a preferred
way to represent device IDs in DT. I looked at the driver's code and it
seemed slightly incorrect to me. Let me comment on the relevant patch shortly.

Would be good to have things like this documented in the GScaler bindings
documentation. These aliases seem quite essential for the driver to work.

>> Also I think all DT related patches should be posted to
>> devicetree-discuss@lists.ozlabs.org as well.
>>
> 
> I will post the v2 version of this patch to the specified mailing list.
> 
>>
>>>          gic:interrupt-controller@10481000 {
>>>                  compatible = "arm,cortex-a9-gic";
>>>                  #interrupt-cells =<3>;
>>> @@ -424,4 +431,28 @@
>>>                          #gpio-cells =<4>;
>>>                  };
>>>          };
>>> +
>>> +       gsc_0:  gsc@0x13e00000 {
>>> +               compatible = "samsung,exynos-gsc";
>>> +               reg =<0x13e00000 0x1000>;
>>> +               interrupts =<0 85 0>;
>>> +       };
>>> +
>>> +       gsc_1:  gsc@0x13e10000 {
>>> +               compatible = "samsung,exynos-gsc";
>>> +               reg =<0x13e10000 0x1000>;
>>> +               interrupts =<0 86 0>;
>>> +       };
>>> +
>>> +       gsc_2:  gsc@0x13e20000 {
>>> +               compatible = "samsung,exynos-gsc";
>>> +               reg =<0x13e20000 0x1000>;
>>> +               interrupts =<0 87 0>;
>>> +       };
>>> +
>>> +       gsc_3:  gsc@0x13e30000 {
>>> +               compatible = "samsung,exynos-gsc";
>>> +               reg =<0x13e30000 0x1000>;
>>> +               interrupts =<0 88 0>;
>>> +       };
>>>    };
>>> diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c
>>> b/arch/arm/mach-exynos/mach-exynos5-dt.c
>>> index 7b1e11a..76b081c 100644
>>> --- a/arch/arm/mach-exynos/mach-exynos5-dt.c
>>> +++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
>>> @@ -50,6 +50,14 @@ static const struct of_dev_auxdata
>>> exynos5250_auxdata_lookup[] __initconst = {
>>>          OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA0, "dma-pl330.0",
>>> NULL),
>>>          OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA1, "dma-pl330.1",
>>> NULL),
>>>          OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_MDMA1, "dma-pl330.2",
>>> NULL),
>>> +       OF_DEV_AUXDATA("samsung,exynos-gsc", 0x13E00000,
>>> +                               "exynos-gsc.0", NULL),
>>> +       OF_DEV_AUXDATA("samsung,exynos-gsc", 0x13E10000,
>>> +                               "exynos-gsc.1", NULL),
>>> +       OF_DEV_AUXDATA("samsung,exynos-gsc", 0x13E20000,
>>> +                               "exynos-gsc.2", NULL),
>>> +       OF_DEV_AUXDATA("samsung,exynos-gsc", 0x13E30000,
>>> +                               "exynos-gsc.3", NULL),
>>
>>
>> It's probably better to add relevant entry at
>> arch/arm/mach-exynos/include/mach/map. It's just a one line, e.g.
>>
>> /* x = 0...3 */
>> #define EXYNOS5_PA_GSC(x)       (0x13e00000 + ((x) * 0x10000))
>>
>> And use it here instead of plain numbers.
> 
> OK. will address these comments in v2.

Thanks.

--
Regards,
Sylwester