[RFC,v3,00/15] Support ROHM BD71828 PMIC
mbox series

Message ID cover.1572606437.git.matti.vaittinen@fi.rohmeurope.com
Headers show
Series
  • Support ROHM BD71828 PMIC
Related show

Message

Vaittinen, Matti Nov. 1, 2019, 11:28 a.m. UTC
Patch series introducing support for ROHM BD71828 PMIC

ROHM BD71828 is a power management IC containing 7 bucks and 7 LDOs. All
regulators can either be controlled individually via I2C. Bucks 1,2,6 and
7 can also be assigned to a "regulator group" controlled by run-levels.
Eg. Run level specific voltages and enable/disable statuses for each of
these bucks can be set via register interface. The buck run-level group
assignment (selection if buck is to be controlled individually or via
run-levels) can be changed at run-time via I2C.

Run level changes can then be initiated wither via I2C writes or GPIO.
and when run-level is changed, state of all bucks which are set to be
controlled via run-levels are changed accrdingly.

This control mechanism selection (I2C or GPIO) is selected by data in
one time programmable PMIC memory area (during production) and can't be
changed later.

In addition to the bucks and LDOs there are:

- The usual clk gate
- 4 IO pins (mostly usable as GPO or tied to specific purpose)
- power button support
- RTC
- two LEDs
- battery charger
- HALL sensor input

This patch series adds support to regulators, clk, RTC, and GPIOs. LED
support will be added later when fate of RFC "Add DT node finding and
parsing to core" is known.

https://lore.kernel.org/lkml/cover.1572351774.git.matti.vaittinen@fi.rohmeurope.com/

Power-supply driver for charger is "under construction" and not included
in this RFC series.

Reason for RFC status is the regulator grouping to run-levels.

Patches 8 and 9 bring more or less the usual regulator support.

Patches 10, 11, 12 add run-level control which I am not entirely happy
with. I don't like sysfs interface for run-level control. I am not
entirely happy with the in-kernel APIs either as those provide run-level
control via one *regulator pointer - but change impacts to more than one
regulator.

All suggestions are appreciated and welcome.

Rest of the patches should be business as usual.

Changelog v3:
  DT-Bindings:
    - yamlify
    - add LED binding doc
  CLK:
    - Move clk register definitions from MFD headers to clk driver
  GPIO:
    - Add generic direction define and use it.
  LED:
    - Drop LED driver from the series (for now).

Changelog v2: Mainly RTC and GPIO fixes suggested by Alexandre and Bartosz

  General:
    -Patch ordering changed to provide dt binding documents right after the
     MFD core.
  DT-Bindings for regulators (Patch 3)
    -Fix typo in PMIC model number
  RTC (patch 11)
    -Reverted renaming in order to reduce patch size.
    -Reworded commit message
  BD71828 regulator (patch 7)
    -Add MODULE_ALIAS
  GPIO (patch 12)
    -Remove file-name from comment
    -prefix IN and OUT defines with chip type
    -improved documentation for the INPUT only pin.
    -removed empty left-over function
    -removed unnecessary #ifdef CONFIG_OF_GPIO
    -removed unnecessary error print
    -Add MODULE_ALIAS

Patch 1:
        BD71828 MFD core.
Patch 2:
	dt-bindings for BD71828 PMIC
Patch 3:
        dt-bindings for regulators on BD71828 PMIC
Patch 4:
        dt-bindings for LEDs on BD71828 PMIC
Patch 5:
	Power button support using GPIO keys.
Patch 6:
        CLK gate support using existing clk-bd718x7
Patch 7:
        Move clk register definitions from headers to driver
Patch 8:
        Split existing bd718x7 regulator driver to generic ROHM dt
        parsing portion (used by more than one ROHM drivers) and
        bd718x8 specific parts
Patch 9:
        Basic regulator support (individual control via I2C). This
        should be pretty standard stuff.
Patch 10:
        Add support for getting regulator voltages when GPIO controlled
        run-levels are used. Allow specifying voltages for run-levels
        via DT. Allow controlling run-levels via sysfs entries (I am not
        happy about this. Probably should only provide in-kernel API for
        this or is there better ideas? Showing can be done vis sysfs?
        Debugfs?)
