Re: [Intel-gfx] [PATCH 2/4] drm/i915: Consistent struct device naming

[Jani Nikula]

On Tue, 02 Aug 2016, Joonas Lahtinen  wrote:
> On ma, 2016-08-01 at 19:57 +0100, Chris Wilson wrote:
>> On Mon, Aug 01, 2016 at 06:38:53PM +0300, David Weinehall wrote:
>> > 
>> > We currently have a mix of struct device *device, struct device *kdev,
>> > and struct device *dev (the latter forcing us to refer to
>> > struct drm_device as something else than the normal dev).
>> > 
>> > To simplify things, always use kdev when referring to struct device.
>> > 
>> > Signed-off-by: David Weinehall 
>> kdev may be confused with kdev_t, but seems reasonable due to kobj.
>> 
>> This patch is an improvement, so
>> Reviewed-by: Chris Wilson 
>> 
>> but I was wondering if
>> 
>> struct device *dev;
>> struct drm_device *drm;
>> struct i915_device *i915;
>> 
>
> I'd vote for this scheme.

In the driver,

30 out of 52 struct device * decls use "dev".

1233 out of 1251 struct drm_device * decls use "dev".

2112 out of 2154 struct drm_i915_private * decls use "dev_priv".

By all means change struct device *dev to something else, like kdev, or
whatever.

But do we really not have better things to do than come up with ways to
create tons of rename churn, increasing the cognitive burden of
developers, forcing rebases of tons of in-flight code making it harder
to get code in, creating conflicts for backporting fixes, and so on?

I'm firmly behind sticking with struct drm_i915_private *dev_priv and
struct drm_device *dev.


BR,
Jani.

>
> Regards, Joonas
>
>> (struct i915_device is move apt now than drm_i915_private due to the
>> subclassing)
>> 
>> made more sense as a longterm goal?
>> -Chris
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Consistent struct device naming

[Joonas Lahtinen]

On ma, 2016-08-01 at 19:57 +0100, Chris Wilson wrote:
> On Mon, Aug 01, 2016 at 06:38:53PM +0300, David Weinehall wrote:
> > 
> > We currently have a mix of struct device *device, struct device *kdev,
> > and struct device *dev (the latter forcing us to refer to
> > struct drm_device as something else than the normal dev).
> > 
> > To simplify things, always use kdev when referring to struct device.
> > 
> > Signed-off-by: David Weinehall 
> kdev may be confused with kdev_t, but seems reasonable due to kobj.
> 
> This patch is an improvement, so
> Reviewed-by: Chris Wilson 
> 
> but I was wondering if
> 
> struct device *dev;
> struct drm_device *drm;
> struct i915_device *i915;
> 

I'd vote for this scheme.

Regards, Joonas

> (struct i915_device is move apt now than drm_i915_private due to the
> subclassing)
> 
> made more sense as a longterm goal?
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Consistent struct device naming

[Chris Wilson]

On Mon, Aug 01, 2016 at 06:38:53PM +0300, David Weinehall wrote:
> We currently have a mix of struct device *device, struct device *kdev,
> and struct device *dev (the latter forcing us to refer to
> struct drm_device as something else than the normal dev).
> 
> To simplify things, always use kdev when referring to struct device.
> 
> Signed-off-by: David Weinehall 

kdev may be confused with kdev_t, but seems reasonable due to kobj.

This patch is an improvement, so
Reviewed-by: Chris Wilson 

but I was wondering if

struct device *dev;
struct drm_device *drm;
struct i915_device *i915;

(struct i915_device is move apt now than drm_i915_private due to the
subclassing)

made more sense as a longterm goal?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/4] drm/i915: Consistent struct device naming

[David Weinehall]

We currently have a mix of struct device *device, struct device *kdev,
and struct device *dev (the latter forcing us to refer to
struct drm_device as something else than the normal dev).

To simplify things, always use kdev when referring to struct device.

