Re: [PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Marek Vasut <marek.vasut@gmail.com>]

On 04/18/2017 03:59 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi Geert,

> On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Add bindings for the GyroADC block and it's associated clock.
> 
> bindings??

That's fixed...

>> --- a/arch/arm/boot/dts/r8a7791.dtsi
>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
>> @@ -776,6 +776,15 @@
>>                 status = "disabled";
>>         };
>>
>> +       adc: adc@e6e54000 {
>> +               compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
>> +               reg = <0 0xe6e54000 0 64>;
>> +               clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&adc_clk>;
>> +               clock-names = "fck", "if";
>> +               power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
>> +               status = "disabled";
>> +       };
>> +
>>         scif2: serial@e6e58000 {
>>                 compatible = "renesas,scif-r8a7791", "renesas,rcar-gen2-scif",
>>                              "renesas,scif";
>> @@ -1133,6 +1142,13 @@
>>                         clock-frequency = <0>;
>>                 };
>>
>> +               /* GyroADC clock */
>> +               adc_clk: adc_clk {
>> +                       compatible = "fixed-clock";
>> +                       #clock-cells = <0>;
>> +                       clock-frequency = <65000000>;
>> +               };
> 
> Why do you have to add a clock?
> I think you should just refer to the on-SoC peripheral clock: &cp_clk.

I think you are right, in fact, see below ...

>> +
>>                 /* Special CPG clocks */
>>                 cpg_clocks: cpg_clocks@e6150000 {
>>                         compatible = "renesas,r8a7791-cpg-clocks",
>> @@ -1432,6 +1448,7 @@
>>                                  <&hp_clk>, <&hp_clk>;
> 
> Missing addition of the parent clock for the newly added module clock.
> Perhaps this should be the peripheral clock (<&cp_clk>)?
> 
> Oops, that means there's no need to have two clock inputs in the adc device
> node, and thus we screwed up when reviewing the GyroADC bindings :-(

I think you're right. I should be just getting the clk rate of the fck
and derive the gyroadc timings from that, correct ? I can send a patch
for the driver to just ignore the second clock entry and update the DT
binding document to drop the "if" clock (?) .

>>                         #clock-cells = <1>;
>>                         clock-indices = <
>> +                               R8A7791_CLK_GYROADC
>>                                 R8A7791_CLK_GPIO7 R8A7791_CLK_GPIO6 R8A7791_CLK_GPIO5 R8A7791_CLK_GPIO4
>>                                 R8A7791_CLK_GPIO3 R8A7791_CLK_GPIO2 R8A7791_CLK_GPIO1 R8A7791_CLK_GPIO0
>>                                 R8A7791_CLK_RCAN1 R8A7791_CLK_RCAN0 R8A7791_CLK_QSPI_MOD R8A7791_CLK_I2C5

[...]

-- 
Best regards,
Marek Vasut