On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:

wt., 12 lut 2019 o 10:55 Lee Jones [off-list ref] napisał(a):

quoted

On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:

quoted

wt., 12 lut 2019 o 09:36 Lee Jones [off-list ref] napisał(a):

quoted

On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:

quoted

From: Bartosz Golaszewski <redacted>

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 <redacted>
---
 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

[...]

quoted

quoted

quoted

+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.

quoted

+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.

quoted

quoted

quoted

+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.

quoted

quoted

quoted

+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.

quoted

quoted

quoted

+                                   &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?

quoted

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.

quoted

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?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog