| Message ID | 20190302004935.28906-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/vlv: Move czclk to intel_pm | expand |
On Fri, 01 Mar 2019, José Roberto de Souza <jose.souza@intel.com> wrote: > i915_load_modeset_init() sounds horrible also lets rename it so > the future cleanup function of it can be easially recognized. We load the driver, but init modeset. The name implies modeset init at driver load. The modeset load/unload sounds a bit odd to me. Even more so with begin/finish load and begin unload. It's not obvious to me what those should do. They're not a pattern we have. I'm not all that averse to renaming stuff in general, but any rename increases the cognitive burden for a while, and should make things *easier* to understand. I don't think that's the case in this series. I can understand i915_load_modeset_init sounding funny. If you insist on renaming, I'd go with i915_driver_modeset_init. It's in line with the rest of the functions. Here are my suggestions for the names: i915_modeset_load/i915_modeset_begin_load -> i915_driver_modeset_init i915_modeset_unload/i915_modeset_begin_unload -> i915_driver_modeset_cleanup or _fini i915_modeset_finish_load -> i915_driver_modeset_init_late BR, Jani. > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index c08abdef5eb6..90c77fab3d70 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -639,7 +639,7 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { > .can_switch = i915_switcheroo_can_switch, > }; > > -static int i915_load_modeset_init(struct drm_device *dev) > +static int i915_modeset_load(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = to_i915(dev); > struct pci_dev *pdev = dev_priv->drm.pdev; > @@ -1748,7 +1748,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) > if (ret < 0) > goto out_cleanup_mmio; > > - ret = i915_load_modeset_init(&dev_priv->drm); > + ret = i915_modeset_load(&dev_priv->drm); > if (ret < 0) > goto out_cleanup_hw;
On Mon, Mar 04, 2019 at 01:00:40PM +0200, Jani Nikula wrote: >On Fri, 01 Mar 2019, José Roberto de Souza <jose.souza@intel.com> wrote: >> i915_load_modeset_init() sounds horrible also lets rename it so >> the future cleanup function of it can be easially recognized. > >We load the driver, but init modeset. The name implies modeset init at >driver load. > >The modeset load/unload sounds a bit odd to me. Even more so with >begin/finish load and begin unload. It's not obvious to me what those >should do. They're not a pattern we have. > >I'm not all that averse to renaming stuff in general, but any rename >increases the cognitive burden for a while, and should make things >*easier* to understand. I don't think that's the case in this series. > >I can understand i915_load_modeset_init sounding funny. If you insist on >renaming, I'd go with i915_driver_modeset_init. It's in line with the >rest of the functions. > >Here are my suggestions for the names: > >i915_modeset_load/i915_modeset_begin_load >-> i915_driver_modeset_init > >i915_modeset_unload/i915_modeset_begin_unload >-> i915_driver_modeset_cleanup or _fini > >i915_modeset_finish_load >-> i915_driver_modeset_init_late +1 for the name suggestions Lucas De Marchi > > >BR, >Jani. > > >> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index c08abdef5eb6..90c77fab3d70 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -639,7 +639,7 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { >> .can_switch = i915_switcheroo_can_switch, >> }; >> >> -static int i915_load_modeset_init(struct drm_device *dev) >> +static int i915_modeset_load(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = to_i915(dev); >> struct pci_dev *pdev = dev_priv->drm.pdev; >> @@ -1748,7 +1748,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) >> if (ret < 0) >> goto out_cleanup_mmio; >> >> - ret = i915_load_modeset_init(&dev_priv->drm); >> + ret = i915_modeset_load(&dev_priv->drm); >> if (ret < 0) >> goto out_cleanup_hw; > >-- >Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index c08abdef5eb6..90c77fab3d70 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -639,7 +639,7 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { .can_switch = i915_switcheroo_can_switch, }; -static int i915_load_modeset_init(struct drm_device *dev) +static int i915_modeset_load(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev; @@ -1748,7 +1748,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret < 0) goto out_cleanup_mmio; - ret = i915_load_modeset_init(&dev_priv->drm); + ret = i915_modeset_load(&dev_priv->drm); if (ret < 0) goto out_cleanup_hw;
i915_load_modeset_init() sounds horrible also lets rename it so the future cleanup function of it can be easially recognized. Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)