2021-04-07 19:37:34

by Pavle Rohalj

[permalink] [raw]
Subject: [PATCH v2 01/49] staging: sm750fb: Update dvi_ctrl_device to snake case

Fix "Avoid CamelCase" checkpatch.pl checks for dvi_ctrl_device structure and
its usages.

Signed-off-by: Pavle Rohalj <[email protected]>
---
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[] = {
#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_DVICTRL_RESETCHIP pfn_reset_chip;
+ PFN_DVICTRL_GETCHIPSTRING pfn_get_chip_string;
+ PFN_DVICTRL_GETVENDORID pfn_get_vendor_id;
+ PFN_DVICTRL_GETDEVICEID pfn_get_device_id;
+ PFN_DVICTRL_SETPOWER pfn_set_power;
+ PFN_DVICTRL_HOTPLUGDETECTION pfn_enable_hot_plug_detection;
+ PFN_DVICTRL_ISCONNECTED pfn_is_connected;
+ PFN_DVICTRL_CHECKINTERRUPT pfn_check_interrupt;
+ PFN_DVICTRL_CLEARINTERRUPT pfn_clear_interrupt;
};

#define DVI_CTRL_SII164
diff --git a/drivers/staging/sm750fb/ddk750_sii164.c b/drivers/staging/sm750fb/ddk750_sii164.c
index 73e0e9f41ec5..6c343e2e0433 100644
--- a/drivers/staging/sm750fb/ddk750_sii164.c
+++ b/drivers/staging/sm750fb/ddk750_sii164.c
@@ -29,13 +29,13 @@ static char *gDviCtrlChipName = "Silicon Image SiI 164";
#endif

/*
- * sii164GetVendorID
+ * sii164_get_vendor_id
* This function gets the vendor ID of the DVI controller chip.
*
* Output:
* Vendor ID
*/
-unsigned short sii164GetVendorID(void)
+unsigned short sii164_get_vendor_id(void)
{
unsigned short vendorID;

@@ -48,13 +48,13 @@ unsigned short sii164GetVendorID(void)
}

/*
- * sii164GetDeviceID
+ * sii164_get_device_id
* This function gets the device ID of the DVI controller chip.
*
* Output:
* Device ID
*/
-unsigned short sii164GetDeviceID(void)
+unsigned short sii164_get_device_id(void)
{
unsigned short deviceID;

@@ -72,7 +72,7 @@ unsigned short sii164GetDeviceID(void)
*/

/*
- * sii164InitChip
+ * sii164_init_chip
* This function initialize and detect the DVI controller chip.
*
* Input:
@@ -118,7 +118,7 @@ unsigned short sii164GetDeviceID(void)
* 0 - Success
* -1 - Fail.
*/
-long sii164InitChip(unsigned char edge_select,
+long sii164_init_chip(unsigned char edge_select,
unsigned char bus_select,
unsigned char dual_edge_clk_select,
unsigned char hsync_enable,
@@ -140,8 +140,8 @@ long sii164InitChip(unsigned char edge_select,
#endif

/* Check if SII164 Chip exists */
- if ((sii164GetVendorID() == SII164_VENDOR_ID) &&
- (sii164GetDeviceID() == SII164_DEVICE_ID)) {
+ if ((sii164_get_vendor_id() == SII164_VENDOR_ID) &&
+ (sii164_get_device_id() == SII164_DEVICE_ID)) {
/*
* Initialize SII164 controller chip.
*/
@@ -250,36 +250,36 @@ long sii164InitChip(unsigned char edge_select,
#ifdef SII164_FULL_FUNCTIONS

/*
- * sii164ResetChip
+ * sii164_reset_chip
* This function resets the DVI Controller Chip.
*/
-void sii164ResetChip(void)
+void sii164_reset_chip(void)
{
/* Power down */
- sii164SetPower(0);
- sii164SetPower(1);
+ sii164_set_power(0);
+ sii164_set_power(1);
}

/*
- * sii164GetChipString
+ * sii164_get_chip_string
* This function returns a char string name of the current DVI Controller
* chip.
*
* It's convenient for application need to display the chip name.
*/
-char *sii164GetChipString(void)
+char *sii164_get_chip_string(void)
{
return gDviCtrlChipName;
}

/*
- * sii164SetPower
+ * sii164_set_power
* This function sets the power configuration of the DVI Controller Chip.
*
* Input:
* powerUp - Flag to set the power down or up
*/
-void sii164SetPower(unsigned char powerUp)
+void sii164_set_power(unsigned char powerUp)
{
unsigned char config;

@@ -329,12 +329,12 @@ void sii164SelectHotPlugDetectionMode(enum sii164_hot_plug_mode hotPlugMode)
}

/*
- * sii164EnableHotPlugDetection
+ * sii164_enable_hot_plug_detection
* This function enables the Hot Plug detection.
*
* enableHotPlug - Enable (=1) / disable (=0) Hot Plug detection
*/
-void sii164EnableHotPlugDetection(unsigned char enableHotPlug)
+void sii164_enable_hot_plug_detection(unsigned char enableHotPlug)
{
unsigned char detectReg;

@@ -350,14 +350,14 @@ void sii164EnableHotPlugDetection(unsigned char enableHotPlug)
}

/*
- * sii164IsConnected
+ * sii164_is_connected
* Check if the DVI Monitor is connected.
*
* Output:
* 0 - Not Connected
* 1 - Connected
*/
-unsigned char sii164IsConnected(void)
+unsigned char sii164_is_connected(void)
{
unsigned char hotPlugValue;

@@ -370,14 +370,14 @@ unsigned char sii164IsConnected(void)
}

/*
- * sii164CheckInterrupt
+ * sii164_check_interrupt
* Checks if interrupt has occurred.
*
* Output:
* 0 - No interrupt
* 1 - Interrupt occurs
*/
-unsigned char sii164CheckInterrupt(void)
+unsigned char sii164_check_interrupt(void)
{
unsigned char detectReg;

@@ -390,10 +390,10 @@ unsigned char sii164CheckInterrupt(void)
}

/*
- * sii164ClearInterrupt
+ * sii164_clear_interrupt
* Clear the hot plug interrupt.
*/
-void sii164ClearInterrupt(void)
+void sii164_clear_interrupt(void)
{
unsigned char detectReg;

diff --git a/drivers/staging/sm750fb/ddk750_sii164.h b/drivers/staging/sm750fb/ddk750_sii164.h
index d940cb729066..cf17b9029496 100644
--- a/drivers/staging/sm750fb/ddk750_sii164.h
+++ b/drivers/staging/sm750fb/ddk750_sii164.h
@@ -16,7 +16,7 @@ enum sii164_hot_plug_mode {
};

/* Silicon Image SiI164 chip prototype */
-long sii164InitChip(unsigned char edgeSelect,
+long sii164_init_chip(unsigned char edgeSelect,
unsigned char busSelect,
unsigned char dualEdgeClkSelect,
unsigned char hsyncEnable,
@@ -27,17 +27,17 @@ long sii164InitChip(unsigned char edgeSelect,
unsigned char pllFilterEnable,
unsigned char pllFilterValue);

-unsigned short sii164GetVendorID(void);
-unsigned short sii164GetDeviceID(void);
+unsigned short sii164_get_vendor_id(void);
+unsigned short sii164_get_device_id(void);

#ifdef SII164_FULL_FUNCTIONS
-void sii164ResetChip(void);
-char *sii164GetChipString(void);
-void sii164SetPower(unsigned char powerUp);
-void sii164EnableHotPlugDetection(unsigned char enableHotPlug);
-unsigned char sii164IsConnected(void);
-unsigned char sii164CheckInterrupt(void);
-void sii164ClearInterrupt(void);
+void sii164_reset_chip(void);
+char *sii164_get_chip_string(void);
+void sii164_set_power(unsigned char powerUp);
+void sii164_enable_hot_plug_detection(unsigned char enableHotPlug);
+unsigned char sii164_is_connected(void);
+unsigned char sii164_check_interrupt(void);
+void sii164_clear_interrupt(void);
#endif
/*
* below register definition is used for
--
2.30.2


2021-04-07 20:35:16

by Greg Kroah-Hartman

[permalink] [raw]
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 <[email protected]>
> ---
> 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

2021-04-07 20:36:37

by Pavle Rohalj

[permalink] [raw]
Subject: Re: [PATCH v2 01/49] staging: sm750fb: Update dvi_ctrl_device to snake case

On Wed, Apr 07, 2021 at 10:31:21AM +0200, Greg KH wrote:
> 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 <[email protected]>
> > ---
> > 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 :)
>

Didn't catch that--makes sense.

> > #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".
>

I changed that in one of the next few patches, but now I realize I should have
combined the two changes.

> 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

Sounds good. Thank you for the feedback. I will work more on this.

-Pavle