Re: [Intel-gfx] [PATCH 2/2] drm/i915/uncore: rename i915_reg_read_ioctl intel_uncore_reg_read_ioctl

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

On Wed, 05 Jan 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 05/01/2022 10:05, Jani Nikula wrote:
>> Follow the usual naming convention.
>
> But intel_uncore_ prefix usually means functions takes intel_uncore as 
> the first argument.
>
> Maybe solution here is that i915_reg_read_ioctl does not belong in 
> intel_uncore.c, it being the UAPI layer thing? I guess arguments could 
> be made for either way.

My position is that the function and file prefixes go hand in
hand. You'll always know where to place a function, and you'll always
know where the function is to be found.

If you can *also* make the context argument follow the pattern, it's
obviously better, and indicates the division to files is working out
nicely. However, in a lot of cases you'll need to pass struct
drm_i915_private or similar as the first parameter to e.g. init
functions. It can't be the rigid rule.

I'm fine with moving the entire function somewhere else, as long as the
declaration is not in i915_drv.h. There's no longer a i915_drv.c, and
i915_drv.h should not have function declarations at all.


BR,
Jani.

>
> Regards,
>
> Tvrtko
>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_driver.c  | 2 +-
>>   drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
>>   drivers/gpu/drm/i915/intel_uncore.h | 4 ++--
>>   3 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>> index 95174938b160..f9a494e159dc 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -1805,7 +1805,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>>   	DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE_EXT, i915_gem_context_create_ioctl, DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_RENDER_ALLOW),
>> -	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_RENDER_ALLOW),
>> +	DRM_IOCTL_DEF_DRV(I915_REG_READ, intel_uncore_reg_read_ioctl, DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_gem_context_reset_stats_ioctl, DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index fc25ebf1a593..33f95bb2d3d5 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -2269,8 +2269,8 @@ static const struct reg_whitelist {
>>   	.size = 8
>>   } };
>>   
>> -int i915_reg_read_ioctl(struct drm_device *dev,
>> -			void *data, struct drm_file *file)
>> +int intel_uncore_reg_read_ioctl(struct drm_device *dev,
>> +				void *data, struct drm_file *file)
>>   {
>>   	struct drm_i915_private *i915 = to_i915(dev);
>>   	struct intel_uncore *uncore = &i915->uncore;
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
>> index 3a87bbd906f8..697ac4586159 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>> @@ -457,7 +457,7 @@ static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
>>   #define raw_reg_write(base, reg, value) \
>>   	writel(value, base + i915_mmio_reg_offset(reg))
>>   
>> -int i915_reg_read_ioctl(struct drm_device *dev, void *data,
>> -			struct drm_file *file);
>> +int intel_uncore_reg_read_ioctl(struct drm_device *dev, void *data,
>> +				struct drm_file *file);
>>   
>>   #endif /* !__INTEL_UNCORE_H__ */
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center