On Wed, Aug 19, 2015 at 10:37:54AM +0530, Archit Taneja wrote:

Hi,

On 06/30/2015 10:54 AM, Archit Taneja wrote:

...

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

The method presented in these patches is to provide an API to create a 'dummy' mipi_dsi_device. This device is populated with the desired DSI params, which are passed on to the host via mipi_dsi_attach().

This method will require the device driver to get a phandle to the DSI host since there is no parent-child relation between the two.

Is there a better way to do this? Please let me know!

Any comments on this?

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

i2c@... { ...

dsi-device@... { ... dsi-bus = <&dsi>; ... };

... };

dsi@... { ... };

Inversing the relationship would become something like this:

i2c@... { ... };

dsi@... { ...

peripheral@... { ... i2c-bus = <&i2c>; ... };

... };

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

i2c@... { ...

i2cdsi: dsi-device@... { ... };

... };

dsi@... { ...

peripheral@... { ... control = <&i2cdsi>; ... };

... };

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

Thierry

On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:

Hi Thierry, Archit,

Am Mittwoch, den 19.08.2015, 15:13 +0200 schrieb Thierry Reding:

...

On Wed, Aug 19, 2015 at 10:37:54AM +0530, Archit Taneja wrote:

...

Hi,

On 06/30/2015 10:54 AM, Archit Taneja wrote:

...

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

The method presented in these patches is to provide an API to create a 'dummy' mipi_dsi_device. This device is populated with the desired DSI params, which are passed on to the host via mipi_dsi_attach().

This method will require the device driver to get a phandle to the DSI host since there is no parent-child relation between the two.

Is there a better way to do this? Please let me know!

Any comments on this?

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

i2c@... { ...

dsi-device@... {
	...
	dsi-bus = <&dsi>;
	...
};

...

};

dsi@... { ... };

Inversing the relationship would become something like this:

i2c@... { ... };

dsi@... { ...

peripheral@... {
	...
	i2c-bus = <&i2c>;
	...
};

...

};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

i2c@... { ...

i2cdsi: dsi-device@... {
	...
};

...

};

dsi@... { ...

peripheral@... {
	...
	control = <&i2cdsi>;
	...
};

...

};

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

Multiple device drivers in both the media and DRM universe have shown that they are a working way to represent those data bus connections between devices. I know this might make things a bit more complicated for Tegra DRM, where you have a nice parent<->child relationship between the components even on the control path so far, but we should really move into the direction of more drivers using the standardized bindings for this stuff, instead of doing another round of NIH.

Why are you bringing up Tegra DRM? I don't see how it's relevant in any way to this discussion.

Thierry

On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

...

On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:

...

Hi Thierry, Archit,

[...]

...

...

...

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

i2c@... { ...

dsi-device@... {
	...
	dsi-bus = <&dsi>;
	...
};

...

};

dsi@... { ... };

Inversing the relationship would become something like this:

i2c@... { ... };

dsi@... { ...

peripheral@... {
	...
	i2c-bus = <&i2c>;
	...
};

...

};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

i2c@... { ...

i2cdsi: dsi-device@... {
	...
};

...

};

dsi@... { ...

peripheral@... {
	...
	control = <&i2cdsi>;
	...
};

...

};

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

...

...

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

You can then use of-graph to model the data path between the DSI encoder and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Thierry

Hi Thierry, Lucas,

On 08/19/2015 08:32 PM, Thierry Reding wrote:

On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:

...

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

...

On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:

...

Hi Thierry, Archit,

[...]

...

...

...

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

i2c@... { ...

dsi-device@... {
	...
	dsi-bus = <&dsi>;
	...
};

...

};

dsi@... { ... };

Inversing the relationship would become something like this:

i2c@... { ... };

dsi@... { ...

peripheral@... {
	...
	i2c-bus = <&i2c>;
	...
};

...

};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

i2c@... { ...

i2cdsi: dsi-device@... {
	...
};

...

};

dsi@... { ...

peripheral@... {
	...
	control = <&i2cdsi>;
	...
};

...

};

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

...

...

...

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

You can then use of-graph to model the data path between the DSI encoder and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. { adv7533@.. {

port { adv_in: endpoint { remote-endpoint = <&dsi_out>; }; }; }; };

dsi_host@.. { ... ...

port { dsi_out: endpoint { remote-endpoint = <&adv_in>; } }; };

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Could you explain what you meant by this? I.e. describing the kind of DSI device?

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

From an adv75xx driver perspective, it should also support the ADV7511 chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Thanks, Archit

On 08/20/2015 05:18 PM, Thierry Reding wrote:

On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:

...

Hi Thierry, Lucas,

On 08/19/2015 08:32 PM, Thierry Reding wrote:

...

On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:

...

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

...

On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:

...

Hi Thierry, Archit,

[...]

...

...

> Perhaps a better way would be to invert this relationship. According to > your proposal we'd have to have DT like this: > > i2c@... { > ... > > dsi-device@... { > ... > dsi-bus = <&dsi>; > ... > }; > > ... > }; > > dsi@... { > ... > }; > > Inversing the relationship would become something like this: > > i2c@... { > ... > }; > > dsi@... { > ... > > peripheral@... { > ... > i2c-bus = <&i2c>; > ... > }; > > ... > }; > > Both of those aren't fundamentally different, and they both have the > disavantage of lacking ways to transport configuration data that the > other bus needs to instantiate the dummy device (such as the reg > property for example, denoting the I2C slave address or the DSI VC). > > So how about we create two devices in the device tree and fuse them at > the driver level: > > i2c@... { > ... > > i2cdsi: dsi-device@... { > ... > }; > > ... > }; > > dsi@... { > ... > > peripheral@... { > ... > control = <&i2cdsi>; > ... > }; > > ... > }; > > This way we'll get both an I2C device and a DSI device that we can fully > describe using the standard device tree bindings. At driver time we can > get the I2C device from the phandle in the control property of the DSI > device and use it to execute I2C transactions. > I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

...

...

...

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

You can then use of-graph to model the data path between the DSI encoder and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. { adv7533@.. {

port {
	adv_in: endpoint {
		remote-endpoint = <&dsi_out>;
	};
};

}; };

dsi_host@.. { ... ...

port { dsi_out: endpoint { remote-endpoint = <&adv_in>; } }; };

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

...

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

...

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

...

...

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

...

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

...

From an adv75xx driver perspective, it should also support the ADV7511 chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

Thanks, Archit

On 09/07/2015 01:46 PM, Archit Taneja wrote:

Thierry,

On 08/21/2015 11:39 AM, Archit Taneja wrote:

...

On 08/20/2015 05:18 PM, Thierry Reding wrote:

...

On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:

...

Hi Thierry, Lucas,

On 08/19/2015 08:32 PM, Thierry Reding wrote:

...

On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:

...

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding: > On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote: >> Hi Thierry, Archit, >> [...] >>> Perhaps a better way would be to invert this relationship. >>> According to >>> your proposal we'd have to have DT like this: >>> >>> i2c@... { >>> ... >>> >>> dsi-device@... { >>> ... >>> dsi-bus = <&dsi>; >>> ... >>> }; >>> >>> ... >>> }; >>> >>> dsi@... { >>> ... >>> }; >>> >>> Inversing the relationship would become something like this: >>> >>> i2c@... { >>> ... >>> }; >>> >>> dsi@... { >>> ... >>> >>> peripheral@... { >>> ... >>> i2c-bus = <&i2c>; >>> ... >>> }; >>> >>> ... >>> }; >>> >>> Both of those aren't fundamentally different, and they both have >>> the >>> disavantage of lacking ways to transport configuration data that >>> the >>> other bus needs to instantiate the dummy device (such as the reg >>> property for example, denoting the I2C slave address or the DSI >>> VC). >>> >>> So how about we create two devices in the device tree and fuse >>> them at >>> the driver level: >>> >>> i2c@... { >>> ... >>> >>> i2cdsi: dsi-device@... { >>> ... >>> }; >>> >>> ... >>> }; >>> >>> dsi@... { >>> ... >>> >>> peripheral@... { >>> ... >>> control = <&i2cdsi>; >>> ... >>> }; >>> >>> ... >>> }; >>> >>> This way we'll get both an I2C device and a DSI device that we >>> can fully >>> describe using the standard device tree bindings. At driver time >>> we can >>> get the I2C device from the phandle in the control property of >>> the DSI >>> device and use it to execute I2C transactions. >>> >> I don't really like to see that you are inventing yet-another-way to >> handle devices connected to multiple buses. >> >> Devicetree is structured along the control buses, even if the >> devices >> are connected to multiple buses, in the DT they are always >> children of >> the bus that is used to control their registers from the CPUs >> perspective. So a DSI encoder that is controlled through i2c is >> clearly >> a child of the i2c master controller and only of that one. > > I think that's a flawed interpretation of what's going on here. The > device in fact has two interfaces: one is I2C, the other is DSI. > In my > opinion that's reason enough to represent it as two logical devices. > Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

...

>> If you need to model connections between devices that are not >> reflected >> through the control bus hierarchy you should really consider >> using the >> standardized of-graph bindings. >> (Documentation/devicetree/bindings/graph.txt) > > The problem is that the original proposal would instantiate a dummy > device, so it wouldn't be represented in DT at all. So unless you > do add > two logical devices to DT (one for each bus interface), you don't > have > anything to glue together with an OF graph. > I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

You can then use of-graph to model the data path between the DSI encoder and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. { adv7533@.. {

    port {
        adv_in: endpoint {
            remote-endpoint = <&dsi_out>;
        };
    };
};

};

dsi_host@.. { ... ...

port {
    dsi_out: endpoint {
        remote-endpoint = <&adv_in>;
    }
};

};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

...

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

...

...

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

...

...

...

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

...

...

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

...

...

From an adv75xx driver perspective, it should also support the ADV7511 chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7db...

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

• The i2c driver sets the driver data as its client data using i2c_set_clientdata()
• Parse the i2c-control phandle to get the corresponding i2c client.
• Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here. The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Regards Andrzej

Thanks, Archit

On Thu, Sep 10, 2015 at 11:45:35AM +0530, Archit Taneja wrote:

On 09/08/2015 03:57 PM, Andrzej Hajda wrote:

...

On 09/07/2015 01:46 PM, Archit Taneja wrote:

...

Thierry,

On 08/21/2015 11:39 AM, Archit Taneja wrote:

...

On 08/20/2015 05:18 PM, Thierry Reding wrote:

...

On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:

...

Hi Thierry, Lucas,

On 08/19/2015 08:32 PM, Thierry Reding wrote: >On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote: >>Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding: >>>On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote: >>>>Hi Thierry, Archit, >>>> >>[...] >>>>>Perhaps a better way would be to invert this relationship. >>>>>According to >>>>>your proposal we'd have to have DT like this: >>>>> >>>>> i2c@... { >>>>> ... >>>>> >>>>> dsi-device@... { >>>>> ... >>>>> dsi-bus = <&dsi>; >>>>> ... >>>>> }; >>>>> >>>>> ... >>>>> }; >>>>> >>>>> dsi@... { >>>>> ... >>>>> }; >>>>> >>>>>Inversing the relationship would become something like this: >>>>> >>>>> i2c@... { >>>>> ... >>>>> }; >>>>> >>>>> dsi@... { >>>>> ... >>>>> >>>>> peripheral@... { >>>>> ... >>>>> i2c-bus = <&i2c>; >>>>> ... >>>>> }; >>>>> >>>>> ... >>>>> }; >>>>> >>>>>Both of those aren't fundamentally different, and they both have >>>>>the >>>>>disavantage of lacking ways to transport configuration data that >>>>>the >>>>>other bus needs to instantiate the dummy device (such as the reg >>>>>property for example, denoting the I2C slave address or the DSI >>>>>VC). >>>>> >>>>>So how about we create two devices in the device tree and fuse >>>>>them at >>>>>the driver level: >>>>> >>>>> i2c@... { >>>>> ... >>>>> >>>>> i2cdsi: dsi-device@... { >>>>> ... >>>>> }; >>>>> >>>>> ... >>>>> }; >>>>> >>>>> dsi@... { >>>>> ... >>>>> >>>>> peripheral@... { >>>>> ... >>>>> control = <&i2cdsi>; >>>>> ... >>>>> }; >>>>> >>>>> ... >>>>> }; >>>>> >>>>>This way we'll get both an I2C device and a DSI device that we >>>>>can fully >>>>>describe using the standard device tree bindings. At driver time >>>>>we can >>>>>get the I2C device from the phandle in the control property of >>>>>the DSI >>>>>device and use it to execute I2C transactions. >>>>> >>>>I don't really like to see that you are inventing yet-another-way to >>>>handle devices connected to multiple buses. >>>> >>>>Devicetree is structured along the control buses, even if the >>>>devices >>>>are connected to multiple buses, in the DT they are always >>>>children of >>>>the bus that is used to control their registers from the CPUs >>>>perspective. So a DSI encoder that is controlled through i2c is >>>>clearly >>>>a child of the i2c master controller and only of that one. >>> >>>I think that's a flawed interpretation of what's going on here. The >>>device in fact has two interfaces: one is I2C, the other is DSI. >>>In my >>>opinion that's reason enough to represent it as two logical devices. >>> >>Does it really have 2 control interfaces that are used at the same >>time? >>Or is the DSI connection a passive data bus if the register control >>happens through i2c? > >The interfaces may not be used at the same time, and the DSI interface >may even be crippled, but the device is still addressable from the DSI >host controller, if for nothing else than for routing the video stream. > >>>>If you need to model connections between devices that are not >>>>reflected >>>>through the control bus hierarchy you should really consider >>>>using the >>>>standardized of-graph bindings. >>>>(Documentation/devicetree/bindings/graph.txt) >>> >>>The problem is that the original proposal would instantiate a dummy >>>device, so it wouldn't be represented in DT at all. So unless you >>>do add >>>two logical devices to DT (one for each bus interface), you don't >>>have >>>anything to glue together with an OF graph. >>> >>I see that the having dummy device is the least desirable solution. >>But >>if there is only one control bus to the device I think it should be >>one >>device sitting beneath the control bus. >> >>You can then use of-graph to model the data path between the DSI >>encoder >>and device. > >But you will be needing a device below the DSI host controller to >represent that endpoint of the connection. The DSI host controller >itself is in no way connected to the I2C adapter. You would have to >add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. { adv7533@.. {

    port {
        adv_in: endpoint {
            remote-endpoint = <&dsi_out>;
        };
    };
};

};

dsi_host@.. { ... ...

port {
    dsi_out: endpoint {
        remote-endpoint = <&adv_in>;
    }
};

};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

>hook up with a control endpoint. And then you'll need more quirks >to describe what kind of DSI device this is.

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

...

...

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

...

...

>On the other hand if you properly instantiate the DSI device you can >easily write a driver for it, and the driver will set up the correct >properties as implied by the compatible string. Once you have that you >can easily hook it up to the I2C control interface in whatever way you >like, be that an OF graph or just a simple unidirectional link by >phandle. >

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

...

...

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

...

...

From an adv75xx driver perspective, it should also support the ADV7511 chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7db...

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

• The i2c driver sets the driver data as its client data using i2c_set_clientdata()
• Parse the i2c-control phandle to get the corresponding i2c client.
• Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here. The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.

Let's wait for someone from the device tree maintainers to comment instead of going around in circles.

Thierry

On 09/15/2015 05:32 AM, Archit Taneja wrote:

Hi Rob, Mark,

We've been trying to figure out the right way to represent a class of display encoder devices in DT.

I've been meaning to reply on this.

These devices have registers that are generally configured via i2c. Once the device is configured, it takes in video data from the mipi dsi bus.

Until now, all the devices we've supported devices that can be are configured by the dsi bus itself, and hence, we've been able to represent them in DT as children under the dsi host.

For the above class of devices (using both i2c and dsi), we aren't able to conclude upon what's the better approach among the two:

1. Represent the device via 2 different nodes in DT. One would be

a child under an i2c adapter, the other a child of a dsi host. We would have two device drivers, one i2c client, and the other a mipi dsi device.

2. Represent the device as an i2c client in DT. Provide an api

to create "dummy" dsi devices. The i2c client driver would use this api to register a dsi device, and link itself with the dsi host.

What do you think would be the way to go here? I guess you might have faced something similar in other subsystems.

The closest thing I can think of are WiFi/BT combo chips that use SDIO+UART. In that case, 2 nodes makes sense as these chips are essentially 2 independent functions in a single chip and both interfaces are control interfaces (as well as data). This case is a bit different with both interfaces being tied to the same function.

So in this case, I think option 2 is the right way for the reasons already outlined in this thread. I think there needs to be more consistency in how the of-graph connections are defined with the core doing more of the graph traversing. So having an i2c device plus of-graph seems more consistent with other non-DSI cases.

The main open issue seemed to be setting the VC. At least for the ADV7533, it can be whatever you want it to be. The host and device just need to agree. I see no need for that to be in DT at least in this case. But then I'm not sure what are typical constraints for VC assignment. I'd guess that constraints are on the device side and hosts can support whatever the device wants. If so, then it can purely be up to the driver to set.

Implementation-wise, I don't think that I'd call this a dummy device. There are multiple ways for devices to get created in the driver model. DT is just one way. You are just creating the device in a different way outside of DT which is fine.

Rob

I've given a relatively brief description of the problem. There were some more nitty gritties discussed in this thread before.

Thierry, Andrzej, Lucas,

Please feel free to add your comments if I have missed out on something.

Thanks, Archit

On 09/10/2015 01:02 PM, Thierry Reding wrote:

...

On Thu, Sep 10, 2015 at 11:45:35AM +0530, Archit Taneja wrote:

...

On 09/08/2015 03:57 PM, Andrzej Hajda wrote:

...

On 09/07/2015 01:46 PM, Archit Taneja wrote:

...

Thierry,

On 08/21/2015 11:39 AM, Archit Taneja wrote:

...