Patch 11:
        Support setting/getting run-levels when they are controlled via
        I2C instead of GPIO. Add in-kernel API for settin run-level
        voltages for regulators at run-time.
Patch 12:
        Add in-kernel APIs for changing the RUN-level. Safer than sysfs
        I guess. But is there some better method for controlling this
        kind of dynamic group of regulators?
Patch 13:
        Support BD71828 RTC block using BD70528 RTC driver
Patch 14:
        Add generic GPIO direction defines
Patch 15:
        Allow control of GP(I)O pins on BD71828 via GPIO subsystem
Patch 16:
        Support toggling the LEDs

This patch series is based on v5.4-rc5

---

Matti Vaittinen (15):
  mfd: bd71828: Support ROHM BD71828 PMIC - core
  dt-bindings: mfd: Document ROHM BD71828 bindings
  dt-bindings: regulator: Document ROHM BD71282 regulator bindings
  dt-bindings: leds: ROHM BD71282 PMIC LED driver
  mfd: input: bd71828: Add power-key support
  clk: bd718x7: Support ROHM BD71828 clk block
  clk: bd718x7: simplify header dependencies
  regulator: bd718x7: Split driver to common and bd718x7 specific parts
  regulator: bd71828: Basic support for ROHM bd71828 PMIC regulators
  regulator: bd71828: Add GPIO based run-level control for regulators
  regulator: bd71828: enhanced run-level support
  regulator: bd71828: Support in-kernel APIs to change run-level
  rtc: bd70528 add BD71828 support
  gpio: Add definition for GPIO direction
  gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs

 .../bindings/leds/rohm,leds-bd71828.yaml      |   46 +
 .../bindings/mfd/rohm,bd71828-pmic.yaml       |  249 +++
 .../regulator/rohm,bd71828-regulator.yaml     |  123 ++
 drivers/clk/Kconfig                           |    6 +-
 drivers/clk/clk-bd718x7.c                     |   37 +-
 drivers/gpio/Kconfig                          |   12 +
 drivers/gpio/Makefile                         |    1 +
 drivers/gpio/gpio-bd71828.c                   |  149 ++
 drivers/mfd/Kconfig                           |   15 +
 drivers/mfd/Makefile                          |    2 +-
 drivers/mfd/rohm-bd71828.c                    |  350 ++++
 drivers/regulator/Kconfig                     |   16 +
 drivers/regulator/Makefile                    |    2 +
 drivers/regulator/bd71828-regulator.c         | 1443 +++++++++++++++++
 drivers/regulator/bd718x7-regulator.c         |  183 +--
 drivers/regulator/rohm-regulator.c            |   95 ++
 drivers/rtc/Kconfig                           |    3 +-
 drivers/rtc/rtc-bd70528.c                     |  150 +-
 include/linux/gpio/driver.h                   |    3 +
 include/linux/mfd/rohm-bd70528.h              |   19 +-
 include/linux/mfd/rohm-bd71828.h              |  428 +++++
 include/linux/mfd/rohm-bd718x7.h              |    6 -
 include/linux/mfd/rohm-generic.h              |   45 +
 include/linux/mfd/rohm-shared.h               |   27 +
 24 files changed, 3230 insertions(+), 180 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/rohm,leds-bd71828.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml
 create mode 100644 drivers/gpio/gpio-bd71828.c
 create mode 100644 drivers/mfd/rohm-bd71828.c
 create mode 100644 drivers/regulator/bd71828-regulator.c
 create mode 100644 drivers/regulator/rohm-regulator.c
 create mode 100644 include/linux/mfd/rohm-bd71828.h
 create mode 100644 include/linux/mfd/rohm-shared.h

Comments

