On 2018-04-20 12:24, Russell King - ARM Linux wrote: > On Fri, Apr 20, 2018 at 01:06:49PM +0300, Laurent Pinchart wrote: >> Hi Peter, >> >> Thank you for the patch. >> >> On Thursday, 19 April 2018 19:27:51 EEST Peter Rosin wrote: >>> This makes this driver work with all(?) drivers that are not >>> componentized and instead expect to connect to a panel/bridge. That >>> said, the only one tested is atmel_hlcdc. >>> >>> This hooks the relevant work function previously called by the encoder >>> and the component also to the bridge, since the encoder goes away when >>> connecting to the bridge interface of the driver and the equivalent of >>> bind/unbind of the component is handled by bridge attach/detach. >>> >>> The lifetime requirements of a bridge and a component are slightly >>> different, which is the reason for struct tda998x_bridge. >> >> Couldn't you move the allocation and initialization (tda998x_create) of the >> tda998x_priv structure to probe time ? I think you wouldn't need a separate >> structure in that case. Unless I'm mistaken there would be an added benefit of >> separating component and bridge initialization, resulting in the encoder not >> being initialized at all if the component isn't used. You wouldn't need to add >> a local_encoder parameter to the tda998x_init() function. > > No, I don't like that idea one bit, as I've stated in the past about the > component API. The same (probably) goes for the bridge stuff too. > > Consider the following: > > Your DRM system is initialised. You then remove a module, which results > in the DRM system being torn down. You re-insert the module (eg, having > made a change to it). The DRM system is then re-initialised. > > At this point, what is the state of variables such as priv->is_on if > you allocate the structure at probe time? > > What about all the other variables in the driver private structure - are > you sure that the driver can cope with random values from the previous > "usage" remaining there? > > At the moment, this isn't a concern for the driver because we > dev_kzalloc() the structure in the bind callback. Move that to the > probe function, and the structure is no longer re-initialised each > time, and so it retains the previous state. The driver is not setup > to cope with that. > > So, to work around that, you would need to reinitialise _everything_ > in the structure that the driver requires, which IMHO is a very > open to bugs (eg, if a member is missed, or added without the > necessary re-initialisation), _especially_ when this is not a path > that will get regular testing. > > If you want to do this for a subset of data, it would be much better > to separate them into independent structures (maybe one embedded into > the other) so that this problem can not occur. That way, a subset > of the data can be memset() when bound to the rest of the DRM system > ensuring a consistent driver state and still achieve what you're > suggesting. This was the exact reason I added struct tda998x_bridge. It seemed very risky to move the tda998x_create call (or some of its meat) to the probe function. Even if that could be done I think it should definitely be a separate patch. Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
