Re: [RFC v2 1/4] drm/i915/display: add support for DMC wakelocks

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 16 Jan 2024, "Coelho, Luciano" <luciano.coelho@intel.com> wrote:
> On Tue, 2024-01-16 at 14:28 +0200, Jani Nikula wrote:
>> On Thu, 11 Jan 2024, Luca Coelho <luciano.coelho@intel.com> wrote:
>> > diff --git a/drivers/gpu/drm/i915/display/intel_wakelock.h b/drivers/gpu/drm/i915/display/intel_wakelock.h
>> > new file mode 100644
>> > index 000000000000..a47205e1ea32
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/i915/display/intel_wakelock.h
>> > @@ -0,0 +1,34 @@
>> > +/* SPDX-License-Identifier: MIT */
>> > +/*
>> > + * Copyright (C) 2023 Intel Corporation
>> > + */
>> > +
>> > +#ifndef __INTEL_WAKELOCK_H__
>> > +#define __INTEL_WAKELOCK_H__
>> > +
>> > +#include <linux/types.h>
>> > +#include <linux/workqueue.h>
>> > +#include <linux/refcount.h>
>> > +
>> > +#include "i915_reg_defs.h"
>> > +
>> > +struct drm_i915_private;
>> > +
>> > +struct intel_display_wl {
>> > +   spinlock_t lock;
>> > +   bool enabled;
>> > +   refcount_t refcount;
>> > +   struct delayed_work work;
>> > +};
>> > +
>> > +struct intel_display_wl_range {
>> > +   u32 start;
>> > +   u32 end;
>> > +};
>> > +
>> > +void intel_display_wl_init(struct drm_i915_private *i915);
>> > +void intel_display_wl_enable(struct drm_i915_private *i915);
>> > +void intel_display_wl_get(struct drm_i915_private *i915, i915_reg_t reg);
>> > +void intel_display_wl_put(struct drm_i915_private *i915, i915_reg_t reg);
>>
>> Nitpick, I'll want the name prefix to match the filename,
>> i.e. intel_foo.[ch] have functions named intel_foo_*.
>
> Okay, so intel_wakelock_*(), specifically in this case?

Yes, though my comment was rather more generic.

As to the actual naming here, I think the first question really is if
"intel wakelock" is specific enough for what you're suggesting.

If yes, I do prefer wakelock over wl. But I don't know. :)

BR,
Jani.



>
> I can also rename the file to intel_wl.[ch] if we want to save
> horizontal real estate, in which case the function names would be
> intel_wl_*(), right?








>
> Thanks!
>
> --
> Cheers,
> Luca.

-- 
Jani Nikula, Intel