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

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

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.

-- 
Regards,

Laurent Pinchart