Vaittinen, Matti Nov. 1, 2019, 11:53 a.m. UTC | #1
On Fri, 2019-11-01 at 13:28 +0200, Matti Vaittinen wrote:
> Patch series introducing support for ROHM BD71828 PMIC
> 
...
> 
> Patch 14:
>         Add generic GPIO direction defines
> Patch 15:
>         Allow control of GP(I)O pins on BD71828 via GPIO subsystem
> Patch 16:
>         Support toggling the LEDs

Sorry Folks - this is a left over. I dropped the LED driverfrom the series for now.
Linus Walleij Nov. 3, 2019, 10:27 p.m. UTC | #2
On Fri, Nov 1, 2019 at 12:43 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:

> Bucks 1,2,6 and 7 on ROHM BD71828 can be either controlled as
> individual regulartors - or they can be grouped to a group of
> regulators that are controlled by 'run levels'. This can be
> done via I2C. Each regulator can be assigned a voltage and
> enable/disable status for each run-level. These statuses are
> also changeable via I2C.
>
> Run-levels can then be changed either by I2C or GPIO. This
> control mechanism is selected by data in one time programmable
> area (during production) and can't be changed later.
>
> Allow regulators to be controlled via run-levels and allow
> getting/setting the current run-level also via GPIO.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

I like the way you use the gpio API so FWIW:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I do not understand the regulator parts of the patch.

Yours,
Linus Walleij
Linus Walleij Nov. 3, 2019, 10:30 p.m. UTC | #3
Hi Matti!

Good initiative (and I will see a ton of janitorial patches as a
result of this...)

On Fri, Nov 1, 2019 at 12:50 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:

> At least for me it is difficult to remember the meaning of GPIO
> direction values. Define GPIO_IN and GPIO_OUT so that occasional
> GPIO contributors would not need to always check the meaning of
> hard coded values 1 and 0.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
(...)
> +#define GPIO_IN                1
> +#define GPIO_OUT       0

Please spell it out or people will be confused:

GPIO_LINE_DIRECTION_IN
GPIO_LINE_DIRECTION_OUT

Yours,
Linus Walleij
Vaittinen, Matti Nov. 4, 2019, 6:57 a.m. UTC | #4
Hello Linus,

On Sun, 2019-11-03 at 23:30 +0100, Linus Walleij wrote:
> Hi Matti!
> 
> Good initiative (and I will see a ton of janitorial patches as a
> result of this...)

Yep. I think I might pull this change out of the RFC and send it
separately. I can also do some conversions for existing drivers - but I
won't probably be able to do all of the drivers. I see no way of doing
any search and replace scripting here - this conversion is going to be
manual work :/

> 
> On Fri, Nov 1, 2019 at 12:50 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > At least for me it is difficult to remember the meaning of GPIO
> > direction values. Define GPIO_IN and GPIO_OUT so that occasional
> > GPIO contributors would not need to always check the meaning of
> > hard coded values 1 and 0.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> (...)
> > +#define GPIO_IN                1
> > +#define GPIO_OUT       0
> 
> Please spell it out or people will be confused:
> 
> GPIO_LINE_DIRECTION_IN
> GPIO_LINE_DIRECTION_OUT

Right. Besides the 0Day test suite did already spot a few redefinition
problems when some drivers do define GPIO_IN and/or GPIO_OUT... So I'll
change the defines to what you suggest here.

Br,
	Matti Vaittinen
Vaittinen, Matti Nov. 4, 2019, 7:05 a.m. UTC | #5
Hello Linus,

On Sun, 2019-11-03 at 23:27 +0100, Linus Walleij wrote:
> On Fri, Nov 1, 2019 at 12:43 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > Bucks 1,2,6 and 7 on ROHM BD71828 can be either controlled as
> > individual regulartors - or they can be grouped to a group of
> > regulators that are controlled by 'run levels'. This can be
> > done via I2C. Each regulator can be assigned a voltage and
> > enable/disable status for each run-level. These statuses are
> > also changeable via I2C.
> > 
> > Run-levels can then be changed either by I2C or GPIO. This
> > control mechanism is selected by data in one time programmable
> > area (during production) and can't be changed later.
> > 
> > Allow regulators to be controlled via run-levels and allow
> > getting/setting the current run-level also via GPIO.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> 
> I like the way you use the gpio API so FWIW:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks. And I like the GPIO set multiple - that's required in order to
do some of the run-level changes without intermediate states. (Eg. both
DVS GPIOs need to be toggled via single register write).

> I do not understand the regulator parts of the patch.

I'm sorry. The patch is not clearest one what comes to the regulator
stuff. I can try splitting it to smaller and more logical changes if
you, Mark or other interested people hope to get it splitted. Or
perhaps it would be simplest to review if it was all in one patch? 

Rationale for splitting it in first place was that I hoped the basic
support (first two regulator patches) to be acceptable without huge
changes - whereas the follow up patches are more like question that how
the heck should I implement this :] I've not hit similar 'change bunch
of regulator states at one go' drivers/hardware before.

Br,
	Matti Vaittinen
Vaittinen, Matti Nov. 4, 2019, 3:48 p.m. UTC | #6
Hello All,

On Sun, 2019-11-03 at 23:30 +0100, Linus Walleij wrote:
> Hi Matti!
> 
> Good initiative (and I will see a ton of janitorial patches as a
> result of this...)

I have somewhere near 62 patches waiting to be sent =) They're pretty
small but I'd appreciate thorough review as they're mostly untested...
Do you mind receiving them all in one go? Or do you think I should send
the series in smaller chuncks?

> On Fri, Nov 1, 2019 at 12:50 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > At least for me it is difficult to remember the meaning of GPIO
> > direction values. Define GPIO_IN and GPIO_OUT so that occasional
> > GPIO contributors would not need to always check the meaning of
> > hard coded values 1 and 0.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> (...)
> > +#define GPIO_IN                1
> > +#define GPIO_OUT       0
> 
> Please spell it out or people will be confused:
> 
> GPIO_LINE_DIRECTION_IN
> GPIO_LINE_DIRECTION_OUT
> 
> Yours,
> Linus Walleij
Linus Walleij Nov. 5, 2019, 1:24 p.m. UTC | #7
On Mon, Nov 4, 2019 at 8:05 AM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> On Sun, 2019-11-03 at 23:27 +0100, Linus Walleij wrote:

> > I do not understand the regulator parts of the patch.
>
> I'm sorry. The patch is not clearest one what comes to the regulator
> stuff. I can try splitting it to smaller and more logical changes if
> you, Mark or other interested people hope to get it splitted. Or
> perhaps it would be simplest to review if it was all in one patch?

As long as the regulator experts are happy with the format,
stay with that. I am just a drive-by coder when it comes to regulators.

Yours,
Linus Walleij
Vaittinen, Matti Nov. 5, 2019, 2:07 p.m. UTC | #8
On Tue, 2019-11-05 at 14:24 +0100, Linus Walleij wrote:
> On Mon, Nov 4, 2019 at 8:05 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Sun, 2019-11-03 at 23:27 +0100, Linus Walleij wrote:
> > > I do not understand the regulator parts of the patch.
> > 
> > I'm sorry. The patch is not clearest one what comes to the
> > regulator
> > stuff. I can try splitting it to smaller and more logical changes
> > if
> > you, Mark or other interested people hope to get it splitted. Or
> > perhaps it would be simplest to review if it was all in one patch?
> 
> As long as the regulator experts are happy with the format,
> stay with that. I am just a drive-by coder when it comes to
> regulators.

"Drive-by coder" - I do definitely like how that sounds :] Maybe I can
borrow that. But even the "drive-by" reviewing is hard. And I guess it
should be made as easy as it can...

Br,
	Matti
Linus Walleij Nov. 5, 2019, 3:05 p.m. UTC | #9
On Mon, Nov 4, 2019 at 4:48 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:

> > Good initiative (and I will see a ton of janitorial patches as a
> > result of this...)
>
> I have somewhere near 62 patches waiting to be sent =) They're pretty
> small but I'd appreciate thorough review as they're mostly untested...
> Do you mind receiving them all in one go? Or do you think I should send
> the series in smaller chuncks?