Signed-off-by: David Weinehall 
---
 drivers/gpu/drm/i915/i915_drv.c | 96 -
 drivers/gpu/drm/i915/i915_drv.h |  4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c |  6 +--
 drivers/gpu/drm/i915/i915_sysfs.c   | 62 ++---
 drivers/gpu/drm/i915/intel_audio.c  | 38 ++---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 36 ++---
 6 files changed, 121 insertions(+), 121 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 83afdd0597b5..b186e342f548 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -77,7 +77,7 @@ __i915_printk(struct drm_i915_private *dev_priv, const char 
*level,
  const char *fmt, ...)
 {
static bool shown_bug_once;
-   struct device *dev = dev_priv->drm.dev;
+   struct device *kdev = dev_priv->drm.dev;
bool is_error = level[1] <= KERN_ERR[1];
bool is_debug = level[1] == KERN_DEBUG[1];
struct va_format vaf;
@@ -91,11 +91,11 @@ __i915_printk(struct drm_i915_private *dev_priv, const char 
*level,
vaf.fmt = fmt;
vaf.va = 
 
-   dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
+   dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV",
   __builtin_return_address(0), );
 
if (is_error && !shown_bug_once) {
-   dev_notice(dev, "%s", FDO_BUG_MSG);
+   dev_notice(kdev, "%s", FDO_BUG_MSG);
shown_bug_once = true;
}
 
@@ -1475,9 +1475,9 @@ out:
return error;
 }
 
-static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
+static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 {
-   struct drm_i915_private *dev_priv = to_i915(drm_dev);
+   struct drm_i915_private *dev_priv = to_i915(dev);
bool fw_csr;
int ret;
 
@@ -1511,7 +1511,7 @@ static int i915_drm_suspend_late(struct drm_device 
*drm_dev, bool hibernation)
goto out;
}
 
-   pci_disable_device(drm_dev->pdev);
+   pci_disable_device(dev->pdev);
/*
 * During hibernation on some platforms the BIOS may try to access
 * the device even though it's already in D3 and hang the machine. So
@@ -1525,7 +1525,7 @@ static int i915_drm_suspend_late(struct drm_device 
*drm_dev, bool hibernation)
 * Acer Aspire 1830T
 */
if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6))
-   pci_set_power_state(drm_dev->pdev, PCI_D3hot);
+   pci_set_power_state(dev->pdev, PCI_D3hot);
 
dev_priv->suspended_to_idle = suspend_to_idle(dev_priv);
 
@@ -1822,25 +1822,25 @@ error:
return ret;
 }
 
-static int i915_pm_suspend(struct device *dev)
+static int i915_pm_suspend(struct device *kdev)
 {
-   struct pci_dev *pdev = to_pci_dev(dev);
-   struct drm_device *drm_dev = pci_get_drvdata(pdev);
+   struct pci_dev *pdev = to_pci_dev(kdev);
+   struct drm_device *dev = pci_get_drvdata(pdev);
 
-   if (!drm_dev) {
-   dev_err(dev, "DRM not initialized, aborting suspend.\n");
+   if (!dev) {
+   dev_err(kdev, "DRM not initialized, aborting suspend.\n");
return -ENODEV;
}
 
-   if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+   if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
 
-   return i915_drm_suspend(drm_dev);
+   return i915_drm_suspend(dev);
 }
 
-static int i915_pm_suspend_late(struct device *dev)
+static int i915_pm_suspend_late(struct device *kdev)
 {
-   struct drm_device *drm_dev = _to_i915(dev)->drm;
+   struct drm_device *dev = _to_i915(kdev)->drm;
 
/*
 * We have a suspend ordering issue with the snd-hda driver also
@@ -1851,57 +1851,57 @@ static int i915_pm_suspend_late(struct device *dev)
 * FIXME: This should be solved with a special hdmi sink device or
 * similar so that power domains can be employed.
 */
-   if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+   if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
 
-   return i915_drm_suspend_late(drm_dev, false);
+   return i915_drm_suspend_late(dev, false);
 }
 
-static int i915_pm_poweroff_late(struct device *dev)
+static int i915_pm_poweroff_late(struct device *kdev)
 {
-   struct drm_device *drm_dev = _to_i915(dev)->drm;
+   struct drm_device *dev = _to_i915(kdev)->drm;
 
-   if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+   if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
return