Re: [PATCH v3 2/7] dt-bindings: media: Add MAX2175 binding description

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ramesh,

Thank you for the patch.

On Tuesday 07 Feb 2017 15:02:32 Ramesh Shanmugasundaram wrote:
> Add device tree binding documentation for MAX2175 Rf to bits tuner
> device.
> 
> Signed-off-by: Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@bp.renesas.com> ---
>  .../devicetree/bindings/media/i2c/max2175.txt      | 61 +++++++++++++++++++
>  .../devicetree/bindings/property-units.txt         |  1 +
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/max2175.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/max2175.txt
> b/Documentation/devicetree/bindings/media/i2c/max2175.txt new file mode
> 100644
> index 0000000..f591ab4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/max2175.txt
> @@ -0,0 +1,61 @@
> +Maxim Integrated MAX2175 RF to Bits tuner
> +-----------------------------------------
> +
> +The MAX2175 IC is an advanced analog/digital hybrid-radio receiver with
> +RF to Bits® front-end designed for software-defined radio solutions.
> +
> +Required properties:
> +--------------------
> +- compatible: "maxim,max2175" for MAX2175 RF-to-bits tuner.
> +- clocks: phandle to the fixed xtal clock.
> +- clock-names: name of the fixed xtal clock.

I would mention that the name has to be "xtal". Maybe something like

- clock-names: name of the fixed xtal clock, shall be "xtal".

> +- port: child port node of a tuner that defines the local and remote
> +  endpoints. The remote endpoint is assumed to be an SDR device
> +  that is capable of receiving the digital samples from the tuner.

You should refer to the OF graphs bindings here. How about the following to 
document the port node ?

- port: child port node corresponding to the I2S output, in accordance with 
the video interface bindings defined in
Documentation/devicetree/bindings/media/video-interfaces.txt. The port node 
must contain at least one endpoint.

> +Optional properties:
> +--------------------
> +- maxim,slave	      : phandle to the master tuner if it is a slave. 
This
> +			is used to define two tuners in diversity mode
> +			(1 master, 1 slave). By default each tuner is an
> +			individual master.

It seems weird to me to name a property "slave" when it points to the master 
tuner. Shouldn't it be named "maxim,master" ?

> +- maxim,refout-load-pF: load capacitance value (in pF) on reference
> +			output drive level. The possible load values are
> +			 0 (default - refout disabled)
> +			10
> +			20
> +			30
> +			40
> +			60
> +			70
> +- maxim,am-hiz	      : empty property indicates AM Hi-Z filter path 
is
> +			selected for AM antenna input. By default this
> +			filter path is not used.

Isn't this something that should be selected at runtime through a control ? Or 
does the hardware design dictate whether the filter has to be used or must not 
be used ?

> +Example:
> +--------
> +
> +Board specific DTS file
> +
> +/* Fixed XTAL clock node */
> +maxim_xtal: clock {
> +	compatible = "fixed-clock";
> +	#clock-cells = <0>;
> +	clock-frequency = <36864000>;
> +};
> +
> +/* A tuner device instance under i2c bus */
> +max2175_0: tuner@60 {
> +	compatible = "maxim,max2175";
> +	reg = <0x60>;
> +	clocks = <&maxim_xtal>;
> +	clock-names = "xtal";
> +	maxim,refout-load-pF = <10>;
> +
> +	port {
> +		max2175_0_ep: endpoint {
> +			remote-endpoint = <&slave_rx_device>;
> +		};
> +	};
> +
> +};
> diff --git a/Documentation/devicetree/bindings/property-units.txt
> b/Documentation/devicetree/bindings/property-units.txt index
> 12278d7..f1f1c22 100644
> --- a/Documentation/devicetree/bindings/property-units.txt
> +++ b/Documentation/devicetree/bindings/property-units.txt
> @@ -28,6 +28,7 @@ Electricity
>  -ohms		: Ohms
>  -micro-ohms	: micro Ohms
>  -microvolt	: micro volts
> +-pF		: pico farads
> 
>  Temperature
>  ----------------------------------------

-- 
Regards,

Laurent Pinchart