I would be fine with one patch introducing the defines and then
one big patch switching everybody and their dog over to using
these definitions.

I usually keep to a patch being "one technical step" and it is
clearly (IMO) one step to introduce the defines and one step
to make use of it in all legacy drivers.

It's late in the kernel cycle but this particular part (the defines
and switching over old driver to use it) I'd be happy
to merge for v5.5.

Yours,
Linus Walleij
Vaittinen, Matti Nov. 6, 2019, 6:51 a.m. UTC | #10
Hello Linus,

On Tue, 2019-11-05 at 16:05 +0100, Linus Walleij wrote:
> On Mon, Nov 4, 2019 at 4:48 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> 
> > > Good initiative (and I will see a ton of janitorial patches as a
> > > result of this...)
> > 
> > I have somewhere near 62 patches waiting to be sent =) They're
> > pretty
> > small but I'd appreciate thorough review as they're mostly
> > untested...
> > Do you mind receiving them all in one go? Or do you think I should
> > send
> > the series in smaller chuncks?
> 
> I would be fine with one patch introducing the defines and then
> one big patch switching everybody and their dog over to using
> these definitions.
> 
> I usually keep to a patch being "one technical step" and it is
> clearly (IMO) one step to introduce the defines and one step
> to make use of it in all legacy drivers.
> 
> It's late in the kernel cycle but this particular part (the defines
> and switching over old driver to use it) I'd be happy
> to merge for v5.5.

I'll prepare one patch for defines and one large patch for driver
changes on top of GPIO tree then. I'll leave out the last one 62/62 -
it can be applied later if it is considered a good idea - I'd
appreciate if you / Bartosz had the time to check it though. 

> 
> Yours,
> Linus Walleij
Lee Jones Nov. 11, 2019, 10:57 a.m. UTC | #11
On Fri, 01 Nov 2019, Matti Vaittinen wrote:

> BD71828GW is a single-chip power management IC for battery-powered portable
> devices. The IC integrates 7 buck converters, 7 LDOs, and a 1500 mA
> single-cell linear charger. Also included is a Coulomb counter, a real-time
> clock (RTC), 3 GPO/regulator control pins, HALL input and a 32.768 kHz
> clock gate.
> 
> Add MFD core driver providing interrupt controller facilities and i2c
> access to sub device drivers.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> 
> No changes compared to v2
> 
>  drivers/mfd/Kconfig              |  15 ++
>  drivers/mfd/Makefile             |   2 +-
>  drivers/mfd/rohm-bd71828.c       | 322 +++++++++++++++++++++++
>  include/linux/mfd/rohm-bd71828.h | 425 +++++++++++++++++++++++++++++++
>  include/linux/mfd/rohm-generic.h |   1 +
>  5 files changed, 764 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mfd/rohm-bd71828.c
>  create mode 100644 include/linux/mfd/rohm-bd71828.h

/me wonders why this is still an RFC after 3 revisions?

[...]

> +static struct mfd_cell bd71828_mfd_cells[] = {
> +	{ .name = "bd71828-pmic", },
> +	{ .name = "bd71828-gpio", },
> +	{ .name = "bd71828-led", },
> +	/*
> +	 * We use BD71837 driver to drive the clock block. Only differences to
> +	 * BD70528 clock gate are the register address and mask.
> +	 */
> +	{ .name = "bd718xx-clk", },
> +	{
> +		.name = "bd71827-power",

Why isn't this on one line, like the others above?

> +	}, {
> +		.name = "bd70528-rtc",
> +		.resources = rtc_irqs,
> +		.num_resources = ARRAY_SIZE(rtc_irqs),
> +	},
> +};

[...]

> +unsigned int bit0_offsets[] = {11};		/* RTC IRQ register */
> +unsigned int bit1_offsets[] = {10};		/* TEMP IRQ register */
> +unsigned int bit2_offsets[] = {6, 7, 8, 9};	/* BAT MON IRQ registers */
> +unsigned int bit3_offsets[] = {5};		/* BAT IRQ register */
> +unsigned int bit4_offsets[] = {4};		/* CHG IRQ register */
> +unsigned int bit5_offsets[] = {3};		/* VSYS IRQ register */
> +unsigned int bit6_offsets[] = {1, 2};		/* DCIN IRQ registers */

