Re: [PATCH v0 12/15] ARM: STi: DT: STiH416: 416 DT Entry for clockgen B/C/D/E/F

[Gabriel Fernandez <gabriel.fernandez@st.com>]

[-- Attachment #1: Type: text/plain, Size: 3641 bytes --]

Hi Lee,

On 02/27/2014 05:36 PM, Lee Jones wrote:
>> Patch adds DT entries for clockgen B/C/D/E/F
>>
>> Signed-off-by: Pankaj Dev <pankaj.dev@st.com>
> You need to add your Signed-off-by too.
ok
>
>> ---
>>   arch/arm/boot/dts/stih416-clock.dtsi | 170 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 170 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stih416-clock.dtsi b/arch/arm/boot/dts/stih416-clock.dtsi
>> index f63b0a1..6b2e387 100644
>> --- a/arch/arm/boot/dts/stih416-clock.dtsi
>> +++ b/arch/arm/boot/dts/stih416-clock.dtsi
>> @@ -503,5 +503,175 @@
>>   						/* Remaining outputs unused */
>>   			};
>>   		};
> This doesn't look right. Have you indented one tab too far?
ok i have reworked tabulation on this patch
>
>> +                /*
>> +                 * Frequency synthesizers on the SASG2.
>> +                 *
>> +                 */
> Too many *'s
ok
>
> <snip>
>
>> +                CLK_S_VCC_HD: CLK_S_VCC_HD {
>> +                        #clock-cells = <0>;
>> +                        compatible = "st,stih416-clkgenc-vcc-hd", "st,clkgen-mux";
>> +                        reg = <0xfe8308b8 4>; /* SYSCFG2558 */
> 0x4
ok
>
>> +                /*
>> +                 * Add a dummy clock for the HDMI PHY for the VCC input mux
>> +                 */
>> +                CLK_S_TMDS_FROMPHY: CLK_S_TMDS_FROMPHY {
>> +                        #clock-cells = <0>;
>> +                        compatible = "fixed-clock";
>> +                        clock-frequency = <0>;
> What happens when the clock frequency is 0?
Nothing, this poses no problem.
This input clock is not managed by the driver.

>> +                };
>> +
>> +                CLOCKGEN_C_VCC: CLOCKGEN_C_VCC {
>> +                        #clock-cells = <1>;
>> +                        compatible = "st,stih416-clkgenc", "st,clkgen-vcc";
>> +                        reg = <0xfe8308ac 12>; /* SYSCFG2555,2556,2557 */
> 0x12, or 0x0C, whichever is appropriate.
ok
>
>> +                        clocks = <&CLK_S_VCC_HD>, <&CLOCKGEN_C 1>,
>> +                                 <&CLK_S_TMDS_FROMPHY>, <&CLOCKGEN_C 2>;
> One per line would probably be better, save confusing them for pairs.
ok
>
> <snip>
>
>> +		/*
>> +		 * Frequency synthesizers on the MPE42
>> +		 */
> Alignment.

ok i transform into 1 line : /* Frequency synthesizers on the MPE42*/

>
> <snip>
>
>> +                CLOCKGEN_F: CLOCKGEN_F {
>> +                        #clock-cells = <1>;
>> +                        compatible = "st,stih416-quadfs660-F", "st,quadfs";
>> +                        reg = <0xfd320878 0xF0>;
>> +
>> +                        clocks = <&CLK_SYSIN>;
>> +                        clock-output-names = "CLK_M_MAIN_VIDFS",
>> +                                             "CLK_M_HVA_FS",
>> +                                             "CLK_M_FVDP_VCPU",
>> +					     "CLK_M_FVDP_PROC_FS";
> Tabbing. Ensure you're using tabs (and not spaces) everywhere.
ok
>
> <snip>
>
>> +                        reg = <0xfd320910 4>; /* SYSCFG8580 */
> 0x...
>
> Do this for all of the below too.
ok
>
>
> <snip>
>
>> +                        clock-output-names  =
>> +                                "CLK_M_PIX_MAIN_PIPE",  "CLK_M_PIX_AUX_PIPE",
>> +                                "CLK_M_PIX_MAIN_CRU",   "CLK_M_PIX_AUX_CRU",
>> +                                "CLK_M_XFER_BE_COMPO",  "CLK_M_XFER_PIP_COMPO",
>> +                                "CLK_M_XFER_AUX_COMPO", "CLK_M_VSENS",
>> +                                "CLK_M_PIX_HDMIRX_0",   "CLK_M_PIX_HDMIRX_1";
>> +                };
>>   	};
>>   };
ok
> Something strange going on with these.
>


[-- Attachment #2: Type: text/html, Size: 7211 bytes --]