| Message ID | 20190205091237.6448-6-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mfd: add support for max77650 PMIC | expand |
On Tue, 05 Feb 2019, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Add the core mfd driver for max77650 PMIC. We define five sub-devices > for which the drivers will be added in subsequent patches. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/mfd/Kconfig | 11 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77650.h | 59 ++++++ > 4 files changed, 413 insertions(+) > create mode 100644 drivers/mfd/max77650.c > create mode 100644 include/linux/mfd/max77650.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 76f9909cf396..a80c3fe80fbe 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -734,6 +734,17 @@ config MFD_MAX77620 > provides common support for accessing the device; additional drivers > must be enabled in order to use the functionality of the device. > > +config MFD_MAX77650 > + tristate "Maxim MAX77650/77651 PMIC Support" > + depends on I2C > + depends on OF || COMPILE_TEST > + select MFD_CORE > + select REGMAP_I2C > + help > + Say yes here to add support for Maxim Semiconductor MAX77650 and > + MAX77651 Power Management ICs. This is the core multifunction > + driver for interacting with the device. > + > config MFD_MAX77686 > tristate "Maxim Semiconductor MAX77686/802 PMIC Support" > depends on I2C > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 12980a4ad460..3b912a4015d1 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150) += da9150-core.o > > obj-$(CONFIG_MFD_MAX14577) += max14577.o > obj-$(CONFIG_MFD_MAX77620) += max77620.o > +obj-$(CONFIG_MFD_MAX77650) += max77650.o > obj-$(CONFIG_MFD_MAX77686) += max77686.o > obj-$(CONFIG_MFD_MAX77693) += max77693.o > obj-$(CONFIG_MFD_MAX77843) += max77843.o > diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c > new file mode 100644 > index 000000000000..7c6164f1fde4 > --- /dev/null > +++ b/drivers/mfd/max77650.c > @@ -0,0 +1,342 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Copyright (C) 2018 BayLibre SAS > +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > +// > +// Core MFD driver for MAXIM 77650/77651 charger/power-supply. > + > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/max77650.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +#define MAX77650_INT_GPI_F_MSK BIT(0) > +#define MAX77650_INT_GPI_R_MSK BIT(1) > +#define MAX77650_INT_GPI_MSK \ > + (MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK) > +#define MAX77650_INT_nEN_F_MSK BIT(2) > +#define MAX77650_INT_nEN_R_MSK BIT(3) > +#define MAX77650_INT_TJAL1_R_MSK BIT(4) > +#define MAX77650_INT_TJAL2_R_MSK BIT(5) > +#define MAX77650_INT_DOD_R_MSK BIT(6) > + > +#define MAX77650_INT_THM_MSK BIT(0) > +#define MAX77650_INT_CHG_MSK BIT(1) > +#define MAX77650_INT_CHGIN_MSK BIT(2) > +#define MAX77650_INT_TJ_REG_MSK BIT(3) > +#define MAX77650_INT_CHGIN_CTRL_MSK BIT(4) > +#define MAX77650_INT_SYS_CTRL_MSK BIT(5) > +#define MAX77650_INT_SYS_CNFG_MSK BIT(6) > + > +#define MAX77650_INT_GLBL_OFFSET 0 > +#define MAX77650_INT_CHG_OFFSET 1 > + > +#define MAX77650_SBIA_LPM_MASK BIT(5) > +#define MAX77650_SBIA_LPM_DISABLED 0x00 > + > +enum { > + MAX77650_INT_GPI = 0, > + MAX77650_INT_nEN_F, > + MAX77650_INT_nEN_R, > + MAX77650_INT_TJAL1_R, > + MAX77650_INT_TJAL2_R, > + MAX77650_INT_DOD_R, > + MAX77650_INT_THM, > + MAX77650_INT_CHG, > + MAX77650_INT_CHGIN, > + MAX77650_INT_TJ_REG, > + MAX77650_INT_CHGIN_CTRL, > + MAX77650_INT_SYS_CTRL, > + MAX77650_INT_SYS_CNFG, > +}; > + > +enum { > + MAX77650_CELL_REGULATOR = 0, > + MAX77650_CELL_CHARGER, > + MAX77650_CELL_GPIO, > + MAX77650_CELL_LED, > + MAX77650_CELL_ONKEY, > + MAX77650_NUM_CELLS, > +}; > + > +struct max77650_irq_mapping { > + int cell_num; > + const int *irqs; > + const char *const *irq_names; > + unsigned int num_irqs; > +}; > + > +static const int max77650_charger_irqs[] = { > + MAX77650_INT_CHG, > + MAX77650_INT_CHGIN, > +}; > + > +static const int max77650_gpio_irqs[] = { > + MAX77650_INT_GPI, > +}; > + > +static const int max77650_onkey_irqs[] = { > + MAX77650_INT_nEN_F, > + MAX77650_INT_nEN_R, > +}; > + > +static const char *const max77650_charger_irq_names[] = { > + "CHG", > + "CHGIN", > +}; > + > +static const char *const max77650_gpio_irq_names[] = { > + "GPI", > +}; > + > +static const char *const max77650_onkey_irq_names[] = { > + "nEN_F", > + "nEN_R", > +}; > + > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = { > + { > + .cell_num = MAX77650_CELL_CHARGER, > + .irqs = max77650_charger_irqs, > + .irq_names = max77650_charger_irq_names, > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs), > + }, > + { > + .cell_num = MAX77650_CELL_GPIO, > + .irqs = max77650_gpio_irqs, > + .irq_names = max77650_gpio_irq_names, > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs), > + }, > + { > + .cell_num = MAX77650_CELL_ONKEY, > + .irqs = max77650_onkey_irqs, > + .irq_names = max77650_onkey_irq_names, > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs), > + }, > +}; This is all a bit convoluted and nasty TBH. > +static const struct mfd_cell max77650_cells[] = { > + [MAX77650_CELL_REGULATOR] = { > + .name = "max77650-regulator", > + .of_compatible = "maxim,max77650-regulator", > + }, > + [MAX77650_CELL_CHARGER] = { > + .name = "max77650-charger", > + .of_compatible = "maxim,max77650-charger", > + }, > + [MAX77650_CELL_GPIO] = { > + .name = "max77650-gpio", > + .of_compatible = "maxim,max77650-gpio", > + }, > + [MAX77650_CELL_LED] = { > + .name = "max77650-led", > + .of_compatible = "maxim,max77650-led", > + }, > + [MAX77650_CELL_ONKEY] = { > + .name = "max77650-onkey", > + .of_compatible = "maxim,max77650-onkey", > + }, > +}; Why are you numbering the cells? There is no need to do this. > +static const struct regmap_irq max77650_irqs[] = { > + [MAX77650_INT_GPI] = { > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > + .mask = MAX77650_INT_GPI_MSK, > + .type = { > + .type_falling_val = MAX77650_INT_GPI_F_MSK, > + .type_rising_val = MAX77650_INT_GPI_R_MSK, > + .types_supported = IRQ_TYPE_EDGE_BOTH, > + }, > + }, > + [MAX77650_INT_nEN_F] = { > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > + .mask = MAX77650_INT_nEN_F_MSK, > + }, > + [MAX77650_INT_nEN_R] = { > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > + .mask = MAX77650_INT_nEN_R_MSK, > + }, > + [MAX77650_INT_TJAL1_R] = { > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > + .mask = MAX77650_INT_TJAL1_R_MSK, > + }, > + [MAX77650_INT_TJAL2_R] = { > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > + .mask = MAX77650_INT_TJAL2_R_MSK, > + }, > + [MAX77650_INT_DOD_R] = { > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > + .mask = MAX77650_INT_DOD_R_MSK, > + }, > + [MAX77650_INT_THM] = { > + .reg_offset = MAX77650_INT_CHG_OFFSET, > + .mask = MAX77650_INT_THM_MSK, > + }, > + [MAX77650_INT_CHG] = { > + .reg_offset = MAX77650_INT_CHG_OFFSET, > + .mask = MAX77650_INT_CHG_MSK, > + }, > + [MAX77650_INT_CHGIN] = { > + .reg_offset = MAX77650_INT_CHG_OFFSET, > + .mask = MAX77650_INT_CHGIN_MSK, > + }, > + [MAX77650_INT_TJ_REG] = { > + .reg_offset = MAX77650_INT_CHG_OFFSET, > + .mask = MAX77650_INT_TJ_REG_MSK, > + }, > + [MAX77650_INT_CHGIN_CTRL] = { > + .reg_offset = MAX77650_INT_CHG_OFFSET, > + .mask = MAX77650_INT_CHGIN_CTRL_MSK, > + }, > + [MAX77650_INT_SYS_CTRL] = { > + .reg_offset = MAX77650_INT_CHG_OFFSET, > + .mask = MAX77650_INT_SYS_CTRL_MSK, > + }, > + [MAX77650_INT_SYS_CNFG] = { > + .reg_offset = MAX77650_INT_CHG_OFFSET, > + .mask = MAX77650_INT_SYS_CNFG_MSK, > + }, > +}; If you get rid of all of the horrible hoop jumping in *_setup_irqs(), you can use REGMAP_IRQ_REG() like everyone else does. > +static const struct regmap_irq_chip max77650_irq_chip = { > + .name = "max77650-irq", > + .irqs = max77650_irqs, > + .num_irqs = ARRAY_SIZE(max77650_irqs), > + .num_regs = 2, > + .status_base = MAX77650_REG_INT_GLBL, > + .mask_base = MAX77650_REG_INTM_GLBL, > + .type_in_mask = true, > + .type_invert = true, > + .init_ack_masked = true, > + .clear_on_unmask = true, > +}; > + > +static const struct regmap_config max77650_regmap_config = { > + .name = "max77650", > + .reg_bits = 8, > + .val_bits = 8, > +}; > + > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells) > +{ > + const struct max77650_irq_mapping *mapping; > + struct regmap_irq_chip_data *irq_data; > + struct i2c_client *i2c; > + struct mfd_cell *cell; > + struct resource *res; > + struct regmap *map; > + int i, j, irq, rv; > + > + i2c = to_i2c_client(dev); > + > + map = dev_get_regmap(dev, NULL); > + if (!map) > + return -ENODEV; > + > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq, > + IRQF_ONESHOT | IRQF_SHARED, -1, What is -1? Are you sure this isn't defined somewhere? > + &max77650_irq_chip, &irq_data); > + if (rv) > + return rv; > + > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) { > + mapping = &max77650_irq_mapping_table[i]; > + cell = &cells[mapping->cell_num]; > + > + res = devm_kcalloc(dev, sizeof(*res), > + mapping->num_irqs, GFP_KERNEL); > + if (!res) > + return -ENOMEM; > + > + cell->resources = res; > + cell->num_resources = mapping->num_irqs; > + > + for (j = 0; j < mapping->num_irqs; j++) { > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]); > + if (irq < 0) > + return irq; > + > + res[j].start = res[j].end = irq; > + res[j].flags = IORESOURCE_IRQ; > + res[j].name = mapping->irq_names[j]; > + } > + } This is the first time I've seen it done like this (and I hate it). Why are you storing the virqs in resources? I think this is highly irregular. > + return 0; > +} > + > +static int max77650_i2c_probe(struct i2c_client *i2c) > +{ > + struct device *dev = &i2c->dev; > + struct mfd_cell *cells; > + struct regmap *map; > + unsigned int val; > + int rv; > + > + map = devm_regmap_init_i2c(i2c, &max77650_regmap_config); > + if (IS_ERR(map)) What error messages does devm_regmap_init_i2c() report? Does it print out its own error messages internally? If not it would be better to provide a suitable error message here. > + return PTR_ERR(map); > + > + rv = regmap_read(map, MAX77650_REG_CID, &val); > + if (rv) Better to provide a suitable error message here. > + return rv; > + > + switch (MAX77650_CID_BITS(val)) { > + case MAX77650_CID_77650A: > + case MAX77650_CID_77650C: > + case MAX77650_CID_77651A: > + case MAX77650_CID_77651B: > + break; > + default: Better to provide a suitable error message here. > + return -ENODEV; > + } > + > + /* > + * This IC has a low-power mode which reduces the quiescent current > + * consumption to ~5.6uA but is only suitable for systems consuming > + * less than ~2mA. Since this is not likely the case even on > + * linux-based wearables - keep the chip in normal power mode. > + */ > + rv = regmap_update_bits(map, > + MAX77650_REG_CNFG_GLBL, > + MAX77650_SBIA_LPM_MASK, > + MAX77650_SBIA_LPM_DISABLED); > + if (rv) Better to provide a suitable error message here. > + return rv; > + > + cells = devm_kmemdup(dev, max77650_cells, > + sizeof(max77650_cells), GFP_KERNEL); > + if (!cells) > + return -ENOMEM; > + > + rv = max77650_setup_irqs(dev, cells); > + if (rv) > + return rv; > + > + return devm_mfd_add_devices(dev, -1, cells, Use the correct defines instead of -1. `git grep mfd_add_devices` > + MAX77650_NUM_CELLS, NULL, 0, NULL); > +} > + > +static const struct of_device_id max77650_of_match[] = { > + { .compatible = "maxim,max77650" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max77650_of_match); > + > +static struct i2c_driver max77650_i2c_driver = { > + .driver = { > + .name = "max77650", > + .of_match_table = of_match_ptr(max77650_of_match), > + }, > + .probe_new = max77650_i2c_probe, > +}; > +module_i2c_driver(max77650_i2c_driver); > + > +MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver"); > +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h > new file mode 100644 > index 000000000000..c809e211a8cd > --- /dev/null > +++ b/include/linux/mfd/max77650.h > @@ -0,0 +1,59 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 BayLibre SAS > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > + * > + * Common definitions for MAXIM 77650/77651 charger/power-supply. > + */ > + > +#ifndef MAX77650_H > +#define MAX77650_H > + > +#include <linux/bits.h> > + > +#define MAX77650_REG_INT_GLBL 0x00 > +#define MAX77650_REG_INT_CHG 0x01 > +#define MAX77650_REG_STAT_CHG_A 0x02 > +#define MAX77650_REG_STAT_CHG_B 0x03 > +#define MAX77650_REG_ERCFLAG 0x04 > +#define MAX77650_REG_STAT_GLBL 0x05 > +#define MAX77650_REG_INTM_GLBL 0x06 > +#define MAX77650_REG_INTM_CHG 0x07 > +#define MAX77650_REG_CNFG_GLBL 0x10 > +#define MAX77650_REG_CID 0x11 > +#define MAX77650_REG_CNFG_GPIO 0x12 > +#define MAX77650_REG_CNFG_CHG_A 0x18 > +#define MAX77650_REG_CNFG_CHG_B 0x19 > +#define MAX77650_REG_CNFG_CHG_C 0x1a > +#define MAX77650_REG_CNFG_CHG_D 0x1b > +#define MAX77650_REG_CNFG_CHG_E 0x1c > +#define MAX77650_REG_CNFG_CHG_F 0x1d > +#define MAX77650_REG_CNFG_CHG_G 0x1e > +#define MAX77650_REG_CNFG_CHG_H 0x1f > +#define MAX77650_REG_CNFG_CHG_I 0x20 > +#define MAX77650_REG_CNFG_SBB_TOP 0x28 > +#define MAX77650_REG_CNFG_SBB0_A 0x29 > +#define MAX77650_REG_CNFG_SBB0_B 0x2a > +#define MAX77650_REG_CNFG_SBB1_A 0x2b > +#define MAX77650_REG_CNFG_SBB1_B 0x2c > +#define MAX77650_REG_CNFG_SBB2_A 0x2d > +#define MAX77650_REG_CNFG_SBB2_B 0x2e > +#define MAX77650_REG_CNFG_LDO_A 0x38 > +#define MAX77650_REG_CNFG_LDO_B 0x39 > +#define MAX77650_REG_CNFG_LED0_A 0x40 > +#define MAX77650_REG_CNFG_LED1_A 0x41 > +#define MAX77650_REG_CNFG_LED2_A 0x42 > +#define MAX77650_REG_CNFG_LED0_B 0x43 > +#define MAX77650_REG_CNFG_LED1_B 0x44 > +#define MAX77650_REG_CNFG_LED2_B 0x45 > +#define MAX77650_REG_CNFG_LED_TOP 0x46 > + > +#define MAX77650_CID_MASK GENMASK(3, 0) > +#define MAX77650_CID_BITS(_reg) (_reg & MAX77650_CID_MASK) > + > +#define MAX77650_CID_77650A 0x03 > +#define MAX77650_CID_77650C 0x0a > +#define MAX77650_CID_77651A 0x06 > +#define MAX77650_CID_77651B 0x08 > + > +#endif /* MAX77650_H */
wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a): > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices > > for which the drivers will be added in subsequent patches. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/mfd/Kconfig | 11 ++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++ > > include/linux/mfd/max77650.h | 59 ++++++ > > 4 files changed, 413 insertions(+) > > create mode 100644 drivers/mfd/max77650.c > > create mode 100644 include/linux/mfd/max77650.h > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 76f9909cf396..a80c3fe80fbe 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -734,6 +734,17 @@ config MFD_MAX77620 > > provides common support for accessing the device; additional drivers > > must be enabled in order to use the functionality of the device. > > > > +config MFD_MAX77650 > > + tristate "Maxim MAX77650/77651 PMIC Support" > > + depends on I2C > > + depends on OF || COMPILE_TEST > > + select MFD_CORE > > + select REGMAP_I2C > > + help > > + Say yes here to add support for Maxim Semiconductor MAX77650 and > > + MAX77651 Power Management ICs. This is the core multifunction > > + driver for interacting with the device. > > + > > config MFD_MAX77686 > > tristate "Maxim Semiconductor MAX77686/802 PMIC Support" > > depends on I2C > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 12980a4ad460..3b912a4015d1 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150) += da9150-core.o > > > > obj-$(CONFIG_MFD_MAX14577) += max14577.o > > obj-$(CONFIG_MFD_MAX77620) += max77620.o > > +obj-$(CONFIG_MFD_MAX77650) += max77650.o > > obj-$(CONFIG_MFD_MAX77686) += max77686.o > > obj-$(CONFIG_MFD_MAX77693) += max77693.o > > obj-$(CONFIG_MFD_MAX77843) += max77843.o > > diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c > > new file mode 100644 > > index 000000000000..7c6164f1fde4 > > --- /dev/null > > +++ b/drivers/mfd/max77650.c > > @@ -0,0 +1,342 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// > > +// Copyright (C) 2018 BayLibre SAS > > +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > +// > > +// Core MFD driver for MAXIM 77650/77651 charger/power-supply. > > + > > +#include <linux/i2c.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/mfd/core.h> > > +#include <linux/mfd/max77650.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > +#define MAX77650_INT_GPI_F_MSK BIT(0) > > +#define MAX77650_INT_GPI_R_MSK BIT(1) > > +#define MAX77650_INT_GPI_MSK \ > > + (MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK) > > +#define MAX77650_INT_nEN_F_MSK BIT(2) > > +#define MAX77650_INT_nEN_R_MSK BIT(3) > > +#define MAX77650_INT_TJAL1_R_MSK BIT(4) > > +#define MAX77650_INT_TJAL2_R_MSK BIT(5) > > +#define MAX77650_INT_DOD_R_MSK BIT(6) > > + > > +#define MAX77650_INT_THM_MSK BIT(0) > > +#define MAX77650_INT_CHG_MSK BIT(1) > > +#define MAX77650_INT_CHGIN_MSK BIT(2) > > +#define MAX77650_INT_TJ_REG_MSK BIT(3) > > +#define MAX77650_INT_CHGIN_CTRL_MSK BIT(4) > > +#define MAX77650_INT_SYS_CTRL_MSK BIT(5) > > +#define MAX77650_INT_SYS_CNFG_MSK BIT(6) > > + > > +#define MAX77650_INT_GLBL_OFFSET 0 > > +#define MAX77650_INT_CHG_OFFSET 1 > > + > > +#define MAX77650_SBIA_LPM_MASK BIT(5) > > +#define MAX77650_SBIA_LPM_DISABLED 0x00 > > + > > +enum { > > + MAX77650_INT_GPI = 0, > > + MAX77650_INT_nEN_F, > > + MAX77650_INT_nEN_R, > > + MAX77650_INT_TJAL1_R, > > + MAX77650_INT_TJAL2_R, > > + MAX77650_INT_DOD_R, > > + MAX77650_INT_THM, > > + MAX77650_INT_CHG, > > + MAX77650_INT_CHGIN, > > + MAX77650_INT_TJ_REG, > > + MAX77650_INT_CHGIN_CTRL, > > + MAX77650_INT_SYS_CTRL, > > + MAX77650_INT_SYS_CNFG, > > +}; > > + > > +enum { > > + MAX77650_CELL_REGULATOR = 0, > > + MAX77650_CELL_CHARGER, > > + MAX77650_CELL_GPIO, > > + MAX77650_CELL_LED, > > + MAX77650_CELL_ONKEY, > > + MAX77650_NUM_CELLS, > > +}; > > + > > +struct max77650_irq_mapping { > > + int cell_num; > > + const int *irqs; > > + const char *const *irq_names; > > + unsigned int num_irqs; > > +}; > > + > > +static const int max77650_charger_irqs[] = { > > + MAX77650_INT_CHG, > > + MAX77650_INT_CHGIN, > > +}; > > + > > +static const int max77650_gpio_irqs[] = { > > + MAX77650_INT_GPI, > > +}; > > + > > +static const int max77650_onkey_irqs[] = { > > + MAX77650_INT_nEN_F, > > + MAX77650_INT_nEN_R, > > +}; > > + > > +static const char *const max77650_charger_irq_names[] = { > > + "CHG", > > + "CHGIN", > > +}; > > + > > +static const char *const max77650_gpio_irq_names[] = { > > + "GPI", > > +}; > > + > > +static const char *const max77650_onkey_irq_names[] = { > > + "nEN_F", > > + "nEN_R", > > +}; > > + > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = { > > + { > > + .cell_num = MAX77650_CELL_CHARGER, > > + .irqs = max77650_charger_irqs, > > + .irq_names = max77650_charger_irq_names, > > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs), > > + }, > > + { > > + .cell_num = MAX77650_CELL_GPIO, > > + .irqs = max77650_gpio_irqs, > > + .irq_names = max77650_gpio_irq_names, > > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs), > > + }, > > + { > > + .cell_num = MAX77650_CELL_ONKEY, > > + .irqs = max77650_onkey_irqs, > > + .irq_names = max77650_onkey_irq_names, > > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs), > > + }, > > +}; > > This is all a bit convoluted and nasty TBH. > > > +static const struct mfd_cell max77650_cells[] = { > > + [MAX77650_CELL_REGULATOR] = { > > + .name = "max77650-regulator", > > + .of_compatible = "maxim,max77650-regulator", > > + }, > > + [MAX77650_CELL_CHARGER] = { > > + .name = "max77650-charger", > > + .of_compatible = "maxim,max77650-charger", > > + }, > > + [MAX77650_CELL_GPIO] = { > > + .name = "max77650-gpio", > > + .of_compatible = "maxim,max77650-gpio", > > + }, > > + [MAX77650_CELL_LED] = { > > + .name = "max77650-led", > > + .of_compatible = "maxim,max77650-led", > > + }, > > + [MAX77650_CELL_ONKEY] = { > > + .name = "max77650-onkey", > > + .of_compatible = "maxim,max77650-onkey", > > + }, > > +}; > > Why are you numbering the cells? There is no need to do this. > Just for better readability. It makes sense to me coupled with MAX77650_NUM_CELLS. > > +static const struct regmap_irq max77650_irqs[] = { > > + [MAX77650_INT_GPI] = { > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > + .mask = MAX77650_INT_GPI_MSK, > > + .type = { > > + .type_falling_val = MAX77650_INT_GPI_F_MSK, > > + .type_rising_val = MAX77650_INT_GPI_R_MSK, > > + .types_supported = IRQ_TYPE_EDGE_BOTH, > > + }, > > + }, > > + [MAX77650_INT_nEN_F] = { > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > + .mask = MAX77650_INT_nEN_F_MSK, > > + }, > > + [MAX77650_INT_nEN_R] = { > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > + .mask = MAX77650_INT_nEN_R_MSK, > > + }, > > + [MAX77650_INT_TJAL1_R] = { > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > + .mask = MAX77650_INT_TJAL1_R_MSK, > > + }, > > + [MAX77650_INT_TJAL2_R] = { > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > + .mask = MAX77650_INT_TJAL2_R_MSK, > > + }, > > + [MAX77650_INT_DOD_R] = { > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > + .mask = MAX77650_INT_DOD_R_MSK, > > + }, > > + [MAX77650_INT_THM] = { > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > + .mask = MAX77650_INT_THM_MSK, > > + }, > > + [MAX77650_INT_CHG] = { > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > + .mask = MAX77650_INT_CHG_MSK, > > + }, > > + [MAX77650_INT_CHGIN] = { > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > + .mask = MAX77650_INT_CHGIN_MSK, > > + }, > > + [MAX77650_INT_TJ_REG] = { > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > + .mask = MAX77650_INT_TJ_REG_MSK, > > + }, > > + [MAX77650_INT_CHGIN_CTRL] = { > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > + .mask = MAX77650_INT_CHGIN_CTRL_MSK, > > + }, > > + [MAX77650_INT_SYS_CTRL] = { > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > + .mask = MAX77650_INT_SYS_CTRL_MSK, > > + }, > > + [MAX77650_INT_SYS_CNFG] = { > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > + .mask = MAX77650_INT_SYS_CNFG_MSK, > > + }, > > +}; > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(), > you can use REGMAP_IRQ_REG() like everyone else does. > I could even use it now - except for the first interrupt. I decided to not use it everywhere as it looks much better that way than having REGMAP_IRQ_REG() for all definitions and then the first one sticking out like that. It just looks better. > > +static const struct regmap_irq_chip max77650_irq_chip = { > > + .name = "max77650-irq", > > + .irqs = max77650_irqs, > > + .num_irqs = ARRAY_SIZE(max77650_irqs), > > + .num_regs = 2, > > + .status_base = MAX77650_REG_INT_GLBL, > > + .mask_base = MAX77650_REG_INTM_GLBL, > > + .type_in_mask = true, > > + .type_invert = true, > > + .init_ack_masked = true, > > + .clear_on_unmask = true, > > +}; > > + > > +static const struct regmap_config max77650_regmap_config = { > > + .name = "max77650", > > + .reg_bits = 8, > > + .val_bits = 8, > > +}; > > + > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells) > > +{ > > + const struct max77650_irq_mapping *mapping; > > + struct regmap_irq_chip_data *irq_data; > > + struct i2c_client *i2c; > > + struct mfd_cell *cell; > > + struct resource *res; > > + struct regmap *map; > > + int i, j, irq, rv; > > + > > + i2c = to_i2c_client(dev); > > + > > + map = dev_get_regmap(dev, NULL); > > + if (!map) > > + return -ENODEV; > > + > > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq, > > + IRQF_ONESHOT | IRQF_SHARED, -1, > > What is -1? Are you sure this isn't defined somewhere? > I don't see any define for negative irq_base argument. I can add that in a separate series and convert the users, but for now I'd stick with -1. > > + &max77650_irq_chip, &irq_data); > > + if (rv) > > + return rv; > > + > > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) { > > + mapping = &max77650_irq_mapping_table[i]; > > + cell = &cells[mapping->cell_num]; > > + > > + res = devm_kcalloc(dev, sizeof(*res), > > + mapping->num_irqs, GFP_KERNEL); > > + if (!res) > > + return -ENOMEM; > > + > > + cell->resources = res; > > + cell->num_resources = mapping->num_irqs; > > + > > + for (j = 0; j < mapping->num_irqs; j++) { > > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]); > > + if (irq < 0) > > + return irq; > > + > > + res[j].start = res[j].end = irq; > > + res[j].flags = IORESOURCE_IRQ; > > + res[j].name = mapping->irq_names[j]; > > + } > > + } > > This is the first time I've seen it done like this (and I hate it). > > Why are you storing the virqs in resources? > > I think this is highly irregular. > I initially just passed the regmap_irq_chip_data over i2c clientdata and sub-drivers would look up virq numbers from it but was advised by Dmitry Torokhov to use resources instead. After implementing it this way I too think it's more elegant in sub-drivers who can simply do platform_get_irq_byname(). Do you have a different idea? What exactly don't you like about this? > > + return 0; > > +} > > + > > +static int max77650_i2c_probe(struct i2c_client *i2c) > > +{ > > + struct device *dev = &i2c->dev; > > + struct mfd_cell *cells; > > + struct regmap *map; > > + unsigned int val; > > + int rv; > > + > > + map = devm_regmap_init_i2c(i2c, &max77650_regmap_config); > > + if (IS_ERR(map)) > > What error messages does devm_regmap_init_i2c() report? Does it print > out its own error messages internally? If not it would be better to > provide a suitable error message here. > > > + return PTR_ERR(map); > > + > > + rv = regmap_read(map, MAX77650_REG_CID, &val); > > + if (rv) > > Better to provide a suitable error message here. > > > + return rv; > > + > > + switch (MAX77650_CID_BITS(val)) { > > + case MAX77650_CID_77650A: > > + case MAX77650_CID_77650C: > > + case MAX77650_CID_77651A: > > + case MAX77650_CID_77651B: > > + break; > > + default: > > Better to provide a suitable error message here. > > > + return -ENODEV; > > + } > > + > > + /* > > + * This IC has a low-power mode which reduces the quiescent current > > + * consumption to ~5.6uA but is only suitable for systems consuming > > + * less than ~2mA. Since this is not likely the case even on > > + * linux-based wearables - keep the chip in normal power mode. > > + */ > > + rv = regmap_update_bits(map, > > + MAX77650_REG_CNFG_GLBL, > > + MAX77650_SBIA_LPM_MASK, > > + MAX77650_SBIA_LPM_DISABLED); > > + if (rv) > > Better to provide a suitable error message here. > > > + return rv; > > + > > + cells = devm_kmemdup(dev, max77650_cells, > > + sizeof(max77650_cells), GFP_KERNEL); > > + if (!cells) > > + return -ENOMEM; > > + > > + rv = max77650_setup_irqs(dev, cells); > > + if (rv) > > + return rv; > > + > > + return devm_mfd_add_devices(dev, -1, cells, > > Use the correct defines instead of -1. > Will do that and add the error messages. Bart > `git grep mfd_add_devices` > > > + MAX77650_NUM_CELLS, NULL, 0, NULL); > > +} > > + > > +static const struct of_device_id max77650_of_match[] = { > > + { .compatible = "maxim,max77650" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, max77650_of_match); > > + > > +static struct i2c_driver max77650_i2c_driver = { > > + .driver = { > > + .name = "max77650", > > + .of_match_table = of_match_ptr(max77650_of_match), > > + }, > > + .probe_new = max77650_i2c_probe, > > +}; > > +module_i2c_driver(max77650_i2c_driver); > > + > > +MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver"); > > +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>"); > > +MODULE_LICENSE("GPL v2"); > > diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h > > new file mode 100644 > > index 000000000000..c809e211a8cd > > --- /dev/null > > +++ b/include/linux/mfd/max77650.h > > @@ -0,0 +1,59 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2018 BayLibre SAS > > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > + * > > + * Common definitions for MAXIM 77650/77651 charger/power-supply. > > + */ > > + > > +#ifndef MAX77650_H > > +#define MAX77650_H > > + > > +#include <linux/bits.h> > > + > > +#define MAX77650_REG_INT_GLBL 0x00 > > +#define MAX77650_REG_INT_CHG 0x01 > > +#define MAX77650_REG_STAT_CHG_A 0x02 > > +#define MAX77650_REG_STAT_CHG_B 0x03 > > +#define MAX77650_REG_ERCFLAG 0x04 > > +#define MAX77650_REG_STAT_GLBL 0x05 > > +#define MAX77650_REG_INTM_GLBL 0x06 > > +#define MAX77650_REG_INTM_CHG 0x07 > > +#define MAX77650_REG_CNFG_GLBL 0x10 > > +#define MAX77650_REG_CID 0x11 > > +#define MAX77650_REG_CNFG_GPIO 0x12 > > +#define MAX77650_REG_CNFG_CHG_A 0x18 > > +#define MAX77650_REG_CNFG_CHG_B 0x19 > > +#define MAX77650_REG_CNFG_CHG_C 0x1a > > +#define MAX77650_REG_CNFG_CHG_D 0x1b > > +#define MAX77650_REG_CNFG_CHG_E 0x1c > > +#define MAX77650_REG_CNFG_CHG_F 0x1d > > +#define MAX77650_REG_CNFG_CHG_G 0x1e > > +#define MAX77650_REG_CNFG_CHG_H 0x1f > > +#define MAX77650_REG_CNFG_CHG_I 0x20 > > +#define MAX77650_REG_CNFG_SBB_TOP 0x28 > > +#define MAX77650_REG_CNFG_SBB0_A 0x29 > > +#define MAX77650_REG_CNFG_SBB0_B 0x2a > > +#define MAX77650_REG_CNFG_SBB1_A 0x2b > > +#define MAX77650_REG_CNFG_SBB1_B 0x2c > > +#define MAX77650_REG_CNFG_SBB2_A 0x2d > > +#define MAX77650_REG_CNFG_SBB2_B 0x2e > > +#define MAX77650_REG_CNFG_LDO_A 0x38 > > +#define MAX77650_REG_CNFG_LDO_B 0x39 > > +#define MAX77650_REG_CNFG_LED0_A 0x40 > > +#define MAX77650_REG_CNFG_LED1_A 0x41 > > +#define MAX77650_REG_CNFG_LED2_A 0x42 > > +#define MAX77650_REG_CNFG_LED0_B 0x43 > > +#define MAX77650_REG_CNFG_LED1_B 0x44 > > +#define MAX77650_REG_CNFG_LED2_B 0x45 > > +#define MAX77650_REG_CNFG_LED_TOP 0x46 > > + > > +#define MAX77650_CID_MASK GENMASK(3, 0) > > +#define MAX77650_CID_BITS(_reg) (_reg & MAX77650_CID_MASK) > > + > > +#define MAX77650_CID_77650A 0x03 > > +#define MAX77650_CID_77650C 0x0a > > +#define MAX77650_CID_77651A 0x06 > > +#define MAX77650_CID_77651B 0x08 > > + > > +#endif /* MAX77650_H */ > > -- > Lee Jones [李琼斯] > Linaro Services Technical Lead > Linaro.org │ Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices > > > for which the drivers will be added in subsequent patches. > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > --- > > > drivers/mfd/Kconfig | 11 ++ > > > drivers/mfd/Makefile | 1 + > > > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/max77650.h | 59 ++++++ > > > 4 files changed, 413 insertions(+) > > > create mode 100644 drivers/mfd/max77650.c > > > create mode 100644 include/linux/mfd/max77650.h [...] > > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = { > > > + { > > > + .cell_num = MAX77650_CELL_CHARGER, > > > + .irqs = max77650_charger_irqs, > > > + .irq_names = max77650_charger_irq_names, > > > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs), > > > + }, > > > + { > > > + .cell_num = MAX77650_CELL_GPIO, > > > + .irqs = max77650_gpio_irqs, > > > + .irq_names = max77650_gpio_irq_names, > > > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs), > > > + }, > > > + { > > > + .cell_num = MAX77650_CELL_ONKEY, > > > + .irqs = max77650_onkey_irqs, > > > + .irq_names = max77650_onkey_irq_names, > > > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs), > > > + }, > > > +}; > > > > This is all a bit convoluted and nasty TBH. > > > > > +static const struct mfd_cell max77650_cells[] = { > > > + [MAX77650_CELL_REGULATOR] = { > > > + .name = "max77650-regulator", > > > + .of_compatible = "maxim,max77650-regulator", > > > + }, > > > + [MAX77650_CELL_CHARGER] = { > > > + .name = "max77650-charger", > > > + .of_compatible = "maxim,max77650-charger", > > > + }, > > > + [MAX77650_CELL_GPIO] = { > > > + .name = "max77650-gpio", > > > + .of_compatible = "maxim,max77650-gpio", > > > + }, > > > + [MAX77650_CELL_LED] = { > > > + .name = "max77650-led", > > > + .of_compatible = "maxim,max77650-led", > > > + }, > > > + [MAX77650_CELL_ONKEY] = { > > > + .name = "max77650-onkey", > > > + .of_compatible = "maxim,max77650-onkey", > > > + }, > > > +}; > > > > Why are you numbering the cells? There is no need to do this. > > > > Just for better readability. It makes sense to me coupled with > MAX77650_NUM_CELLS. You have it the wrong way around. You define the cell data, then provide the number of them using ARRAY_SIZE(). The enum containing MAX77650_NUM_CELLS should not exist. > > > +static const struct regmap_irq max77650_irqs[] = { > > > + [MAX77650_INT_GPI] = { > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > + .mask = MAX77650_INT_GPI_MSK, > > > + .type = { > > > + .type_falling_val = MAX77650_INT_GPI_F_MSK, > > > + .type_rising_val = MAX77650_INT_GPI_R_MSK, > > > + .types_supported = IRQ_TYPE_EDGE_BOTH, > > > + }, > > > + }, > > > + [MAX77650_INT_nEN_F] = { > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > + .mask = MAX77650_INT_nEN_F_MSK, > > > + }, > > > + [MAX77650_INT_nEN_R] = { > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > + .mask = MAX77650_INT_nEN_R_MSK, > > > + }, > > > + [MAX77650_INT_TJAL1_R] = { > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > + .mask = MAX77650_INT_TJAL1_R_MSK, > > > + }, > > > + [MAX77650_INT_TJAL2_R] = { > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > + .mask = MAX77650_INT_TJAL2_R_MSK, > > > + }, > > > + [MAX77650_INT_DOD_R] = { > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > + .mask = MAX77650_INT_DOD_R_MSK, > > > + }, > > > + [MAX77650_INT_THM] = { > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > + .mask = MAX77650_INT_THM_MSK, > > > + }, > > > + [MAX77650_INT_CHG] = { > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > + .mask = MAX77650_INT_CHG_MSK, > > > + }, > > > + [MAX77650_INT_CHGIN] = { > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > + .mask = MAX77650_INT_CHGIN_MSK, > > > + }, > > > + [MAX77650_INT_TJ_REG] = { > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > + .mask = MAX77650_INT_TJ_REG_MSK, > > > + }, > > > + [MAX77650_INT_CHGIN_CTRL] = { > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > + .mask = MAX77650_INT_CHGIN_CTRL_MSK, > > > + }, > > > + [MAX77650_INT_SYS_CTRL] = { > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > + .mask = MAX77650_INT_SYS_CTRL_MSK, > > > + }, > > > + [MAX77650_INT_SYS_CNFG] = { > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > + .mask = MAX77650_INT_SYS_CNFG_MSK, > > > + }, > > > +}; > > > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(), > > you can use REGMAP_IRQ_REG() like everyone else does. > > > > I could even use it now - except for the first interrupt. I decided to > not use it everywhere as it looks much better that way than having > REGMAP_IRQ_REG() for all definitions and then the first one sticking > out like that. It just looks better. The way it's done at the moment looks terrible. Please use the MACROs to simplify as much of the code as possible. > > > +static const struct regmap_irq_chip max77650_irq_chip = { > > > + .name = "max77650-irq", > > > + .irqs = max77650_irqs, > > > + .num_irqs = ARRAY_SIZE(max77650_irqs), > > > + .num_regs = 2, > > > + .status_base = MAX77650_REG_INT_GLBL, > > > + .mask_base = MAX77650_REG_INTM_GLBL, > > > + .type_in_mask = true, > > > + .type_invert = true, > > > + .init_ack_masked = true, > > > + .clear_on_unmask = true, > > > +}; > > > + > > > +static const struct regmap_config max77650_regmap_config = { > > > + .name = "max77650", > > > + .reg_bits = 8, > > > + .val_bits = 8, > > > +}; > > > + > > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells) > > > +{ > > > + const struct max77650_irq_mapping *mapping; > > > + struct regmap_irq_chip_data *irq_data; > > > + struct i2c_client *i2c; > > > + struct mfd_cell *cell; > > > + struct resource *res; > > > + struct regmap *map; > > > + int i, j, irq, rv; > > > + > > > + i2c = to_i2c_client(dev); > > > + > > > + map = dev_get_regmap(dev, NULL); > > > + if (!map) > > > + return -ENODEV; > > > + > > > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq, > > > + IRQF_ONESHOT | IRQF_SHARED, -1, > > > > What is -1? Are you sure this isn't defined somewhere? > > > > I don't see any define for negative irq_base argument. I can add that > in a separate series and convert the users, but for now I'd stick with > -1. IMO it should be defined. You can define it locally for now. > > > + &max77650_irq_chip, &irq_data); > > > + if (rv) > > > + return rv; > > > + > > > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) { > > > + mapping = &max77650_irq_mapping_table[i]; > > > + cell = &cells[mapping->cell_num]; > > > + > > > + res = devm_kcalloc(dev, sizeof(*res), > > > + mapping->num_irqs, GFP_KERNEL); > > > + if (!res) > > > + return -ENOMEM; > > > + > > > + cell->resources = res; > > > + cell->num_resources = mapping->num_irqs; > > > + > > > + for (j = 0; j < mapping->num_irqs; j++) { > > > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]); > > > + if (irq < 0) > > > + return irq; > > > + > > > + res[j].start = res[j].end = irq; > > > + res[j].flags = IORESOURCE_IRQ; > > > + res[j].name = mapping->irq_names[j]; > > > + } > > > + } > > > > This is the first time I've seen it done like this (and I hate it). > > > > Why are you storing the virqs in resources? > > > > I think this is highly irregular. > > > > I initially just passed the regmap_irq_chip_data over i2c clientdata > and sub-drivers would look up virq numbers from it but was advised by > Dmitry Torokhov to use resources instead. After implementing it this > way I too think it's more elegant in sub-drivers who can simply do > platform_get_irq_byname(). Do you have a different idea? > What exactly don't you like about this? * The declaration of a superfluous struct * 100 lines of additional/avoidable code * Hacky hoop jumping trying to fudge VIRQs into resources * Resources were designed for HWIRQs (unless a domain is present) * Loads of additional/avoidable CPU cycles setting all this up Need I go on? :) Surely the fact that you are using both sides of an API (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must set some alarm bells ringing? This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. And for what? To avoid passing IRQ data to a child driver?
wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices > > > > for which the drivers will be added in subsequent patches. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > --- > > > > drivers/mfd/Kconfig | 11 ++ > > > > drivers/mfd/Makefile | 1 + > > > > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++ > > > > include/linux/mfd/max77650.h | 59 ++++++ > > > > 4 files changed, 413 insertions(+) > > > > create mode 100644 drivers/mfd/max77650.c > > > > create mode 100644 include/linux/mfd/max77650.h > > [...] > > > > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = { > > > > + { > > > > + .cell_num = MAX77650_CELL_CHARGER, > > > > + .irqs = max77650_charger_irqs, > > > > + .irq_names = max77650_charger_irq_names, > > > > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs), > > > > + }, > > > > + { > > > > + .cell_num = MAX77650_CELL_GPIO, > > > > + .irqs = max77650_gpio_irqs, > > > > + .irq_names = max77650_gpio_irq_names, > > > > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs), > > > > + }, > > > > + { > > > > + .cell_num = MAX77650_CELL_ONKEY, > > > > + .irqs = max77650_onkey_irqs, > > > > + .irq_names = max77650_onkey_irq_names, > > > > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs), > > > > + }, > > > > +}; > > > > > > This is all a bit convoluted and nasty TBH. > > > > > > > +static const struct mfd_cell max77650_cells[] = { > > > > + [MAX77650_CELL_REGULATOR] = { > > > > + .name = "max77650-regulator", > > > > + .of_compatible = "maxim,max77650-regulator", > > > > + }, > > > > + [MAX77650_CELL_CHARGER] = { > > > > + .name = "max77650-charger", > > > > + .of_compatible = "maxim,max77650-charger", > > > > + }, > > > > + [MAX77650_CELL_GPIO] = { > > > > + .name = "max77650-gpio", > > > > + .of_compatible = "maxim,max77650-gpio", > > > > + }, > > > > + [MAX77650_CELL_LED] = { > > > > + .name = "max77650-led", > > > > + .of_compatible = "maxim,max77650-led", > > > > + }, > > > > + [MAX77650_CELL_ONKEY] = { > > > > + .name = "max77650-onkey", > > > > + .of_compatible = "maxim,max77650-onkey", > > > > + }, > > > > +}; > > > > > > Why are you numbering the cells? There is no need to do this. > > > > > > > Just for better readability. It makes sense to me coupled with > > MAX77650_NUM_CELLS. > > You have it the wrong way around. You define the cell data, then > provide the number of them using ARRAY_SIZE(). The enum containing > MAX77650_NUM_CELLS should not exist. > > > > > +static const struct regmap_irq max77650_irqs[] = { > > > > + [MAX77650_INT_GPI] = { > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > + .mask = MAX77650_INT_GPI_MSK, > > > > + .type = { > > > > + .type_falling_val = MAX77650_INT_GPI_F_MSK, > > > > + .type_rising_val = MAX77650_INT_GPI_R_MSK, > > > > + .types_supported = IRQ_TYPE_EDGE_BOTH, > > > > + }, > > > > + }, > > > > + [MAX77650_INT_nEN_F] = { > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > + .mask = MAX77650_INT_nEN_F_MSK, > > > > + }, > > > > + [MAX77650_INT_nEN_R] = { > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > + .mask = MAX77650_INT_nEN_R_MSK, > > > > + }, > > > > + [MAX77650_INT_TJAL1_R] = { > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > + .mask = MAX77650_INT_TJAL1_R_MSK, > > > > + }, > > > > + [MAX77650_INT_TJAL2_R] = { > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > + .mask = MAX77650_INT_TJAL2_R_MSK, > > > > + }, > > > > + [MAX77650_INT_DOD_R] = { > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > + .mask = MAX77650_INT_DOD_R_MSK, > > > > + }, > > > > + [MAX77650_INT_THM] = { > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > + .mask = MAX77650_INT_THM_MSK, > > > > + }, > > > > + [MAX77650_INT_CHG] = { > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > + .mask = MAX77650_INT_CHG_MSK, > > > > + }, > > > > + [MAX77650_INT_CHGIN] = { > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > + .mask = MAX77650_INT_CHGIN_MSK, > > > > + }, > > > > + [MAX77650_INT_TJ_REG] = { > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > + .mask = MAX77650_INT_TJ_REG_MSK, > > > > + }, > > > > + [MAX77650_INT_CHGIN_CTRL] = { > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > + .mask = MAX77650_INT_CHGIN_CTRL_MSK, > > > > + }, > > > > + [MAX77650_INT_SYS_CTRL] = { > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > + .mask = MAX77650_INT_SYS_CTRL_MSK, > > > > + }, > > > > + [MAX77650_INT_SYS_CNFG] = { > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > + .mask = MAX77650_INT_SYS_CNFG_MSK, > > > > + }, > > > > +}; > > > > > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(), > > > you can use REGMAP_IRQ_REG() like everyone else does. > > > > > > > I could even use it now - except for the first interrupt. I decided to > > not use it everywhere as it looks much better that way than having > > REGMAP_IRQ_REG() for all definitions and then the first one sticking > > out like that. It just looks better. > > The way it's done at the moment looks terrible. > > Please use the MACROs to simplify as much of the code as possible. > > > > > +static const struct regmap_irq_chip max77650_irq_chip = { > > > > + .name = "max77650-irq", > > > > + .irqs = max77650_irqs, > > > > + .num_irqs = ARRAY_SIZE(max77650_irqs), > > > > + .num_regs = 2, > > > > + .status_base = MAX77650_REG_INT_GLBL, > > > > + .mask_base = MAX77650_REG_INTM_GLBL, > > > > + .type_in_mask = true, > > > > + .type_invert = true, > > > > + .init_ack_masked = true, > > > > + .clear_on_unmask = true, > > > > +}; > > > > + > > > > +static const struct regmap_config max77650_regmap_config = { > > > > + .name = "max77650", > > > > + .reg_bits = 8, > > > > + .val_bits = 8, > > > > +}; > > > > + > > > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells) > > > > +{ > > > > + const struct max77650_irq_mapping *mapping; > > > > + struct regmap_irq_chip_data *irq_data; > > > > + struct i2c_client *i2c; > > > > + struct mfd_cell *cell; > > > > + struct resource *res; > > > > + struct regmap *map; > > > > + int i, j, irq, rv; > > > > + > > > > + i2c = to_i2c_client(dev); > > > > + > > > > + map = dev_get_regmap(dev, NULL); > > > > + if (!map) > > > > + return -ENODEV; > > > > + > > > > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq, > > > > + IRQF_ONESHOT | IRQF_SHARED, -1, > > > > > > What is -1? Are you sure this isn't defined somewhere? > > > > > > > I don't see any define for negative irq_base argument. I can add that > > in a separate series and convert the users, but for now I'd stick with > > -1. > > IMO it should be defined. You can define it locally for now. > > > > > + &max77650_irq_chip, &irq_data); > > > > + if (rv) > > > > + return rv; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) { > > > > + mapping = &max77650_irq_mapping_table[i]; > > > > + cell = &cells[mapping->cell_num]; > > > > + > > > > + res = devm_kcalloc(dev, sizeof(*res), > > > > + mapping->num_irqs, GFP_KERNEL); > > > > + if (!res) > > > > + return -ENOMEM; > > > > + > > > > + cell->resources = res; > > > > + cell->num_resources = mapping->num_irqs; > > > > + > > > > + for (j = 0; j < mapping->num_irqs; j++) { > > > > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]); > > > > + if (irq < 0) > > > > + return irq; > > > > + > > > > + res[j].start = res[j].end = irq; > > > > + res[j].flags = IORESOURCE_IRQ; > > > > + res[j].name = mapping->irq_names[j]; > > > > + } > > > > + } > > > > > > This is the first time I've seen it done like this (and I hate it). > > > > > > Why are you storing the virqs in resources? > > > > > > I think this is highly irregular. > > > > > > > I initially just passed the regmap_irq_chip_data over i2c clientdata > > and sub-drivers would look up virq numbers from it but was advised by > > Dmitry Torokhov to use resources instead. After implementing it this > > way I too think it's more elegant in sub-drivers who can simply do > > platform_get_irq_byname(). Do you have a different idea? > > > What exactly don't you like about this? > > * The declaration of a superfluous struct > * 100 lines of additional/avoidable code > * Hacky hoop jumping trying to fudge VIRQs into resources > * Resources were designed for HWIRQs (unless a domain is present) > * Loads of additional/avoidable CPU cycles setting all this up While the above may be right, this one is negligible and you know it. :) > > Need I go on? :) > > Surely the fact that you are using both sides of an API > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > set some alarm bells ringing? > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > And for what? To avoid passing IRQ data to a child driver? > What do you propose? Should I go back to the approach in v1 and pass the regmap_irq_chip_data to child drivers? @Dmitry: what do you think? Bart
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices > > > > > for which the drivers will be added in subsequent patches. > > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > --- > > > > > drivers/mfd/Kconfig | 11 ++ > > > > > drivers/mfd/Makefile | 1 + > > > > > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++ > > > > > include/linux/mfd/max77650.h | 59 ++++++ > > > > > 4 files changed, 413 insertions(+) > > > > > create mode 100644 drivers/mfd/max77650.c > > > > > create mode 100644 include/linux/mfd/max77650.h > > > > [...] > > > > > > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = { > > > > > + { > > > > > + .cell_num = MAX77650_CELL_CHARGER, > > > > > + .irqs = max77650_charger_irqs, > > > > > + .irq_names = max77650_charger_irq_names, > > > > > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs), > > > > > + }, > > > > > + { > > > > > + .cell_num = MAX77650_CELL_GPIO, > > > > > + .irqs = max77650_gpio_irqs, > > > > > + .irq_names = max77650_gpio_irq_names, > > > > > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs), > > > > > + }, > > > > > + { > > > > > + .cell_num = MAX77650_CELL_ONKEY, > > > > > + .irqs = max77650_onkey_irqs, > > > > > + .irq_names = max77650_onkey_irq_names, > > > > > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs), > > > > > + }, > > > > > +}; > > > > > > > > This is all a bit convoluted and nasty TBH. > > > > > > > > > +static const struct mfd_cell max77650_cells[] = { > > > > > + [MAX77650_CELL_REGULATOR] = { > > > > > + .name = "max77650-regulator", > > > > > + .of_compatible = "maxim,max77650-regulator", > > > > > + }, > > > > > + [MAX77650_CELL_CHARGER] = { > > > > > + .name = "max77650-charger", > > > > > + .of_compatible = "maxim,max77650-charger", > > > > > + }, > > > > > + [MAX77650_CELL_GPIO] = { > > > > > + .name = "max77650-gpio", > > > > > + .of_compatible = "maxim,max77650-gpio", > > > > > + }, > > > > > + [MAX77650_CELL_LED] = { > > > > > + .name = "max77650-led", > > > > > + .of_compatible = "maxim,max77650-led", > > > > > + }, > > > > > + [MAX77650_CELL_ONKEY] = { > > > > > + .name = "max77650-onkey", > > > > > + .of_compatible = "maxim,max77650-onkey", > > > > > + }, > > > > > +}; > > > > > > > > Why are you numbering the cells? There is no need to do this. > > > > > > > > > > Just for better readability. It makes sense to me coupled with > > > MAX77650_NUM_CELLS. > > > > You have it the wrong way around. You define the cell data, then > > provide the number of them using ARRAY_SIZE(). The enum containing > > MAX77650_NUM_CELLS should not exist. > > > > > > > +static const struct regmap_irq max77650_irqs[] = { > > > > > + [MAX77650_INT_GPI] = { > > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > > + .mask = MAX77650_INT_GPI_MSK, > > > > > + .type = { > > > > > + .type_falling_val = MAX77650_INT_GPI_F_MSK, > > > > > + .type_rising_val = MAX77650_INT_GPI_R_MSK, > > > > > + .types_supported = IRQ_TYPE_EDGE_BOTH, > > > > > + }, > > > > > + }, > > > > > + [MAX77650_INT_nEN_F] = { > > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > > + .mask = MAX77650_INT_nEN_F_MSK, > > > > > + }, > > > > > + [MAX77650_INT_nEN_R] = { > > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > > + .mask = MAX77650_INT_nEN_R_MSK, > > > > > + }, > > > > > + [MAX77650_INT_TJAL1_R] = { > > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > > + .mask = MAX77650_INT_TJAL1_R_MSK, > > > > > + }, > > > > > + [MAX77650_INT_TJAL2_R] = { > > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > > + .mask = MAX77650_INT_TJAL2_R_MSK, > > > > > + }, > > > > > + [MAX77650_INT_DOD_R] = { > > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET, > > > > > + .mask = MAX77650_INT_DOD_R_MSK, > > > > > + }, > > > > > + [MAX77650_INT_THM] = { > > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > > + .mask = MAX77650_INT_THM_MSK, > > > > > + }, > > > > > + [MAX77650_INT_CHG] = { > > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > > + .mask = MAX77650_INT_CHG_MSK, > > > > > + }, > > > > > + [MAX77650_INT_CHGIN] = { > > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > > + .mask = MAX77650_INT_CHGIN_MSK, > > > > > + }, > > > > > + [MAX77650_INT_TJ_REG] = { > > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > > + .mask = MAX77650_INT_TJ_REG_MSK, > > > > > + }, > > > > > + [MAX77650_INT_CHGIN_CTRL] = { > > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > > + .mask = MAX77650_INT_CHGIN_CTRL_MSK, > > > > > + }, > > > > > + [MAX77650_INT_SYS_CTRL] = { > > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > > + .mask = MAX77650_INT_SYS_CTRL_MSK, > > > > > + }, > > > > > + [MAX77650_INT_SYS_CNFG] = { > > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET, > > > > > + .mask = MAX77650_INT_SYS_CNFG_MSK, > > > > > + }, > > > > > +}; > > > > > > > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(), > > > > you can use REGMAP_IRQ_REG() like everyone else does. > > > > > > > > > > I could even use it now - except for the first interrupt. I decided to > > > not use it everywhere as it looks much better that way than having > > > REGMAP_IRQ_REG() for all definitions and then the first one sticking > > > out like that. It just looks better. > > > > The way it's done at the moment looks terrible. > > > > Please use the MACROs to simplify as much of the code as possible. > > > > > > > +static const struct regmap_irq_chip max77650_irq_chip = { > > > > > + .name = "max77650-irq", > > > > > + .irqs = max77650_irqs, > > > > > + .num_irqs = ARRAY_SIZE(max77650_irqs), > > > > > + .num_regs = 2, > > > > > + .status_base = MAX77650_REG_INT_GLBL, > > > > > + .mask_base = MAX77650_REG_INTM_GLBL, > > > > > + .type_in_mask = true, > > > > > + .type_invert = true, > > > > > + .init_ack_masked = true, > > > > > + .clear_on_unmask = true, > > > > > +}; > > > > > + > > > > > +static const struct regmap_config max77650_regmap_config = { > > > > > + .name = "max77650", > > > > > + .reg_bits = 8, > > > > > + .val_bits = 8, > > > > > +}; > > > > > + > > > > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells) > > > > > +{ > > > > > + const struct max77650_irq_mapping *mapping; > > > > > + struct regmap_irq_chip_data *irq_data; > > > > > + struct i2c_client *i2c; > > > > > + struct mfd_cell *cell; > > > > > + struct resource *res; > > > > > + struct regmap *map; > > > > > + int i, j, irq, rv; > > > > > + > > > > > + i2c = to_i2c_client(dev); > > > > > + > > > > > + map = dev_get_regmap(dev, NULL); > > > > > + if (!map) > > > > > + return -ENODEV; > > > > > + > > > > > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq, > > > > > + IRQF_ONESHOT | IRQF_SHARED, -1, > > > > > > > > What is -1? Are you sure this isn't defined somewhere? > > > > > > > > > > I don't see any define for negative irq_base argument. I can add that > > > in a separate series and convert the users, but for now I'd stick with > > > -1. > > > > IMO it should be defined. You can define it locally for now. > > > > > > > + &max77650_irq_chip, &irq_data); > > > > > + if (rv) > > > > > + return rv; > > > > > + > > > > > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) { > > > > > + mapping = &max77650_irq_mapping_table[i]; > > > > > + cell = &cells[mapping->cell_num]; > > > > > + > > > > > + res = devm_kcalloc(dev, sizeof(*res), > > > > > + mapping->num_irqs, GFP_KERNEL); > > > > > + if (!res) > > > > > + return -ENOMEM; > > > > > + > > > > > + cell->resources = res; > > > > > + cell->num_resources = mapping->num_irqs; > > > > > + > > > > > + for (j = 0; j < mapping->num_irqs; j++) { > > > > > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]); > > > > > + if (irq < 0) > > > > > + return irq; > > > > > + > > > > > + res[j].start = res[j].end = irq; > > > > > + res[j].flags = IORESOURCE_IRQ; > > > > > + res[j].name = mapping->irq_names[j]; > > > > > + } > > > > > + } > > > > > > > > This is the first time I've seen it done like this (and I hate it). > > > > > > > > Why are you storing the virqs in resources? > > > > > > > > I think this is highly irregular. > > > > > > > > > > I initially just passed the regmap_irq_chip_data over i2c clientdata > > > and sub-drivers would look up virq numbers from it but was advised by > > > Dmitry Torokhov to use resources instead. After implementing it this > > > way I too think it's more elegant in sub-drivers who can simply do > > > platform_get_irq_byname(). Do you have a different idea? > > > > > What exactly don't you like about this? > > > > * The declaration of a superfluous struct > > * 100 lines of additional/avoidable code > > * Hacky hoop jumping trying to fudge VIRQs into resources > > * Resources were designed for HWIRQs (unless a domain is present) > > * Loads of additional/avoidable CPU cycles setting all this up > > While the above may be right, this one is negligible and you know it. :) You have nested for() loops. You *are* wasting lots of cycles. > > Need I go on? :) > > > > Surely the fact that you are using both sides of an API > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > set some alarm bells ringing? > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > And for what? To avoid passing IRQ data to a child driver? > > What do you propose? Should I go back to the approach in v1 and pass > the regmap_irq_chip_data to child drivers? I'm saying you should remove all of this hackery and pass IRQs as they are supposed to be passed (like everyone else does). > @Dmitry: what do you think?
wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > * The declaration of a superfluous struct > > > * 100 lines of additional/avoidable code > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > * Resources were designed for HWIRQs (unless a domain is present) > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > While the above may be right, this one is negligible and you know it. :) > > You have nested for() loops. You *are* wasting lots of cycles. > > > > Need I go on? :) > > > > > > Surely the fact that you are using both sides of an API > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > set some alarm bells ringing? > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > What do you propose? Should I go back to the approach in v1 and pass > > the regmap_irq_chip_data to child drivers? > > I'm saying you should remove all of this hackery and pass IRQs as they > are supposed to be passed (like everyone else does). > I'm not sure what you mean by "like everyone else does" - different mfd drivers seem to be doing different things. Is a simple struct containing virtual irq numbers passed to sub-drivers fine? Bart
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > * The declaration of a superfluous struct > > > > * 100 lines of additional/avoidable code > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > Need I go on? :) > > > > > > > > Surely the fact that you are using both sides of an API > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > set some alarm bells ringing? > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > the regmap_irq_chip_data to child drivers? > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > are supposed to be passed (like everyone else does). > > I'm not sure what you mean by "like everyone else does" - different > mfd drivers seem to be doing different things. Is a simple struct > containing virtual irq numbers passed to sub-drivers fine? How do you plan on deriving the VIRQs to place into the struct?
wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > * The declaration of a superfluous struct > > > > > * 100 lines of additional/avoidable code > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > Need I go on? :) > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > set some alarm bells ringing? > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > the regmap_irq_chip_data to child drivers? > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > are supposed to be passed (like everyone else does). > > > > I'm not sure what you mean by "like everyone else does" - different > > mfd drivers seem to be doing different things. Is a simple struct > > containing virtual irq numbers passed to sub-drivers fine? > > How do you plan on deriving the VIRQs to place into the struct? Exampe: struct max77650_gpio_pdata { int gpi_irq; }; In MFD driver: struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); gpio_cell.platform_data = gpio_data; In GPIO driver: struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; int irq = gpio_data->gpi_irq; Bart
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > * The declaration of a superfluous struct > > > > > > * 100 lines of additional/avoidable code > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > > > Need I go on? :) > > > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > > set some alarm bells ringing? > > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > > the regmap_irq_chip_data to child drivers? > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > > are supposed to be passed (like everyone else does). > > > > > > I'm not sure what you mean by "like everyone else does" - different > > > mfd drivers seem to be doing different things. Is a simple struct > > > containing virtual irq numbers passed to sub-drivers fine? > > > > How do you plan on deriving the VIRQs to place into the struct? > > Exampe: > > struct max77650_gpio_pdata { > int gpi_irq; > }; > > In MFD driver: > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); > > gpio_cell.platform_data = gpio_data; > > In GPIO driver: > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; > > int irq = gpio_data->gpi_irq; Definitely not. What you're trying to do is a hack. If you're using Regmap to handle your IRQs, then you should use Regmap in the client to pull them out. Setting them via Regmap, then pulling them out again in the *same driver*, only to store them in platform data to be passed to a child device is bonkers. *Either* use the MFD provided platform-data helpers *or* pass and handle them via the Regmap APIs, *not* both.
On Tue, 12 Feb 2019, Lee Jones wrote: > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > * The declaration of a superfluous struct > > > > > > > * 100 lines of additional/avoidable code > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > > > > > Need I go on? :) > > > > > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > > > set some alarm bells ringing? > > > > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > > > the regmap_irq_chip_data to child drivers? > > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > > > are supposed to be passed (like everyone else does). > > > > > > > > I'm not sure what you mean by "like everyone else does" - different > > > > mfd drivers seem to be doing different things. Is a simple struct > > > > containing virtual irq numbers passed to sub-drivers fine? > > > > > > How do you plan on deriving the VIRQs to place into the struct? > > > > Exampe: > > > > struct max77650_gpio_pdata { > > int gpi_irq; > > }; > > > > In MFD driver: > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); > > > > gpio_cell.platform_data = gpio_data; > > > > In GPIO driver: > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; > > > > int irq = gpio_data->gpi_irq; > > Definitely not. What you're trying to do is a hack. > > If you're using Regmap to handle your IRQs, then you should use Regmap > in the client to pull them out. Setting them via Regmap, then pulling > them out again in the *same driver*, only to store them in platform > data to be passed to a child device is bonkers. > > *Either* use the MFD provided platform-data helpers *or* pass and > handle them via the Regmap APIs, *not* both. Right, a plan has been formed. Hopefully this works and you can avoid all this dancing around. Firstly, you need to make a small change to: drivers/base/regmap/regmap-irq.c Add the following function: struct irq_domain *regmap_irq_get_domain(struct regmap *map) As you can see, it will return the IRQ Domain for the chip. You can then pass this IRQ domain to mfd_add_devices() and it will do the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can remove all the nastiness in max77650_setup_irqs() and have the Input device use the standard (e.g. platform_get_irq()) APIs. How does that Sound?
śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a): > > On Tue, 12 Feb 2019, Lee Jones wrote: > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > * The declaration of a superfluous struct > > > > > > > > * 100 lines of additional/avoidable code > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > > > > > > > Need I go on? :) > > > > > > > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > > > > set some alarm bells ringing? > > > > > > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > > > > the regmap_irq_chip_data to child drivers? > > > > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > > > > are supposed to be passed (like everyone else does). > > > > > > > > > > I'm not sure what you mean by "like everyone else does" - different > > > > > mfd drivers seem to be doing different things. Is a simple struct > > > > > containing virtual irq numbers passed to sub-drivers fine? > > > > > > > > How do you plan on deriving the VIRQs to place into the struct? > > > > > > Exampe: > > > > > > struct max77650_gpio_pdata { > > > int gpi_irq; > > > }; > > > > > > In MFD driver: > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); > > > > > > gpio_cell.platform_data = gpio_data; > > > > > > In GPIO driver: > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; > > > > > > int irq = gpio_data->gpi_irq; > > > > Definitely not. What you're trying to do is a hack. > > > > If you're using Regmap to handle your IRQs, then you should use Regmap > > in the client to pull them out. Setting them via Regmap, then pulling > > them out again in the *same driver*, only to store them in platform > > data to be passed to a child device is bonkers. > > > > *Either* use the MFD provided platform-data helpers *or* pass and > > handle them via the Regmap APIs, *not* both. > > Right, a plan has been formed. > > Hopefully this works and you can avoid all this dancing around. > > Firstly, you need to make a small change to: > > drivers/base/regmap/regmap-irq.c > > Add the following function: > > struct irq_domain *regmap_irq_get_domain(struct regmap *map) > We already do have such function and a lot of mfd drivers actually use it. > As you can see, it will return the IRQ Domain for the chip. > > You can then pass this IRQ domain to mfd_add_devices() and it will do > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can > remove all the nastiness in max77650_setup_irqs() and have the Input > device use the standard (e.g. platform_get_irq()) APIs. > > How does that Sound? > This does sound better! Why didn't you lead with that in the first place? It's a pity it's not documented, I had to look at the code to find out irq resources would get translated in mfd_add_devices() if a domain is present. In that case - I really don't see a reason to create a superfluous structure to only hold the main regmap - we can very well get it from the parent device in sub-drivers as I do now. Thanks, Bartosz
On Wed, 13 Feb 2019, Bartosz Golaszewski wrote: > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > On Tue, 12 Feb 2019, Lee Jones wrote: > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > > > * The declaration of a superfluous struct > > > > > > > > > * 100 lines of additional/avoidable code > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > > > > > > > > > Need I go on? :) > > > > > > > > > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > > > > > set some alarm bells ringing? > > > > > > > > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > > > > > the regmap_irq_chip_data to child drivers? > > > > > > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > > > > > are supposed to be passed (like everyone else does). > > > > > > > > > > > > I'm not sure what you mean by "like everyone else does" - different > > > > > > mfd drivers seem to be doing different things. Is a simple struct > > > > > > containing virtual irq numbers passed to sub-drivers fine? > > > > > > > > > > How do you plan on deriving the VIRQs to place into the struct? > > > > > > > > Exampe: > > > > > > > > struct max77650_gpio_pdata { > > > > int gpi_irq; > > > > }; > > > > > > > > In MFD driver: > > > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); > > > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); > > > > > > > > gpio_cell.platform_data = gpio_data; > > > > > > > > In GPIO driver: > > > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; > > > > > > > > int irq = gpio_data->gpi_irq; > > > > > > Definitely not. What you're trying to do is a hack. > > > > > > If you're using Regmap to handle your IRQs, then you should use Regmap > > > in the client to pull them out. Setting them via Regmap, then pulling > > > them out again in the *same driver*, only to store them in platform > > > data to be passed to a child device is bonkers. > > > > > > *Either* use the MFD provided platform-data helpers *or* pass and > > > handle them via the Regmap APIs, *not* both. > > > > Right, a plan has been formed. > > > > Hopefully this works and you can avoid all this dancing around. > > > > Firstly, you need to make a small change to: > > > > drivers/base/regmap/regmap-irq.c > > > > Add the following function: > > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map) > > We already do have such function and a lot of mfd drivers actually use it. Even better. > > As you can see, it will return the IRQ Domain for the chip. > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can > > remove all the nastiness in max77650_setup_irqs() and have the Input > > device use the standard (e.g. platform_get_irq()) APIs. > > > > How does that Sound? > > This does sound better! Why didn't you lead with that in the first place? I'm not even going to dignify that stupid question with a response. > It's a pity it's not documented, I had to look at the code to find out > irq resources would get translated in mfd_add_devices() if a domain is > present. Where is it likely to be documented? MFD is pretty simple and seldom needs explanation. A 3 second look at the API you're trying to use (instead of blind copy & paste) would have told you that it's possible to take an IRQ domain as an argument. It's only the craziness in this patch which forced me to look into how Regmap handles IRQs and come up with a subsequent solution which fits your use-case. > In that case - I really don't see a reason to create a superfluous > structure to only hold the main regmap - we can very well get it from > the parent device in sub-drivers as I do now. The whole point of this solution is that you don't need to pass anything non-standard (i.e. not provided by the current APIs) to the child device.
śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a): > > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote: > > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > On Tue, 12 Feb 2019, Lee Jones wrote: > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > > > > > * The declaration of a superfluous struct > > > > > > > > > > * 100 lines of additional/avoidable code > > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > > > > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > > > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > > > > > > > > > > > Need I go on? :) > > > > > > > > > > > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > > > > > > set some alarm bells ringing? > > > > > > > > > > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > > > > > > the regmap_irq_chip_data to child drivers? > > > > > > > > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > > > > > > are supposed to be passed (like everyone else does). > > > > > > > > > > > > > > I'm not sure what you mean by "like everyone else does" - different > > > > > > > mfd drivers seem to be doing different things. Is a simple struct > > > > > > > containing virtual irq numbers passed to sub-drivers fine? > > > > > > > > > > > > How do you plan on deriving the VIRQs to place into the struct? > > > > > > > > > > Exampe: > > > > > > > > > > struct max77650_gpio_pdata { > > > > > int gpi_irq; > > > > > }; > > > > > > > > > > In MFD driver: > > > > > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); > > > > > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); > > > > > > > > > > gpio_cell.platform_data = gpio_data; > > > > > > > > > > In GPIO driver: > > > > > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; > > > > > > > > > > int irq = gpio_data->gpi_irq; > > > > > > > > Definitely not. What you're trying to do is a hack. > > > > > > > > If you're using Regmap to handle your IRQs, then you should use Regmap > > > > in the client to pull them out. Setting them via Regmap, then pulling > > > > them out again in the *same driver*, only to store them in platform > > > > data to be passed to a child device is bonkers. > > > > > > > > *Either* use the MFD provided platform-data helpers *or* pass and > > > > handle them via the Regmap APIs, *not* both. > > > > > > Right, a plan has been formed. > > > > > > Hopefully this works and you can avoid all this dancing around. > > > > > > Firstly, you need to make a small change to: > > > > > > drivers/base/regmap/regmap-irq.c > > > > > > Add the following function: > > > > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map) > > > > We already do have such function and a lot of mfd drivers actually use it. > > Even better. > > > > As you can see, it will return the IRQ Domain for the chip. > > > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do > > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can > > > remove all the nastiness in max77650_setup_irqs() and have the Input > > > device use the standard (e.g. platform_get_irq()) APIs. > > > > > > How does that Sound? > > > > This does sound better! Why didn't you lead with that in the first place? > > I'm not even going to dignify that stupid question with a response. > It's not a stupid question and you're being unnecessarily rude. As an expert in the subsystem you maintain you could have answered simply with a "this is wrong, retrieve the irq domain from the regmap irq_chip and pass it over to mfd_add_devices, the mfd core will create appropriate mappings". > > It's a pity it's not documented, I had to look at the code to find out > > irq resources would get translated in mfd_add_devices() if a domain is > > present. > > Where is it likely to be documented? MFD is pretty simple and seldom > needs explanation. A 3 second look at the API you're trying to use > (instead of blind copy & paste) would have told you that it's possible > to take an IRQ domain as an argument. > > It's only the craziness in this patch which forced me to look into how > Regmap handles IRQs and come up with a subsequent solution which fits > your use-case. > > > In that case - I really don't see a reason to create a superfluous > > structure to only hold the main regmap - we can very well get it from > > the parent device in sub-drivers as I do now. > > The whole point of this solution is that you don't need to pass > anything non-standard (i.e. not provided by the current APIs) to the > child device. > I don't understand what you're saying here. Bartosz
On Wed, 13 Feb 2019, Bartosz Golaszewski wrote: > śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote: > > > > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > On Tue, 12 Feb 2019, Lee Jones wrote: > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > > > > > > > * The declaration of a superfluous struct > > > > > > > > > > > * 100 lines of additional/avoidable code > > > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > > > > > > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > > > > > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > > > > > > > > > > > > > Need I go on? :) > > > > > > > > > > > > > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > > > > > > > set some alarm bells ringing? > > > > > > > > > > > > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > > > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > > > > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > > > > > > > the regmap_irq_chip_data to child drivers? > > > > > > > > > > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > > > > > > > are supposed to be passed (like everyone else does). > > > > > > > > > > > > > > > > I'm not sure what you mean by "like everyone else does" - different > > > > > > > > mfd drivers seem to be doing different things. Is a simple struct > > > > > > > > containing virtual irq numbers passed to sub-drivers fine? > > > > > > > > > > > > > > How do you plan on deriving the VIRQs to place into the struct? > > > > > > > > > > > > Exampe: > > > > > > > > > > > > struct max77650_gpio_pdata { > > > > > > int gpi_irq; > > > > > > }; > > > > > > > > > > > > In MFD driver: > > > > > > > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); > > > > > > > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); > > > > > > > > > > > > gpio_cell.platform_data = gpio_data; > > > > > > > > > > > > In GPIO driver: > > > > > > > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; > > > > > > > > > > > > int irq = gpio_data->gpi_irq; > > > > > > > > > > Definitely not. What you're trying to do is a hack. > > > > > > > > > > If you're using Regmap to handle your IRQs, then you should use Regmap > > > > > in the client to pull them out. Setting them via Regmap, then pulling > > > > > them out again in the *same driver*, only to store them in platform > > > > > data to be passed to a child device is bonkers. > > > > > > > > > > *Either* use the MFD provided platform-data helpers *or* pass and > > > > > handle them via the Regmap APIs, *not* both. > > > > > > > > Right, a plan has been formed. > > > > > > > > Hopefully this works and you can avoid all this dancing around. > > > > > > > > Firstly, you need to make a small change to: > > > > > > > > drivers/base/regmap/regmap-irq.c > > > > > > > > Add the following function: > > > > > > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map) > > > > > > We already do have such function and a lot of mfd drivers actually use it. > > > > Even better. > > > > > > As you can see, it will return the IRQ Domain for the chip. > > > > > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do > > > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can > > > > remove all the nastiness in max77650_setup_irqs() and have the Input > > > > device use the standard (e.g. platform_get_irq()) APIs. > > > > > > > > How does that Sound? > > > > > > This does sound better! Why didn't you lead with that in the first place? > > > > I'm not even going to dignify that stupid question with a response. > > It's not a stupid question and you're being unnecessarily rude. As an > expert in the subsystem you maintain you could have answered simply > with a "this is wrong, retrieve the irq domain from the regmap > irq_chip and pass it over to mfd_add_devices, the mfd core will create > appropriate mappings". Could be culture clash, but I found the question offensive which is why I chose not to answer it. The reason is actually explained below: "It's only the craziness in this patch which forced me to look into how Regmap handles IRQs and come up with a subsequent solution which fits your use-case." Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain could be fetched and reused here didn't enter my thought process until I delved into the inner workings of Regmap. Yes, I know MFD pretty well, but I only tend to deep-dive into other subsystems, particularly ones as complicated as Regmap, when it's necessary to do so. Now I know a little more about it, I can provide the feedback you suggest going forward. > > > It's a pity it's not documented, I had to look at the code to find out > > > irq resources would get translated in mfd_add_devices() if a domain is > > > present. > > > > Where is it likely to be documented? MFD is pretty simple and seldom > > needs explanation. A 3 second look at the API you're trying to use > > (instead of blind copy & paste) would have told you that it's possible > > to take an IRQ domain as an argument. > > > > It's only the craziness in this patch which forced me to look into how > > Regmap handles IRQs and come up with a subsequent solution which fits > > your use-case. > > > > > In that case - I really don't see a reason to create a superfluous > > > structure to only hold the main regmap - we can very well get it from > > > the parent device in sub-drivers as I do now. > > > > The whole point of this solution is that you don't need to pass > > anything non-standard (i.e. not provided by the current APIs) to the > > child device. > > I don't understand what you're saying here. I'm saying that the structure you speak of is no longer required.
śr., 13 lut 2019 o 11:39 Lee Jones <lee.jones@linaro.org> napisał(a): > > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote: > > > śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > On Tue, 12 Feb 2019, Lee Jones wrote: > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a): > > > > > > > > > > > > > > > > > > > > > > > > * The declaration of a superfluous struct > > > > > > > > > > > > * 100 lines of additional/avoidable code > > > > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources > > > > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present) > > > > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up > > > > > > > > > > > > > > > > > > > > > > While the above may be right, this one is negligible and you know it. :) > > > > > > > > > > > > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles. > > > > > > > > > > > > > > > > > > > > > > Need I go on? :) > > > > > > > > > > > > > > > > > > > > > > > > Surely the fact that you are using both sides of an API > > > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must > > > > > > > > > > > > set some alarm bells ringing? > > > > > > > > > > > > > > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess. > > > > > > > > > > > > > > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver? > > > > > > > > > > > > > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass > > > > > > > > > > > the regmap_irq_chip_data to child drivers? > > > > > > > > > > > > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they > > > > > > > > > > are supposed to be passed (like everyone else does). > > > > > > > > > > > > > > > > > > I'm not sure what you mean by "like everyone else does" - different > > > > > > > > > mfd drivers seem to be doing different things. Is a simple struct > > > > > > > > > containing virtual irq numbers passed to sub-drivers fine? > > > > > > > > > > > > > > > > How do you plan on deriving the VIRQs to place into the struct? > > > > > > > > > > > > > > Exampe: > > > > > > > > > > > > > > struct max77650_gpio_pdata { > > > > > > > int gpi_irq; > > > > > > > }; > > > > > > > > > > > > > > In MFD driver: > > > > > > > > > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data)); > > > > > > > > > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM); > > > > > > > > > > > > > > gpio_cell.platform_data = gpio_data; > > > > > > > > > > > > > > In GPIO driver: > > > > > > > > > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data; > > > > > > > > > > > > > > int irq = gpio_data->gpi_irq; > > > > > > > > > > > > Definitely not. What you're trying to do is a hack. > > > > > > > > > > > > If you're using Regmap to handle your IRQs, then you should use Regmap > > > > > > in the client to pull them out. Setting them via Regmap, then pulling > > > > > > them out again in the *same driver*, only to store them in platform > > > > > > data to be passed to a child device is bonkers. > > > > > > > > > > > > *Either* use the MFD provided platform-data helpers *or* pass and > > > > > > handle them via the Regmap APIs, *not* both. > > > > > > > > > > Right, a plan has been formed. > > > > > > > > > > Hopefully this works and you can avoid all this dancing around. > > > > > > > > > > Firstly, you need to make a small change to: > > > > > > > > > > drivers/base/regmap/regmap-irq.c > > > > > > > > > > Add the following function: > > > > > > > > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map) > > > > > > > > We already do have such function and a lot of mfd drivers actually use it. > > > > > > Even better. > > > > > > > > As you can see, it will return the IRQ Domain for the chip. > > > > > > > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do > > > > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can > > > > > remove all the nastiness in max77650_setup_irqs() and have the Input > > > > > device use the standard (e.g. platform_get_irq()) APIs. > > > > > > > > > > How does that Sound? > > > > > > > > This does sound better! Why didn't you lead with that in the first place? > > > > > > I'm not even going to dignify that stupid question with a response. > > > > It's not a stupid question and you're being unnecessarily rude. As an > > expert in the subsystem you maintain you could have answered simply > > with a "this is wrong, retrieve the irq domain from the regmap > > irq_chip and pass it over to mfd_add_devices, the mfd core will create > > appropriate mappings". > > Could be culture clash, but I found the question offensive which is > why I chose not to answer it. The reason is actually explained below: > It wasn't meant to be offensive. I guess when dealing with non-native English speakers over e-mail it's best to assume good faith. > "It's only the craziness in this patch which forced me to look into how > Regmap handles IRQs and come up with a subsequent solution which fits > your use-case." > > Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain > could be fetched and reused here didn't enter my thought process until > I delved into the inner workings of Regmap. > > Yes, I know MFD pretty well, but I only tend to deep-dive into other > subsystems, particularly ones as complicated as Regmap, when it's > necessary to do so. Now I know a little more about it, I can provide > the feedback you suggest going forward. > > > > > It's a pity it's not documented, I had to look at the code to find out > > > > irq resources would get translated in mfd_add_devices() if a domain is > > > > present. > > > > > > Where is it likely to be documented? MFD is pretty simple and seldom > > > needs explanation. A 3 second look at the API you're trying to use > > > (instead of blind copy & paste) would have told you that it's possible > > > to take an IRQ domain as an argument. > > > > > > It's only the craziness in this patch which forced me to look into how > > > Regmap handles IRQs and come up with a subsequent solution which fits > > > your use-case. > > > > > > > In that case - I really don't see a reason to create a superfluous > > > > structure to only hold the main regmap - we can very well get it from > > > > the parent device in sub-drivers as I do now. > > > > > > The whole point of this solution is that you don't need to pass > > > anything non-standard (i.e. not provided by the current APIs) to the > > > child device. > > > > I don't understand what you're saying here. > > I'm saying that the structure you speak of is no longer required. > Thanks for clarifying. Bartosz
Hi! > > As you can see, it will return the IRQ Domain for the chip. > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can > > remove all the nastiness in max77650_setup_irqs() and have the Input > > device use the standard (e.g. platform_get_irq()) APIs. > > > > How does that Sound? > > This does sound better! Why didn't you lead with that in the first >place? TBH, this can be considered rude and I'm not surprised Lee reacted the way he did. > It's a pity it's not documented, I had to look at the code to find out > irq resources would get translated in mfd_add_devices() if a domain is > present. I guess documentation patch would be welcome. Best regards, Pavel
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 76f9909cf396..a80c3fe80fbe 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -734,6 +734,17 @@ config MFD_MAX77620 provides common support for accessing the device; additional drivers must be enabled in order to use the functionality of the device. +config MFD_MAX77650 + tristate "Maxim MAX77650/77651 PMIC Support" + depends on I2C + depends on OF || COMPILE_TEST + select MFD_CORE + select REGMAP_I2C + help + Say yes here to add support for Maxim Semiconductor MAX77650 and + MAX77651 Power Management ICs. This is the core multifunction + driver for interacting with the device. + config MFD_MAX77686 tristate "Maxim Semiconductor MAX77686/802 PMIC Support" depends on I2C diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 12980a4ad460..3b912a4015d1 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150) += da9150-core.o obj-$(CONFIG_MFD_MAX14577) += max14577.o obj-$(CONFIG_MFD_MAX77620) += max77620.o +obj-$(CONFIG_MFD_MAX77650) += max77650.o obj-$(CONFIG_MFD_MAX77686) += max77686.o obj-$(CONFIG_MFD_MAX77693) += max77693.o obj-$(CONFIG_MFD_MAX77843) += max77843.o diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c new file mode 100644 index 000000000000..7c6164f1fde4 --- /dev/null +++ b/drivers/mfd/max77650.c @@ -0,0 +1,342 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Copyright (C) 2018 BayLibre SAS +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> +// +// Core MFD driver for MAXIM 77650/77651 charger/power-supply. + +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/mfd/core.h> +#include <linux/mfd/max77650.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#define MAX77650_INT_GPI_F_MSK BIT(0) +#define MAX77650_INT_GPI_R_MSK BIT(1) +#define MAX77650_INT_GPI_MSK \ + (MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK) +#define MAX77650_INT_nEN_F_MSK BIT(2) +#define MAX77650_INT_nEN_R_MSK BIT(3) +#define MAX77650_INT_TJAL1_R_MSK BIT(4) +#define MAX77650_INT_TJAL2_R_MSK BIT(5) +#define MAX77650_INT_DOD_R_MSK BIT(6) + +#define MAX77650_INT_THM_MSK BIT(0) +#define MAX77650_INT_CHG_MSK BIT(1) +#define MAX77650_INT_CHGIN_MSK BIT(2) +#define MAX77650_INT_TJ_REG_MSK BIT(3) +#define MAX77650_INT_CHGIN_CTRL_MSK BIT(4) +#define MAX77650_INT_SYS_CTRL_MSK BIT(5) +#define MAX77650_INT_SYS_CNFG_MSK BIT(6) + +#define MAX77650_INT_GLBL_OFFSET 0 +#define MAX77650_INT_CHG_OFFSET 1 + +#define MAX77650_SBIA_LPM_MASK BIT(5) +#define MAX77650_SBIA_LPM_DISABLED 0x00 + +enum { + MAX77650_INT_GPI = 0, + MAX77650_INT_nEN_F, + MAX77650_INT_nEN_R, + MAX77650_INT_TJAL1_R, + MAX77650_INT_TJAL2_R, + MAX77650_INT_DOD_R, + MAX77650_INT_THM, + MAX77650_INT_CHG, + MAX77650_INT_CHGIN, + MAX77650_INT_TJ_REG, + MAX77650_INT_CHGIN_CTRL, + MAX77650_INT_SYS_CTRL, + MAX77650_INT_SYS_CNFG, +}; + +enum { + MAX77650_CELL_REGULATOR = 0, + MAX77650_CELL_CHARGER, + MAX77650_CELL_GPIO, + MAX77650_CELL_LED, + MAX77650_CELL_ONKEY, + MAX77650_NUM_CELLS, +}; + +struct max77650_irq_mapping { + int cell_num; + const int *irqs; + const char *const *irq_names; + unsigned int num_irqs; +}; + +static const int max77650_charger_irqs[] = { + MAX77650_INT_CHG, + MAX77650_INT_CHGIN, +}; + +static const int max77650_gpio_irqs[] = { + MAX77650_INT_GPI, +}; + +static const int max77650_onkey_irqs[] = { + MAX77650_INT_nEN_F, + MAX77650_INT_nEN_R, +}; + +static const char *const max77650_charger_irq_names[] = { + "CHG", + "CHGIN", +}; + +static const char *const max77650_gpio_irq_names[] = { + "GPI", +}; + +static const char *const max77650_onkey_irq_names[] = { + "nEN_F", + "nEN_R", +}; + +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = { + { + .cell_num = MAX77650_CELL_CHARGER, + .irqs = max77650_charger_irqs, + .irq_names = max77650_charger_irq_names, + .num_irqs = ARRAY_SIZE(max77650_charger_irqs), + }, + { + .cell_num = MAX77650_CELL_GPIO, + .irqs = max77650_gpio_irqs, + .irq_names = max77650_gpio_irq_names, + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs), + }, + { + .cell_num = MAX77650_CELL_ONKEY, + .irqs = max77650_onkey_irqs, + .irq_names = max77650_onkey_irq_names, + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs), + }, +}; + +static const struct mfd_cell max77650_cells[] = { + [MAX77650_CELL_REGULATOR] = { + .name = "max77650-regulator", + .of_compatible = "maxim,max77650-regulator", + }, + [MAX77650_CELL_CHARGER] = { + .name = "max77650-charger", + .of_compatible = "maxim,max77650-charger", + }, + [MAX77650_CELL_GPIO] = { + .name = "max77650-gpio", + .of_compatible = "maxim,max77650-gpio", + }, + [MAX77650_CELL_LED] = { + .name = "max77650-led", + .of_compatible = "maxim,max77650-led", + }, + [MAX77650_CELL_ONKEY] = { + .name = "max77650-onkey", + .of_compatible = "maxim,max77650-onkey", + }, +}; + +static const struct regmap_irq max77650_irqs[] = { + [MAX77650_INT_GPI] = { + .reg_offset = MAX77650_INT_GLBL_OFFSET, + .mask = MAX77650_INT_GPI_MSK, + .type = { + .type_falling_val = MAX77650_INT_GPI_F_MSK, + .type_rising_val = MAX77650_INT_GPI_R_MSK, + .types_supported = IRQ_TYPE_EDGE_BOTH, + }, + }, + [MAX77650_INT_nEN_F] = { + .reg_offset = MAX77650_INT_GLBL_OFFSET, + .mask = MAX77650_INT_nEN_F_MSK, + }, + [MAX77650_INT_nEN_R] = { + .reg_offset = MAX77650_INT_GLBL_OFFSET, + .mask = MAX77650_INT_nEN_R_MSK, + }, + [MAX77650_INT_TJAL1_R] = { + .reg_offset = MAX77650_INT_GLBL_OFFSET, + .mask = MAX77650_INT_TJAL1_R_MSK, + }, + [MAX77650_INT_TJAL2_R] = { + .reg_offset = MAX77650_INT_GLBL_OFFSET, + .mask = MAX77650_INT_TJAL2_R_MSK, + }, + [MAX77650_INT_DOD_R] = { + .reg_offset = MAX77650_INT_GLBL_OFFSET, + .mask = MAX77650_INT_DOD_R_MSK, + }, + [MAX77650_INT_THM] = { + .reg_offset = MAX77650_INT_CHG_OFFSET, + .mask = MAX77650_INT_THM_MSK, + }, + [MAX77650_INT_CHG] = { + .reg_offset = MAX77650_INT_CHG_OFFSET, + .mask = MAX77650_INT_CHG_MSK, + }, + [MAX77650_INT_CHGIN] = { + .reg_offset = MAX77650_INT_CHG_OFFSET, + .mask = MAX77650_INT_CHGIN_MSK, + }, + [MAX77650_INT_TJ_REG] = { + .reg_offset = MAX77650_INT_CHG_OFFSET, + .mask = MAX77650_INT_TJ_REG_MSK, + }, + [MAX77650_INT_CHGIN_CTRL] = { + .reg_offset = MAX77650_INT_CHG_OFFSET, + .mask = MAX77650_INT_CHGIN_CTRL_MSK, + }, + [MAX77650_INT_SYS_CTRL] = { + .reg_offset = MAX77650_INT_CHG_OFFSET, + .mask = MAX77650_INT_SYS_CTRL_MSK, + }, + [MAX77650_INT_SYS_CNFG] = { + .reg_offset = MAX77650_INT_CHG_OFFSET, + .mask = MAX77650_INT_SYS_CNFG_MSK, + }, +}; + +static const struct regmap_irq_chip max77650_irq_chip = { + .name = "max77650-irq", + .irqs = max77650_irqs, + .num_irqs = ARRAY_SIZE(max77650_irqs), + .num_regs = 2, + .status_base = MAX77650_REG_INT_GLBL, + .mask_base = MAX77650_REG_INTM_GLBL, + .type_in_mask = true, + .type_invert = true, + .init_ack_masked = true, + .clear_on_unmask = true, +}; + +static const struct regmap_config max77650_regmap_config = { + .name = "max77650", + .reg_bits = 8, + .val_bits = 8, +}; + +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells) +{ + const struct max77650_irq_mapping *mapping; + struct regmap_irq_chip_data *irq_data; + struct i2c_client *i2c; + struct mfd_cell *cell; + struct resource *res; + struct regmap *map; + int i, j, irq, rv; + + i2c = to_i2c_client(dev); + + map = dev_get_regmap(dev, NULL); + if (!map) + return -ENODEV; + + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq, + IRQF_ONESHOT | IRQF_SHARED, -1, + &max77650_irq_chip, &irq_data); + if (rv) + return rv; + + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) { + mapping = &max77650_irq_mapping_table[i]; + cell = &cells[mapping->cell_num]; + + res = devm_kcalloc(dev, sizeof(*res), + mapping->num_irqs, GFP_KERNEL); + if (!res) + return -ENOMEM; + + cell->resources = res; + cell->num_resources = mapping->num_irqs; + + for (j = 0; j < mapping->num_irqs; j++) { + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]); + if (irq < 0) + return irq; + + res[j].start = res[j].end = irq; + res[j].flags = IORESOURCE_IRQ; + res[j].name = mapping->irq_names[j]; + } + } + + return 0; +} + +static int max77650_i2c_probe(struct i2c_client *i2c) +{ + struct device *dev = &i2c->dev; + struct mfd_cell *cells; + struct regmap *map; + unsigned int val; + int rv; + + map = devm_regmap_init_i2c(i2c, &max77650_regmap_config); + if (IS_ERR(map)) + return PTR_ERR(map); + + rv = regmap_read(map, MAX77650_REG_CID, &val); + if (rv) + return rv; + + switch (MAX77650_CID_BITS(val)) { + case MAX77650_CID_77650A: + case MAX77650_CID_77650C: + case MAX77650_CID_77651A: + case MAX77650_CID_77651B: + break; + default: + return -ENODEV; + } + + /* + * This IC has a low-power mode which reduces the quiescent current + * consumption to ~5.6uA but is only suitable for systems consuming + * less than ~2mA. Since this is not likely the case even on + * linux-based wearables - keep the chip in normal power mode. + */ + rv = regmap_update_bits(map, + MAX77650_REG_CNFG_GLBL, + MAX77650_SBIA_LPM_MASK, + MAX77650_SBIA_LPM_DISABLED); + if (rv) + return rv; + + cells = devm_kmemdup(dev, max77650_cells, + sizeof(max77650_cells), GFP_KERNEL); + if (!cells) + return -ENOMEM; + + rv = max77650_setup_irqs(dev, cells); + if (rv) + return rv; + + return devm_mfd_add_devices(dev, -1, cells, + MAX77650_NUM_CELLS, NULL, 0, NULL); +} + +static const struct of_device_id max77650_of_match[] = { + { .compatible = "maxim,max77650" }, + { } +}; +MODULE_DEVICE_TABLE(of, max77650_of_match); + +static struct i2c_driver max77650_i2c_driver = { + .driver = { + .name = "max77650", + .of_match_table = of_match_ptr(max77650_of_match), + }, + .probe_new = max77650_i2c_probe, +}; +module_i2c_driver(max77650_i2c_driver); + +MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver"); +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h new file mode 100644 index 000000000000..c809e211a8cd --- /dev/null +++ b/include/linux/mfd/max77650.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2018 BayLibre SAS + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> + * + * Common definitions for MAXIM 77650/77651 charger/power-supply. + */ + +#ifndef MAX77650_H +#define MAX77650_H + +#include <linux/bits.h> + +#define MAX77650_REG_INT_GLBL 0x00 +#define MAX77650_REG_INT_CHG 0x01 +#define MAX77650_REG_STAT_CHG_A 0x02 +#define MAX77650_REG_STAT_CHG_B 0x03 +#define MAX77650_REG_ERCFLAG 0x04 +#define MAX77650_REG_STAT_GLBL 0x05 +#define MAX77650_REG_INTM_GLBL 0x06 +#define MAX77650_REG_INTM_CHG 0x07 +#define MAX77650_REG_CNFG_GLBL 0x10 +#define MAX77650_REG_CID 0x11 +#define MAX77650_REG_CNFG_GPIO 0x12 +#define MAX77650_REG_CNFG_CHG_A 0x18 +#define MAX77650_REG_CNFG_CHG_B 0x19 +#define MAX77650_REG_CNFG_CHG_C 0x1a +#define MAX77650_REG_CNFG_CHG_D 0x1b +#define MAX77650_REG_CNFG_CHG_E 0x1c +#define MAX77650_REG_CNFG_CHG_F 0x1d +#define MAX77650_REG_CNFG_CHG_G 0x1e +#define MAX77650_REG_CNFG_CHG_H 0x1f +#define MAX77650_REG_CNFG_CHG_I 0x20 +#define MAX77650_REG_CNFG_SBB_TOP 0x28 +#define MAX77650_REG_CNFG_SBB0_A 0x29 +#define MAX77650_REG_CNFG_SBB0_B 0x2a +#define MAX77650_REG_CNFG_SBB1_A 0x2b +#define MAX77650_REG_CNFG_SBB1_B 0x2c +#define MAX77650_REG_CNFG_SBB2_A 0x2d +#define MAX77650_REG_CNFG_SBB2_B 0x2e +#define MAX77650_REG_CNFG_LDO_A 0x38 +#define MAX77650_REG_CNFG_LDO_B 0x39 +#define MAX77650_REG_CNFG_LED0_A 0x40 +#define MAX77650_REG_CNFG_LED1_A 0x41 +#define MAX77650_REG_CNFG_LED2_A 0x42 +#define MAX77650_REG_CNFG_LED0_B 0x43 +#define MAX77650_REG_CNFG_LED1_B 0x44 +#define MAX77650_REG_CNFG_LED2_B 0x45 +#define MAX77650_REG_CNFG_LED_TOP 0x46 + +#define MAX77650_CID_MASK GENMASK(3, 0) +#define MAX77650_CID_BITS(_reg) (_reg & MAX77650_CID_MASK) + +#define MAX77650_CID_77650A 0x03 +#define MAX77650_CID_77650C 0x0a +#define MAX77650_CID_77651A 0x06 +#define MAX77650_CID_77651B 0x08 + +#endif /* MAX77650_H */