| Message ID | 20250318-mdb-max7360-support-v5-10-fb20baf97da0@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Tue, Mar 18, 2025 at 05:26:26PM +0100, Mathieu Dubois-Briand wrote: > Add driver for Maxim Integrated MAX7360 rotary encoder controller, > supporting a single rotary switch. ... > + help > + If you say yes here you get support for the rotary encoder on the > + Maxim MAX7360 I/O Expander. > + > + To compile this driver as a module, choose M here: the > + module will be called max7360_rotary. Seems all your patches have unaligned line widths in the help texts. ... + bitfield.h + device/devres.h + dev_printk.h > +#include <linux/init.h> > +#include <linux/input.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/max7360.h> > +#include <linux/property.h> > +#include <linux/of.h> Why? > +#include <linux/platform_device.h> > +#include <linux/pm_wakeirq.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> Not needed. + types.h ... > +static irqreturn_t max7360_rotary_irq(int irq, void *data) > +{ > + struct max7360_rotary *max7360_rotary = data; > + int val; > + int ret; > + > + ret = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val); > + if (ret < 0) { > + dev_err(&max7360_rotary->input->dev, > + "Failed to read rotary counter\n"); > + return IRQ_NONE; > + } > + > + if (val == 0) { > + dev_dbg(&max7360_rotary->input->dev, > + "Got a spurious interrupt\n"); > + > + return IRQ_NONE; > + } > + > + input_report_rel(max7360_rotary->input, max7360_rotary->axis, > + (int8_t)val); This is strange: 1) why casting to begin with? 2) why to C type and not kernel (s8) type? > + input_sync(max7360_rotary->input); > + > + return IRQ_HANDLED; > +} ... > +static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary) > +{ > + int val; > + int ret; > + > + val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) | > + FIELD_PREP(MAX7360_ROT_INTCNT, 1) | MAX7360_ROT_INTCNT_DLY; > + ret = regmap_write(max7360_rotary->regmap, MAX7360_REG_RTRCFG, val); > + if (ret) { > + dev_err(&max7360_rotary->input->dev, > + "Failed to set max7360 rotary encoder configuration\n"); > + return ret; > + } > + > + return 0; Just return ret; ? > +} ... > +static int max7360_rotary_probe(struct platform_device *pdev) > +{ > + struct max7360_rotary *max7360_rotary; struct device *dev = &pdev->dev; > + struct input_dev *input; > + int irq; > + int ret; > + if (!pdev->dev.parent) > + return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n"); Just check for regmap. > + irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent), > + "inti"); One line (and instead of that dance with container_of(), just use fwnode_irq_get_byname(). > + if (irq < 0) > + return irq; > + > + max7360_rotary = devm_kzalloc(&pdev->dev, sizeof(*max7360_rotary), > + GFP_KERNEL); One line. > + if (!max7360_rotary) > + return -ENOMEM; > + > + max7360_rotary->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!max7360_rotary->regmap) > + dev_err_probe(&pdev->dev, -ENODEV, > + "Could not get parent regmap\n"); One line. > + device_property_read_u32(pdev->dev.parent, "linux,axis", > + &max7360_rotary->axis); > + device_property_read_u32(pdev->dev.parent, "rotary-debounce-delay-ms", > + &max7360_rotary->debounce_ms); No, instead of parent, make sure that fwnode is propagated to the children (and tadaam, it's done in MFD already). > + if (max7360_rotary->debounce_ms > MAX7360_ROT_DEBOUNCE_MAX) > + return dev_err_probe(&pdev->dev, -EINVAL, > + "Invalid debounce timing: %u\n", > + max7360_rotary->debounce_ms); > + > + input = devm_input_allocate_device(&pdev->dev); > + if (!input) > + return -ENOMEM; > + > + max7360_rotary->input = input; > + > + input->id.bustype = BUS_I2C; > + input->name = pdev->name; > + input->dev.parent = &pdev->dev; > + > + input_set_capability(input, EV_REL, max7360_rotary->axis); > + input_set_drvdata(input, max7360_rotary); > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, > + max7360_rotary_irq, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED, What's wrong with the firmware description of the interrupt? > + "max7360-rotary", max7360_rotary); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Failed to register interrupt\n"); > + > + ret = input_register_device(input); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Could not register input device\n"); One line. > + platform_set_drvdata(pdev, max7360_rotary); Is it used? > + device_init_wakeup(&pdev->dev, true); > + ret = dev_pm_set_wake_irq(&pdev->dev, irq); > + if (ret) > + dev_warn(&pdev->dev, "Failed to set up wakeup irq: %d\n", ret); > + > + ret = max7360_rotary_hw_init(max7360_rotary); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Failed to initialize max7360 rotary\n"); In the other driver the HW initialisation and wake setup were swapped, can you choose one way? Otherwise both cases need a comment to explain the order of calls. > + return 0; > +}
Hi Mathieu, kernel test robot noticed the following build errors: [auto build test ERROR on a64dcfb451e254085a7daee5fe51bf22959d52d3] url: https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750 base: a64dcfb451e254085a7daee5fe51bf22959d52d3 patch link: https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-10-fb20baf97da0%40bootlin.com patch subject: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192326.KOCnEkdj-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 8.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192326.KOCnEkdj-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503192326.KOCnEkdj-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/input/misc/max7360-rotary.c: In function 'max7360_rotary_hw_init': >> drivers/input/misc/max7360-rotary.c:58:8: error: implicit declaration of function 'FIELD_PREP'; did you mean 'EV_REP'? [-Werror=implicit-function-declaration] val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) | ^~~~~~~~~~ EV_REP cc1: some warnings being treated as errors vim +58 drivers/input/misc/max7360-rotary.c 52 53 static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary) 54 { 55 int val; 56 int ret; 57 > 58 val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) | 59 FIELD_PREP(MAX7360_ROT_INTCNT, 1) | MAX7360_ROT_INTCNT_DLY; 60 ret = regmap_write(max7360_rotary->regmap, MAX7360_REG_RTRCFG, val); 61 if (ret) { 62 dev_err(&max7360_rotary->input->dev, 63 "Failed to set max7360 rotary encoder configuration\n"); 64 return ret; 65 } 66 67 return 0; 68 } 69
Hi Mathieu,
kernel test robot noticed the following build warnings:
[auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3]
url: https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base: a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link: https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-10-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary
config: nios2-kismet-CONFIG_PINCTRL_MAX7360-CONFIG_INPUT_MAX7360_ROTARY-0-0 (https://download.01.org/0day-ci/archive/20250320/202503200825.rmtamLkh-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250320/202503200825.rmtamLkh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503200825.rmtamLkh-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for PINCTRL_MAX7360 when selected by INPUT_MAX7360_ROTARY
WARNING: unmet direct dependencies detected for PINCTRL_MAX7360
Depends on [n]: PINCTRL [=n] && MFD_MAX7360 [=y]
Selected by [y]:
- INPUT_MAX7360_ROTARY [=y] && INPUT [=y] && INPUT_MISC [=y] && MFD_MAX7360 [=y]
On Wed Mar 19, 2025 at 1:11 PM CET, Andy Shevchenko wrote: > On Tue, Mar 18, 2025 at 05:26:26PM +0100, Mathieu Dubois-Briand wrote: > > Add driver for Maxim Integrated MAX7360 rotary encoder controller, > > supporting a single rotary switch. > > ... > > > +static irqreturn_t max7360_rotary_irq(int irq, void *data) > > +{ > > + struct max7360_rotary *max7360_rotary = data; > > + int val; > > + int ret; > > + > > + ret = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val); > > + if (ret < 0) { > > + dev_err(&max7360_rotary->input->dev, > > + "Failed to read rotary counter\n"); > > + return IRQ_NONE; > > + } > > + > > + if (val == 0) { > > + dev_dbg(&max7360_rotary->input->dev, > > + "Got a spurious interrupt\n"); > > + > > + return IRQ_NONE; > > + } > > + > > + input_report_rel(max7360_rotary->input, max7360_rotary->axis, > > + (int8_t)val); > > This is strange: > 1) why casting to begin with? > 2) why to C type and not kernel (s8) type? > I believe the cast is needed, as, while the value read with regmap_read() is stored in an int, the underlying value is indeed a signed 8 bits integer. Without cast negative values will not be correct: -1 (0xFF) -> will be interpreted as 255 (0x000000FF). Ok for s8. Thanks for your review Mathieu
On Tue, Mar 25, 2025 at 04:56:20PM +0100, Mathieu Dubois-Briand wrote: > On Wed Mar 19, 2025 at 1:11 PM CET, Andy Shevchenko wrote: > > On Tue, Mar 18, 2025 at 05:26:26PM +0100, Mathieu Dubois-Briand wrote: ... > > > + int val; Btw, this has to be unsigned to match the API. > > > + int ret; > > > + > > > + ret = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val); > > > + if (ret < 0) { > > > + dev_err(&max7360_rotary->input->dev, > > > + "Failed to read rotary counter\n"); > > > + return IRQ_NONE; > > > + } > > > + input_report_rel(max7360_rotary->input, max7360_rotary->axis, > > > + (int8_t)val); > > > > This is strange: > > 1) why casting to begin with? > > 2) why to C type and not kernel (s8) type? > > I believe the cast is needed, as, while the value read with > regmap_read() is stored in an int, the underlying value is indeed a > signed 8 bits integer. > > Without cast negative values will not be correct: -1 (0xFF) -> will be > interpreted as 255 (0x000000FF). With the above fix it makes sense, but it's not clear for the reader still. What you want is most likely to call sign_extend32().
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig index 13d135257e06..77b07e053265 100644 --- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -230,6 +230,17 @@ config INPUT_M68K_BEEP tristate "M68k Beeper support" depends on M68K +config INPUT_MAX7360_ROTARY + tristate "Maxim MAX7360 Rotary Encoder" + depends on MFD_MAX7360 + select PINCTRL_MAX7360 + help + If you say yes here you get support for the rotary encoder on the + Maxim MAX7360 I/O Expander. + + To compile this driver as a module, choose M here: the + module will be called max7360_rotary. + config INPUT_MAX77650_ONKEY tristate "Maxim MAX77650 ONKEY support" depends on MFD_MAX77650 diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile index 6d91804d0a6f..c454fba3a3ae 100644 --- a/drivers/input/misc/Makefile +++ b/drivers/input/misc/Makefile @@ -51,6 +51,7 @@ obj-$(CONFIG_INPUT_IQS7222) += iqs7222.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_MAX7360_ROTARY) += max7360-rotary.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 diff --git a/drivers/input/misc/max7360-rotary.c b/drivers/input/misc/max7360-rotary.c new file mode 100644 index 000000000000..3046ef64dd56 --- /dev/null +++ b/drivers/input/misc/max7360-rotary.c @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2025 Bootlin + * + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> + */ + +#include <linux/init.h> +#include <linux/input.h> +#include <linux/interrupt.h> +#include <linux/mfd/max7360.h> +#include <linux/property.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_wakeirq.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +struct max7360_rotary { + u32 axis; + struct input_dev *input; + unsigned int debounce_ms; + struct regmap *regmap; +}; + +static irqreturn_t max7360_rotary_irq(int irq, void *data) +{ + struct max7360_rotary *max7360_rotary = data; + int val; + int ret; + + ret = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val); + if (ret < 0) { + dev_err(&max7360_rotary->input->dev, + "Failed to read rotary counter\n"); + return IRQ_NONE; + } + + if (val == 0) { + dev_dbg(&max7360_rotary->input->dev, + "Got a spurious interrupt\n"); + + return IRQ_NONE; + } + + input_report_rel(max7360_rotary->input, max7360_rotary->axis, + (int8_t)val); + input_sync(max7360_rotary->input); + + return IRQ_HANDLED; +} + +static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary) +{ + int val; + int ret; + + val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) | + FIELD_PREP(MAX7360_ROT_INTCNT, 1) | MAX7360_ROT_INTCNT_DLY; + ret = regmap_write(max7360_rotary->regmap, MAX7360_REG_RTRCFG, val); + if (ret) { + dev_err(&max7360_rotary->input->dev, + "Failed to set max7360 rotary encoder configuration\n"); + return ret; + } + + return 0; +} + +static int max7360_rotary_probe(struct platform_device *pdev) +{ + struct max7360_rotary *max7360_rotary; + struct input_dev *input; + int irq; + int ret; + + if (!pdev->dev.parent) + return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n"); + + irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent), + "inti"); + if (irq < 0) + return irq; + + max7360_rotary = devm_kzalloc(&pdev->dev, sizeof(*max7360_rotary), + GFP_KERNEL); + if (!max7360_rotary) + return -ENOMEM; + + max7360_rotary->regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!max7360_rotary->regmap) + dev_err_probe(&pdev->dev, -ENODEV, + "Could not get parent regmap\n"); + + device_property_read_u32(pdev->dev.parent, "linux,axis", + &max7360_rotary->axis); + device_property_read_u32(pdev->dev.parent, "rotary-debounce-delay-ms", + &max7360_rotary->debounce_ms); + if (max7360_rotary->debounce_ms > MAX7360_ROT_DEBOUNCE_MAX) + return dev_err_probe(&pdev->dev, -EINVAL, + "Invalid debounce timing: %u\n", + max7360_rotary->debounce_ms); + + input = devm_input_allocate_device(&pdev->dev); + if (!input) + return -ENOMEM; + + max7360_rotary->input = input; + + input->id.bustype = BUS_I2C; + input->name = pdev->name; + input->dev.parent = &pdev->dev; + + input_set_capability(input, EV_REL, max7360_rotary->axis); + input_set_drvdata(input, max7360_rotary); + + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + max7360_rotary_irq, + IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED, + "max7360-rotary", max7360_rotary); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to register interrupt\n"); + + ret = input_register_device(input); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Could not register input device\n"); + + platform_set_drvdata(pdev, max7360_rotary); + + device_init_wakeup(&pdev->dev, true); + ret = dev_pm_set_wake_irq(&pdev->dev, irq); + if (ret) + dev_warn(&pdev->dev, "Failed to set up wakeup irq: %d\n", ret); + + ret = max7360_rotary_hw_init(max7360_rotary); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to initialize max7360 rotary\n"); + + return 0; +} + +static void max7360_rotary_remove(struct platform_device *pdev) +{ + dev_pm_clear_wake_irq(&pdev->dev); +} + +static struct platform_driver max7360_rotary_driver = { + .driver = { + .name = "max7360-rotary", + }, + .probe = max7360_rotary_probe, + .remove = max7360_rotary_remove, +}; +module_platform_driver(max7360_rotary_driver); + +MODULE_DESCRIPTION("MAX7360 Rotary driver"); +MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>"); +MODULE_LICENSE("GPL");
Add driver for Maxim Integrated MAX7360 rotary encoder controller, supporting a single rotary switch. Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> --- drivers/input/misc/Kconfig | 11 +++ drivers/input/misc/Makefile | 1 + drivers/input/misc/max7360-rotary.c | 161 ++++++++++++++++++++++++++++++++++++ 3 files changed, 173 insertions(+)