diff mbox series

[1/8] staging: sm250fb: remove USE_HW_I2C check

Message ID 20250417190302.13811-2-rubenru09@aol.com
State New
Headers show
Series staging: sm750fb: cleanup ddk750_sii164 | expand

Commit Message

Ruben Wauters April 17, 2025, 7:02 p.m. UTC 
Removes the USE_HW_I2C check and function defines in
ddk750_sii164.c.

The software equivalents were never used due to
USE_HW_I2C being defined just before the ifdef, meaning
the hardware versions were always used.

The define names were also triggering checkpatch.pl's
camel case check.

Signed-off-by: Ruben Wauters <rubenru09@aol.com>

---

I am somewhat unsure whether this is the way to go or
the correct way would be to add an option/opportunity for
the software version to be used. Currently the hardware
version is always used, but I am unsure if there ever even
would be a case where you would want to use the software
version over the hardware version.

I do not have the hardware in question so I cannot test
what the difference between the two versions exactly is.

I also note that the removal is mentioned in the TODO,
however once again this is currently hardcoded.
---
 drivers/staging/sm750fb/ddk750_sii164.c | 63 ++++++++++---------------
 1 file changed, 24 insertions(+), 39 deletions(-)

Comments

Ruben Wauters April 18, 2025, 11:42 a.m. UTC | #1 
On Fri, 2025-04-18 at 12:33 +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 17, 2025 at 08:02:49PM +0100, Ruben Wauters wrote:
> > Removes the USE_HW_I2C check and function defines in
> > ddk750_sii164.c.
> > 
> > The software equivalents were never used due to
> > USE_HW_I2C being defined just before the ifdef, meaning
> > the hardware versions were always used.
> > 
> > The define names were also triggering checkpatch.pl's
> > camel case check.
> > 
> > Signed-off-by: Ruben Wauters <rubenru09@aol.com>
> > 
> > ---
> > 
> > I am somewhat unsure whether this is the way to go or
> > the correct way would be to add an option/opportunity for
> > the software version to be used. Currently the hardware
> > version is always used, but I am unsure if there ever even
> > would be a case where you would want to use the software
> > version over the hardware version.
> 
> Then the code can be added back, not an issue.
> 
> But you forgot this same check in
> drivers/staging/sm750fb/ddk750_hwi2c.c, right?

This check can indeed be removed I suppose, might be worth also
removing the USE_DVICHIP checks also, especially when defined

There's also a USE_HW_I2C check in ddk750.h, which defines which header
to use, however I'm somewhat unsure exactly what the best way to go
about addressing that is, since it's not defined before including it
does make me wonder whether the HW files are even used at all.

Something about this seems entirely wrong to me, again I don't have the
hardware to test, but why would SW files be used when the HW files work
fine? Is it intended that the HW files are used instead? it's a bit
inconsistent, especially since in ddk750_sii164.c the HW ones are
explicitly used over the SW ones
Why is the SW files preferred in sm750_hw.c then?
I believe this is something of an oversight and the files should
probably use the HW ones instead of the SW ones.

I am curious to know your thoughts on this (and anyone else's)

> Also, what about removing the sm750_sw_i2c_write_reg() and other
> functions that are now never referenced?  Can you add that to this
> patch
> series?  A single series that just removes the use of USE_HW_I2C and
> the
> now unneeded functions would be best, as it's not really a "coding
> style" fix, but rather a code cleanup thing, right?

I will create a separate patch series for removing unneeded code, since
it does look like a lot more code removal might be needed than I
originally thought.

> thanks,
> 
> greg k-h
diff mbox series

Patch

diff --git a/drivers/staging/sm750fb/ddk750_sii164.c b/drivers/staging/sm750fb/ddk750_sii164.c
index 89700fc5dd2e..dd7811b18bf6 100644
--- a/drivers/staging/sm750fb/ddk750_sii164.c
+++ b/drivers/staging/sm750fb/ddk750_sii164.c
@@ -8,17 +8,6 @@ 
 /* I2C Address of each SII164 chip */
 #define SII164_I2C_ADDRESS                  0x70
 
-/* Define this definition to use hardware i2c. */
-#define USE_HW_I2C
-
-#ifdef USE_HW_I2C
-    #define i2cWriteReg sm750_hw_i2c_write_reg
-    #define i2cReadReg  sm750_hw_i2c_read_reg
-#else
-    #define i2cWriteReg sm750_sw_i2c_write_reg
-    #define i2cReadReg  sm750_sw_i2c_read_reg
-#endif
-
 /* SII164 Vendor and Device ID */
 #define SII164_VENDOR_ID                    0x0001
 #define SII164_DEVICE_ID                    0x0006
@@ -39,10 +28,10 @@  unsigned short sii164_get_vendor_id(void)
 {
 	unsigned short vendorID;
 
-	vendorID = ((unsigned short)i2cReadReg(SII164_I2C_ADDRESS,
-					       SII164_VENDOR_ID_HIGH) << 8) |
-		   (unsigned short)i2cReadReg(SII164_I2C_ADDRESS,
-					      SII164_VENDOR_ID_LOW);
+	vendorID = ((unsigned short)sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS,
+							  SII164_VENDOR_ID_HIGH) << 8) |
+		   (unsigned short)sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS,
+							 SII164_VENDOR_ID_LOW);
 
 	return vendorID;
 }
@@ -58,10 +47,10 @@  unsigned short sii164_get_device_id(void)
 {
 	unsigned short device_id;
 
-	device_id = ((unsigned short)i2cReadReg(SII164_I2C_ADDRESS,
-					       SII164_DEVICE_ID_HIGH) << 8) |
-		   (unsigned short)i2cReadReg(SII164_I2C_ADDRESS,
-					      SII164_DEVICE_ID_LOW);
+	device_id = ((unsigned short)sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS,
+							   SII164_DEVICE_ID_HIGH) << 8) |
+		   (unsigned short)sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS,
+							 SII164_DEVICE_ID_LOW);
 
 	return device_id;
 }
@@ -132,12 +121,8 @@  long sii164_init_chip(unsigned char edge_select,
 	unsigned char config;
 
 	/* Initialize the i2c bus */
-#ifdef USE_HW_I2C
 	/* Use fast mode. */
 	sm750_hw_i2c_init(1);
-#else
-	sm750_sw_i2c_init(DEFAULT_I2C_SCL, DEFAULT_I2C_SDA);
-#endif
 
 	/* Check if SII164 Chip exists */
 	if ((sii164_get_vendor_id() == SII164_VENDOR_ID) &&
@@ -176,7 +161,7 @@  long sii164_init_chip(unsigned char edge_select,
 		else
 			config |= SII164_CONFIGURATION_VSYNC_AS_IS;
 
-		i2cWriteReg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
+		sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
 
 		/*
 		 * De-skew enabled with default 111b value.
@@ -214,7 +199,7 @@  long sii164_init_chip(unsigned char edge_select,
 			config |= SII164_DESKEW_8_STEP;
 			break;
 		}
-		i2cWriteReg(SII164_I2C_ADDRESS, SII164_DESKEW, config);
+		sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_DESKEW, config);
 
 		/* Enable/Disable Continuous Sync. */
 		if (continuous_sync_enable == 0)
@@ -231,12 +216,12 @@  long sii164_init_chip(unsigned char edge_select,
 		/* Set the PLL Filter value */
 		config |= ((pll_filter_value & 0x07) << 1);
 
-		i2cWriteReg(SII164_I2C_ADDRESS, SII164_PLL, config);
+		sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_PLL, config);
 
 		/* Recover from Power Down and enable output. */
-		config = i2cReadReg(SII164_I2C_ADDRESS, SII164_CONFIGURATION);
+		config = sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS, SII164_CONFIGURATION);
 		config |= SII164_CONFIGURATION_POWER_NORMAL;
-		i2cWriteReg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
+		sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
 
 		return 0;
 	}
@@ -283,17 +268,17 @@  void sii164_set_power(unsigned char powerUp)
 {
 	unsigned char config;
 
-	config = i2cReadReg(SII164_I2C_ADDRESS, SII164_CONFIGURATION);
+	config = sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS, SII164_CONFIGURATION);
 	if (powerUp == 1) {
 		/* Power up the chip */
 		config &= ~SII164_CONFIGURATION_POWER_MASK;
 		config |= SII164_CONFIGURATION_POWER_NORMAL;
-		i2cWriteReg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
+		sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
 	} else {
 		/* Power down the chip */
 		config &= ~SII164_CONFIGURATION_POWER_MASK;
 		config |= SII164_CONFIGURATION_POWER_DOWN;
-		i2cWriteReg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
+		sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_CONFIGURATION, config);
 	}
 }
 
@@ -306,7 +291,7 @@  void sii164SelectHotPlugDetectionMode(enum sii164_hot_plug_mode hotPlugMode)
 {
 	unsigned char detectReg;
 
-	detectReg = i2cReadReg(SII164_I2C_ADDRESS, SII164_DETECT) &
+	detectReg = sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS, SII164_DETECT) &
 		    ~SII164_DETECT_MONITOR_SENSE_OUTPUT_FLAG;
 	switch (hotPlugMode) {
 	case SII164_HOTPLUG_DISABLE:
@@ -325,7 +310,7 @@  void sii164SelectHotPlugDetectionMode(enum sii164_hot_plug_mode hotPlugMode)
 		break;
 	}
 
-	i2cWriteReg(SII164_I2C_ADDRESS, SII164_DETECT, detectReg);
+	sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_DETECT, detectReg);
 }
 
 /*
@@ -338,7 +323,7 @@  void sii164_enable_hot_plug_detection(unsigned char enable_hot_plug)
 {
 	unsigned char detectReg;
 
-	detectReg = i2cReadReg(SII164_I2C_ADDRESS, SII164_DETECT);
+	detectReg = sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS, SII164_DETECT);
 
 	/* Depending on each DVI controller, need to enable the hot plug based
 	 * on each individual chip design.
@@ -361,7 +346,7 @@  unsigned char sii164_is_connected(void)
 {
 	unsigned char hotPlugValue;
 
-	hotPlugValue = i2cReadReg(SII164_I2C_ADDRESS, SII164_DETECT) &
+	hotPlugValue = sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS, SII164_DETECT) &
 		       SII164_DETECT_HOT_PLUG_STATUS_MASK;
 	if (hotPlugValue == SII164_DETECT_HOT_PLUG_STATUS_ON)
 		return 1;
@@ -381,7 +366,7 @@  unsigned char sii164_check_interrupt(void)
 {
 	unsigned char detectReg;
 
-	detectReg = i2cReadReg(SII164_I2C_ADDRESS, SII164_DETECT) &
+	detectReg = sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS, SII164_DETECT) &
 		    SII164_DETECT_MONITOR_STATE_MASK;
 	if (detectReg == SII164_DETECT_MONITOR_STATE_CHANGE)
 		return 1;
@@ -398,9 +383,9 @@  void sii164_clear_interrupt(void)
 	unsigned char detectReg;
 
 	/* Clear the MDI interrupt */
-	detectReg = i2cReadReg(SII164_I2C_ADDRESS, SII164_DETECT);
-	i2cWriteReg(SII164_I2C_ADDRESS, SII164_DETECT,
-		    detectReg | SII164_DETECT_MONITOR_STATE_CLEAR);
+	detectReg = sm750_hw_i2c_read_reg(SII164_I2C_ADDRESS, SII164_DETECT);
+	sm750_hw_i2c_write_reg(SII164_I2C_ADDRESS, SII164_DETECT,
+			       detectReg | SII164_DETECT_MONITOR_STATE_CLEAR);
 }
 
 #endif