From: Lee Jones <lee.jones@linaro.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Linus Walleij <linus.walleij@linaro.org>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Jacek Anaszewski <jacek.anaszewski@gmail.com>,
Pavel Machek <pavel@ucw.cz>, Sebastian Reichel <sre@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
Linux Input <linux-input@vger.kernel.org>,
Linux LED Subsystem <linux-leds@vger.kernel.org>,
Linux PM list <linux-pm@vger.kernel.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
Date: Tue, 12 Feb 2019 09:54:57 +0000 [thread overview]
Message-ID: <20190212095457.GA20638@dell> (raw)
In-Reply-To: <CAMRc=MderQf20_8aG4-otBkCv60FSNSSqV3NVALPkFL-MqmJbg@mail.gmail.com>
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?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
next prev parent reply other threads:[~2019-02-12 9:55 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-05 9:12 [PATCH v4 00/10] mfd: add support for max77650 PMIC Bartosz Golaszewski
2019-02-05 9:12 ` [PATCH v4 01/10] dt-bindings: mfd: add DT bindings for max77650 Bartosz Golaszewski
2019-02-08 12:15 ` Linus Walleij
2019-02-05 9:12 ` [PATCH v4 02/10] dt-bindings: power: supply: " Bartosz Golaszewski
2019-02-05 9:12 ` [PATCH v4 03/10] dt-bindings: leds: " Bartosz Golaszewski
2019-02-05 9:12 ` [PATCH v4 04/10] dt-bindings: input: " Bartosz Golaszewski
2019-02-05 9:12 ` [PATCH v4 05/10] mfd: max77650: new core mfd driver Bartosz Golaszewski
2019-02-12 8:36 ` Lee Jones
2019-02-12 8:52 ` Bartosz Golaszewski
2019-02-12 9:54 ` Lee Jones [this message]
2019-02-12 10:12 ` Bartosz Golaszewski
2019-02-12 10:18 ` Lee Jones
2019-02-12 10:24 ` Bartosz Golaszewski
2019-02-12 11:14 ` Lee Jones
2019-02-12 12:29 ` Bartosz Golaszewski
2019-02-12 13:20 ` Lee Jones
2019-02-13 9:25 ` Lee Jones
2019-02-13 9:35 ` Bartosz Golaszewski
2019-02-13 9:53 ` Lee Jones
2019-02-13 10:15 ` Bartosz Golaszewski
2019-02-13 10:39 ` Lee Jones
2019-02-13 10:46 ` Bartosz Golaszewski
2019-02-13 11:02 ` Pavel Machek
2019-02-05 9:12 ` [PATCH v4 06/10] power: supply: max77650: add support for battery charger Bartosz Golaszewski
2019-02-12 8:23 ` Lee Jones
2019-02-05 9:12 ` [PATCH v4 07/10] gpio: max77650: add GPIO support Bartosz Golaszewski
2019-02-08 11:22 ` Linus Walleij
2019-02-05 9:12 ` [PATCH v4 08/10] leds: max77650: add LEDs support Bartosz Golaszewski
2019-02-05 9:12 ` [PATCH v4 09/10] input: max77650: add onkey support Bartosz Golaszewski
2019-02-05 9:12 ` [PATCH v4 10/10] MAINTAINERS: add an entry for max77650 mfd driver Bartosz Golaszewski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190212095457.GA20638@dell \
--to=lee.jones@linaro.org \
--cc=bgolaszewski@baylibre.com \
--cc=brgl@bgdev.pl \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jacek.anaszewski@gmail.com \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pavel@ucw.cz \
--cc=robh+dt@kernel.org \
--cc=sre@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).