Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@xxxxxxxxxx> napisał(a):
>
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@xxxxxxxxxx> napisał(a):
> > >
> > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxx>
> > > > ---
> > > >  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




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux