[v4,05/10] mfd: max77650: new core mfd driver
diff mbox series

Message ID 20190205091237.6448-6-brgl@bgdev.pl
State Superseded
Headers show
Series
  • mfd: add support for max77650 PMIC
Related show

Commit Message

Bartosz Golaszewski Feb. 5, 2019, 9:12 a.m. UTC
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

Comments

Lee Jones Feb. 12, 2019, 8:36 a.m. UTC | #1
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 */
Bartosz Golaszewski Feb. 12, 2019, 8:52 a.m. UTC | #2
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
Lee Jones Feb. 12, 2019, 9:54 a.m. UTC | #3
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?
Bartosz Golaszewski Feb. 12, 2019, 10:12 a.m. UTC | #4
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
Lee Jones Feb. 12, 2019, 10:18 a.m. UTC | #5
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?
Bartosz Golaszewski Feb. 12, 2019, 10:24 a.m. UTC | #6
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
Lee Jones Feb. 12, 2019, 11:14 a.m. UTC | #7
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?
Bartosz Golaszewski Feb. 12, 2019, 12:29 p.m. UTC | #8
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
Lee Jones Feb. 12, 2019, 1:20 p.m. UTC | #9
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.
Lee Jones Feb. 13, 2019, 9:25 a.m. UTC | #10
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?
Bartosz Golaszewski Feb. 13, 2019, 9:35 a.m. UTC | #11
ś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
Lee Jones Feb. 13, 2019, 9:53 a.m. UTC | #12
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.
Bartosz Golaszewski Feb. 13, 2019, 10:15 a.m. UTC | #13
ś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
Lee Jones Feb. 13, 2019, 10:39 a.m. UTC | #14
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.
Bartosz Golaszewski Feb. 13, 2019, 10:46 a.m. UTC | #15
ś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
Pavel Machek Feb. 13, 2019, 11:02 a.m. UTC | #16
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

Patch
diff mbox series

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 */