On 06/11/2013 05:10 PM, Francisco Jerez wrote: > Sebastian Hesselbarth<sebastian.hesselbarth at gmail.com> writes: >>> - I think we could also drop the call to ->set_config since presumably an >>> of-enabled driver grabbed any required info already from the dt. >> [...] >>> I think this way we could still share encoder slaves across tons of >>> platforms, only the init sequence (and specifically how they get at their > > The "set_config" hook is just the way a DRM driver communicates those > board-specific differences in the init sequence to the slave encoder > driver. I don't think it would make sense to remove it unless we make > sure it's being called elsewhere.
Francisco, for the non-DT case all probe related stuff could be passed with board_info.platform_data. For the DT case, the i2c device driver will parse properties into .platform_data. IMHO in the end, .set_config can be safely removed. But for now and especially because I only have a DT-only platform to test, leave it there and rework existing drivers and drm master encoders to pass it that way. >> IMHO, the whole i2c encoder stuff is just wrong. Not any i2c slave >> driver is even really using its probe(). Everything is packed somewhere >> because it just worked.. this is at least a start. >> > Why is that? What do you mean by the probe hooks not being used? > ch7006 and sil164 rely on it being called on initialization. You are right, I remembered wrong. >> I suggest this to get merged to at least allow to have DT slaves, >> then start with improving tda998x as an example, then maybe rethink >> drm_slave_encoder completely, e.g. >> >> - generalize the concept to SPI attached encoders, "internal" encoders.. > > drm_encoder_slave is bus-agnostic. drm_i2c_encoder_init() is just a > helper function taking care of bus-specific details like the creation of > the underlying I2C device object, which cannot be made bus-agnostic for > obvious reasons. You're welcome to implement SPI and internal > counterparts of drm_i2c_encoder_init(). Hehe, true again. I like to help and improve it wherever possible. But to be honest, DRM is not an easy starter for blindly touch commonly shared functions. If I break Dove, there is little people complaining. If I also break x86 complaining quickly becomes ranting ;) >> - find a way to setup .encoder_type and .connector_type correctly > > I guess encoder_type could be initialized correctly from the slave > encoder_init() hook -- that hasn't been necessary until now because the > DRM driver making use of the slave encoder has been expected to have > some other means to find out encoder types [e.g. device-specific BIOS > tables]. OTOH, I don't think that setting connector types is the slave > encoder's business. Well, I do not completely agree here. Actually, the connector type cannot be set from any encoder in the chain in a consitent way. But at least the slave encoder is closer to it. For Dove (and many other SoCs) the lcd controller is just a dumb rgb scan-out controller. It makes no difference if you directly connect an LCD panel or have a HDMI encoder finally connected to an DVI plug. The LCD controller shouldn't really care because it always sees a dumb rgb receiver, the HDMI encoder at least can say it is either HDMI or DVI plug. For DT, I tend to have a video-card node comprising lcd controllers, video memory range, external encoder phandle. Maybe put the board- specific connector here, and then it is up the the master encoder (or video card "driver") to set the correct connector. Sebastian
