On 27.08.2018 19:59, Russell King - ARM Linux wrote:
Hi Andrzej,
On Mon, Aug 27, 2018 at 06:15:59PM +0200, Andrzej Hajda wrote:
On 30.07.2018 18:42, Russell King wrote:
static void tda998x_destroy(struct tda998x_priv *priv) {
- drm_bridge_remove(&priv->bridge);
- /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex);
- INIT_LIST_HEAD(&priv->bridge.list);
This line can be probably removed, unless there is a reason I am not aware of.
The addition above of drm_bridge_remove() to tda998x_destroy() means that we end up calling this function in the error cleanup path. This avoids unnecessary complexity with lots of different gotos - tda998x has had a long history of not cleaning up stuff properly.
1. bridge.list is/should be a private field of drm_bridge framework, so it's direct usage in driver looks like layer violation. 2. Calling drm_bridge_remove() without drm_bridge_add() is not strictly forbidden, but at least looks very suspicious. Even if current implementation tolerates it, it can change in the future.
Neither argument is a blocker IMO so if you prefer to stay with current solution please add a comment in the code explaining why do you initializes list field, the code at first sight looks suspicious.
devm interfaces for bridge do not help avoid that - devm stuff only works if everything that is registered previously is cleaned up via devm mechanisms to ensure that a device's interface becomes unavailable before stuff (eg, edid timers, detect work) is started to be cleaned up. Otherwise, there's a chance of this stuff being triggered during tear-down.
+static int tda998x_bind(struct device *dev, struct device *master, void *data) +{
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- ret = tda998x_create(client, priv);
- if (ret)
return ret;- ret = tda998x_encoder_init(dev, drm);
- if (ret) {
tda998x_destroy(priv);return ret;- }
- return 0;
It could be replaced by: ret = tda998x_encoder_init(dev, drm); if (ret) tda998x_destroy(priv); return ret;
but this is probably matter of taste.
It's not clear to me what "It" is - I think you're suggesting combining tda998x_create() and tda998x_encoder_init() ?
No, just simplifying error path.
The code is structured this way to make the following patches easier - there is no point of combining things only to have to then break them apart again in a later patch. Please see patch 7, where tda998x_create() moves out of this function, where exactly this happens.
OK. As I said: up to you.
Moreover I guess priv->is_on could be removed if enable/disable callbacks are called only by drm_core, but this is for another patch.
Is it guaranteed that a bridge ->enable or ->disable callback won't be called twice, even for legacy drivers? I think atomic guarantees this but I don't think it's guaranteed for legacy drivers.
I'm guessing Rob had a reason why he added the check when he originally created the driver (encoder ->dpms can be called for the same dpms state multiple times?)
OK, my guess was incorrect.
Regards
Andrzej