Something actually wrong with the tabbing here, or is this a
Git/patch/mailer anomaly?

[...]

> +static int bd71828_i2c_probe(struct i2c_client *i2c,
> +			     const struct i2c_device_id *id)
> +{
> +	struct rohm_regmap_dev *chip;
> +	struct regmap_irq_chip_data *irq_data;
> +	int ret;
> +
> +	if (!i2c->irq) {
> +		dev_err(&i2c->dev, "No IRQ configured\n");
> +		return -EINVAL;
> +	}
> +
> +	chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&i2c->dev, chip);
> +
> +	chip->chip_type = ROHM_CHIP_TYPE_BD71828;
> +	chip->regmap = devm_regmap_init_i2c(i2c, &bd71828_regmap);
> +	if (IS_ERR(chip->regmap)) {
> +		dev_err(&i2c->dev, "Failed to initialize Regmap\n");
> +		return PTR_ERR(chip->regmap);
> +	}
> +
> +	ret = devm_regmap_add_irq_chip(&i2c->dev, chip->regmap,
> +				       i2c->irq, IRQF_ONESHOT, 0,
> +				       &bd71828_irq_chip, &irq_data);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to add IRQ chip\n");
> +		return ret;
> +	}

Nit: '\n' here.

> +	dev_dbg(&i2c->dev, "Registered %d IRQs for chip\n",
> +		bd71828_irq_chip.num_irqs);
> +
> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> +				   bd71828_mfd_cells,
> +				   ARRAY_SIZE(bd71828_mfd_cells), NULL, 0,
> +				   regmap_irq_get_domain(irq_data));
> +	if (ret)
> +		dev_err(&i2c->dev, "Failed to create subdevices\n");
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id bd71828_of_match[] = {
> +	{ .compatible = "rohm,bd71828", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, bd71828_of_match);
> +
> +static struct i2c_driver bd71828_drv = {
> +	.driver = {
> +		.name = "rohm-bd71828",
> +		.of_match_table = bd71828_of_match,
> +	},
> +	.probe = &bd71828_i2c_probe,

If 'id' isn't used, perhaps you should be using probe2?

[...]
Lee Jones Nov. 11, 2019, 10:59 a.m. UTC | #12
On Fri, 01 Nov 2019, Matti Vaittinen wrote:

> Use gpio_keys to send power input-event to user-space when power
> button (short) press is detected.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> 
> Changes from v2 - No changes
> 
>  drivers/mfd/rohm-bd71828.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> index b7de79e1fcdb..f77ba1ec3e99 100644
> --- a/drivers/mfd/rohm-bd71828.c
> +++ b/drivers/mfd/rohm-bd71828.c
> @@ -4,7 +4,9 @@
>  //
>  // ROHM BD71828 PMIC driver
>  
> +#include <linux/gpio_keys.h>
>  #include <linux/i2c.h>
> +#include <linux/input.h>
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
>  #include <linux/irq.h>
> @@ -15,6 +17,18 @@
>  #include <linux/regmap.h>
>  #include <linux/types.h>
>  
> +static struct gpio_keys_button button = {
> +	.code = KEY_POWER,
> +	.gpio = -1,
> +	.type = EV_KEY,
> +};
> +
> +static struct gpio_keys_platform_data bd71828_powerkey_data = {
> +	.buttons = &button,
> +	.nbuttons = 1,
> +	.name = "bd71828-pwrkey",
> +};
> +
>  static const struct resource rtc_irqs[] = {
>  	DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC0, "bd71828-rtc-alm-0"),
>  	DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC1, "bd71828-rtc-alm-1"),
> @@ -36,6 +50,10 @@ static struct mfd_cell bd71828_mfd_cells[] = {
>  		.name = "bd70528-rtc",
>  		.resources = rtc_irqs,
>  		.num_resources = ARRAY_SIZE(rtc_irqs),
> +	}, {
> +		.name = "gpio-keys",
> +		.platform_data = &bd71828_powerkey_data,
> +		.pdata_size = sizeof(bd71828_powerkey_data),
>  	},
>  };
>  
> @@ -288,9 +306,19 @@ static int bd71828_i2c_probe(struct i2c_client *i2c,
>  		dev_err(&i2c->dev, "Failed to add IRQ chip\n");
>  		return ret;
>  	}
> +

