| Message ID | 20190118134244.22253-13-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mfd: add support for max77650 PMIC | expand |
Hi Bartosz, On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Add support for the push- and slide-button events for max77650. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/input/misc/Kconfig | 9 ++ > drivers/input/misc/Makefile | 1 + > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++ > 3 files changed, 145 insertions(+) > create mode 100644 drivers/input/misc/max77650-onkey.c > > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > index ca59a2be9bc5..bb9c45c1269e 100644 > --- a/drivers/input/misc/Kconfig > +++ b/drivers/input/misc/Kconfig > @@ -180,6 +180,15 @@ config INPUT_M68K_BEEP > tristate "M68k Beeper support" > depends on M68K > > +config INPUT_MAX77650_ONKEY > + tristate "Maxim MAX77650 ONKEY support" > + depends on MFD_MAX77650 > + help > + Support the ONKEY of the MAX77650 PMIC as an input device. > + > + To compile this driver as a module, choose M here: the module > + will be called max77650-onkey. > + > config INPUT_MAX77693_HAPTIC > tristate "MAXIM MAX77693/MAX77843 haptic controller support" > depends on (MFD_MAX77693 || MFD_MAX77843) && PWM > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile > index 9d0f9d1ff68f..5bd53590ce60 100644 > --- a/drivers/input/misc/Makefile > +++ b/drivers/input/misc/Makefile > @@ -43,6 +43,7 @@ obj-$(CONFIG_INPUT_IXP4XX_BEEPER) += ixp4xx-beeper.o > obj-$(CONFIG_INPUT_KEYSPAN_REMOTE) += keyspan_remote.o > obj-$(CONFIG_INPUT_KXTJ9) += kxtj9.o > obj-$(CONFIG_INPUT_M68K_BEEP) += m68kspkr.o > +obj-$(CONFIG_INPUT_MAX77650_ONKEY) += max77650-onkey.o > obj-$(CONFIG_INPUT_MAX77693_HAPTIC) += max77693-haptic.o > obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o > obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o > diff --git a/drivers/input/misc/max77650-onkey.c b/drivers/input/misc/max77650-onkey.c > new file mode 100644 > index 000000000000..cc7e83f589cd > --- /dev/null > +++ b/drivers/input/misc/max77650-onkey.c > @@ -0,0 +1,135 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 BayLibre SAS > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > + * > + * ONKEY driver for MAXIM 77650/77651 charger/power-supply. > + */ > + > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/max77650.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#define MAX77650_ONKEY_MODE_MASK BIT(3) > +#define MAX77650_ONKEY_MODE_PUSH 0x00 > +#define MAX77650_ONKEY_MODE_SLIDE BIT(3) > + > +struct max77650_onkey { > + struct input_dev *input; > + unsigned int code; > +}; > + > +static irqreturn_t > +max77650_onkey_report(struct max77650_onkey *onkey, int value) > +{ > + input_report_key(onkey->input, onkey->code, value); > + input_sync(onkey->input); > + > + return IRQ_HANDLED; It is weird that report function returns irqreturn_t. I'd simply moved input_report_key()/input_sync() into real IRQ handlers. > +} > + > +static irqreturn_t max77650_onkey_falling(int irq, void *data) > +{ > + struct max77650_onkey *onkey = data; > + > + return max77650_onkey_report(onkey, 0); > +} > + > +static irqreturn_t max77650_onkey_rising(int irq, void *data) > +{ > + struct max77650_onkey *onkey = data; > + > + return max77650_onkey_report(onkey, 1); > +} > + > +static int max77650_onkey_probe(struct platform_device *pdev) > +{ > + struct regmap_irq_chip_data *irq_data; > + struct max77650_onkey *onkey; > + struct device *dev, *parent; > + int irq_r, irq_f, rv, mode; Please call "rv" "error". > + struct i2c_client *i2c; > + const char *mode_prop; > + struct regmap *map; > + > + dev = &pdev->dev; > + parent = dev->parent; > + i2c = to_i2c_client(parent); > + irq_data = i2c_get_clientdata(i2c); > + > + map = dev_get_regmap(parent, NULL); > + if (!map) > + return -ENODEV; > + > + onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL); > + if (!onkey) > + return -ENOMEM; > + > + rv = device_property_read_u32(dev, "linux,code", &onkey->code); > + if (rv) > + onkey->code = KEY_POWER; > + > + rv = device_property_read_string(dev, "maxim,onkey-mode", &mode_prop); > + if (rv) > + mode_prop = "push"; > + > + if (strcmp(mode_prop, "push") == 0) > + mode = MAX77650_ONKEY_MODE_PUSH; > + else if (strcmp(mode_prop, "slide") == 0) > + mode = MAX77650_ONKEY_MODE_SLIDE; > + else > + return -EINVAL; > + > + rv = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL, > + MAX77650_ONKEY_MODE_MASK, mode); > + if (rv) > + return rv; > + > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F); > + if (irq_f <= 0) > + return -EINVAL; > + > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R); > + if (irq_r <= 0) > + return -EINVAL; Ugh, it would be better if you handled IRQ mapping in the MFD piece and passed it as resources of platform device. Then you'd simply call platform_get_irq() here and did not have to reach into parent device for "irq_dara". > + > + onkey->input = devm_input_allocate_device(dev); > + if (!onkey->input) > + return -ENOMEM; > + > + onkey->input->name = "max77650_onkey"; > + onkey->input->phys = "max77650_onkey/input0"; > + onkey->input->id.bustype = BUS_I2C; > + onkey->input->dev.parent = dev; Not needed since devm_input_allocate_device sets parent for you. > + input_set_capability(onkey->input, EV_KEY, onkey->code); > + > + rv = devm_request_threaded_irq(dev, irq_f, NULL, Why threaded interrupt with only hard interrupt handler? If parent interrupt is threaded use "any_context_irq" here. > + max77650_onkey_falling, > + IRQF_ONESHOT, "onkey-down", onkey); Why do you need oneshot with interrupt that is essentially not threaded? > + if (rv) > + return rv; > + > + rv = devm_request_threaded_irq(dev, irq_r, NULL, > + max77650_onkey_rising, > + IRQF_ONESHOT, "onkey-up", onkey); > + if (rv) > + return rv; > + > + return input_register_device(onkey->input); > +} > + > +static struct platform_driver max77650_onkey_driver = { > + .driver = { > + .name = "max77650-onkey", > + }, > + .probe = max77650_onkey_probe, > +}; > +module_platform_driver(max77650_onkey_driver); > + > +MODULE_DESCRIPTION("MAXIM 77650/77651 ONKEY driver"); > +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>"); > +MODULE_LICENSE("GPL"); SPDX header say GPL-2.0 so please "GPL v2" here as "GPL" means v2 and later. Thanks.
sob., 19 sty 2019 o 10:03 Dmitry Torokhov <dmitry.torokhov@gmail.com> napisał(a): > > Hi Bartosz, > > On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Add support for the push- and slide-button events for max77650. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/input/misc/Kconfig | 9 ++ > > drivers/input/misc/Makefile | 1 + > > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++ > > 3 files changed, 145 insertions(+) > > create mode 100644 drivers/input/misc/max77650-onkey.c > > > > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig > > index ca59a2be9bc5..bb9c45c1269e 100644 > > --- a/drivers/input/misc/Kconfig > > +++ b/drivers/input/misc/Kconfig > > @@ -180,6 +180,15 @@ config INPUT_M68K_BEEP > > tristate "M68k Beeper support" > > depends on M68K > > > > +config INPUT_MAX77650_ONKEY > > + tristate "Maxim MAX77650 ONKEY support" > > + depends on MFD_MAX77650 > > + help > > + Support the ONKEY of the MAX77650 PMIC as an input device. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called max77650-onkey. > > + > > config INPUT_MAX77693_HAPTIC > > tristate "MAXIM MAX77693/MAX77843 haptic controller support" > > depends on (MFD_MAX77693 || MFD_MAX77843) && PWM > > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile > > index 9d0f9d1ff68f..5bd53590ce60 100644 > > --- a/drivers/input/misc/Makefile > > +++ b/drivers/input/misc/Makefile > > @@ -43,6 +43,7 @@ obj-$(CONFIG_INPUT_IXP4XX_BEEPER) += ixp4xx-beeper.o > > obj-$(CONFIG_INPUT_KEYSPAN_REMOTE) += keyspan_remote.o > > obj-$(CONFIG_INPUT_KXTJ9) += kxtj9.o > > obj-$(CONFIG_INPUT_M68K_BEEP) += m68kspkr.o > > +obj-$(CONFIG_INPUT_MAX77650_ONKEY) += max77650-onkey.o > > obj-$(CONFIG_INPUT_MAX77693_HAPTIC) += max77693-haptic.o > > obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o > > obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o > > diff --git a/drivers/input/misc/max77650-onkey.c b/drivers/input/misc/max77650-onkey.c > > new file mode 100644 > > index 000000000000..cc7e83f589cd > > --- /dev/null > > +++ b/drivers/input/misc/max77650-onkey.c > > @@ -0,0 +1,135 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2018 BayLibre SAS > > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > + * > > + * ONKEY driver for MAXIM 77650/77651 charger/power-supply. > > + */ > > + > > +#include <linux/i2c.h> > > +#include <linux/input.h> > > +#include <linux/interrupt.h> > > +#include <linux/mfd/max77650.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > + > > +#define MAX77650_ONKEY_MODE_MASK BIT(3) > > +#define MAX77650_ONKEY_MODE_PUSH 0x00 > > +#define MAX77650_ONKEY_MODE_SLIDE BIT(3) > > + > > +struct max77650_onkey { > > + struct input_dev *input; > > + unsigned int code; > > +}; > > + > > +static irqreturn_t > > +max77650_onkey_report(struct max77650_onkey *onkey, int value) > > +{ > > + input_report_key(onkey->input, onkey->code, value); > > + input_sync(onkey->input); > > + > > + return IRQ_HANDLED; > > It is weird that report function returns irqreturn_t. I'd simply moved > input_report_key()/input_sync() into real IRQ handlers. > > > +} > > + > > +static irqreturn_t max77650_onkey_falling(int irq, void *data) > > +{ > > + struct max77650_onkey *onkey = data; > > + > > + return max77650_onkey_report(onkey, 0); > > +} > > + > > +static irqreturn_t max77650_onkey_rising(int irq, void *data) > > +{ > > + struct max77650_onkey *onkey = data; > > + > > + return max77650_onkey_report(onkey, 1); > > +} > > + > > +static int max77650_onkey_probe(struct platform_device *pdev) > > +{ > > + struct regmap_irq_chip_data *irq_data; > > + struct max77650_onkey *onkey; > > + struct device *dev, *parent; > > + int irq_r, irq_f, rv, mode; > > Please call "rv" "error". > > > + struct i2c_client *i2c; > > + const char *mode_prop; > > + struct regmap *map; > > + > > + dev = &pdev->dev; > > + parent = dev->parent; > > + i2c = to_i2c_client(parent); > > + irq_data = i2c_get_clientdata(i2c); > > + > > + map = dev_get_regmap(parent, NULL); > > + if (!map) > > + return -ENODEV; > > + > > + onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL); > > + if (!onkey) > > + return -ENOMEM; > > + > > + rv = device_property_read_u32(dev, "linux,code", &onkey->code); > > + if (rv) > > + onkey->code = KEY_POWER; > > + > > + rv = device_property_read_string(dev, "maxim,onkey-mode", &mode_prop); > > + if (rv) > > + mode_prop = "push"; > > + > > + if (strcmp(mode_prop, "push") == 0) > > + mode = MAX77650_ONKEY_MODE_PUSH; > > + else if (strcmp(mode_prop, "slide") == 0) > > + mode = MAX77650_ONKEY_MODE_SLIDE; > > + else > > + return -EINVAL; > > + > > + rv = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL, > > + MAX77650_ONKEY_MODE_MASK, mode); > > + if (rv) > > + return rv; > > + > > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F); > > + if (irq_f <= 0) > > + return -EINVAL; > > + > > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R); > > + if (irq_r <= 0) > > + return -EINVAL; > > Ugh, it would be better if you handled IRQ mapping in the MFD piece and > passed it as resources of platform device. Then you'd simply call > platform_get_irq() here and did not have to reach into parent device for > "irq_dara". > > > + > > + onkey->input = devm_input_allocate_device(dev); > > + if (!onkey->input) > > + return -ENOMEM; > > + > > + onkey->input->name = "max77650_onkey"; > > + onkey->input->phys = "max77650_onkey/input0"; > > + onkey->input->id.bustype = BUS_I2C; > > + onkey->input->dev.parent = dev; > > Not needed since devm_input_allocate_device sets parent for you. > > > + input_set_capability(onkey->input, EV_KEY, onkey->code); > > + > > + rv = devm_request_threaded_irq(dev, irq_f, NULL, > > Why threaded interrupt with only hard interrupt handler? If parent > interrupt is threaded use "any_context_irq" here. > Hi Dmitry, actually it's the other way around. Take a look at the function prototype for devm_request_threaded_irq()[1]. The third parameter is the hard-irq handler (NULL in my patch), the fourth is the thread function. Actually even if I did what you're saying - it would never work as this is a nested irq for which the hard-irq handler is never called. For the rest: I'll fix it in v2. Best regards, Bartosz Golaszewski [1] https://elixir.bootlin.com/linux/latest/source/kernel/irq/devres.c#L52 > > + max77650_onkey_falling, > > + IRQF_ONESHOT, "onkey-down", onkey); > > Why do you need oneshot with interrupt that is essentially not threaded? > > > + if (rv) > > + return rv; > > + > > + rv = devm_request_threaded_irq(dev, irq_r, NULL, > > + max77650_onkey_rising, > > + IRQF_ONESHOT, "onkey-up", onkey); > > + if (rv) > > + return rv; > > + > > + return input_register_device(onkey->input); > > +} > > + > > +static struct platform_driver max77650_onkey_driver = { > > + .driver = { > > + .name = "max77650-onkey", > > + }, > > + .probe = max77650_onkey_probe, > > +}; > > +module_platform_driver(max77650_onkey_driver); > > + > > +MODULE_DESCRIPTION("MAXIM 77650/77651 ONKEY driver"); > > +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>"); > > +MODULE_LICENSE("GPL"); > > SPDX header say GPL-2.0 so please "GPL v2" here as "GPL" means v2 and > later. > > Thanks. > > -- > Dmitry
On Mon, Jan 21, 2019 at 11:52:50AM +0100, Bartosz Golaszewski wrote: > sob., 19 sty 2019 o 10:03 Dmitry Torokhov <dmitry.torokhov@gmail.com> > napisał(a): > > > > Hi Bartosz, > > > > On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote: > > > + input_set_capability(onkey->input, EV_KEY, onkey->code); > > > + > > > + rv = devm_request_threaded_irq(dev, irq_f, NULL, > > > > Why threaded interrupt with only hard interrupt handler? If parent > > interrupt is threaded use "any_context_irq" here. > > > > Hi Dmitry, > > actually it's the other way around. Take a look at the function > prototype for devm_request_threaded_irq()[1]. > > The third parameter is the hard-irq handler (NULL in my patch), the > fourth is the thread function. Actually even if I did what you're > saying - it would never work as this is a nested irq for which the > hard-irq handler is never called. Sorry, my eyes must have crossed. Still, from the driver POV the interrupt does not have to be threaded, this is dictated by the constraints beyond the driver control. For these cases we have devm_request_any_context_irq() that takes essentially only "hard" IRQ handler, but internally either does request_irq() or request_threaded_irq(), depending on the context (whether the interrupt is nested or not). Using devm_request_any_context_irq() should not have any behavioral changes, but documents the logic better. Thanks.
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Add support for the push- and slide-button events for max77650. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/input/misc/Kconfig | 9 ++ > > drivers/input/misc/Makefile | 1 + > > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++ > > 3 files changed, 145 insertions(+) > > create mode 100644 drivers/input/misc/max77650-onkey.c [...] Moving things around a bit: > > +static int max77650_onkey_probe(struct platform_device *pdev) > > +{ > > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F); > > + if (irq_f <= 0) > > + return -EINVAL; > > + > > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R); > > + if (irq_r <= 0) > > + return -EINVAL; > > Ugh, it would be better if you handled IRQ mapping in the MFD piece and > passed it as resources of platform device. Then you'd simply call > platform_get_irq() here and did not have to reach into parent device for > "irq_dara". These device IRQs were defined and registered with the Regmap *set* (actually init()) APIs and thus should be pulled out using the appropriate reverse Regmap *get* APIs here in the device. Registering them with Regmap *and* pulling them back out again in the same (MFD in this case) driver, only to register them as platform data is certainly not how I see the API being designed/used. > > + struct regmap_irq_chip_data *irq_data; > > + struct device *dev, *parent; > > + dev = &pdev->dev; > > + parent = dev->parent; > > + i2c = to_i2c_client(parent); > > + irq_data = i2c_get_clientdata(i2c); > > + > > + map = dev_get_regmap(parent, NULL); > > + if (!map) > > + return -ENODEV; However, this hoop jumping is a bit crazy and for the most part superfluous. Instead, create a struct of attributes you wish to share with the child devices (containing both regmap (which you should call regmap and not map by the way) and irq_data) and pass it as either platform data (preferred) or as device data. If you choose the latter, you do not need to convert from 'device' to 'i2c' to do the look-up. Since this function (probe()) is provided with a platform_device, you can just use platform_get_drvdata() to achieve the same as above. If you go the preferred (platform data) route, then you should use dev_get_platdata() instead.
Hi Lee, On Tue, Feb 12, 2019 at 12:34 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > Add support for the push- and slide-button events for max77650. > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > --- > > > drivers/input/misc/Kconfig | 9 ++ > > > drivers/input/misc/Makefile | 1 + > > > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++ > > > 3 files changed, 145 insertions(+) > > > create mode 100644 drivers/input/misc/max77650-onkey.c > > [...] > > Moving things around a bit: > > > > +static int max77650_onkey_probe(struct platform_device *pdev) > > > +{ > > > > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F); > > > + if (irq_f <= 0) > > > + return -EINVAL; > > > + > > > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R); > > > + if (irq_r <= 0) > > > + return -EINVAL; > > > > Ugh, it would be better if you handled IRQ mapping in the MFD piece and > > passed it as resources of platform device. Then you'd simply call > > platform_get_irq() here and did not have to reach into parent device for > > "irq_dara". > > These device IRQs were defined and registered with the Regmap *set* > (actually init()) APIs and thus should be pulled out using the > appropriate reverse Regmap *get* APIs here in the device. > > Registering them with Regmap *and* pulling them back out again in the > same (MFD in this case) driver, only to register them as platform data > is certainly not how I see the API being designed/used. > > > > + struct regmap_irq_chip_data *irq_data; > > > + struct device *dev, *parent; > > > > + dev = &pdev->dev; > > > + parent = dev->parent; > > > + i2c = to_i2c_client(parent); > > > + irq_data = i2c_get_clientdata(i2c); > > > + > > > + map = dev_get_regmap(parent, NULL); > > > + if (!map) > > > + return -ENODEV; > > However, this hoop jumping is a bit crazy and for the most part > superfluous. Instead, create a struct of attributes you wish to share > with the child devices (containing both regmap (which you should call > regmap and not map by the way) and irq_data) and pass it as either > platform data (preferred) or as device data. > > If you choose the latter, you do not need to convert from 'device' to > 'i2c' to do the look-up. Since this function (probe()) is provided > with a platform_device, you can just use platform_get_drvdata() to > achieve the same as above. > > If you go the preferred (platform data) route, then you should use > dev_get_platdata() instead. By doing what you are suggesting (introducing platform data) you introducing strong dependency between MFD and input piece for no different reason. With the current implementation the parent can be reworked completely without involving onkey driver. I find such clean separation desirable. We are trying to move away form platform data where it makes sense. Thanks.
On Tue, 12 Feb 2019, Dmitry Torokhov wrote: > Hi Lee, > > On Tue, Feb 12, 2019 at 12:34 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > Add support for the push- and slide-button events for max77650. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > --- > > > > drivers/input/misc/Kconfig | 9 ++ > > > > drivers/input/misc/Makefile | 1 + > > > > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++ > > > > 3 files changed, 145 insertions(+) > > > > create mode 100644 drivers/input/misc/max77650-onkey.c > > > > [...] > > > > Moving things around a bit: > > > > > > +static int max77650_onkey_probe(struct platform_device *pdev) > > > > +{ > > > > > > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F); > > > > + if (irq_f <= 0) > > > > + return -EINVAL; > > > > + > > > > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R); > > > > + if (irq_r <= 0) > > > > + return -EINVAL; > > > > > > Ugh, it would be better if you handled IRQ mapping in the MFD piece and > > > passed it as resources of platform device. Then you'd simply call > > > platform_get_irq() here and did not have to reach into parent device for > > > "irq_dara". > > > > These device IRQs were defined and registered with the Regmap *set* > > (actually init()) APIs and thus should be pulled out using the > > appropriate reverse Regmap *get* APIs here in the device. > > > > Registering them with Regmap *and* pulling them back out again in the > > same (MFD in this case) driver, only to register them as platform data > > is certainly not how I see the API being designed/used. > > > > > > + struct regmap_irq_chip_data *irq_data; > > > > + struct device *dev, *parent; > > > > > > + dev = &pdev->dev; > > > > + parent = dev->parent; > > > > + i2c = to_i2c_client(parent); > > > > + irq_data = i2c_get_clientdata(i2c); > > > > + > > > > + map = dev_get_regmap(parent, NULL); > > > > + if (!map) > > > > + return -ENODEV; > > > > However, this hoop jumping is a bit crazy and for the most part > > superfluous. Instead, create a struct of attributes you wish to share > > with the child devices (containing both regmap (which you should call > > regmap and not map by the way) and irq_data) and pass it as either > > platform data (preferred) or as device data. > > > > If you choose the latter, you do not need to convert from 'device' to > > 'i2c' to do the look-up. Since this function (probe()) is provided > > with a platform_device, you can just use platform_get_drvdata() to > > achieve the same as above. > > > > If you go the preferred (platform data) route, then you should use > > dev_get_platdata() instead. > > By doing what you are suggesting (introducing platform data) you > introducing strong dependency between MFD and input piece for no > different reason. With the current implementation the parent can be > reworked completely without involving onkey driver. > > I find such clean separation desirable. We are trying to move away > form platform data where it makes sense. Never mind. We found a way forward where everyone wins! Thanks for your time.
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig index ca59a2be9bc5..bb9c45c1269e 100644 --- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -180,6 +180,15 @@ config INPUT_M68K_BEEP tristate "M68k Beeper support" depends on M68K +config INPUT_MAX77650_ONKEY + tristate "Maxim MAX77650 ONKEY support" + depends on MFD_MAX77650 + help + Support the ONKEY of the MAX77650 PMIC as an input device. + + To compile this driver as a module, choose M here: the module + will be called max77650-onkey. + config INPUT_MAX77693_HAPTIC tristate "MAXIM MAX77693/MAX77843 haptic controller support" depends on (MFD_MAX77693 || MFD_MAX77843) && PWM diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile index 9d0f9d1ff68f..5bd53590ce60 100644 --- a/drivers/input/misc/Makefile +++ b/drivers/input/misc/Makefile @@ -43,6 +43,7 @@ obj-$(CONFIG_INPUT_IXP4XX_BEEPER) += ixp4xx-beeper.o obj-$(CONFIG_INPUT_KEYSPAN_REMOTE) += keyspan_remote.o obj-$(CONFIG_INPUT_KXTJ9) += kxtj9.o obj-$(CONFIG_INPUT_M68K_BEEP) += m68kspkr.o +obj-$(CONFIG_INPUT_MAX77650_ONKEY) += max77650-onkey.o obj-$(CONFIG_INPUT_MAX77693_HAPTIC) += max77693-haptic.o obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o diff --git a/drivers/input/misc/max77650-onkey.c b/drivers/input/misc/max77650-onkey.c new file mode 100644 index 000000000000..cc7e83f589cd --- /dev/null +++ b/drivers/input/misc/max77650-onkey.c @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 BayLibre SAS + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> + * + * ONKEY driver for MAXIM 77650/77651 charger/power-supply. + */ + +#include <linux/i2c.h> +#include <linux/input.h> +#include <linux/interrupt.h> +#include <linux/mfd/max77650.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define MAX77650_ONKEY_MODE_MASK BIT(3) +#define MAX77650_ONKEY_MODE_PUSH 0x00 +#define MAX77650_ONKEY_MODE_SLIDE BIT(3) + +struct max77650_onkey { + struct input_dev *input; + unsigned int code; +}; + +static irqreturn_t +max77650_onkey_report(struct max77650_onkey *onkey, int value) +{ + input_report_key(onkey->input, onkey->code, value); + input_sync(onkey->input); + + return IRQ_HANDLED; +} + +static irqreturn_t max77650_onkey_falling(int irq, void *data) +{ + struct max77650_onkey *onkey = data; + + return max77650_onkey_report(onkey, 0); +} + +static irqreturn_t max77650_onkey_rising(int irq, void *data) +{ + struct max77650_onkey *onkey = data; + + return max77650_onkey_report(onkey, 1); +} + +static int max77650_onkey_probe(struct platform_device *pdev) +{ + struct regmap_irq_chip_data *irq_data; + struct max77650_onkey *onkey; + struct device *dev, *parent; + int irq_r, irq_f, rv, mode; + struct i2c_client *i2c; + const char *mode_prop; + struct regmap *map; + + dev = &pdev->dev; + parent = dev->parent; + i2c = to_i2c_client(parent); + irq_data = i2c_get_clientdata(i2c); + + map = dev_get_regmap(parent, NULL); + if (!map) + return -ENODEV; + + onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL); + if (!onkey) + return -ENOMEM; + + rv = device_property_read_u32(dev, "linux,code", &onkey->code); + if (rv) + onkey->code = KEY_POWER; + + rv = device_property_read_string(dev, "maxim,onkey-mode", &mode_prop); + if (rv) + mode_prop = "push"; + + if (strcmp(mode_prop, "push") == 0) + mode = MAX77650_ONKEY_MODE_PUSH; + else if (strcmp(mode_prop, "slide") == 0) + mode = MAX77650_ONKEY_MODE_SLIDE; + else + return -EINVAL; + + rv = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL, + MAX77650_ONKEY_MODE_MASK, mode); + if (rv) + return rv; + + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F); + if (irq_f <= 0) + return -EINVAL; + + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R); + if (irq_r <= 0) + return -EINVAL; + + onkey->input = devm_input_allocate_device(dev); + if (!onkey->input) + return -ENOMEM; + + onkey->input->name = "max77650_onkey"; + onkey->input->phys = "max77650_onkey/input0"; + onkey->input->id.bustype = BUS_I2C; + onkey->input->dev.parent = dev; + input_set_capability(onkey->input, EV_KEY, onkey->code); + + rv = devm_request_threaded_irq(dev, irq_f, NULL, + max77650_onkey_falling, + IRQF_ONESHOT, "onkey-down", onkey); + if (rv) + return rv; + + rv = devm_request_threaded_irq(dev, irq_r, NULL, + max77650_onkey_rising, + IRQF_ONESHOT, "onkey-up", onkey); + if (rv) + return rv; + + return input_register_device(onkey->input); +} + +static struct platform_driver max77650_onkey_driver = { + .driver = { + .name = "max77650-onkey", + }, + .probe = max77650_onkey_probe, +}; +module_platform_driver(max77650_onkey_driver); + +MODULE_DESCRIPTION("MAXIM 77650/77651 ONKEY driver"); +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>"); +MODULE_LICENSE("GPL");