On 08/20/2015 05:18 PM, Thierry Reding wrote: > On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote: >> Hi Thierry, Lucas, >> >> >> On 08/19/2015 08:32 PM, Thierry Reding wrote: >>> On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote: >>>> Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding: >>>>> On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote: >>>>>> Hi Thierry, Archit, >>>>>> >>>> [...] >>>>>>> Perhaps a better way would be to invert this relationship. >>>>>>> According to >>>>>>> your proposal we'd have to have DT like this: >>>>>>> >>>>>>> i2c@... { >>>>>>> ... >>>>>>> >>>>>>> dsi-device@... { >>>>>>> ... >>>>>>> dsi-bus = <&dsi>; >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> dsi@... { >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> Inversing the relationship would become something like this: >>>>>>> >>>>>>> i2c@... { >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> dsi@... { >>>>>>> ... >>>>>>> >>>>>>> peripheral@... { >>>>>>> ... >>>>>>> i2c-bus = <&i2c>; >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> Both of those aren't fundamentally different, and they both >>>>>>> have >>>>>>> the >>>>>>> disavantage of lacking ways to transport configuration data >>>>>>> that >>>>>>> the >>>>>>> other bus needs to instantiate the dummy device (such as >>>>>>> the reg >>>>>>> property for example, denoting the I2C slave address or the >>>>>>> DSI >>>>>>> VC). >>>>>>> >>>>>>> So how about we create two devices in the device tree and fuse >>>>>>> them at >>>>>>> the driver level: >>>>>>> >>>>>>> i2c@... { >>>>>>> ... >>>>>>> >>>>>>> i2cdsi: dsi-device@... { >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> dsi@... { >>>>>>> ... >>>>>>> >>>>>>> peripheral@... { >>>>>>> ... >>>>>>> control = <&i2cdsi>; >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> ... >>>>>>> }; >>>>>>> >>>>>>> This way we'll get both an I2C device and a DSI device that we >>>>>>> can fully >>>>>>> describe using the standard device tree bindings. At driver >>>>>>> time >>>>>>> we can >>>>>>> get the I2C device from the phandle in the control property of >>>>>>> the DSI >>>>>>> device and use it to execute I2C transactions. >>>>>>> >>>>>> I don't really like to see that you are inventing >>>>>> yet-another-way to >>>>>> handle devices connected to multiple buses. >>>>>> >>>>>> Devicetree is structured along the control buses, even if the >>>>>> devices >>>>>> are connected to multiple buses, in the DT they are always >>>>>> children of >>>>>> the bus that is used to control their registers from the CPUs >>>>>> perspective. So a DSI encoder that is controlled through i2c is >>>>>> clearly >>>>>> a child of the i2c master controller and only of that one. >>>>> >>>>> I think that's a flawed interpretation of what's going on >>>>> here. The >>>>> device in fact has two interfaces: one is I2C, the other is DSI. >>>>> In my >>>>> opinion that's reason enough to represent it as two logical >>>>> devices. >>>>> >>>> Does it really have 2 control interfaces that are used at the >>>> same >>>> time? >>>> Or is the DSI connection a passive data bus if the register >>>> control >>>> happens through i2c? >>> >>> The interfaces may not be used at the same time, and the DSI >>> interface >>> may even be crippled, but the device is still addressable from >>> the DSI >>> host controller, if for nothing else than for routing the video >>> stream. >>> >>>>>> If you need to model connections between devices that are not >>>>>> reflected >>>>>> through the control bus hierarchy you should really consider >>>>>> using the >>>>>> standardized of-graph bindings. >>>>>> (Documentation/devicetree/bindings/graph.txt) >>>>> >>>>> The problem is that the original proposal would instantiate a >>>>> dummy >>>>> device, so it wouldn't be represented in DT at all. So unless >>>>> you >>>>> do add >>>>> two logical devices to DT (one for each bus interface), you >>>>> don't >>>>> have >>>>> anything to glue together with an OF graph. >>>>> >>>> I see that the having dummy device is the least desirable >>>> solution. >>>> But >>>> if there is only one control bus to the device I think it >>>> should be >>>> one >>>> device sitting beneath the control bus. >>>> >>>> You can then use of-graph to model the data path between the DSI >>>> encoder >>>> and device. >>> >>> But you will be needing a device below the DSI host controller to >>> represent that endpoint of the connection. The DSI host controller >>> itself is in no way connected to the I2C adapter. You would >>> have to >>> add some sort of quirk to the DSI controller binding to allow >>> it to >> >> Thanks for the review. >> >> I implemented this to support ADV7533 DSI to HDMI encoder chip, >> which >> has a DSI video bus and an i2c control bus. >> >> There weren't any quirks as such in the device tree when I tried to >> implement this. The DT seems to manage fine without a node >> corresponding to a mipi_dsi_device: >> >> i2c_adap@.. { >> adv7533@.. { >> >> port { >> adv_in: endpoint { >> remote-endpoint = <&dsi_out>; >> }; >> }; >> }; >> }; >> >> dsi_host@.. { >> ... >> ... >> >> port { >> dsi_out: endpoint { >> remote-endpoint = <&adv_in>; >> } >> }; >> }; >> >> It's the i2c driver's job to parse the graph and retrieve the >> phandle to the dsi host. Using this, it can proceed with >> registering itself to this host using the new dsi funcs. This >> patch does the same for the adv7533 i2c driver: >> >> http://www.spinics.net/lists/dri-devel/msg86840.html >> >>> hook up with a control endpoint. And then you'll need more quirks >>> to describe what kind of DSI device this is. >> >> Could you explain what you meant by this? I.e. describing the kind >> of DSI device? > > Describing the number of lanes, specifying the virtual channel, mode > flags, etc. You could probably set the number of lanes and mode > flags > via the I2C driver, but especially the virtual channel cannot be set > because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

> >> The dsi device created isn't really a dummy device as such. It's >> dummy in the sense that there isn't a real dsi driver associated >> with it. The dsi device is still used to attach to a mipi dsi host, >> the way normal dsi devices do. > > I understand, but I don't see why it has to be instantiated by > the I2C > driver, that's what I find backwards. There is already a standard > way > for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

> >>> On the other hand if you properly instantiate the DSI device >>> you can >>> easily write a driver for it, and the driver will set up the >>> correct >>> properties as implied by the compatible string. Once you have >>> that you >>> can easily hook it up to the I2C control interface in whatever >>> way you >>> like, be that an OF graph or just a simple unidirectional link by >>> phandle. >>> >> >> With the fused approach you suggested, we would have 2 drivers: >> one i2c >> and the other dsi. The i2c client driver would be more or less >> minimal, >> preparing an i2c_client device for the dsi driver to use. Is my >> understanding correct? > > Correct. That's kind of similar to the way an HDMI encoder driver > would > use an I2C adapter to query EDID. The i2c_client device would be > a means > for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

> >> We can do without dummy dsi devices with this method. But, >> representing >> a device with 2 DT nodes seems a bit off. We'd also need to >> compatible >> strings for the same device, one for the i2c part, and the other >> for >> the dsi part. > > I agree that this somewhat stretches the capabilities of device > tree. > Another alternative I guess would be to not have a compatible > string for > the I2C device at all (that's technically not valid, I guess) > because we > really don't need an I2C driver for the device. What we really > need is a > DSI driver with a means to talk over some I2C bus with some other > part > of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

> >> From an adv75xx driver perspective, it should also support the >> ADV7511 >> chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't >> need a >> DSI DT node. It would be a bit inconsistent to have the bindings >> require both DSI and I2C nodes for one chip, and only I2C node >> for the >> other, with both chips being supported by the same driver. > > Why would that be inconsistent? That sounds like the most accurate > representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7db...

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

• The i2c driver sets the driver data as its client data using i2c_set_clientdata()
• Parse the i2c-control phandle to get the corresponding i2c client.
• Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here. The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.

Let's wait for someone from the device tree maintainers to comment instead of going around in circles.

Thierry

Hi Rob, Mark,

We've been trying to figure out the right way to represent a class of display encoder devices in DT.

These devices have registers that are generally configured via i2c. Once the device is configured, it takes in video data from the mipi dsi bus.

Until now, all the devices we've supported devices that can be are configured by the dsi bus itself, and hence, we've been able to represent them in DT as children under the dsi host.

For the above class of devices (using both i2c and dsi), we aren't able to conclude upon what's the better approach among the two:

1. Represent the device via 2 different nodes in DT. One would be a child under an i2c adapter, the other a child of a dsi host. We would have two device drivers, one i2c client, and the other a mipi dsi device.

2. Represent the device as an i2c client in DT. Provide an api to create "dummy" dsi devices. The i2c client driver would use this api to register a dsi device, and link itself with the dsi host.

What do you think would be the way to go here? I guess you might have faced something similar in other subsystems.

I've given a relatively brief description of the problem. There were some more nitty gritties discussed in this thread before.

Thierry, Andrzej, Lucas,

Please feel free to add your comments if I have missed out on something.

Thanks, Archit

On 09/10/2015 01:02 PM, Thierry Reding wrote:

On Thu, Sep 10, 2015 at 11:45:35AM +0530, Archit Taneja wrote:

...

On 09/08/2015 03:57 PM, Andrzej Hajda wrote:

...

On 09/07/2015 01:46 PM, Archit Taneja wrote:

...

Thierry,

On 08/21/2015 11:39 AM, Archit Taneja wrote:

...

On 08/20/2015 05:18 PM, Thierry Reding wrote:

...

On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote: > Hi Thierry, Lucas, > > > On 08/19/2015 08:32 PM, Thierry Reding wrote: >> On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote: >>> Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding: >>>> On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote: >>>>> Hi Thierry, Archit, >>>>> >>> [...] >>>>>> Perhaps a better way would be to invert this relationship. >>>>>> According to >>>>>> your proposal we'd have to have DT like this: >>>>>> >>>>>> i2c@... { >>>>>> ... >>>>>> >>>>>> dsi-device@... { >>>>>> ... >>>>>> dsi-bus = <&dsi>; >>>>>> ... >>>>>> }; >>>>>> >>>>>> ... >>>>>> }; >>>>>> >>>>>> dsi@... { >>>>>> ... >>>>>> }; >>>>>> >>>>>> Inversing the relationship would become something like this: >>>>>> >>>>>> i2c@... { >>>>>> ... >>>>>> }; >>>>>> >>>>>> dsi@... { >>>>>> ... >>>>>> >>>>>> peripheral@... { >>>>>> ... >>>>>> i2c-bus = <&i2c>; >>>>>> ... >>>>>> }; >>>>>> >>>>>> ... >>>>>> }; >>>>>> >>>>>> Both of those aren't fundamentally different, and they both have >>>>>> the >>>>>> disavantage of lacking ways to transport configuration data that >>>>>> the >>>>>> other bus needs to instantiate the dummy device (such as the reg >>>>>> property for example, denoting the I2C slave address or the DSI >>>>>> VC). >>>>>> >>>>>> So how about we create two devices in the device tree and fuse >>>>>> them at >>>>>> the driver level: >>>>>> >>>>>> i2c@... { >>>>>> ... >>>>>> >>>>>> i2cdsi: dsi-device@... { >>>>>> ... >>>>>> }; >>>>>> >>>>>> ... >>>>>> }; >>>>>> >>>>>> dsi@... { >>>>>> ... >>>>>> >>>>>> peripheral@... { >>>>>> ... >>>>>> control = <&i2cdsi>; >>>>>> ... >>>>>> }; >>>>>> >>>>>> ... >>>>>> }; >>>>>> >>>>>> This way we'll get both an I2C device and a DSI device that we >>>>>> can fully >>>>>> describe using the standard device tree bindings. At driver time >>>>>> we can >>>>>> get the I2C device from the phandle in the control property of >>>>>> the DSI >>>>>> device and use it to execute I2C transactions. >>>>>> >>>>> I don't really like to see that you are inventing yet-another-way to >>>>> handle devices connected to multiple buses. >>>>> >>>>> Devicetree is structured along the control buses, even if the >>>>> devices >>>>> are connected to multiple buses, in the DT they are always >>>>> children of >>>>> the bus that is used to control their registers from the CPUs >>>>> perspective. So a DSI encoder that is controlled through i2c is >>>>> clearly >>>>> a child of the i2c master controller and only of that one. >>>> >>>> I think that's a flawed interpretation of what's going on here. The >>>> device in fact has two interfaces: one is I2C, the other is DSI. >>>> In my >>>> opinion that's reason enough to represent it as two logical devices. >>>> >>> Does it really have 2 control interfaces that are used at the same >>> time? >>> Or is the DSI connection a passive data bus if the register control >>> happens through i2c? >> >> The interfaces may not be used at the same time, and the DSI interface >> may even be crippled, but the device is still addressable from the DSI >> host controller, if for nothing else than for routing the video stream. >> >>>>> If you need to model connections between devices that are not >>>>> reflected >>>>> through the control bus hierarchy you should really consider >>>>> using the >>>>> standardized of-graph bindings. >>>>> (Documentation/devicetree/bindings/graph.txt) >>>> >>>> The problem is that the original proposal would instantiate a dummy >>>> device, so it wouldn't be represented in DT at all. So unless you >>>> do add >>>> two logical devices to DT (one for each bus interface), you don't >>>> have >>>> anything to glue together with an OF graph. >>>> >>> I see that the having dummy device is the least desirable solution. >>> But >>> if there is only one control bus to the device I think it should be >>> one >>> device sitting beneath the control bus. >>> >>> You can then use of-graph to model the data path between the DSI >>> encoder >>> and device. >> >> But you will be needing a device below the DSI host controller to >> represent that endpoint of the connection. The DSI host controller >> itself is in no way connected to the I2C adapter. You would have to >> add some sort of quirk to the DSI controller binding to allow it to > > Thanks for the review. > > I implemented this to support ADV7533 DSI to HDMI encoder chip, which > has a DSI video bus and an i2c control bus. > > There weren't any quirks as such in the device tree when I tried to > implement this. The DT seems to manage fine without a node > corresponding to a mipi_dsi_device: > > i2c_adap@.. { > adv7533@.. { > > port { > adv_in: endpoint { > remote-endpoint = <&dsi_out>; > }; > }; > }; > }; > > dsi_host@.. { > ... > ... > > port { > dsi_out: endpoint { > remote-endpoint = <&adv_in>; > } > }; > }; > > It's the i2c driver's job to parse the graph and retrieve the > phandle to the dsi host. Using this, it can proceed with > registering itself to this host using the new dsi funcs. This > patch does the same for the adv7533 i2c driver: > > http://www.spinics.net/lists/dri-devel/msg86840.html > >> hook up with a control endpoint. And then you'll need more quirks >> to describe what kind of DSI device this is. > > Could you explain what you meant by this? I.e. describing the kind > of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

...

> The dsi device created isn't really a dummy device as such. It's > dummy in the sense that there isn't a real dsi driver associated > with it. The dsi device is still used to attach to a mipi dsi host, > the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

...

>> On the other hand if you properly instantiate the DSI device you can >> easily write a driver for it, and the driver will set up the correct >> properties as implied by the compatible string. Once you have that you >> can easily hook it up to the I2C control interface in whatever way you >> like, be that an OF graph or just a simple unidirectional link by >> phandle. >> > > With the fused approach you suggested, we would have 2 drivers: one i2c > and the other dsi. The i2c client driver would be more or less minimal, > preparing an i2c_client device for the dsi driver to use. Is my > understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

...

> We can do without dummy dsi devices with this method. But, representing > a device with 2 DT nodes seems a bit off. We'd also need to compatible > strings for the same device, one for the i2c part, and the other for > the dsi part.

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

...

> From an adv75xx driver perspective, it should also support the ADV7511 > chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a > DSI DT node. It would be a bit inconsistent to have the bindings > require both DSI and I2C nodes for one chip, and only I2C node for the > other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7db...

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

• The i2c driver sets the driver data as its client data using i2c_set_clientdata()
• Parse the i2c-control phandle to get the corresponding i2c client.
• Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here. The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.

Let's wait for someone from the device tree maintainers to comment instead of going around in circles.

Thierry

Hi Archit,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

reproduce: make htmldocs

All warnings (new ones prefixed by >>):

drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt' include/drm/drm_crtc.h:310: warning: No description found for parameter 'mode_blob' include/drm/drm_crtc.h:748: warning: No description found for parameter 'tile_blob_ptr' include/drm/drm_crtc.h:787: warning: No description found for parameter 'rotation' include/drm/drm_crtc.h:883: warning: No description found for parameter 'mutex' include/drm/drm_crtc.h:883: warning: No description found for parameter 'helper_private' include/drm/drm_crtc.h:931: warning: Excess struct/union/enum/typedef member 'base' description in 'drm_bridge' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tile_idr' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'delayed_event' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'edid_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dpms_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'path_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tile_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'plane_type_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'rotation_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_x' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_y' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_w' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_h' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_x' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_y' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_w' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_h' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_fb_id' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_id' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_active' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_mode_id' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dvi_i_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dvi_i_select_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_select_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_mode_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_left_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_right_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_top_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_bottom_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_brightness_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_contrast_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_flicker_reduction_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_overscan_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_saturation_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_hue_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'scaling_mode_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'aspect_ratio_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'suggested_y_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'allow_fb_modifiers' Warning(include/drm/drm_modeset_lock.h:47): Incorrect use of kernel-doc format: * Contended lock: if a lock is contended you should only call Warning(include/drm/drm_modeset_lock.h:54): Incorrect use of kernel-doc format: * list of held locks (drm_modeset_lock) Warning(include/drm/drm_modeset_lock.h:59): Incorrect use of kernel-doc format: * Trylock mode, use only for panic handlers! Warning(include/drm/drm_modeset_lock.h:74): Incorrect use of kernel-doc format: * modeset lock Warning(include/drm/drm_modeset_lock.h:79): Incorrect use of kernel-doc format: * Resources that are locked as part of an atomic update are added include/drm/drm_dp_helper.h:706: warning: No description found for parameter 'i2c_nack_count' include/drm/drm_dp_helper.h:706: warning: No description found for parameter 'i2c_defer_count' drivers/gpu/drm/drm_dp_mst_topology.c:2226: warning: No description found for parameter 'connector' include/drm/drm_dp_mst_helper.h:97: warning: No description found for parameter 'cached_edid' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'max_dpcd_transaction_bytes' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'sink_count' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'total_slots' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'avail_slots' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'total_pbn' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'qlock' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_msg_downq' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_msg_upq' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_down_in_progress' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_up_in_progress' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'payload_lock' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'proposed_vcpis' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'payloads' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'payload_mask' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'vcpi_mask' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_waitq' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'work' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'tx_work' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'destroy_connector_list' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'destroy_connector_lock' include/drm/drm_dp_mst_helper.h:471: warning: No description found for parameter 'destroy_connector_work' drivers/gpu/drm/drm_dp_mst_topology.c:2226: warning: No description found for parameter 'connector'

...

include/drm/drm_mipi_dsi.h:104: warning: No description found for parameter 'list'

include/drm/drmP.h:164: warning: No description found for parameter 'fmt' include/drm/drmP.h:180: warning: No description found for parameter 'fmt' include/drm/drmP.h:198: warning: No description found for parameter 'fmt' include/drm/drmP.h:238: warning: No description found for parameter 'dev' include/drm/drmP.h:238: warning: No description found for parameter 'data' include/drm/drmP.h:238: warning: No description found for parameter 'file_priv' include/drm/drmP.h:271: warning: No description found for parameter 'ioctl' include/drm/drmP.h:271: warning: No description found for parameter '_func' include/drm/drmP.h:271: warning: No description found for parameter '_flags' include/drm/drmP.h:344: warning: cannot understand function prototype: 'struct drm_lock_data ' include/drm/drmP.h:397: warning: cannot understand function prototype: 'struct drm_driver ' include/drm/drmP.h:646: warning: cannot understand function prototype: 'struct drm_info_list ' include/drm/drmP.h:656: warning: cannot understand function prototype: 'struct drm_info_node ' include/drm/drmP.h:666: warning: cannot understand function prototype: 'struct drm_minor ' include/drm/drmP.h:712: warning: cannot understand function prototype: 'struct drm_device ' drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args' drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1027: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1027: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1027: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1192: warning: No description found for parameter 'rps' drivers/gpu/drm/i915/i915_gem.c:1398: warning: No description found for parameter 'req' drivers/gpu/drm/i915/i915_gem.c:1433: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:1433: warning: No description found for parameter 'readonly' drivers/gpu/drm/i915/i915_gem.c:1556: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1556: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1556: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1664: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1664: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:1664: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'vma' drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'vmf' drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'size' drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'tiling_mode' drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'fenced' drivers/gpu/drm/i915/i915_gem.c:1952: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment' drivers/gpu/drm/i915/i915_gem.c:2815: warning: No description found for parameter 'ring' drivers/gpu/drm/i915/i915_gem.c:2944: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:2994: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_gem.c:2994: warning: No description found for parameter 'data' drivers/gpu/drm/i915/i915_gem.c:2994: warning: No description found for parameter 'file' drivers/gpu/drm/i915/i915_gem.c:2994: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl' drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'vm' drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'ggtt_view' drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'alignment' drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'flags' drivers/gpu/drm/i915/i915_gem.c:3576: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3576: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_gem.c:3858: warning: No description found for parameter 'obj' drivers/gpu/drm/i915/i915_gem.c:3858: warning: No description found for parameter 'write' drivers/gpu/drm/i915/i915_gem.c:4962: warning: No description found for parameter 'old' drivers/gpu/drm/i915/i915_gem.c:4962: warning: No description found for parameter 'new' drivers/gpu/drm/i915/i915_gem.c:4962: warning: No description found for parameter 'frontbuffer_bits' drivers/gpu/drm/i915/intel_lrc.c:782: warning: No description found for parameter 'req' drivers/gpu/drm/i915/intel_lrc.c:782: warning: Excess function parameter 'request' description in 'intel_logical_ring_begin' drivers/gpu/drm/i915/intel_lrc.c:782: warning: Excess function parameter 'ctx' description in 'intel_logical_ring_begin' drivers/gpu/drm/i915/intel_lrc.c:837: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'ring' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:782: warning: No description found for parameter 'req' drivers/gpu/drm/i915/intel_lrc.c:782: warning: Excess function parameter 'request' description in 'intel_logical_ring_begin' drivers/gpu/drm/i915/intel_lrc.c:782: warning: Excess function parameter 'ctx' description in 'intel_logical_ring_begin' drivers/gpu/drm/i915/intel_lrc.c:837: warning: No description found for parameter 'params' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'dev' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'file' description in 'intel_execlists_submission' drivers/gpu/drm/i915/intel_lrc.c:837: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'

vim +/list +104 include/drm/drm_mipi_dsi.h

068a0023 Andrzej Hajda 2013-12-04 88 struct mipi_dsi_device *dsi); 068a0023 Andrzej Hajda 2013-12-04 89 int (*detach)(struct mipi_dsi_host *host, 068a0023 Andrzej Hajda 2013-12-04 90 struct mipi_dsi_device *dsi); 068a0023 Andrzej Hajda 2013-12-04 91 ssize_t (*transfer)(struct mipi_dsi_host *host, ed6ff40e Thierry Reding 2014-08-05 92 const struct mipi_dsi_msg *msg); 068a0023 Andrzej Hajda 2013-12-04 93 }; 068a0023 Andrzej Hajda 2013-12-04 94 068a0023 Andrzej Hajda 2013-12-04 95 /** 068a0023 Andrzej Hajda 2013-12-04 96 * struct mipi_dsi_host - DSI host device 068a0023 Andrzej Hajda 2013-12-04 97 * @dev: driver model device node for this DSI host 068a0023 Andrzej Hajda 2013-12-04 98 * @ops: DSI host operations 068a0023 Andrzej Hajda 2013-12-04 99 */ 068a0023 Andrzej Hajda 2013-12-04 100 struct mipi_dsi_host { 068a0023 Andrzej Hajda 2013-12-04 101 struct device *dev; 068a0023 Andrzej Hajda 2013-12-04 102 const struct mipi_dsi_host_ops *ops; b3e0f8f8 Archit Taneja 2015-10-06 103 struct list_head list; 068a0023 Andrzej Hajda 2013-12-04 @104 }; 068a0023 Andrzej Hajda 2013-12-04 105 068a0023 Andrzej Hajda 2013-12-04 106 int mipi_dsi_host_register(struct mipi_dsi_host *host); 068a0023 Andrzej Hajda 2013-12-04 107 void mipi_dsi_host_unregister(struct mipi_dsi_host *host); b3e0f8f8 Archit Taneja 2015-10-06 108 struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node); 068a0023 Andrzej Hajda 2013-12-04 109 068a0023 Andrzej Hajda 2013-12-04 110 /* DSI mode flags */ 068a0023 Andrzej Hajda 2013-12-04 111 068a0023 Andrzej Hajda 2013-12-04 112 /* video mode */