This should be fixed in the last patch.

>  	dev_dbg(&i2c->dev, "Registered %d IRQs for chip\n",
>  		bd71828_irq_chip.num_irqs);
>  
> +	ret = regmap_irq_get_virq(irq_data, BD71828_INT_SHORTPUSH);
> +

Remove this empty line.

> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Failed to get the power-key IRQ\n");
> +		return ret;
> +	}
> +
> +	button.irq = ret;
> +

Once fixed, please apply my:

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
Vaittinen, Matti Nov. 11, 2019, 11:07 a.m. UTC | #13
Hello Lee,

On Mon, 2019-11-11 at 10:59 +0000, Lee Jones wrote:
> On Fri, 01 Nov 2019, Matti Vaittinen wrote:
> 
> > Use gpio_keys to send power input-event to user-space when power
> > button (short) press is detected.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > 
> > Changes from v2 - No changes
> > 
> >  drivers/mfd/rohm-bd71828.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-
> > bd71828.c
> > index b7de79e1fcdb..f77ba1ec3e99 100644
> > --- a/drivers/mfd/rohm-bd71828.c
> > +++ b/drivers/mfd/rohm-bd71828.c
> > @@ -4,7 +4,9 @@
> >  //
> >  // ROHM BD71828 PMIC driver
> >  
> > +#include <linux/gpio_keys.h>
> >  #include <linux/i2c.h>
> > +#include <linux/input.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/ioport.h>
> >  #include <linux/irq.h>
> > @@ -15,6 +17,18 @@
> >  #include <linux/regmap.h>
> >  #include <linux/types.h>
> >  
> > +static struct gpio_keys_button button = {
> > +	.code = KEY_POWER,
> > +	.gpio = -1,
> > +	.type = EV_KEY,
> > +};
> > +
> > +static struct gpio_keys_platform_data bd71828_powerkey_data = {
> > +	.buttons = &button,
> > +	.nbuttons = 1,
> > +	.name = "bd71828-pwrkey",
> > +};
> > +
> >  static const struct resource rtc_irqs[] = {
> >  	DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC0, "bd71828-rtc-alm-0"),
> >  	DEFINE_RES_IRQ_NAMED(BD71828_INT_RTC1, "bd71828-rtc-alm-1"),
> > @@ -36,6 +50,10 @@ static struct mfd_cell bd71828_mfd_cells[] = {
> >  		.name = "bd70528-rtc",
> >  		.resources = rtc_irqs,
> >  		.num_resources = ARRAY_SIZE(rtc_irqs),
> > +	}, {
> > +		.name = "gpio-keys",
> > +		.platform_data = &bd71828_powerkey_data,
> > +		.pdata_size = sizeof(bd71828_powerkey_data),
> >  	},
> >  };
> >  
> > @@ -288,9 +306,19 @@ static int bd71828_i2c_probe(struct i2c_client
> > *i2c,
> >  		dev_err(&i2c->dev, "Failed to add IRQ chip\n");
> >  		return ret;
> >  	}
> > +
> 
> This should be fixed in the last patch.
> 
> >  	dev_dbg(&i2c->dev, "Registered %d IRQs for chip\n",
> >  		bd71828_irq_chip.num_irqs);
> >  
> > +	ret = regmap_irq_get_virq(irq_data, BD71828_INT_SHORTPUSH);
> > +
> 
> Remove this empty line.
> 
> > +	if (ret < 0) {
> > +		dev_err(&i2c->dev, "Failed to get the power-key
> > IRQ\n");
> > +		return ret;
> > +	}
> > +
> > +	button.irq = ret;
> > +
> 
> Once fixed, please apply my:
> 
> For my own reference:
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> 

Thanks for checking this :) I'll apply fixes in patch v4.
Vaittinen, Matti Nov. 11, 2019, 11:20 a.m. UTC | #14
Hello Lee,

