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

[Ramesh Shanmugasundaram]

Hi Laurent,

> On Tuesday 11 Apr 2017 09:57:45 Ramesh Shanmugasundaram wrote:
> > > 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
> > >>  ---
> > >>
> > >>  .../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 000..f591ab4
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/media/i2c/max2175.txt
> 
> [snip]
> 
> > >> +- 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 ?
> >
> > This is dictated by the h/w design and not selectable at run-time.
> > I will update these changes in the next patchset.
> 
> In that case I'm fine with a property, but could we name it in such a way
> that it describes the hardware instead of instructing the software on how
> to configure the device ? For instance (and this is a made-up example as I
> don't know exactly how this works), if the AM Hi-Z filter is required when
> dealing with AM frequencies and forbidden when dealing with other
> frequency bands, and
> *if* boards have to be designed specifically for one frequency band (AM,
> FM, VHF, L, ...) without any way to accept different bands, then you could
> instead use
> 
>   maxim,frequency-band = "AM";
> 
> and enable the filter accordingly in the driver. This would be in my
> opinion a better system hardware description.

I am not sure. The AM antenna input path has a default filter and AM Hi-Z 
filter. H/W dictates the path to be used for AM input only and this is fixed. 
The device can be configured to use different bands at runtime & not AM only. I 
could edit the description as below:

- maxim,am-hiz: empty property indicates AM Hi-Z filter path usage for 
AM antenna
input as dictated by hardware design. By default this 
filter path is not used.

Is it any better? Do you still think the property name should be changed please?

Thanks,
Ramesh

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

[Laurent Pinchart]

Hi Ramesh,

On Tuesday 11 Apr 2017 09:57:45 Ramesh Shanmugasundaram wrote:
> > 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
> >>  ---
> >> 
> >>  .../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 000..f591ab4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/max2175.txt

[snip]

> >> +- 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 ?
> 
> This is dictated by the h/w design and not selectable at run-time.
> I will update these changes in the next patchset.

In that case I'm fine with a property, but could we name it in such a way that 
it describes the hardware instead of instructing the software on how to 
configure the device ? For instance (and this is a made-up example as I don't 
know exactly how this works), if the AM Hi-Z filter is required when dealing 
with AM frequencies and forbidden when dealing with other frequency bands, and 
*if* boards have to be designed specifically for one frequency band (AM, FM, 
VHF, L, ...) without any way to accept different bands, then you could instead 
use

maxim,frequency-band = "AM";

and enable the filter accordingly in the driver. This would be in my opinion a 
better system hardware description.

-- 
Regards,

Laurent Pinchart

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

[Ramesh Shanmugasundaram]

Hi Laurent,

Thanks for the review comments.

> 
> 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
> >  ---
> >  .../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 000..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(r) 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".

Agreed.

> 
> > +- 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.

Agreed.

> 
> > +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" ?

Agreed.

> 
> > +- 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 ?

This is dictated by the h/w design and not selectable at run-time. 
I will update these changes in the next patchset.

Thanks,
Ramesh

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

[Laurent Pinchart]

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
>  ---
>  .../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 000..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 = <_xtal>;
> + clock-names = "xtal";
> + maxim,refout-load-pF = <10>;
> +
> + port {
> + max2175_0_ep: endpoint {
> + remote-endpoint = <_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

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

[Rob Herring]

On Tue, Feb 07, 2017 at 03:02:32PM +, Ramesh Shanmugasundaram wrote:
> Add device tree binding documentation for MAX2175 Rf to bits tuner
> device.
> 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> ---
>  .../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

Acked-by: Rob Herring