| Message ID | 20200728074425.2749-5-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Convert to managed initialization | expand |
On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote: > Managed releases of the device's I2C adapter simplify the connector's > release. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> I think this breaks bisect, since at this point the release callback is called when the connector is already gone. At the end of the series it's fine again though. I've done a very cursory reading of your series to look for high-level issues, I think overall reasonable. On the series: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> But maybe you want to polish a bit more, up to you. -Daniel > --- > drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index f421a60d8a96..27eb49bd12b3 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -39,6 +39,7 @@ > #include <drm/drm_fourcc.h> > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_gem_vram_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_plane_helper.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_simple_kms_helper.h> > @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) > } > } > > +static void ast_i2c_release(struct drm_device *dev, void *data) > +{ > + struct ast_i2c_chan *i2c = data; > + > + i2c_del_adapter(&i2c->adapter); > + i2c->dev = NULL; /* clear to signal absence of I2C support */ > +} > + > static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) > { > int ret; > @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) > > i2c->dev = dev; /* signals presence of I2C support */ > > - return 0; > + return drmm_add_action_or_reset(dev, ast_i2c_release, i2c); > } > > static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) > @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) > return !!i2c->dev; > } > > -static void ast_i2c_fini(struct ast_i2c_chan *i2c) > -{ > - if (!ast_i2c_is_initialized(i2c)) > - return; > - i2c_del_adapter(&i2c->adapter); > - i2c->dev = NULL; /* clear to signal absence of I2C support */ > -} > - > /* > * Primary plane > */ > @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, > > static void ast_connector_destroy(struct drm_connector *connector) > { > - struct ast_connector *ast_connector = to_ast_connector(connector); > - ast_i2c_fini(&ast_connector->i2c); > drm_connector_cleanup(connector); > kfree(connector); > } > -- > 2.27.0 >
Hi Am 28.07.20 um 11:23 schrieb daniel@ffwll.ch: > On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote: >> Managed releases of the device's I2C adapter simplify the connector's >> release. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > I think this breaks bisect, since at this point the release callback is > called when the connector is already gone. At the end of the series it's > fine again though. > > I've done a very cursory reading of your series to look for high-level > issues, I think overall reasonable. On the series: > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > But maybe you want to polish a bit more, up to you. Thanks. I'll address your points and wait a bit longer. Usually Sam has a number of good comments as well. Best regards Thomas > -Daniel > >> --- >> drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- >> 1 file changed, 10 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >> index f421a60d8a96..27eb49bd12b3 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -39,6 +39,7 @@ >> #include <drm/drm_fourcc.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> #include <drm/drm_gem_vram_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_plane_helper.h> >> #include <drm/drm_probe_helper.h> >> #include <drm/drm_simple_kms_helper.h> >> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) >> } >> } >> >> +static void ast_i2c_release(struct drm_device *dev, void *data) >> +{ >> + struct ast_i2c_chan *i2c = data; >> + >> + i2c_del_adapter(&i2c->adapter); >> + i2c->dev = NULL; /* clear to signal absence of I2C support */ >> +} >> + >> static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) >> { >> int ret; >> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) >> >> i2c->dev = dev; /* signals presence of I2C support */ >> >> - return 0; >> + return drmm_add_action_or_reset(dev, ast_i2c_release, i2c); >> } >> >> static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) >> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) >> return !!i2c->dev; >> } >> >> -static void ast_i2c_fini(struct ast_i2c_chan *i2c) >> -{ >> - if (!ast_i2c_is_initialized(i2c)) >> - return; >> - i2c_del_adapter(&i2c->adapter); >> - i2c->dev = NULL; /* clear to signal absence of I2C support */ >> -} >> - >> /* >> * Primary plane >> */ >> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, >> >> static void ast_connector_destroy(struct drm_connector *connector) >> { >> - struct ast_connector *ast_connector = to_ast_connector(connector); >> - ast_i2c_fini(&ast_connector->i2c); >> drm_connector_cleanup(connector); >> kfree(connector); >> } >> -- >> 2.27.0 >> >
On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote: > Managed releases of the device's I2C adapter simplify the connector's > release. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index f421a60d8a96..27eb49bd12b3 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -39,6 +39,7 @@ > #include <drm/drm_fourcc.h> > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_gem_vram_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_plane_helper.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_simple_kms_helper.h> > @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) > } > } > > +static void ast_i2c_release(struct drm_device *dev, void *data) > +{ > + struct ast_i2c_chan *i2c = data; > + > + i2c_del_adapter(&i2c->adapter); > + i2c->dev = NULL; /* clear to signal absence of I2C support */ > +} > + > static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) > { > int ret; > @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) > > i2c->dev = dev; /* signals presence of I2C support */ > > - return 0; > + return drmm_add_action_or_reset(dev, ast_i2c_release, i2c); > } > > static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) > @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) > return !!i2c->dev; > } > > -static void ast_i2c_fini(struct ast_i2c_chan *i2c) > -{ > - if (!ast_i2c_is_initialized(i2c)) > - return; > - i2c_del_adapter(&i2c->adapter); > - i2c->dev = NULL; /* clear to signal absence of I2C support */ > -} The intro of ast_i2c_fini() and then removal again confuses me a little. But end result looks simple so I guess thats what counts. Sam > - > /* > * Primary plane > */ > @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, > > static void ast_connector_destroy(struct drm_connector *connector) > { > - struct ast_connector *ast_connector = to_ast_connector(connector); > - ast_i2c_fini(&ast_connector->i2c); > drm_connector_cleanup(connector); > kfree(connector); > } > -- > 2.27.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Am 28.07.20 um 11:23 schrieb daniel@ffwll.ch: > On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote: >> Managed releases of the device's I2C adapter simplify the connector's >> release. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > I think this breaks bisect, since at this point the release callback is > called when the connector is already gone. At the end of the series it's > fine again though. I'll move the auto-release of I2C to the end of the series. It should work then. Best regards Thomas > > I've done a very cursory reading of your series to look for high-level > issues, I think overall reasonable. On the series: > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > But maybe you want to polish a bit more, up to you. > -Daniel > >> --- >> drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- >> 1 file changed, 10 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >> index f421a60d8a96..27eb49bd12b3 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -39,6 +39,7 @@ >> #include <drm/drm_fourcc.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> #include <drm/drm_gem_vram_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_plane_helper.h> >> #include <drm/drm_probe_helper.h> >> #include <drm/drm_simple_kms_helper.h> >> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) >> } >> } >> >> +static void ast_i2c_release(struct drm_device *dev, void *data) >> +{ >> + struct ast_i2c_chan *i2c = data; >> + >> + i2c_del_adapter(&i2c->adapter); >> + i2c->dev = NULL; /* clear to signal absence of I2C support */ >> +} >> + >> static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) >> { >> int ret; >> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) >> >> i2c->dev = dev; /* signals presence of I2C support */ >> >> - return 0; >> + return drmm_add_action_or_reset(dev, ast_i2c_release, i2c); >> } >> >> static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) >> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) >> return !!i2c->dev; >> } >> >> -static void ast_i2c_fini(struct ast_i2c_chan *i2c) >> -{ >> - if (!ast_i2c_is_initialized(i2c)) >> - return; >> - i2c_del_adapter(&i2c->adapter); >> - i2c->dev = NULL; /* clear to signal absence of I2C support */ >> -} >> - >> /* >> * Primary plane >> */ >> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, >> >> static void ast_connector_destroy(struct drm_connector *connector) >> { >> - struct ast_connector *ast_connector = to_ast_connector(connector); >> - ast_i2c_fini(&ast_connector->i2c); >> drm_connector_cleanup(connector); >> kfree(connector); >> } >> -- >> 2.27.0 >> >
Hi Am 28.07.20 um 20:06 schrieb Sam Ravnborg: > On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote: >> Managed releases of the device's I2C adapter simplify the connector's >> release. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- >> 1 file changed, 10 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c >> index f421a60d8a96..27eb49bd12b3 100644 >> --- a/drivers/gpu/drm/ast/ast_mode.c >> +++ b/drivers/gpu/drm/ast/ast_mode.c >> @@ -39,6 +39,7 @@ >> #include <drm/drm_fourcc.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> #include <drm/drm_gem_vram_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_plane_helper.h> >> #include <drm/drm_probe_helper.h> >> #include <drm/drm_simple_kms_helper.h> >> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) >> } >> } >> >> +static void ast_i2c_release(struct drm_device *dev, void *data) >> +{ >> + struct ast_i2c_chan *i2c = data; >> + >> + i2c_del_adapter(&i2c->adapter); >> + i2c->dev = NULL; /* clear to signal absence of I2C support */ >> +} >> + >> static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) >> { >> int ret; >> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) >> >> i2c->dev = dev; /* signals presence of I2C support */ >> >> - return 0; >> + return drmm_add_action_or_reset(dev, ast_i2c_release, i2c); >> } >> >> static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) >> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) >> return !!i2c->dev; >> } >> >> -static void ast_i2c_fini(struct ast_i2c_chan *i2c) >> -{ >> - if (!ast_i2c_is_initialized(i2c)) >> - return; >> - i2c_del_adapter(&i2c->adapter); >> - i2c->dev = NULL; /* clear to signal absence of I2C support */ >> -} > The intro of ast_i2c_fini() and then removal again confuses me a little. > But end result looks simple so I guess thats what counts. The intention was to separate _fini from _destroy and make the patch series more readable. But looking at it now, this idea did not really work. I guess, I'll drop _fini. Best regards Thomas > > Sam > >> - >> /* >> * Primary plane >> */ >> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, >> >> static void ast_connector_destroy(struct drm_connector *connector) >> { >> - struct ast_connector *ast_connector = to_ast_connector(connector); >> - ast_i2c_fini(&ast_connector->i2c); >> drm_connector_cleanup(connector); >> kfree(connector); >> } >> -- >> 2.27.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index f421a60d8a96..27eb49bd12b3 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -39,6 +39,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data) } } +static void ast_i2c_release(struct drm_device *dev, void *data) +{ + struct ast_i2c_chan *i2c = data; + + i2c_del_adapter(&i2c->adapter); + i2c->dev = NULL; /* clear to signal absence of I2C support */ +} + static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) { int ret; @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev) i2c->dev = dev; /* signals presence of I2C support */ - return 0; + return drmm_add_action_or_reset(dev, ast_i2c_release, i2c); } static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c) return !!i2c->dev; } -static void ast_i2c_fini(struct ast_i2c_chan *i2c) -{ - if (!ast_i2c_is_initialized(i2c)) - return; - i2c_del_adapter(&i2c->adapter); - i2c->dev = NULL; /* clear to signal absence of I2C support */ -} - /* * Primary plane */ @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, static void ast_connector_destroy(struct drm_connector *connector) { - struct ast_connector *ast_connector = to_ast_connector(connector); - ast_i2c_fini(&ast_connector->i2c); drm_connector_cleanup(connector); kfree(connector); }
Managed releases of the device's I2C adapter simplify the connector's release. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)