Re: [PATCH 1/2] media: rc: meson-irblaster: document device tree bindings

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

Hi Viktor,

On Thu, Jul 1, 2021 at 11:51 PM Viktor Prutyanov
<viktor.prutyanov@phystech.edu> wrote:
>
> This patch adds binding documentation for the IR transmitter
> available in Amlogic Meson SoCs.
This is an interesting piece of hardware where I've always wondered if
there is any device out there which supports this functionality.It
turns out that there is

[...]
> +description: |
> +  Some Amlogic SoCs such as A311D and T950D4 have IR transmitter
> +  (blaster) controller onboard. It is capable of sending IR signals
> +  with arbitrary carrier frequency and duty cycle.
> +
> +properties:
> +  compatible:
> +    const: amlogic,meson-irblaster
if you feel like some registers or register values are specific to
A311D or T950D4 then please also add a SoC-specific compatible string
(for example: amlogic,meson-g12b-irblaster).
An example can be seen in
Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.yaml

[...]
> +  clocks:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 2
from my understanding there are two clock inputs to the hardware
dt-bindings should always describe the hardware, not what the driver
may (or may not) use.
based on that I think you should drop minItems (then minItems will
have the same value as maxItems)

[...]
> +  mod-clock:
> +    oneOf:
> +      - const: sysclk
> +      - const: xtal
Does this "mod-clock" depend on something external to the IR blaster hardware?
If not this should be handled inside the driver only.

From how I understand the register description in the datasheet
there's two clock inputs.
XTAL is internally divided further down with fixed dividers.
Then there's a configurable divider which is then used to generate the
IR signal.
If the sysclk (I assume that this is clk81 - or at least derived from
it) is "too fast" then the driver should just ignore that clock while
the dt-bindings should still describe it (see my comment above)

[...]
> +    meson-irblaster@ff80014c {
node names should be generic, see for example
Documentation/devicetree/bindings/spi/amlogic,meson6-spifc.yaml
(spifc is the name Amlogic has given this IP, but since node names are
supposed to be generic we use spi@...)

However, I am not sure if an IR blaster would be described as
ir-blaster@... or simply ir@...

> +      compatible = "amlogic,meson-irblaster";
> +      reg = <0xff80014c 0x10>;
> +      interrupts = <0 198 IRQ_TYPE_EDGE_RISING>;
> +      clocks = <&clkc CLKID_CLK81 &xtal>;
[...]
> +      clocks = <&clkc CLKID_CLK81 &xtal>;
while this works I think the recommended format is:
    clocks = <&clkc CLKID_CLK81>, <&xtal>


Best regards,
Martin