Re: [PATCH 03/17] drm/mgag200: Embed connector instance in struct mga_device
Hi Thomas. On Wed, Apr 29, 2020 at 04:32:24PM +0200, Thomas Zimmermann wrote: > Storing the connector instance in struct mga_device avoids some > dynamic memory allocation. Done im preparation of converting > mgag200 to simple-KMS helpers. > > Signed-off-by: Thomas Zimmermann One nit below, with that fixed: Acked-by: Sam Ravnborg I expect to see mga_i2c_chan embedded in a later patch... Sam > --- > drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + > drivers/gpu/drm/mgag200/mgag200_mode.c | 54 ++ > 2 files changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h > b/drivers/gpu/drm/mgag200/mgag200_drv.h > index de3181bd63ca0..09b43a0ff6bbf 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h > @@ -164,6 +164,7 @@ struct mga_device { > /* SE model number stored in reg 0x1e24 */ > u32 unique_rev_id; > > + struct mga_connector connector; > struct drm_encoder encoder; > }; > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c > b/drivers/gpu/drm/mgag200/mgag200_mode.c > index ce41bebfdd1a2..eaa3fca7216ac 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -1444,6 +1444,10 @@ static void mga_crtc_init(struct mga_device *mdev) > drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs); > } > > +/* > + * Connector > + */ > + > static int mga_vga_get_modes(struct drm_connector *connector) > { > struct mga_connector *mga_connector = to_mga_connector(connector); > @@ -1568,7 +1572,6 @@ static void mga_connector_destroy(struct drm_connector > *connector) > struct mga_connector *mga_connector = to_mga_connector(connector); > mgag200_i2c_destroy(mga_connector->i2c); > drm_connector_cleanup(connector); > - kfree(connector); > } > > static const struct drm_connector_helper_funcs > mga_vga_connector_helper_funcs = { > @@ -1582,37 +1585,39 @@ static const struct drm_connector_funcs > mga_vga_connector_funcs = { > .destroy = mga_connector_destroy, > }; > > -static struct drm_connector *mga_vga_init(struct drm_device *dev) > +static int mgag200_vga_connector_init(struct mga_device *mdev) > { > - struct drm_connector *connector; > - struct mga_connector *mga_connector; > - > - mga_connector = kzalloc(sizeof(struct mga_connector), GFP_KERNEL); > - if (!mga_connector) > - return NULL; > - > - connector = &mga_connector->base; > - mga_connector->i2c = mgag200_i2c_create(dev); > - if (!mga_connector->i2c) > - DRM_ERROR("failed to add ddc bus\n"); > + struct drm_device *dev = mdev->dev; > + struct mga_connector *mconnector = &mdev->connector; > + struct drm_connector *connector = &mconnector->base; > + struct mga_i2c_chan *i2c; > + int ret; > > - drm_connector_init_with_ddc(dev, connector, > - &mga_vga_connector_funcs, > - DRM_MODE_CONNECTOR_VGA, > - &mga_connector->i2c->adapter); > + i2c = mgag200_i2c_create(dev); > + if (!i2c) > + drm_warn(dev, "failed to add DDC bus\n"); > > + ret = drm_connector_init_with_ddc(dev, connector, > + &mga_vga_connector_funcs, > + DRM_MODE_CONNECTOR_VGA, > + &i2c->adapter); > + if (ret) > + goto err_mgag200_i2c_destroy; > drm_connector_helper_add(connector, &mga_vga_connector_helper_funcs); > > - drm_connector_register(connector); > + mconnector->i2c = i2c; > > - return connector; > -} > + return 0; > > +err_mgag200_i2c_destroy: > + mgag200_i2c_destroy(i2c); > + return ret; > +} > > int mgag200_modeset_init(struct mga_device *mdev) > { > struct drm_encoder *encoder = &mdev->encoder; > - struct drm_connector *connector; > + struct drm_connector *connector = &mdev->connector.base; > int ret; > > mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH; > @@ -1632,9 +1637,10 @@ int mgag200_modeset_init(struct mga_device *mdev) > } > encoder->possible_crtcs = 0x1; > > - connector = mga_vga_init(mdev->dev); > - if (!connector) { > - DRM_ERROR("mga_vga_init failed\n"); > + ret = mgag200_vga_connector_init(mdev); > + if (ret) { > + drm_err(mdev->dev, > + "mga_vga_connector_init() failed, error %d\n", ret); s/mga_vga_connector_init/mgag200_vga_connector_init/ > return -1; > } > > -- > 2.26.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 03/17] drm/mgag200: Embed connector instance in struct mga_device
>-Original Message- >From: dri-devel On Behalf Of >Thomas Zimmermann >Sent: Wednesday, April 29, 2020 10:32 AM >To: airl...@redhat.com; dan...@ffwll.ch; kra...@redhat.com; >nor...@tronnes.org; s...@ravnborg.org; john.p.donne...@oracle.com >Cc: Thomas Zimmermann ; dri- >de...@lists.freedesktop.org >Subject: [PATCH 03/17] drm/mgag200: Embed connector instance in struct >mga_device > >Storing the connector instance in struct mga_device avoids some >dynamic memory allocation. Done im preparation of converting s/im/in/ You might also want to comment that you clean up the i2c info on error. Straight forward replacement of pointer with an embedded data structure: Reviewed-by: Michael J. Ruhl M >mgag200 to simple-KMS helpers. > >Signed-off-by: Thomas Zimmermann >--- > drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + > drivers/gpu/drm/mgag200/mgag200_mode.c | 54 ++--- >- > 2 files changed, 31 insertions(+), 24 deletions(-) > >diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h >b/drivers/gpu/drm/mgag200/mgag200_drv.h >index de3181bd63ca0..09b43a0ff6bbf 100644 >--- a/drivers/gpu/drm/mgag200/mgag200_drv.h >+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h >@@ -164,6 +164,7 @@ struct mga_device { > /* SE model number stored in reg 0x1e24 */ > u32 unique_rev_id; > >+ struct mga_connector connector; > struct drm_encoder encoder; > }; > >diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c >b/drivers/gpu/drm/mgag200/mgag200_mode.c >index ce41bebfdd1a2..eaa3fca7216ac 100644 >--- a/drivers/gpu/drm/mgag200/mgag200_mode.c >+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >@@ -1444,6 +1444,10 @@ static void mga_crtc_init(struct mga_device >*mdev) > drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs); > } > >+/* >+ * Connector >+ */ >+ > static int mga_vga_get_modes(struct drm_connector *connector) > { > struct mga_connector *mga_connector = >to_mga_connector(connector); >@@ -1568,7 +1572,6 @@ static void mga_connector_destroy(struct >drm_connector *connector) > struct mga_connector *mga_connector = >to_mga_connector(connector); > mgag200_i2c_destroy(mga_connector->i2c); > drm_connector_cleanup(connector); >- kfree(connector); > } > > static const struct drm_connector_helper_funcs >mga_vga_connector_helper_funcs = { >@@ -1582,37 +1585,39 @@ static const struct drm_connector_funcs >mga_vga_connector_funcs = { > .destroy = mga_connector_destroy, > }; > >-static struct drm_connector *mga_vga_init(struct drm_device *dev) >+static int mgag200_vga_connector_init(struct mga_device *mdev) > { >- struct drm_connector *connector; >- struct mga_connector *mga_connector; >- >- mga_connector = kzalloc(sizeof(struct mga_connector), >GFP_KERNEL); >- if (!mga_connector) >- return NULL; >- >- connector = &mga_connector->base; >- mga_connector->i2c = mgag200_i2c_create(dev); >- if (!mga_connector->i2c) >- DRM_ERROR("failed to add ddc bus\n"); >+ struct drm_device *dev = mdev->dev; >+ struct mga_connector *mconnector = &mdev->connector; >+ struct drm_connector *connector = &mconnector->base; >+ struct mga_i2c_chan *i2c; >+ int ret; > >- drm_connector_init_with_ddc(dev, connector, >- &mga_vga_connector_funcs, >- DRM_MODE_CONNECTOR_VGA, >- &mga_connector->i2c->adapter); >+ i2c = mgag200_i2c_create(dev); >+ if (!i2c) >+ drm_warn(dev, "failed to add DDC bus\n"); > >+ ret = drm_connector_init_with_ddc(dev, connector, >+&mga_vga_connector_funcs, >+DRM_MODE_CONNECTOR_VGA, >+&i2c->adapter); >+ if (ret) >+ goto err_mgag200_i2c_destroy; > drm_connector_helper_add(connector, >&mga_vga_connector_helper_funcs); > >- drm_connector_register(connector); >+ mconnector->i2c = i2c; > >- return connector; >-} >+ return 0; > >+err_mgag200_i2c_destroy: >+ mgag200_i2c_destroy(i2c); >+ return ret; >+} > > int mgag200_modeset_init(struct mga_device *mdev) > { > struct drm_encoder *encoder = &mdev->encoder; >- struct drm_connector *connector; >+ struct drm_connector *connector = &mdev->connector.base; > int ret; > > mdev->dev->mode_config.max_width = >MGAG200_MAX_FB_WIDTH; >@@ -1632,9 +1637,10 @@ int mgag200_modeset_init(struct mga_device >*mdev) > } > encoder->possible_crtcs = 0x1; > >- connector = mga_vga_init(mdev->dev); >- if (!connector) { >- DRM_ERROR("mga_vga_init failed\n"); >+ ret = mgag200_vga_connector_init(mdev); >+ if (ret) { >+ drm_err(mdev->dev, >+ "mga_vga_connector_init() failed, error %d\n", ret); > return -1; > } > >-- >2.26.0 > >_