Hi Alex, On Mon, Mar 2, 2020 at 4:07 PM Alex Riesen <alexander.rie...@cetitec.com> wrote: > Geert Uytterhoeven, Mon, Mar 02, 2020 14:47:46 +0100: > > On Mon, Mar 2, 2020 at 2:40 PM Alex Riesen <alexander.rie...@cetitec.com> > > wrote: > > > > > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi > > > > > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi > > > > > @@ -322,6 +322,10 @@ > > > > > clock-frequency = <22579200>; > > > > > }; > > > > > > > > > > +&audio_clk_c { > > > > > + clock-frequency = <12288000>; > > > > > +}; > > > > > > > > Does the ADV7482 always generate a 12.288 MHz clock signal? > > > > Or is this programmable? > > > > > > Oops. It looks like it is and the value is derived from the sampling rate > > > (48kHz) and the master clock multiplier. Both hard-coded in the board > > > file. > > > > Where are these hardcoded in the board file? > > In the endpoint definition, > arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts > > So the frequency can be set at the run-time, perhaps even derived from > endpoint connected to the output. In this case, rsnd_endpoint3, > which has the "mclk-fs" setting. Not sure if the sampling rate > can be set to something else for the HDMI, though. > > > Even if they are, technically this is a clock output of the ADV7482. > > ... which I hope to correct as soon as I steal the hardware from whoever stole > it from me... > > > > > > video-receiver@70 { > > > > > compatible = "adi,adv7482"; > > > > > ... > > > > > + clocks = <&rcar_sound 3>, <&audio_clk_c>; > > > > > + clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk"; > > > > > > > > The above declares the Audio CLK C to be a clock input of the ADV7482, > > > > while > > > > it is an output. > > > > > > I would gladly give it right direction if I *really* understood what I was > > > doing... > > > > :-) > > > > > > Furthermore, the DT bindings do not document that clocks can be > > > > specified. > > > > > > Should the DT bindings document that the clock cannot be specified than? > > > > It currently does say so, as it doesn't list "clocks" in its properties > > section. > > The bindings documentation file, which we're talking about here and which does > not list the specifiable input clocks in its properties, is it the > > Documentation/devicetree/bindings/media/i2c/adv748x.txt > > ?
Yes. > > And this absence of documentation also means that whatever clocks (both input > in "clocks=" and output in "#clock-cells") listed in a specific .dts are just > an integration detail? No, the absence probably means that any clock-related properties in a .dts file will just be ignored. Looking at the driver source, it indeed has no support related to clocks at all. > Does this below makes more sense, than? > > video-receiver@70 { > compatible = "adi,adv7482"; > clocks = <&rcar_sound 3>; > clock-names = "clk-hdmi-video"; > adv748x_mclk: mclk { > compatible = "fixed-clock"; > #clock-cells = <0>; > /* frequency hard-coded for illustration */ > clock-frequency = <12288000>; > clock-output-names = "clk-hdmi-i2s-mclk"; > }; > }; The #clock-cells should be in the main video-receiver node. Probably there is more than one clock output, so #clock-cells may be 1? There is no need for a fixed-clock compatible, nor for clock-frequency and clock-output-names. But most important: this should be documented in the adv748x DT bindings, and implemented in the adv748x driver. > Now I'm a bit hazy on how to declare that the MCLK output of the > video-receiver@70 is connected to the Audio Clock C of the SoC... > Probably remove use of "audio_clk_c" completely? Yes, the current audio_clk_c definition in the DTS assumes a fixed crystal. > > > > > @@ -686,7 +700,8 @@ > > > > > }; > > > > > > > > > > sound_pins: sound { > > > > > - groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a"; > > > > > + groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a", > > > > > + "ssi4_data"; > > > > > > > > Missing "ss4_ctrl", for the SCK4 and WS4 pins. > > > > > > I'll add them. > > > As the device seems to function even without thoes, does this mean the > > > pins in the group are used "on demand" by whatever needs them? > > > > Probably the SCK4/WS4 functions are the reset-state defaults. > > That ... might require some trial and testing: when I add them to the group, > the reset defaults will be overridden by the platform initialization, which is > not necessarily the reset default. Will see. Or by the boot loader. Anyway, you need to specify these in the DTS. > > > Does a "clocks = ..." statement always mean input clocks? > > > > Yes it does. > > If a device has clock outputs and is thus a clock provider, it should > > have a #clock-cells property, and this should be documented in the bindings. > > > > A clock consumer will refer to clocks of a provider using the "clocks" > > property, specifying a clock specifier (phandle and zero or more indices) > > for each clock referenced. > > Something like this? > > &rcar_sound { > clocks = ..., > <&adv748x_mclk>, > <&cpg CPG_CORE CPG_AUDIO_CLK_I>; > clock-names = ..., > "clk_c", > "clk_i"; > }; More or less. Might become find_a_better_label_choice: video-receiver@70 { ... }; &rcar_sound { clock = ..., <&find_a_better_label_choice 0>, ... }; as there may be multiple clock outputs on the ADV7482. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel