diff mbox series

[2/4] ARM: ep93xx: keypad: stop using mach/platform.h

Message ID 20190415192734.935387-2-arnd@arndb.de
State Accepted
Commit dfb6db007a56998e53e5ba5fb798b2e830b7feca
Headers show
Series [1/4] ARM: ep93xx: move network platform data to separate header | expand

Commit Message

Arnd Bergmann April 15, 2019, 7:25 p.m. UTC
We can communicate the clock rate using platform data rather than setting
a flag to use a particular value in the driver, which is cleaner and
avoids the dependency.

No platform in the kernel currently defines the ep93xx keypad device
structure, so this is a rather pointless excercise.  Any out of tree
users are probably dead now, but if not, they have to change their
platform code to match the new platform_data structure.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 drivers/input/keyboard/Kconfig              | 2 +-
 drivers/input/keyboard/ep93xx_keypad.c      | 5 +----
 include/linux/platform_data/keypad-ep93xx.h | 4 ++--
 3 files changed, 4 insertions(+), 7 deletions(-)

-- 
2.20.0

Comments

Hartley Sweeten April 15, 2019, 7:58 p.m. UTC | #1
On Monday, April 15, 2019 12:47 PM, Arnd Bergmann wrote:
> On Mon, Apr 15, 2019 at 9:39 PM Hartley Sweeten <HartleyS@visionengravers.com> wrote:

>>> -#define EP93XX_KEYPAD_KDIV           (1<<4)  /* 1/4 clock or 1/16 clock */

>>> -#define EP93XX_KEYPAD_AUTOREPEAT     (1<<5)  /* enable key autorepeat */

>>> +#define EP93XX_KEYPAD_AUTOREPEAT     (1<<4)  /* enable key autorepeat */

>>

>> You have re-defined the keypad register bits here.

>>

>> The KDIV bit changes the clock rate. The AUTOREPEAT bit enables key autorepeat.

>

> As far as I can tell, they are not register bits, just software flags for communicating between a

> board file and the driver, so I assumed I could freely reorganize them.

>

> Did I miss something?


Ugh.. It's been quite a while since I looked at that code...

Your correct, these are just flags to the driver.

The KeyScanInit register does have some bits that these flags effect but the driver deals
with them.

I have been buried updating an old PowerPC hardware/software design and haven't looked
at the EP93xx lately. My EP9307 board does have the keypad interface. Hopefully I will get
some time to check the latest mainline on it sometime soon.

Overall these patches look good. So, for the series...

Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Guenter Roeck April 15, 2019, 8:01 p.m. UTC | #2
On Mon, Apr 15, 2019 at 12:56 PM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
>

> Hi!

>

> On 15/04/2019 21:47, Arnd Bergmann wrote:

> >>> We can communicate the clock rate using platform data rather than setting a

> >>> flag to use a particular value in the driver, which is cleaner and avoids the dependency.

> >>>

> >>> No platform in the kernel currently defines the ep93xx keypad device structure, so this

> >>> is a rather pointless excercise.  Any out of tree users are probably dead now, but if not,

> >>>  they have to change their platform code to match the new platform_data structure.

> >> <snip>

> >>

> >>> diff --git a/include/linux/platform_data/keypad-ep93xx.h b/include/linux/platform_data/keypad-ep93xx.h

> >>> index 0e36818e3680..3054fced8509 100644

> >>> --- a/include/linux/platform_data/keypad-ep93xx.h

> >>> +++ b/include/linux/platform_data/keypad-ep93xx.h

> >>> @@ -9,8 +9,7 @@ struct matrix_keymap_data;

> >>>  #define EP93XX_KEYPAD_DIAG_MODE              (1<<1)  /* diagnostic mode */

> >>>  #define EP93XX_KEYPAD_BACK_DRIVE     (1<<2)  /* back driving mode */

> >>>  #define EP93XX_KEYPAD_TEST_MODE              (1<<3)  /* scan only column 0 */

