by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH 09/22] Input: apanel - switch to using polled mode of input devices

Hi Sven,

On Mon, Oct 21, 2019 at 04:05:40PM -0400, Sven Van Asbroeck wrote:
> Hi Dmitry, a few minor nags below, inline.

Thanks for taking a look.

>
> On Thu, Oct 17, 2019 at 4:42 PM Dmitry Torokhov
> <[email protected]> wrote:
> >
> > We have added polled mode to the normal input devices with the intent of
> > retiring input_polled_dev. This converts apanel driver to use the polling
> > mode of standard input devices and removes dependency on INPUT_POLLDEV.
> >
> > While at it, let's convert the driver to use devm.
> >
> > Signed-off-by: Dmitry Torokhov <[email protected]>
> > ---
> >
> > drivers/input/misc/Kconfig | 1 -
> > drivers/input/misc/apanel.c | 153 +++++++++++++++---------------------
> > 2 files changed, 64 insertions(+), 90 deletions(-)
> >
> > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > index 7d9ae394e597..a6b8af69915d 100644
> > --- a/drivers/input/misc/Kconfig
> > +++ b/drivers/input/misc/Kconfig
> > @@ -257,7 +257,6 @@ config INPUT_MMA8450
> > config INPUT_APANEL
> > tristate "Fujitsu Lifebook Application Panel buttons"
> > depends on X86 && I2C && LEDS_CLASS
> > - select INPUT_POLLDEV
> > select CHECK_SIGNATURE
> > help
> > Say Y here for support of the Application Panel buttons, used on
> > diff --git a/drivers/input/misc/apanel.c b/drivers/input/misc/apanel.c
> > index 53ec40d1b90d..de0deb432fe3 100644
> > --- a/drivers/input/misc/apanel.c
> > +++ b/drivers/input/misc/apanel.c
> > @@ -17,7 +17,7 @@
> > #include <linux/module.h>
> > #include <linux/ioport.h>
> > #include <linux/io.h>
> > -#include <linux/input-polldev.h>
> > +#include <linux/input.h>
> > #include <linux/i2c.h>
> > #include <linux/leds.h>
> >
> > @@ -51,19 +51,28 @@ static enum apanel_chip device_chip[APANEL_DEV_MAX];
> > #define MAX_PANEL_KEYS 12
> >
> > struct apanel {
> > - struct input_polled_dev *ipdev;
> > + struct input_dev *idev;
> > struct i2c_client *client;
> > unsigned short keymap[MAX_PANEL_KEYS];
> > - u16 nkeys;
> > + u16 nkeys;
> > struct led_classdev mail_led;
> > };
> >
> > +static unsigned short apanel_keymap[MAX_PANEL_KEYS] = {
>
> Could this be const?

Yes, I believe so.

>
> > + [0] = KEY_MAIL,
> > + [1] = KEY_WWW,
> > + [2] = KEY_PROG2,
> > + [3] = KEY_PROG1,
> >
> > -static int apanel_probe(struct i2c_client *, const struct i2c_device_id *);
> > + [8] = KEY_FORWARD,
> > + [9] = KEY_REWIND,
> > + [10] = KEY_STOPCD,
> > + [11] = KEY_PLAYPAUSE,
> > +};
> >
> > static void report_key(struct input_dev *input, unsigned keycode)
> > {
> > - pr_debug(APANEL ": report key %#x\n", keycode);
> > + dev_dbg(input->dev.parent, "report key %#x\n", keycode);
> > input_report_key(input, keycode, 1);
> > input_sync(input);
> >
> > @@ -79,10 +88,9 @@ static void report_key(struct input_dev *input, unsigned keycode)
> > * CD keys:
> > * Forward (0x100), Rewind (0x200), Stop (0x400), Pause (0x800)
> > */
> > -static void apanel_poll(struct input_polled_dev *ipdev)
> > +static void apanel_poll(struct input_dev *idev)
> > {
> > - struct apanel *ap = ipdev->private;
> > - struct input_dev *idev = ipdev->input;
> > + struct apanel *ap = input_get_drvdata(idev);
> > u8 cmd = device_chip[APANEL_DEV_APPBTN] == CHIP_OZ992C ? 0 : 8;
> > s32 data;
> > int i;
> > @@ -112,126 +120,93 @@ static int mail_led_set(struct led_classdev *led,
> > return i2c_smbus_write_word_data(ap->client, 0x10, led_bits);
> > }
> >
> > -static int apanel_remove(struct i2c_client *client)
> > -{
> > - struct apanel *ap = i2c_get_clientdata(client);
> > -
> > - if (device_chip[APANEL_DEV_LED] != CHIP_NONE)
> > - led_classdev_unregister(&ap->mail_led);
> > -
> > - input_unregister_polled_device(ap->ipdev);
> > - input_free_polled_device(ap->ipdev);
> > -
> > - return 0;
> > -}
> > -
> > -static void apanel_shutdown(struct i2c_client *client)
> > -{
> > - apanel_remove(client);
> > -}
> > -
> > -static const struct i2c_device_id apanel_id[] = {
> > - { "fujitsu_apanel", 0 },
> > - { }
> > -};
> > -MODULE_DEVICE_TABLE(i2c, apanel_id);
> > -
> > -static struct i2c_driver apanel_driver = {
> > - .driver = {
> > - .name = APANEL,
> > - },
> > - .probe = &apanel_probe,
> > - .remove = &apanel_remove,
> > - .shutdown = &apanel_shutdown,
> > - .id_table = apanel_id,
> > -};
> > -
> > -static struct apanel apanel = {
> > - .keymap = {
> > - [0] = KEY_MAIL,
> > - [1] = KEY_WWW,
> > - [2] = KEY_PROG2,
> > - [3] = KEY_PROG1,
> > -
> > - [8] = KEY_FORWARD,
> > - [9] = KEY_REWIND,
> > - [10] = KEY_STOPCD,
> > - [11] = KEY_PLAYPAUSE,
> > -
> > - },
> > - .mail_led = {
> > - .name = "mail:blue",
> > - .brightness_set_blocking = mail_led_set,
> > - },
> > -};
> > -
> > -/* NB: Only one panel on the i2c. */
> > static int apanel_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > struct apanel *ap;
> > - struct input_polled_dev *ipdev;
> > struct input_dev *idev;
> > u8 cmd = device_chip[APANEL_DEV_APPBTN] == CHIP_OZ992C ? 0 : 8;
> > - int i, err = -ENOMEM;
> > + int i, err;
> >
> > - ap = &apanel;
> > + ap = devm_kzalloc(&client->dev, sizeof(*ap), GFP_KERNEL);
> > + if (!ap)
> > + return -ENOMEM;
> >
> > - ipdev = input_allocate_polled_device();
> > - if (!ipdev)
> > - goto out1;
> > + idev = devm_input_allocate_device(&client->dev);
> > + if (!idev)
> > + return -ENOMEM;
> >
> > - ap->ipdev = ipdev;
> > + ap->idev = idev;
> > ap->client = client;
> >
> > i2c_set_clientdata(client, ap);
> >
> > err = i2c_smbus_write_word_data(client, cmd, 0);
> > if (err) {
> > - dev_warn(&client->dev, APANEL ": smbus write error %d\n",
> > - err);
> > - goto out3;
> > + dev_warn(&client->dev, "smbus write error %d\n", err);
> > + return err;
> > }
> >
> > - ipdev->poll = apanel_poll;
> > - ipdev->poll_interval = POLL_INTERVAL_DEFAULT;
> > - ipdev->private = ap;
> > + input_set_drvdata(idev, ap);
> >
> > - idev = ipdev->input;
> > idev->name = APANEL_NAME " buttons";
> > idev->phys = "apanel/input0";
> > idev->id.bustype = BUS_HOST;
> > - idev->dev.parent = &client->dev;
> > -
> > - set_bit(EV_KEY, idev->evbit);
> >
> > + memcpy(ap->keymap, apanel_keymap, sizeof(apanel_keymap));
> > idev->keycode = ap->keymap;
> > idev->keycodesize = sizeof(ap->keymap[0]);
> > idev->keycodemax = (device_chip[APANEL_DEV_CDBTN] != CHIP_NONE) ? 12 : 4;
> >
> > + set_bit(EV_KEY, idev->evbit);
> > for (i = 0; i < idev->keycodemax; i++)
> > if (ap->keymap[i])
> > set_bit(ap->keymap[i], idev->keybit);
> >
> > - err = input_register_polled_device(ipdev);
> > + err = input_setup_polling(idev, apanel_poll);
> > + if (err)
> > + return err;
> > +
> > + input_set_poll_interval(idev, POLL_INTERVAL_DEFAULT);
> > +
> > + err = input_register_device(idev);
> > if (err)
> > - goto out3;
> > + return err;
> >
> > if (device_chip[APANEL_DEV_LED] != CHIP_NONE) {
> > - err = led_classdev_register(&client->dev, &ap->mail_led);
> > + ap->mail_led.name = "mail:blue";
> > + ap->mail_led.brightness_set_blocking = mail_led_set;
> > + err = devm_led_classdev_register(&client->dev, &ap->mail_led);
> > if (err)
> > - goto out4;
> > + return err;
> > }
> >
> > return 0;
> > -out4:
> > - input_unregister_polled_device(ipdev);
> > -out3:
> > - input_free_polled_device(ipdev);
> > -out1:
> > - return err;
> > }
> >
> > +static void apanel_shutdown(struct i2c_client *client)
> > +{
> > + struct apanel *ap = i2c_get_clientdata(client);
> > +
> > + if (device_chip[APANEL_DEV_LED] != CHIP_NONE)
> > + led_set_brightness(&ap->mail_led, LED_OFF);
>
> Is this code required? Doesn't led_class_dev_unregister()
> (automatically called by devm_) switch the led off?
>
> If it's not required, then the whole shutdown function can
> be removed.

No, it can't as it is not remove() but shutdown() method, the driver is
not being unbound from the device, and devm teardown is not executed
here.

>
> > +}
> > +
> > +static const struct i2c_device_id apanel_id[] = {
> > + { "fujitsu_apanel", 0 },
>
> If the driver_data field of i2c_device_id is unused, would it
> be better to match via apanel_driver.driver.of_match_table ?

This is X86 driver, and the device is instantiated from
drivers/i2c/busses/i2c-i801.c based on DMI match, so using OF is not
really helpful here.

>
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, apanel_id);
> > +
> > +static struct i2c_driver apanel_driver = {
> > + .driver = {
> > + .name = APANEL,
> > + },
> > + .probe = &apanel_probe,
> > + .shutdown = &apanel_shutdown,
>
> The ampersand (&) can be removed.
> I realize that this is just old code that has changed position.

Yeah, I'll drop them.

>
> > + .id_table = apanel_id,
> > +};
> > +
> > /* Scan the system ROM for the signature "FJKEYINF" */
> > static __init const void __iomem *bios_signature(const void __iomem *bios)
> > {
> > --
> > 2.23.0.866.gb869b98d4c-goog
> >

Thanks.

--
Dmitry

2019-10-22 13:58:22

by Sven Van Asbroeck

[permalink] [raw]

Subject: Re: [PATCH 09/22] Input: apanel - switch to using polled mode of input devices

On Mon, Oct 21, 2019 at 5:27 PM Dmitry Torokhov
<[email protected]> wrote:
>
> > > +static const struct i2c_device_id apanel_id[] = {
> > > + { "fujitsu_apanel", 0 },
> >
> > If the driver_data field of i2c_device_id is unused, would it
> > be better to match via apanel_driver.driver.of_match_table ?
>
> This is X86 driver, and the device is instantiated from
> drivers/i2c/busses/i2c-i801.c based on DMI match, so using OF is not
> really helpful here.
>

You're right of course! This driver doesn't even select/need CONFIG_OF :)