Re: [PATCH v3 2/2] media: i2c: adv748x: add adv748x driver

2017-05-23 Thread Kieran Bingham 
On 18/05/17 22:48, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.

Thanks for the review,

I've worked through these, and changes that were straightforward have already
been done locally.

Those that were more complicated or will take more thought are now on my todo
list and being worked through...

There are a sprinkling of questions below so I'll send this now :)


> 
> On Wednesday 17 May 2017 15:13:18 Kieran Bingham wrote:
>> From: Kieran Bingham 
>>
>> Provide basic support for the ADV7481 and ADV7482.
>>
>> The driver is modelled with 2 subdevices to allow simultaneous streaming
>> from the AFE (Analog front end) and HDMI inputs.
> 
> Isn't that now four subdevices ?

Of course :)

> 
>> Presently the HDMI is hardcoded to link to the TXA CSI bus, whilst the
>> AFE is linked to the TXB CSI bus.
>>
>> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
>> and an earlier rework by Niklas Söderlund.
>>
>> Signed-off-by: Kieran Bingham 
> 
> [snip]
> 
>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
>> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt new file mode
>> 100644
>> index ..98d4600b7d26
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
>> @@ -0,0 +1,72 @@
>> +* Analog Devices ADV748X video decoder with HDMI receiver
>> +
>> +The ADV7481, and ADV7482 are multi format video decoders with an integrated
>> +HDMI receiver. It can output CSI-2 on two independent outputs TXA and TXB
> 
> s/It/They/ ?
> 

Ack

>> from +three input sources HDMI, analog and TTL.
>> +
>> +Required Properties:
>> +
>> +  - compatible: Must contain one of the following
>> +- "adi,adv7481" for the ADV7481
>> +- "adi,adv7482" for the ADV7482
>> +
>> +  - reg: I2C slave address
>> +
>> +  - interrupt-names: Should specify the interrupts as "intrq1" and/or
>> "intrq2"
>> + "intrq3" is only available on the adv7480 and adv7481
> 
> The bindings don't cover the ADV7480 yet, I wouldn't mention it here.
>

Ok, removed.

> Which interrupt(s) are mandatory and which are optional ? If they're all 
> mandatory (which I doubt) I would phrase it as 
> 
>   - interrupt-names: Should specify the "intrq1", "intrq2" and "intrq3" 
> interrupts. The "intrq3" interrupt is only available on the adv7481.
> 
> If they're all optional, I would move it to the Optional Properties section 
> and phrase it as
> 
>   - interrupt-names: Should specify the "intrq1", "intrq2" and/or "intrq3" 
> interrupts. All interrupts are optional. The "intrq3" interrupt is only 
> available on the adv7481.
> 

I believe they can be optional.
Added an optional section.


> If some of them only are mandatory,
> 
>   - interrupt-names: Should specify the "intrq1", "intrq2" and/or "intrq3" 
> interrupts. The ... interrupts are mandatory, while the ... interrupts are 
> optional. The "intrq3" interrupt is only available on the adv7481.
> 
>> +  - interrupts: Specify the interrupt lines for the ADV748x
>> +
>> +The device node must contain one 'port' child node per device input and
>> output +port, in accordance with the video interface bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt. The port
>> nodes +are numbered as follows.
>> +
>> +  Name  TypePort
>> +
>> +  HDMI  sink0
>> +  AIN1  sink1
>> +  AIN2  sink2
>> +  AIN3  sink3
>> +  AIN4  sink4
>> +  AIN5  sink5
>> +  AIN6  sink6
>> +  AIN7  sink7
>> +  AIN8  sink8
>> +  TTL   sink9
>> +  TXA   source  10
>> +  TXB   source  11
>> +
>> +The digital output port node must contain at least one source endpoint.
> 
> s/node/nodes/ ?
> s/source //
> 

Fixed

