linux-kernel - Re: [PATCH v2 01/49] staging: sm750fb: Update dvi_ctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Apr 2021 10:31:21 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Pavle Rohalj <pavle.rohalj@...il.com>
Cc:     sudipm.mukherjee@...il.com, teddy.wang@...iconmotion.com,
        linux-fbdev@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 01/49] staging: sm750fb: Update dvi_ctrl_device to
 snake case

On Tue, Apr 06, 2021 at 11:35:56PM -0700, Pavle Rohalj wrote:
> Fix "Avoid CamelCase" checkpatch.pl checks for dvi_ctrl_device structure and
> its usages.
> 
> Signed-off-by: Pavle Rohalj <pavle.rohalj@...il.com>
> ---
>  drivers/staging/sm750fb/ddk750_dvi.c    | 30 ++++++++--------
>  drivers/staging/sm750fb/ddk750_dvi.h    | 20 +++++------
>  drivers/staging/sm750fb/ddk750_sii164.c | 48 ++++++++++++-------------
>  drivers/staging/sm750fb/ddk750_sii164.h | 20 +++++------
>  4 files changed, 59 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/ddk750_dvi.c b/drivers/staging/sm750fb/ddk750_dvi.c
> index cd564ea40779..db19bf732482 100644
> --- a/drivers/staging/sm750fb/ddk750_dvi.c
> +++ b/drivers/staging/sm750fb/ddk750_dvi.c
> @@ -11,20 +11,20 @@
>   * function API. Please set the function pointer to NULL whenever the function
>   * is not supported.
>   */
> -static struct dvi_ctrl_device g_dcftSupportedDviController[] = {
> +static struct dvi_ctrl_device dcft_supported_dvi_controller[] = {

Why the "dcft_" prefix?  We know this is a "dvi control device" by the
fact that the type says it is :)

>  #ifdef DVI_CTRL_SII164
>  	{
> -		.pfnInit = sii164InitChip,
> -		.pfnGetVendorId = sii164GetVendorID,
> -		.pfnGetDeviceId = sii164GetDeviceID,
> +		.pfn_init = sii164_init_chip,
> +		.pfn_get_vendor_id = sii164_get_vendor_id,
> +		.pfn_get_device_id = sii164_get_device_id,
>  #ifdef SII164_FULL_FUNCTIONS
> -		.pfnResetChip = sii164ResetChip,
> -		.pfnGetChipString = sii164GetChipString,
> -		.pfnSetPower = sii164SetPower,
> -		.pfnEnableHotPlugDetection = sii164EnableHotPlugDetection,
> -		.pfnIsConnected = sii164IsConnected,
> -		.pfnCheckInterrupt = sii164CheckInterrupt,
> -		.pfnClearInterrupt = sii164ClearInterrupt,
> +		.pfn_reset_chip = sii164_reset_chip,
> +		.pfn_get_chip_string = sii164_get_chip_string,
> +		.pfn_set_power = sii164_set_power,
> +		.pfn_enable_hot_plug_detection = sii164_enable_hot_plug_detection,
> +		.pfn_is_connected = sii164_is_connected,
> +		.pfn_check_interrupt = sii164_check_interrupt,
> +		.pfn_clear_interrupt = sii164_clear_interrupt,
>  #endif
>  	},
>  #endif
> @@ -41,11 +41,11 @@ int dviInit(unsigned char edge_select,
>  	    unsigned char pll_filter_enable,
>  	    unsigned char pll_filter_value)
>  {
> -	struct dvi_ctrl_device *pCurrentDviCtrl;
> +	struct dvi_ctrl_device *current_dvi_ctrl;
>  
> -	pCurrentDviCtrl = g_dcftSupportedDviController;
> -	if (pCurrentDviCtrl->pfnInit) {
> -		return pCurrentDviCtrl->pfnInit(edge_select,
> +	current_dvi_ctrl = dcft_supported_dvi_controller;
> +	if (current_dvi_ctrl->pfn_init) {
> +		return current_dvi_ctrl->pfn_init(edge_select,
>  						bus_select,
>  						dual_edge_clk_select,
>  						hsync_enable,
> diff --git a/drivers/staging/sm750fb/ddk750_dvi.h b/drivers/staging/sm750fb/ddk750_dvi.h
> index 1c7a565b617a..4ca2591ea94b 100644
> --- a/drivers/staging/sm750fb/ddk750_dvi.h
> +++ b/drivers/staging/sm750fb/ddk750_dvi.h
> @@ -27,16 +27,16 @@ typedef void (*PFN_DVICTRL_CLEARINTERRUPT)(void);
>  
>  /* Structure to hold all the function pointer to the DVI Controller. */
>  struct dvi_ctrl_device {
> -	PFN_DVICTRL_INIT		pfnInit;
> -	PFN_DVICTRL_RESETCHIP		pfnResetChip;
> -	PFN_DVICTRL_GETCHIPSTRING	pfnGetChipString;
> -	PFN_DVICTRL_GETVENDORID		pfnGetVendorId;
> -	PFN_DVICTRL_GETDEVICEID		pfnGetDeviceId;
> -	PFN_DVICTRL_SETPOWER		pfnSetPower;
> -	PFN_DVICTRL_HOTPLUGDETECTION	pfnEnableHotPlugDetection;
> -	PFN_DVICTRL_ISCONNECTED		pfnIsConnected;
> -	PFN_DVICTRL_CHECKINTERRUPT	pfnCheckInterrupt;
> -	PFN_DVICTRL_CLEARINTERRUPT	pfnClearInterrupt;
> +	PFN_DVICTRL_INIT		pfn_init;

"pfn_" means "pointer to a function" which is not needed at all.  Just
make this be "init".

And the whole crazy "PFN_DVICTRL_INIT" also is not needed, just put the
real function prototype in here so that we don't have to unwind the mess
to look it up.

So, this line would look like:
	void (*init)(void);

Much smaller, more obvious, matches the kernel coding style, and is way
easier to understand exactly what is happening here.

Typedefs can be used to hide complexity, but here they are just adding
it, for no good reason at all.

I appreciate long patch series being sent out, but maybe make them
smaller so you do not have to redo 49 patches because you are asked to
make a change on the very first patch like here.  Perhaps stick to 20
max for a bit until you get the process down and understand more about
what the kernel programming style is?

thanks,

greg k-h