:::::: The code at line 104 was first introduced by commit :::::: 068a00233969833f1ba925e7627797489efd6041 drm: Add MIPI DSI bus support

:::::: TO: Andrzej Hajda a.hajda@samsung.com :::::: CC: Thierry Reding treding@nvidia.com

--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation

Hi Archit,

[auto build test ERROR on: v4.4-rc3] [also build test ERROR on: next-20151127]

url: https://github.com/0day-ci/linux/commits/Archit-Taneja/drm-dsi-DSI-for-devic... config: x86_64-allyesdebian (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/gpu/drm/drm_mipi_dsi.c: In function 'of_mipi_dsi_device_add':

...

drivers/gpu/drm/drm_mipi_dsi.c:168:6: error: implicit declaration of function 'of_modalias_node' [-Werror=implicit-function-declaration]

if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) { ^ cc1: some warnings being treated as errors

vim +/of_modalias_node +168 drivers/gpu/drm/drm_mipi_dsi.c

162 { 163 struct device *dev = host->dev; 164 struct mipi_dsi_device_info info = { }; 165 int ret; 166 u32 reg; 167

168 if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {

169 dev_err(dev, "modalias failure on %s\n", node->full_name); 170 return ERR_PTR(-EINVAL); 171 }

--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation

On Mon, 07 Dec 2015, Archit Taneja architt@codeaurora.org wrote:

Hi,

On 11/30/2015 06:15 PM, kbuild test robot wrote:

...

Hi Archit,

[auto build test ERROR on: v4.4-rc3] [also build test ERROR on: next-20151127]

url: https://github.com/0day-ci/linux/commits/Archit-Taneja/drm-dsi-DSI-for-devic... config: x86_64-allyesdebian (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/gpu/drm/drm_mipi_dsi.c: In function 'of_mipi_dsi_device_add':

...

...

drivers/gpu/drm/drm_mipi_dsi.c:168:6: error: implicit declaration of function 'of_modalias_node' [-Werror=implicit-function-declaration]

  if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {

Any suggestions on how to fix this? Is it ok to make DRM_MIPI_DSI depend on CONFIG_OF?

Please don't.

BR, Jani.

On Mon, 07 Dec 2015, Archit Taneja architt@codeaurora.org wrote:

On 12/07/2015 02:15 PM, Jani Nikula wrote:

...

On Mon, 07 Dec 2015, Archit Taneja architt@codeaurora.org wrote:

...

Hi,

On 11/30/2015 06:15 PM, kbuild test robot wrote:

...

Hi Archit,

[auto build test ERROR on: v4.4-rc3] [also build test ERROR on: next-20151127]

url: https://github.com/0day-ci/linux/commits/Archit-Taneja/drm-dsi-DSI-for-devic... config: x86_64-allyesdebian (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64

All errors (new ones prefixed by >>):

 drivers/gpu/drm/drm_mipi_dsi.c: In function 'of_mipi_dsi_device_add':

...

...

drivers/gpu/drm/drm_mipi_dsi.c:168:6: error: implicit declaration of function 'of_modalias_node' [-Werror=implicit-function-declaration]

   if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {

Any suggestions on how to fix this? Is it ok to make DRM_MIPI_DSI depend on CONFIG_OF?

Please don't.

Just curious, how did x86 use DSI if the only way to create DSI devices until now was via DT?

Oh, you want the gory details... we use the DSI code as a library for abstraction and helpers, without actually creating or registering the devices.

BR, Jani.

Archit

...

BR, Jani.

On Mon, 07 Dec 2015, Archit Taneja architt@codeaurora.org wrote:

On 12/07/2015 02:40 PM, Jani Nikula wrote:

...

On Mon, 07 Dec 2015, Archit Taneja architt@codeaurora.org wrote:

...

On 12/07/2015 02:15 PM, Jani Nikula wrote:

...

On Mon, 07 Dec 2015, Archit Taneja architt@codeaurora.org wrote:

...

Any suggestions on how to fix this? Is it ok to make DRM_MIPI_DSI depend on CONFIG_OF?

Please don't.

Just curious, how did x86 use DSI if the only way to create DSI devices until now was via DT?

Oh, you want the gory details... we use the DSI code as a library for abstraction and helpers, without actually creating or registering the devices.

Okay, got it. I'll go with the IS_ENABLED(CONFIG_OF) approach.

Thanks, appreciated, so i915 doesn't need to depend on OF.

Humble request: Next time if I share something that doesn't make sense, please reply with something more than a "Please don't". That just sounds condescending and doesn't really help me with my cause either.

That's a fair request, no need to be humble about it. Apologies.

BR, Jani.

On 11/30/2015 01:01 PM, Archit Taneja wrote:

A driver calling mipi_dsi_device_new might want to unregister the device once it's done. It might also require it in an error handling path in case something didn't go right.

Signed-off-by: Archit Taneja architt@codeaurora.org

Reviewed-by: Andrzej Hajda a.hajda@samsung.com

Regards Andrzej

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

After some discussions on the previous RFC[1], we decided to support this by providing additional API in drm_mipi_dsi which lets us create new DSI devices without the need of them to have a DT node.

[1]: https://lkml.org/lkml/2015/6/30/42

Changes in v4: - Added a new patch that fixes build issues when CONFIG_OF is not set.

Changes in v3:

- Incorporated misc comments by Andrzej. Changed from RFC to a PATCH set. - Fixed htmldocs warnings.

Archit Taneja (6): drm/dsi: check for CONFIG_OF when defining of_mipi_dsi_device_add drm/dsi: Refactor device creation drm/dsi: Try to match non-DT dsi devices drm/dsi: Check for used channels drm/dsi: Add routine to unregister dsi device drm/dsi: Get DSI host by DT device node

drivers/gpu/drm/drm_mipi_dsi.c | 144 ++++++++++++++++++++++++++++++++--------- include/drm/drm_mipi_dsi.h | 29 +++++++++ 2 files changed, 141 insertions(+), 32 deletions(-)

On 1/21/2016 9:16 PM, Thierry Reding wrote:

On Thu, Dec 10, 2015 at 06:11:36PM +0530, Archit Taneja wrote:

...

Simplify the mipi dsi device creation process. device_initialize and

"MIPI" and "DSI", please.

Sure, I'll replace with these and in the other patches.

...

device_add don't need to be called separately when creating mipi_dsi_device's. Use device_register instead to simplify things.

Create a helper function mipi_dsi_device_new which takes in struct mipi_dsi_device_info and mipi_dsi_host. It clubs the functions mipi_dsi_device_alloc and mipi_dsi_device_add into one.

mipi_dsi_device_info acts as a template to populate the dsi device information. This is populated by of_mipi_dsi_device_add and passed to mipi_dsi_device_new.

Later on, we'll provide mipi_dsi_device_new as a standalone way to create a dsi device not available via DT.

The new device creation process tries to closely follow what's been done in i2c_new_device in i2c-core.

Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Archit Taneja architt@codeaurora.org

---

drivers/gpu/drm/drm_mipi_dsi.c | 61 +++++++++++++++++------------------------- include/drm/drm_mipi_dsi.h | 15 +++++++++++ 2 files changed, 40 insertions(+), 36 deletions(-)

To be honest, I'm not sure I like this. If you want to have a simpler helper, why not implement it using the lower-level helpers. Really the only thing you're doing here is add a high-level helper that takes an info struct, whereas previously the same would be done by storing the info directly in the structure between allocation and addition of the device.

Initially the implementation was following that of platform devices, I see no reason to deviate from that. What you want here can easily be

I don't see why we need to call device_initialize and device_add separately for DSI devices. From my (limited) understanding, we should call these separately if we want to take a reference (using get_device()), or set up some private data before the bus's notifier kicks in.

Since the main purpose of the series is not to simplify the device creation code, I can drop this.

done by something like:

struct mipi_dsi_device * mipi_dsi_device_register_full(struct mipi_dsi_host *host, const struct mipi_dsi_device_info *info) { struct mipi_dsi_device *dsi;

dsi = mipi_dsi_device_alloc(host);
if (IS_ERR(dsi))
	return dsi;

dsi->dev.of_node = info->node;
dsi->channel = info->channel;

err = mipi_dsi_device_add(dsi);
if (err < 0) {
	...
}

return dsi;

}

Thierry

This does look less intrusive. I'll consider switching to this.

Thanks, Archit

On 01/21/2016 09:41 PM, Thierry Reding wrote:

On Thu, Dec 10, 2015 at 06:11:38PM +0530, Archit Taneja wrote:

...

We don't check whether a previously registered mipi_dsi_device under the same host shares the same virtual channel.

Before registering, check if any of the registered devices doesn't already have the same virtual channel.

This wasn't crucial when all the devices under a host were populated via DT. Now that we also support creating devices manually, we could end up in a situation where a driver tries to create a device with a virtual channel already taken by a device populated in DT.

Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Archit Taneja architt@codeaurora.org

---

drivers/gpu/drm/drm_mipi_dsi.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)

I don't think this is necessary. The device name will be composed of the host's name, a '.' and the virtual channel ID, and the device core will refuse to create two devices with the same name.

That's a good point. I'll drop this patch.

Archit

Thierry

On 01/21/2016 09:46 PM, Thierry Reding wrote:

On Thu, Dec 10, 2015 at 06:11:40PM +0530, Archit Taneja wrote:

...

mipi_dsi_devices are inherently aware of their host because they share a parent-child hierarchy in the device tree.

non-dsi drivers that create dsi device don't have this data. In order to get this information, they require to a phandle to the dsi host in the device tree.

Maintain a list of all the hosts DSI that are currently registered.

This list will be used to find the mipi_dsi_host corresponding to the device_node passed in of_find_mipi_dsi_host_by_node.

Reviewed-by: Andrzej Hajda a.hajda@samsung.com Signed-off-by: Archit Taneja architt@codeaurora.org

---

drivers/gpu/drm/drm_mipi_dsi.c | 38 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 3 +++ 2 files changed, 41 insertions(+)

Please be more consistent with the spelling. Abbreviations should be all capitals. Also s/mipi_dsi_devices/MIPI DSI devices/, and so on.

I'll ensure the abbreviations are consistent in the next revision.

Otherwise I guess this makes sense.

Thanks for the review.

Thanks, Archit

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

After some discussions on the previous RFC[1], we decided to support this by providing additional API in drm_mipi_dsi.c which lets us create new DSI devices without the need of them to have a DT node.

[1]: https://lkml.org/lkml/2015/6/30/42

Changes in v5: - Simplify refactoring as suggested by Thierry. - Use abbreviations correctly. - Drop "drm/dsi: Check for used channels" - Return the correct encoded error code for of_mipi_dsi_device_add when CONFIG_OF is disabled. - Use mipi_dsi_device_unregister to implement mipi_dsi_remove_device_fn() too.

Changes in v4: - Added a new patch that fixes build issues when CONFIG_OF is not set.

Changes in v3:

- Incorporated misc comments by Andrzej. Changed from RFC to a PATCH set. - Fixed htmldocs warnings.

Archit Taneja (5): drm/dsi: check for CONFIG_OF when defining of_mipi_dsi_device_add drm/dsi: Use mipi_dsi_device_register_full for DSI device creation drm/dsi: Try to match non-DT DSI devices drm/dsi: Add routine to unregister a DSI device drm/dsi: Get DSI host by DT device node

drivers/gpu/drm/drm_mipi_dsi.c | 127 +++++++++++++++++++++++++++++++++++++---- include/drm/drm_mipi_dsi.h | 26 +++++++++ 2 files changed, 141 insertions(+), 12 deletions(-)

On Fri, Feb 12, 2016 at 02:48:29PM +0530, Archit Taneja wrote:

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

After some discussions on the previous RFC[1], we decided to support this by providing additional API in drm_mipi_dsi.c which lets us create new DSI devices without the need of them to have a DT node.

Changes in v5:

• Simplify refactoring as suggested by Thierry.
• Use abbreviations correctly.
• Drop "drm/dsi: Check for used channels"
• Return the correct encoded error code for of_mipi_dsi_device_add when CONFIG_OF is disabled.
• Use mipi_dsi_device_unregister to implement mipi_dsi_remove_device_fn() too.

Changes in v4:

• Added a new patch that fixes build issues when CONFIG_OF is not set.

Changes in v3:

• Incorporated misc comments by Andrzej. Changed from RFC to a PATCH set.
• Fixed htmldocs warnings.

Archit Taneja (5): drm/dsi: check for CONFIG_OF when defining of_mipi_dsi_device_add drm/dsi: Use mipi_dsi_device_register_full for DSI device creation drm/dsi: Try to match non-DT DSI devices drm/dsi: Add routine to unregister a DSI device drm/dsi: Get DSI host by DT device node

drivers/gpu/drm/drm_mipi_dsi.c | 127 +++++++++++++++++++++++++++++++++++++---- include/drm/drm_mipi_dsi.h | 26 +++++++++ 2 files changed, 141 insertions(+), 12 deletions(-)

I've applied this with minor fixups and cleanups to the commit messages.

Thanks, Thierry