| Message ID | 1486479757-32128-3-git-send-email-ramesh.shanmugasundaram@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Tue, Feb 07, 2017 at 03:02:32PM +0000, 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 Acked-by: Rob Herring <robh@kernel.org>
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 > ----------------------------------------
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 > > <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(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
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 > >> <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 [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.
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 > > >> <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 > > [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
Hi Ramesh, On Tuesday 11 Apr 2017 12:19:54 Ramesh Shanmugasundaram wrote: > > 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 > >>>> <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 > > > > [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? I still think this should be renamed, but possibly because I don't understand all the details of this particular feature :-). The property, as named and documented above, describes a software features. It requests the driver to enable the AM Hi-Z filter. DT properties should instead describe the hardware. You should use a property that describes the hardware design, and use that to infer, in the driver, whether to enable or disable the filter.
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. +- 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. + +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. +- 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. + +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 ----------------------------------------
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