Re: [PATCH v2 4/6] v4l: vsp1: Add DT support

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

Hi Sylwester,

On Wednesday 02 April 2014 23:37:48 Sylwester Nawrocki wrote:
> Hi Laurent,
> 
> I've got just couple minor comments...

Thank you for your comments.

> On 04/02/2014 08:01 PM, Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  .../devicetree/bindings/media/renesas,vsp1.txt     | 43 +++++++++++++++++
> >  drivers/media/platform/vsp1/vsp1_drv.c             | 52 +++++++++++++----
> >  2 files changed, 87 insertions(+), 8 deletions(-)
> >  create mode 100644
> >  Documentation/devicetree/bindings/media/renesas,vsp1.txt
> > 
> > Hi,
> > 
> > This is the last call for DT bindings review, with a small change to the
> > bindings compared to v1. If I don't get any reply I'll assume that those
> > (pretty simple) bindings are perfect :-)
> > 
> > Changes since v1:
> > 
> > - Drop the clock-names property, as the VSP1 uses a single clock
> > 
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.txt
> > b/Documentation/devicetree/bindings/media/renesas,vsp1.txt new file mode
> > 100644
> > index 0000000..45c1d3c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.txt
> > @@ -0,0 +1,43 @@
> > +* Renesas VSP1 Video Processing Engine
> > +
> > +The VSP1 is a video processing engine that supports up-/down-scaling,
> > alpha
> > +blending, color space conversion and various other image processing
> > features.
> > +It can be found in the Renesas R-Car second generation SoCs.
> > +
> > +Required properties:
> > +
> > +  - compatible: Must contain "renesas,vsp1"
> > +
> > +  - reg: Base address and length of the registers block for the VSP1.
> > +  - interrupt-parent, interrupts: Specifier for the VSP1 interrupt.
> 
> I don't think 'interrupt-parent' needs to be documented in this device's
> binding, I'd say it belongs more to the interrupt controller binding.
> In any case, I would separate interrupt-parent and interrupt properties,
> as the former contains a phandle to the parent interrupt controller and
> the latter contains the vsp1 interrupt specifier.
> 
> I'd humbly suggest to rephrase it to something along the lines of:
> 
>       - interrupts: should contain the VSP1 interrupt specifier.

Sure, that sounds good to me. I'll remove the interrupt-parent property.

We should really come up with a standard working for interrupt specifiers and 
use it through all the DT bindings...

> > +  - clocks: A phandle + clock-specifier pair for the VSP1 functional
> > clock.
> > +
> > +  - renesas,#rpf: Number of Read Pixel Formatter (RPF) modules in the
> > VSP1.
> > +  - renesas,#uds: Number of Up Down Scaler (UDS) modules in the> VSP1.
> > +  - renesas,#wpf: Number of Write Pixel Formatter (WPF) modules in the
> > VSP1.
> > +
> > +
> > +Optional properties:
> > +
> > +  - renesas,has-lif: Boolean, indicates that the LCD Interface (LIF)
> > module is +    available.
> > +  - renesas,has-lut: Boolean, indicates that the Look Up Table (LUT)
> > module is +    available.
> > +  - renesas,has-sru: Boolean, indicates that the Super Resolution Unit
> > (SRU) +    module is available.
> > +
> > +
> > +Example: R8A7790 (R-Car H2) VSP1-S node
> > +
> > +	vsp1@fe928000 {
> > +		compatible = "renesas,vsp1";
> > +		reg = <0 0xfe928000 0 0x8000>;
> 
> These register ranges look suspicious, shouldn't this be just
> <0xfe928000 0x8000> ? What is the #address-cells and #size-cells
> values for this node ?

Both #address-cells and #size-cells are equal to 2, the R8A7790 support LPAE.

> > +		interrupts = <0 267 IRQ_TYPE_LEVEL_HIGH>;
> > +		clocks = <&mstp1_clks R8A7790_CLK_VSP1_S>;
> > +
> > +		renesas,has-lut;
> > +		renesas,has-sru;
> > +		renesas,#rpf = <5>;
> > +		renesas,#uds = <3>;
> > +		renesas,#wpf = <4>;
> > +	};
> > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> > b/drivers/media/platform/vsp1/vsp1_drv.c index 28e1de3..644650f 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c

[snip]

> > @@ -527,6 +557,11 @@ static int vsp1_remove(struct platform_device *pdev)
> >   	return 0;
> >   }
> > 
> > +static const struct of_device_id vsp1_of_match[] = {
> > +	{ .compatible = "renesas,vsp1" },
> > +	{ },
> > +};
> > +
> >   static struct platform_driver vsp1_platform_driver = {
> >   	.probe		= vsp1_probe,
> >   	.remove		= vsp1_remove,
> > @@ -534,6 +569,7 @@ static struct platform_driver vsp1_platform_driver = {
> >   		.owner	= THIS_MODULE,
> >   		.name	= "vsp1",
> >   		.pm	= &vsp1_pm_ops,
> > +		.of_match_table = of_match_ptr(vsp1_of_match),
> 
> Is of_match_ptr() really useful here, when vsp1_of_match[] array is always
> compiled in ?

Would it be better to compile the vsp1_of_match[] array conditionally ? On the 
other hand the driver is only useful (at least at the moment) on ARM Renesas 
SoCs, which are transitioning to DT anyway.

> >   	},
> >   };
> 
> Otherwise this binding indeed looks perfect to me. ;)

Thank you :-)

-- 
Regards,

Laurent Pinchart