>> +Example:
>> +
>> +video_receiver@70 {
>> +compatible = "adi,adv7482";
>> +reg = <0x70>;
>> +
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +interrupt-parent = <>;
>> +interrupt-names = "intrq1", "intrq2";
>> +interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
>> + <31 IRQ_TYPE_LEVEL_LOW>;
>> +
>> +port@10 {
>> +reg = <10>;
>> +adv7482_txa: endpoint@1 {
> 
> There's no need to number endpoints when there's a single one. Otherwise 
> you'd 
> need a reg property in the endpoint.
> 

Ok - that's Good IMO; Can multiple (more than 2) devices be connected to a
single CSI2 bus?

Even if they can -  I don't think we would support that yet so either way,

Re: [PATCH v3 2/2] media: i2c: adv748x: add adv748x driver

2017-05-18 Thread Laurent Pinchart 
Hi Kieran,

Thank you for the patch.

On Wednesday 17 May 2017 15:13:18 Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Provide basic support for the ADV7481 and ADV7482.
> 
> The driver is modelled with 2 subdevices to allow simultaneous streaming
> from the AFE (Analog front end) and HDMI inputs.

Isn't that now four subdevices ?

> Presently the HDMI is hardcoded to link to the TXA CSI bus, whilst the
> AFE is linked to the TXB CSI bus.
> 
> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
> and an earlier rework by Niklas Söderlund.
> 
> Signed-off-by: Kieran Bingham 

[snip]

> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt new file mode
> 100644
> index ..98d4600b7d26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> @@ -0,0 +1,72 @@
> +* Analog Devices ADV748X video decoder with HDMI receiver
> +
> +The ADV7481, and ADV7482 are multi format video decoders with an integrated
> +HDMI receiver. It can output CSI-2 on two independent outputs TXA and TXB

s/It/They/ ?

> from +three input sources HDMI, analog and TTL.
> +
> +Required Properties:
> +
> +  - compatible: Must contain one of the following
> +- "adi,adv7481" for the ADV7481
> +- "adi,adv7482" for the ADV7482
> +
> +  - reg: I2C slave address
> +
> +  - interrupt-names: Should specify the interrupts as "intrq1" and/or
> "intrq2"
> + "intrq3" is only available on the adv7480 and adv7481

The bindings don't cover the ADV7480 yet, I wouldn't mention it here.

Which interrupt(s) are mandatory and which are optional ? If they're all 
mandatory (which I doubt) I would phrase it as 

  - interrupt-names: Should specify the "intrq1", "intrq2" and "intrq3" 
interrupts. The "intrq3" interrupt is only available on the adv7481.

If they're all optional, I would move it to the Optional Properties section 
and phrase it as

  - interrupt-names: Should specify the "intrq1", "intrq2" and/or "intrq3" 
interrupts. All interrupts are optional. The "intrq3" interrupt is only 
available on the adv7481.

If some of them only are mandatory,

  - interrupt-names: Should specify the "intrq1", "intrq2" and/or "intrq3" 
interrupts. The ... interrupts are mandatory, while the ... interrupts are 
optional. The "intrq3" interrupt is only available on the adv7481.

> +  - interrupts: Specify the interrupt lines for the ADV748x
> +
> +The device node must contain one 'port' child node per device input and
> output +port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt. The port
> nodes +are numbered as follows.
> +
> +  Name  TypePort
> +
> +  HDMI  sink0
> +  AIN1  sink1
> +  AIN2  sink2
> +  AIN3  sink3
> +  AIN4  sink4
> +  AIN5  sink5
> +  AIN6  sink6
> +  AIN7  sink7
> +  AIN8  sink8
> +  TTL   sink9
> +  TXA   source  10
> +  TXB   source  11
> +
> +The digital output port node must contain at least one source endpoint.

s/node/nodes/ ?
s/source //

> +Example:
> +
> +video_receiver@70 {
> +compatible = "adi,adv7482";
> +reg = <0x70>;
> +
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +interrupt-parent = <>;
> +interrupt-names = "intrq1", "intrq2";
> +interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
> + <31 IRQ_TYPE_LEVEL_LOW>;
> +
> +port@10 {
> +reg = <10>;
> +adv7482_txa: endpoint@1 {

There's no need to number endpoints when there's a single one. Otherwise you'd 
need a reg property in the endpoint.

> +clock-lanes = <0>;
> +data-lanes = <1 2 3 4>;
> +remote-endpoint = <_in>;
> +};
> +};
> +
> +port@11 {
> +reg = <11>;
> +adv7482_txb: endpoint@1 {
> +clock-lanes = <0>;
> +data-lanes = <1>;
> +remote-endpoint = <_in>;
> +};
> +};

The example only shows ports 10 and 11. Should all ports be present, even if 
they have no endpoint, because they're present at the hardware level ? That's 
debatable, but if the ports are optional when not connected, I would document 
that explicitly above.

> +