Hi Jacob,

On Wed, May 16, 2018 at 11:54 PM Jacob Chen [off-list ref] wrote:

2018-05-16 22:39 GMT+08:00 Jacob Chen [off-list ref]:

quoted

Hi Laurent,

2018-05-16 13:20 GMT+08:00 Laurent Pinchart <

laurent.pinchart@ideasonboard.com>:

quoted

quoted

Hi Jacob,

Thank you for the patch.

On Thursday, 8 March 2018 11:47:54 EEST Jacob Chen wrote:

quoted

From: Jacob Chen <redacted>

This commit adds a subdev driver for Rockchip MIPI Synopsys DPHY

driver

quoted

quoted

Should this really be a subdev driver ? After a quick look at the

code, the

quoted

quoted

only parameters you need to configure the PHY is the number of lanes

and the

quoted

quoted

data rate. Implementing the whole subdev API seems overcomplicated to

me,

quoted

quoted

especially given that the D-PHY doesn't deal with video streams as

such, but

quoted

quoted

operates one level down. Shouldn't we model the D-PHY using the Linux

PHY

quoted

quoted

framework ? I believe all the features you need are there except for a

D-PHY-

quoted

quoted

specific configuration function that should be very easy to add.

It deserves a subdev driver since the ISP is not the only user.
Other driver, like VIP, use it too.

For example, if there are two sensors connected to a rk3399 board.

Sensor1 --> DPHY1
Sensor2 --> DPHY2

With a subdev phy driver, i can choose either ISP or VIP for
sensor1/sensor2 by enable/disable media link in the run time.
1.
Sensor1 --> DPHY1 ---> VIP
Sensor2 --> DPHY2 ---> ISP1
2.
Sensor1 --> DPHY1 ---> ISP1
Sensor2 --> DPHY2 ---> VIP

What is VIP?

Also, if we model the DPHY using the PHY interface, it will be still
possible to achieve the same, just by toggling the link between sensor and
VIP or ISP1:

1.

Sensor1 -------|~|--- VIP
         \             | (PHY interface)
          \           DPHY1
           \           | (PHY interface)
            \---| |-- ISP1

Sensor2 -------| |-- VIP
         \             | (PHY interface)
          \           DPHY2
           \           | (PHY interface)
            \---|~|-- ISP1

2.

Sensor1 -------| |-- VIP
         \             | (PHY interface)
          \           DPHY1
           \           | (PHY interface)
            \---|~|-- ISP1

Sensor2 -------|~|-- VIP
         \             | (PHY interface)
          \           DPHY2
           \           | (PHY interface)
            \---| |-- ISP1

Best regards,
Tomasz