Re: [PATCH v2 2/2] drm/i915: Introduce intel_reg_types.h

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

On Fri, 23 Aug 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 22 Aug 2019, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
>> On 8/20/19 11:00 AM, Daniele Ceraolo Spurio wrote:
>>> 
>>> 
>>> On 8/20/19 8:42 AM, Michal Wajdeczko wrote:
>>>> On Tue, 20 Aug 2019 04:01:47 +0200, Daniele Ceraolo Spurio 
>>>> <daniele.ceraolospurio@intel.com> wrote:
>>>>
>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_reg_types.h 
>>>>> b/drivers/gpu/drm/i915/intel_reg_types.h
>>>>> new file mode 100644
>>>>> index 000000000000..87bce80dd5ed
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/i915/intel_reg_types.h
>>>>
>>>>
>>>>> +
>>>>> +typedef struct {
>>>>> +    u32 reg;
>>>>> +} i915_reg_t;
>>>>> +
>>>>> +#define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>>>>> +
>>>>> +#define INVALID_MMIO_REG _MMIO(0)
>>>>> +
>>>>> +static inline u32 i915_mmio_reg_offset(i915_reg_t reg)
>>>>> +{
>>>>> +    return reg.reg;
>>>>> +}
>>>>> +
>>>>> +static inline bool i915_mmio_reg_equal(i915_reg_t a, i915_reg_t b)
>>>>> +{
>>>>> +    return i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b);
>>>>> +}
>>>>> +
>>>>> +static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>>>>> +{
>>>>> +    return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>>>>> +}
>>>>> +
>>>>
>>>> hmm, there is now disconnection between prefixes in:
>>>>
>>>>      'intel'_reg_types.h
>>>> and
>>>>      'i915'_reg_t
>>>>      'i915'_mmio_reg_xxx()
>>>>
>>>> that is why I was suggesting to keep:
>>>>
>>>>      'i915'_reg.h (or at your preference 'i915'_reg_types.h)
>>>> with
>>>>      'i915'_reg_t
>>>>      'i915'_mmio_reg_xxx()
>>>>
>>>> and use intel_reg* files for actual hw definitions.
>>>>
>>>> if we don't plan to rename i915_reg_t into intel_mmio_reg_t
>>>> then maybe better to stay with i915_reg_types.h ?
>>>>
>>> 
>>> I'd personally prefer to keep the intel_* prefix and flip i915_reg_t to 
>>> intel_reg_t (as a second step to keep things simple). But given the size 
>>> of the change I'd prefer to hear some more opinions before going through 
>>> with it, so I'll wait a bit for more comments.
>>> 
>>> Daniele
>>> 
>>
>> Chris, Jani, are you ok if I got with Michal's suggestion for now, i.e. 
>> i915_reg_types.h and intel_reg.h?
>
> There's really nothing in this patch that requires you to rename
> i915_reg.h at all. The subject of the patch is about adding a new file
> for the types; the rename seems like an afterthought.
>
> I guess we'll add a display/<something>_reg.h later. But that doesn't
> require this rename either.

To clarify, I think you can just extract i915_reg_types.h (i915
referring to the *driver* here) from i915_reg.h for starters, and
continue with extracting registers to separate files without having to
rename i915_reg.h. Make sense?

BR,
Jani.




>
> BR,
> Jani.
>
>
>
>>
>> Daniele
>>
>>>> Michal
>>>>
>>>> ps. i915/intel prefix rules are killing me too ;)
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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