[Intel-gfx] [PATCH v2 2/2] drm/i915: Introduce intel_reg_types.h

[Michal Wajdeczko]

On Fri, 23 Aug 2019 16:55:21 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio at intel.com> wrote:

>
>
> On 8/23/19 5:36 AM, Jani Nikula wrote:
>> On Fri, 23 Aug 2019, Jani Nikula <jani.nikula at intel.com> wrote:
>>> On Thu, 22 Aug 2019, Daniele Ceraolo Spurio  
>>> <daniele.ceraolospurio at 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 at 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?
>>
>
> Yes, that's what v1 did, but then I got feedback from Michal to rename  
> i915_reg.h to intel_reg.h. Will flip back to v1 for now and then  
> reconsider the naming once i915_reg.h has been broken up a bit more.
>
> Michal, any objection?

There was some misunderstanding here.
But using i915_reg_types.h for 'i915' types is fine for me.

Moving hw register definitions to 'intel' files (to match other files
and naming) can be done later (ended by killing empty i915_reg.h)

Michal

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