Thanks for the review!

I was slightly worried I really managed to piss you off last time :)
Glad to see I didn't burn all the bridges (yet) ;)

On Mon, 2019-11-11 at 10:57 +0000, Lee Jones wrote:
> On Fri, 01 Nov 2019, Matti Vaittinen wrote:
> 
> > BD71828GW is a single-chip power management IC for battery-powered
> > portable
> > devices. The IC integrates 7 buck converters, 7 LDOs, and a 1500 mA
> > single-cell linear charger. Also included is a Coulomb counter, a
> > real-time
> > clock (RTC), 3 GPO/regulator control pins, HALL input and a 32.768
> > kHz
> > clock gate.
> > 
> > Add MFD core driver providing interrupt controller facilities and
> > i2c
> > access to sub device drivers.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > 
> > No changes compared to v2
> > 
> >  drivers/mfd/Kconfig              |  15 ++
> >  drivers/mfd/Makefile             |   2 +-
> >  drivers/mfd/rohm-bd71828.c       | 322 +++++++++++++++++++++++
> >  include/linux/mfd/rohm-bd71828.h | 425
> > +++++++++++++++++++++++++++++++
> >  include/linux/mfd/rohm-generic.h |   1 +
> >  5 files changed, 764 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/mfd/rohm-bd71828.c
> >  create mode 100644 include/linux/mfd/rohm-bd71828.h
> 
> /me wonders why this is still an RFC after 3 revisions?

Because of the regulator part. I've had no comments for it - but I
don't think it should be applied as is in this series. I was kind of
hoping someone more experienced could have pointed me that what I have
tried to achieve here is already handled as <something I am missing
now>.

I don't think we should add sysfs control IF for disabling regulators
(I will drop that completely from first non RFC patch - but I hoped I
might get some friendly pokes/pushes to right direction). Nor am I
happy on how the run-state transitions which impact many regulators are
now handled via single regulator reference - but I can't think of
better approach just now. I hoped I am just missing something which is
obvious to more experienced regulator guys.

If I won't get comments to regulators I'll just drop the sysfs
interfaces (and possibly whole run-level control) and send series
without the RFC then. But I am still cautiously hopeful that Mark has
just a extraordinarily busy moment and will give me some feedback
before I finish v4 :)

> > +unsigned int bit0_offsets[] = {11};		/* RTC IRQ
> > register */
> > +unsigned int bit1_offsets[] = {10};		/* TEMP IRQ
> > register */
> > +unsigned int bit2_offsets[] = {6, 7, 8, 9};	/* BAT MON IRQ
> > registers */
> > +unsigned int bit3_offsets[] = {5};		/* BAT IRQ register */
> > +unsigned int bit4_offsets[] = {4};		/* CHG IRQ register */
> > +unsigned int bit5_offsets[] = {3};		/* VSYS IRQ register */
> > +unsigned int bit6_offsets[] = {1, 2};		/* DCIN IRQ
> > registers */
> 
> Something actually wrong with the tabbing here, or is this a
> Git/patch/mailer anomaly?

I'll check this - I need to staticize these anyways.

> > 
> > +static const struct of_device_id bd71828_of_match[] = {
> > +	{ .compatible = "rohm,bd71828", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, bd71828_of_match);
> > +
> > +static struct i2c_driver bd71828_drv = {
> > +	.driver = {
> > +		.name = "rohm-bd71828",
> > +		.of_match_table = bd71828_of_match,
> > +	},
> > +	.probe = &bd71828_i2c_probe,
> 
> If 'id' isn't used, perhaps you should be using probe2?

probe2? Sounds like I need to do my homework once again :) Thanks for
the pointer.

Rest of the comments were pretty obvious - thanks. I'll fix these for
v4.

Br,
	Matti