> >>> -#define EP93XX_KEYPAD_KDIV           (1<<4)  /* 1/4 clock or 1/16 clock */

> >>> -#define EP93XX_KEYPAD_AUTOREPEAT     (1<<5)  /* enable key autorepeat */

> >>> +#define EP93XX_KEYPAD_AUTOREPEAT     (1<<4)  /* enable key autorepeat */

> >> You have re-defined the keypad register bits here.

> >>

> >> The KDIV bit changes the clock rate. The AUTOREPEAT bit enables key autorepeat.

> > As far as I can tell, they are not register bits, just software flags

> > for communicating between a board file and the driver, so I

> > assumed I could freely reorganize them.

> >

> > Did I miss something?

>

> They are indeed only software flags (just checked datasheet), so you are only changing

> platform data format. But as I do not know any keypad user in person, I'd rely on

> Hartley's opinion in this case (if it's a good idea to change platform data or not).

>


If there are out-of-tree users, it would be their responsibility to
upstream their code. If they don't, any changes in platform data is
their problem, not ours. Either case, platform data does, if anything,
reflect an in-kernel API, and thus is fair game for cleanup.

Guenter
diff mbox series

Patch

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index a878351f1643..b373f3274542 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -194,7 +194,7 @@  config KEYBOARD_LKKBD
 
 config KEYBOARD_EP93XX
 	tristate "EP93xx Matrix Keypad support"
-	depends on ARCH_EP93XX
+	depends on ARCH_EP93XX || COMPILE_TEST
 	select INPUT_MATRIXKMAP
 	help
 	  Say Y here to enable the matrix keypad on the Cirrus EP93XX.
diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c
index f77b295e0123..71472f6257c0 100644
--- a/drivers/input/keyboard/ep93xx_keypad.c
+++ b/drivers/input/keyboard/ep93xx_keypad.c
@@ -137,10 +137,7 @@  static void ep93xx_keypad_config(struct ep93xx_keypad *keypad)
 	struct ep93xx_keypad_platform_data *pdata = keypad->pdata;
 	unsigned int val = 0;
 
-	if (pdata->flags & EP93XX_KEYPAD_KDIV)
-		clk_set_rate(keypad->clk, EP93XX_KEYTCHCLK_DIV4);
-	else
-		clk_set_rate(keypad->clk, EP93XX_KEYTCHCLK_DIV16);
+	clk_set_rate(keypad->clk, pdata->clk_rate);
 
 	if (pdata->flags & EP93XX_KEYPAD_DISABLE_3_KEY)
 		val |= KEY_INIT_DIS3KY;
diff --git a/include/linux/platform_data/keypad-ep93xx.h b/include/linux/platform_data/keypad-ep93xx.h
index 0e36818e3680..3054fced8509 100644
--- a/include/linux/platform_data/keypad-ep93xx.h
+++ b/include/linux/platform_data/keypad-ep93xx.h
@@ -9,8 +9,7 @@  struct matrix_keymap_data;
 #define EP93XX_KEYPAD_DIAG_MODE		(1<<1)	/* diagnostic mode */
 #define EP93XX_KEYPAD_BACK_DRIVE	(1<<2)	/* back driving mode */
 #define EP93XX_KEYPAD_TEST_MODE		(1<<3)	/* scan only column 0 */
-#define EP93XX_KEYPAD_KDIV		(1<<4)	/* 1/4 clock or 1/16 clock */
-#define EP93XX_KEYPAD_AUTOREPEAT	(1<<5)	/* enable key autorepeat */
+#define EP93XX_KEYPAD_AUTOREPEAT	(1<<4)	/* enable key autorepeat */
 
 /**
  * struct ep93xx_keypad_platform_data - platform specific device structure
@@ -24,6 +23,7 @@  struct ep93xx_keypad_platform_data {
 	unsigned int	debounce;
 	unsigned int	prescale;
 	unsigned int	flags;
+	unsigned int	clk_rate;
 };
 
 #define EP93XX_MATRIX_ROWS		(8)