| Message ID | 20190415192734.935387-2-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Monday, April 15, 2019 12:25 PM, 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. Hartley
On Mon, Apr 15, 2019 at 9:39 PM Hartley Sweeten <HartleyS@visionengravers.com> wrote: > > On Monday, April 15, 2019 12:25 PM, 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? Arnd
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). -- Alex.
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>
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
On Mon, Apr 15, 2019 at 09:25:24PM +0200, 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. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Please feel free to merge with the rest of the patches. > --- > 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(-) > > 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) > -- > 2.20.0 >
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)
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(-)