From: Bartosz Golaszewski <brgl@bgdev.pl>
To: Lee Jones <lee.jones@linaro.org>
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 v8 06/11] mfd: max77650: new core mfd driver
Date: Thu, 4 Apr 2019 09:30:52 +0200 [thread overview]
Message-ID: <CAMRc=Mcfgw+tAM+B_phMX6x4qUHiTd6Yn2OnT4_dQ7AzypD-cg@mail.gmail.com> (raw)
In-Reply-To: <20190404071538.GH6830@dell>
czw., 4 kwi 2019 o 09:15 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Wed, 03 Apr 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 | 14 +++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/max77650.c | 234 +++++++++++++++++++++++++++++++++++
> > include/linux/mfd/max77650.h | 59 +++++++++
> > 4 files changed, 308 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 0ce2d8dfc5f1..ade04e124aa0 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -733,6 +733,20 @@ 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 Y here to add support for Maxim Semiconductor MAX77650 and
> > + MAX77651 Power Management ICs. This is the core multifunction
> > + driver for interacting with the device. The module name is
> > + 'max77650'. Additional drivers can be enabled in order to use
> > + the following functionalities of the device: GPIO, regulator,
> > + charger, LED, onkey.
> > +
> > config MFD_MAX77686
> > tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index b4569ed7f3f3..5727d099c16f 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -155,6 +155,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..7a6c0a5cf602
> > --- /dev/null
> > +++ b/drivers/mfd/max77650.c
> > @@ -0,0 +1,234 @@
> > +// 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.
> > +// Programming manual: https://pdfserv.maximintegrated.com/en/an/AN6428.pdf
> > +
> > +#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>
> > +
> > +#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,
> > + 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,
> > +};
> > +
> > +static const struct resource max77650_charger_resources[] = {
> > + DEFINE_RES_IRQ_NAMED(MAX77650_INT_CHG, "CHG"),
> > + DEFINE_RES_IRQ_NAMED(MAX77650_INT_CHGIN, "CHGIN"),
> > +};
> > +
> > +static const struct resource max77650_gpio_resources[] = {
> > + DEFINE_RES_IRQ_NAMED(MAX77650_INT_GPI, "GPI"),
> > +};
> > +
> > +static const struct resource max77650_onkey_resources[] = {
> > + DEFINE_RES_IRQ_NAMED(MAX77650_INT_nEN_F, "nEN_F"),
> > + DEFINE_RES_IRQ_NAMED(MAX77650_INT_nEN_R, "nEN_R"),
> > +};
> > +
> > +static const struct mfd_cell max77650_cells[] = {
> > + {
> > + .name = "max77650-regulator",
> > + .of_compatible = "maxim,max77650-regulator",
>
> > + },
> > + {
>
> Nit: Place these all on the same line, like:
>
> }, {
>
> > + .name = "max77650-charger",
> > + .of_compatible = "maxim,max77650-charger",
> > + .resources = max77650_charger_resources,
> > + .num_resources = ARRAY_SIZE(max77650_charger_resources),
> > + },
> > + {
> > + .name = "max77650-gpio",
> > + .of_compatible = "maxim,max77650-gpio",
> > + .resources = max77650_gpio_resources,
> > + .num_resources = ARRAY_SIZE(max77650_gpio_resources),
> > + },
> > + {
> > + .name = "max77650-led",
> > + .of_compatible = "maxim,max77650-led",
> > + },
> > + {
> > + .name = "max77650-onkey",
> > + .of_compatible = "maxim,max77650-onkey",
> > + .resources = max77650_onkey_resources,
> > + .num_resources = ARRAY_SIZE(max77650_onkey_resources),
> > + },
> > +};
> > +
> > +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,
>
> Not really a fan of these silly double/triple tabs everywhere. I like
> a straight line as much as anyone, but I'd probably remove them from
> here.
>
> > + },
> > + },
> > + REGMAP_IRQ_REG(MAX77650_INT_nEN_F,
> > + MAX77650_INT_GLBL_OFFSET, MAX77650_INT_nEN_F_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_nEN_R,
> > + MAX77650_INT_GLBL_OFFSET, MAX77650_INT_nEN_R_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_TJAL1_R,
> > + MAX77650_INT_GLBL_OFFSET, MAX77650_INT_TJAL1_R_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_TJAL2_R,
> > + MAX77650_INT_GLBL_OFFSET, MAX77650_INT_TJAL2_R_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_DOD_R,
> > + MAX77650_INT_GLBL_OFFSET, MAX77650_INT_DOD_R_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_THM,
> > + MAX77650_INT_CHG_OFFSET, MAX77650_INT_THM_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_CHG,
> > + MAX77650_INT_CHG_OFFSET, MAX77650_INT_CHG_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_CHGIN,
> > + MAX77650_INT_CHG_OFFSET, MAX77650_INT_CHGIN_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_TJ_REG,
> > + MAX77650_INT_CHG_OFFSET, MAX77650_INT_TJ_REG_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_CHGIN_CTRL,
> > + MAX77650_INT_CHG_OFFSET, MAX77650_INT_CHGIN_CTRL_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_SYS_CTRL,
> > + MAX77650_INT_CHG_OFFSET, MAX77650_INT_SYS_CTRL_MSK),
> > + REGMAP_IRQ_REG(MAX77650_INT_SYS_CNFG,
> > + MAX77650_INT_CHG_OFFSET, 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_i2c_probe(struct i2c_client *i2c)
> > +{
> > + struct regmap_irq_chip_data *irq_data;
> > + struct device *dev = &i2c->dev;
> > + struct irq_domain *domain;
> > + struct regmap *map;
> > + unsigned int val;
> > + int rv;
> > +
> > + map = devm_regmap_init_i2c(i2c, &max77650_regmap_config);
> > + if (IS_ERR(map)) {
> > + dev_err(dev, "unable to initialize i2c regmap\n");
>
> "Unable to initialise I2C Regmap"
>
> > + return PTR_ERR(map);
> > + }
> > +
> > + rv = regmap_read(map, MAX77650_REG_CID, &val);
> > + if (rv) {
> > + dev_err(dev, "unable to read the CID from device\n");
>
> "Unable to read Chip ID"
>
> > + 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:
>
> So you're exiting silently here. What is the user going to think?
>
> "Chip not supported - ID: %d"
>
> > + 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) {
> > + dev_err(dev, "unable to change the power mode\n");
>
> "Unable ..."
>
> > + return rv;
> > + }
> > +
> > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > + IRQF_ONESHOT | IRQF_SHARED, 0,
> > + &max77650_irq_chip, &irq_data);
> > + if (rv) {
> > + dev_err(dev, "unable to add the regmap irq chip\n");
>
> "Unable to add Regmap IRQ chip"
>
> > + return rv;
> > + }
> > +
> > + domain = regmap_irq_get_domain(irq_data);
>
> No error checking?
>
This can only fail is irq_data is NULL and since the previous function
succeeded we know it isn't.
Bart
> > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> > + max77650_cells, ARRAY_SIZE(max77650_cells),
> > + NULL, 0, domain);
> > +}
> > +
> > +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,
>
> I wrote this years ago. Hasn't it gone away yet?
>
> > +};
> > +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
next prev parent reply other threads:[~2019-04-04 7:30 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-03 9:00 [PATCH v8 00/11] mfd: add support for max77650 PMIC Bartosz Golaszewski
2019-04-03 9:00 ` [PATCH v8 01/11] dt-bindings: mfd: add DT bindings for max77650 Bartosz Golaszewski
2019-04-03 9:00 ` [PATCH v8 02/11] dt-bindings: power: supply: " Bartosz Golaszewski
2019-04-06 7:07 ` Rob Herring
2019-04-08 12:25 ` Bartosz Golaszewski
2019-04-09 1:10 ` Rob Herring
2019-04-09 11:04 ` Bartosz Golaszewski
2019-04-09 15:38 ` Bartosz Golaszewski
2019-04-11 16:50 ` Rob Herring
2019-04-03 9:01 ` [PATCH v8 03/11] dt-bindings: leds: " Bartosz Golaszewski
2019-04-04 13:22 ` Pavel Machek
2019-04-03 9:01 ` [PATCH v8 04/11] dt-bindings: input: " Bartosz Golaszewski
2019-04-03 9:01 ` [PATCH v8 05/11] mfd: core: document mfd_add_devices() Bartosz Golaszewski
2019-04-04 7:04 ` Lee Jones
2019-04-03 9:01 ` [PATCH v8 06/11] mfd: max77650: new core mfd driver Bartosz Golaszewski
2019-04-04 7:15 ` Lee Jones
2019-04-04 7:30 ` Bartosz Golaszewski [this message]
2019-04-03 9:01 ` [PATCH v8 07/11] power: supply: max77650: add support for battery charger Bartosz Golaszewski
2019-04-03 9:01 ` [PATCH v8 08/11] gpio: max77650: add GPIO support Bartosz Golaszewski
2019-04-03 9:01 ` [PATCH v8 09/11] leds: max77650: add LEDs support Bartosz Golaszewski
2019-04-04 13:22 ` Pavel Machek
2019-04-03 9:01 ` [PATCH v8 10/11] input: max77650: add onkey support Bartosz Golaszewski
2019-04-03 9:01 ` [PATCH v8 11/11] 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='CAMRc=Mcfgw+tAM+B_phMX6x4qUHiTd6Yn2OnT4_dQ7AzypD-cg@mail.gmail.com' \
--to=brgl@bgdev.pl \
--cc=bgolaszewski@baylibre.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jacek.anaszewski@gmail.com \
--cc=lee.jones@linaro.org \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.