Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
Hello Mark,
On 06/09/2014 09:47 PM, Mark Brown wrote:
> On Mon, Jun 09, 2014 at 11:37:46AM +0200, Javier Martinez Canillas wrote:
>
>> +Optional node:
>> +- voltage-regulators : The regulators of max77802 have to be instantiated
>> + under subnode named "voltage-regulators" using the following format.
>
> Every other PMIC calls this node regulators...
>
Ok, I'll change for consistency.
>> +regulator_name {
>> +regulator-compatible = LDOn/BUCKn
>
> regulator-compatible is deprecated, use the node name instead.
>
Ok.
>> +config MFD_MAX77802
>> +bool "Maxim Integrated MAX77802 PMIC Support"
>
> Why is this bool and not tristate?
>
I noticed that the majority of the mfd PMIC drivers were bool and not tristate
so I thought it was a convention. But nothing prevents this driver to be built
as a module so I'll change it to tristate.
>> +int max77802_irq_resume(struct max77802_dev *max77802)
>> +{
>> +/*
>> + * The IRQ that woke us up may still need to be ACK'ed on resume.
>> + * If it isn't ever ACK'ed, future IRQs may not be delivered.
>> + */
>> +if (max77802->irq)
>> +max77802_irq_thread(0, max77802);
>> +
>> +return 0;
>> +}
>
> As covered in another subthread all this code looks like it should be
> regmap-irq.
>
It seems so, I'll take that into account for v2.
>> +if (regmap_read(max77802->regmap,
>> + MAX77802_REG_DEVICE_ID, ) < 0) {
>> +dev_err(max77802->dev,
>> +"device not found on this channel (this is not an
>> error)\n");
>> +return -ENODEV;
>
> If this is not an error why is it printed as dev_err()? It does look
> like an error to me, though.
>
Yeah, it is an error so I'll clean that message.
>> +} else {
>> +dev_info(max77802->dev, "device found\n");
>> +}
>
> These sort of prints are just noise, remove this unless there is some
> revision information you can display. It's also better practice to
> check that the device ID is actually what was expected in case there was
> an error in the DT.
>
Ok, will do.
>> +static const struct i2c_device_id max77802_i2c_id[] = {
>> +{ "max77802", TYPE_MAX77802 },
>> +{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, max77802_i2c_id);
>
> We have type information here but not in the OF ID table (not that we
> ever look at it).
>
Yeah, I'll remove the type information here. It is a left over when trying to
combine both max77802 and max77686 drivers since in a combined driver we need
the type information.
Thanks a lot for your feedback.
Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
Hello Krzysztof,
On 06/09/2014 12:22 PM, Krzysztof Kozlowski wrote:
> On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
>> Maxim MAX77802 is a power management chip that contains 10 high
>> efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
>> to power up application processors and peripherals, a 2-channel
>> 32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
>> to program the individual regulators, clocks outputs and the RTC.
>>
>> This patch adds the core support for MAX77802 PMIC and is based
>> on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree.
>>
>> Signed-off-by: Javier Martinez Canillas
>
> (...)
>
>
>> diff --git a/drivers/mfd/max77802-irq.c b/drivers/mfd/max77802-irq.c
>> new file mode 100644
>> index 000..38a8ce7
>> --- /dev/null
>> +++ b/drivers/mfd/max77802-irq.c
>> @@ -0,0 +1,332 @@
>> +/*
>> + * max77802-irq.c - Interrupt controller support for MAX77802
>> + *
>> + * Copyright (C) 2013-2014 Google, Inc
>> + *
>> + * Copyright (C) 2012 Samsung Electronics Co.Ltd
>> + * Chiwoong Byun
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * This driver is based on max8997-irq.c
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +enum {
>> +MAX77802_DEBUG_IRQ_INFO = 1 << 0,
>> +MAX77802_DEBUG_IRQ_MASK = 1 << 1,
>> +MAX77802_DEBUG_IRQ_INT = 1 << 2,
>> +};
>> +
>> +static int debug_mask = 0;
>> +module_param(debug_mask, int, 0);
>> +MODULE_PARM_DESC(debug_mask, "Set debug_mask : 0x0=off 0x1=IRQ_INFO
>> 0x2=IRQ_MASK 0x4=IRQ_INI)");
>> +
>> +static const u8 max77802_mask_reg[] = {
>> +[PMIC_INT1] = MAX77802_REG_INT1MSK,
>> +[PMIC_INT2] = MAX77802_REG_INT2MSK,
>> +[RTC_INT] = MAX77802_RTC_INTM,
>> +};
>> +
>> +struct max77802_irq_data {
>> +int mask;
>> +enum max77802_irq_source group;
>> +};
>> +
>> +#define DECLARE_IRQ(idx, _group, _mask) \
>> +[(idx)] = { .group = (_group), .mask = (_mask) }
>> +static const struct max77802_irq_data max77802_irqs[] = {
>> +DECLARE_IRQ(MAX77802_PMICIRQ_PWRONF,PMIC_INT1, 1 << 0),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_PWRONR,PMIC_INT1, 1 << 1),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBF, PMIC_INT1, 1 << 2),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBR, PMIC_INT1, 1 << 3),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBF,PMIC_INT1, 1 << 4),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBR,PMIC_INT1, 1 << 5),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_ONKEY1S, PMIC_INT1, 1 << 6),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_MRSTB, PMIC_INT1, 1 << 7),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_140C, PMIC_INT2, 1 << 0),
>> +DECLARE_IRQ(MAX77802_PMICIRQ_120C, PMIC_INT2, 1 << 1),
>> +DECLARE_IRQ(MAX77802_RTCIRQ_RTC60S, RTC_INT, 1 << 0),
>> +DECLARE_IRQ(MAX77802_RTCIRQ_RTCA1, RTC_INT, 1 << 1),
>> +DECLARE_IRQ(MAX77802_RTCIRQ_RTCA2, RTC_INT, 1 << 2),
>> +DECLARE_IRQ(MAX77802_RTCIRQ_SMPL, RTC_INT, 1 << 3),
>> +DECLARE_IRQ(MAX77802_RTCIRQ_RTC1S, RTC_INT, 1 << 4),
>> +DECLARE_IRQ(MAX77802_RTCIRQ_WTSR, RTC_INT, 1 << 5),
>> +};
>
> Why just not use two regmap_irq_chips (for PMIC and RTC blocks)
> replacing whole max77802-irq.c file?
>
Great, I was not aware about regmap_irq_chip. I'll take a look to it for v2.
> Best regards,
> Krzysztof
>
>
Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
On Mon, Jun 09, 2014 at 11:37:46AM +0200, Javier Martinez Canillas wrote:
> +Optional node:
> +- voltage-regulators : The regulators of max77802 have to be instantiated
> + under subnode named "voltage-regulators" using the following format.
Every other PMIC calls this node regulators...
> + regulator_name {
> + regulator-compatible = LDOn/BUCKn
regulator-compatible is deprecated, use the node name instead.
> +config MFD_MAX77802
> + bool "Maxim Integrated MAX77802 PMIC Support"
Why is this bool and not tristate?
> +int max77802_irq_resume(struct max77802_dev *max77802)
> +{
> + /*
> + * The IRQ that woke us up may still need to be ACK'ed on resume.
> + * If it isn't ever ACK'ed, future IRQs may not be delivered.
> + */
> + if (max77802->irq)
> + max77802_irq_thread(0, max77802);
> +
> + return 0;
> +}
As covered in another subthread all this code looks like it should be
regmap-irq.
> + if (regmap_read(max77802->regmap,
> + MAX77802_REG_DEVICE_ID, ) < 0) {
> + dev_err(max77802->dev,
> + "device not found on this channel (this is not an
> error)\n");
> + return -ENODEV;
If this is not an error why is it printed as dev_err()? It does look
like an error to me, though.
> + } else {
> + dev_info(max77802->dev, "device found\n");
> + }
These sort of prints are just noise, remove this unless there is some
revision information you can display. It's also better practice to
check that the device ID is actually what was expected in case there was
an error in the DT.
> +static const struct i2c_device_id max77802_i2c_id[] = {
> + { "max77802", TYPE_MAX77802 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max77802_i2c_id);
We have type information here but not in the OF ID table (not that we
ever look at it).
signature.asc
Description: Digital signature
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
On Mon, Jun 09, 2014 at 12:22:41PM +0200, Krzysztof Kozlowski wrote:
> On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
> > +static const struct max77802_irq_data max77802_irqs[] = {
> > + DECLARE_IRQ(MAX77802_PMICIRQ_PWRONF,PMIC_INT1, 1 << 0),
> > + DECLARE_IRQ(MAX77802_PMICIRQ_PWRONR,PMIC_INT1, 1 << 1),
> Why just not use two regmap_irq_chips (for PMIC and RTC blocks)
> replacing whole max77802-irq.c file?
If they share the same primary IRQ line something like what the arizona
drivers do should handle things well enough.
signature.asc
Description: Digital signature
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
> Maxim MAX77802 is a power management chip that contains 10 high
> efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
> to power up application processors and peripherals, a 2-channel
> 32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
> to program the individual regulators, clocks outputs and the RTC.
>
> This patch adds the core support for MAX77802 PMIC and is based
> on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree.
>
> Signed-off-by: Javier Martinez Canillas
(...)
> diff --git a/drivers/mfd/max77802-irq.c b/drivers/mfd/max77802-irq.c
> new file mode 100644
> index 000..38a8ce7
> --- /dev/null
> +++ b/drivers/mfd/max77802-irq.c
> @@ -0,0 +1,332 @@
> +/*
> + * max77802-irq.c - Interrupt controller support for MAX77802
> + *
> + * Copyright (C) 2013-2014 Google, Inc
> + *
> + * Copyright (C) 2012 Samsung Electronics Co.Ltd
> + * Chiwoong Byun
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * This driver is based on max8997-irq.c
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +enum {
> + MAX77802_DEBUG_IRQ_INFO = 1 << 0,
> + MAX77802_DEBUG_IRQ_MASK = 1 << 1,
> + MAX77802_DEBUG_IRQ_INT = 1 << 2,
> +};
> +
> +static int debug_mask = 0;
> +module_param(debug_mask, int, 0);
> +MODULE_PARM_DESC(debug_mask, "Set debug_mask : 0x0=off 0x1=IRQ_INFO
> 0x2=IRQ_MASK 0x4=IRQ_INI)");
> +
> +static const u8 max77802_mask_reg[] = {
> + [PMIC_INT1] = MAX77802_REG_INT1MSK,
> + [PMIC_INT2] = MAX77802_REG_INT2MSK,
> + [RTC_INT] = MAX77802_RTC_INTM,
> +};
> +
> +struct max77802_irq_data {
> + int mask;
> + enum max77802_irq_source group;
> +};
> +
> +#define DECLARE_IRQ(idx, _group, _mask) \
> + [(idx)] = { .group = (_group), .mask = (_mask) }
> +static const struct max77802_irq_data max77802_irqs[] = {
> + DECLARE_IRQ(MAX77802_PMICIRQ_PWRONF,PMIC_INT1, 1 << 0),
> + DECLARE_IRQ(MAX77802_PMICIRQ_PWRONR,PMIC_INT1, 1 << 1),
> + DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBF, PMIC_INT1, 1 << 2),
> + DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBR, PMIC_INT1, 1 << 3),
> + DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBF,PMIC_INT1, 1 << 4),
> + DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBR,PMIC_INT1, 1 << 5),
> + DECLARE_IRQ(MAX77802_PMICIRQ_ONKEY1S, PMIC_INT1, 1 << 6),
> + DECLARE_IRQ(MAX77802_PMICIRQ_MRSTB, PMIC_INT1, 1 << 7),
> + DECLARE_IRQ(MAX77802_PMICIRQ_140C, PMIC_INT2, 1 << 0),
> + DECLARE_IRQ(MAX77802_PMICIRQ_120C, PMIC_INT2, 1 << 1),
> + DECLARE_IRQ(MAX77802_RTCIRQ_RTC60S, RTC_INT, 1 << 0),
> + DECLARE_IRQ(MAX77802_RTCIRQ_RTCA1, RTC_INT, 1 << 1),
> + DECLARE_IRQ(MAX77802_RTCIRQ_RTCA2, RTC_INT, 1 << 2),
> + DECLARE_IRQ(MAX77802_RTCIRQ_SMPL, RTC_INT, 1 << 3),
> + DECLARE_IRQ(MAX77802_RTCIRQ_RTC1S, RTC_INT, 1 << 4),
> + DECLARE_IRQ(MAX77802_RTCIRQ_WTSR, RTC_INT, 1 << 5),
> +};
Why just not use two regmap_irq_chips (for PMIC and RTC blocks)
replacing whole max77802-irq.c file?
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
Maxim MAX77802 is a power management chip that contains 10 high
efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
to power up application processors and peripherals, a 2-channel
32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
to program the individual regulators, clocks outputs and the RTC.
This patch adds the core support for MAX77802 PMIC and is based
on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree.
Signed-off-by: Javier Martinez Canillas
---
Documentation/devicetree/bindings/mfd/max77802.txt | 88 ++
drivers/mfd/Kconfig| 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77802-irq.c | 332 +
drivers/mfd/max77802.c | 282 +
include/linux/mfd/max77802-private.h | 291 ++
include/linux/mfd/max77802.h | 124
7 files changed, 1131 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/max77802.txt
create mode 100644 drivers/mfd/max77802-irq.c
create mode 100644 drivers/mfd/max77802.c
create mode 100644 include/linux/mfd/max77802-private.h
create mode 100644 include/linux/mfd/max77802.h
diff --git a/Documentation/devicetree/bindings/mfd/max77802.txt
b/Documentation/devicetree/bindings/mfd/max77802.txt
new file mode 100644
index 000..addf02e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max77802.txt
@@ -0,0 +1,88 @@
+Maxim MAX77802 multi-function device
+
+MAX77802 is a Mulitifunction device with PMIC, RTC and Charger on chip. It is
+interfaced to host controller using i2c interface. PMIC, Charger and RTC
+submodules are addressed using same i2c slave address
+
+This document describes the binding for mfd device and PMIC submodule.
+
+Binding for the built-in 32k clock generator block is defined separately
+in bindings/clk/maxim,max77802.txt file.
+
+Required properties:
+- compatible : Must be "maxim,max77802";
+- reg : Specifies the i2c slave address of PMIC block.
+- interrupts : This i2c device has an IRQ line connected to the main SoC.
+- interrupt-parent : The parent interrupt controller.
+
+Optional properties:
+- max77802,pmic-buck-default-dvs-idx: We'll always write this DVS index in the
+ PMIC for BUCKs with DVS (Bucks 1-4, 6).
+ NOTE: at the moment these bindings don't include enough details for actual
+ GPIO-DVS--this just lets you choose which single slot to use.
+
+- max77802,pmic-buck-dvs-gpios: The DVS GPIOs. We'll try to set these GPIOs
+ to match pmic-buck-default-dvs-idx at probe time if they are defined. If
+ some or all of these GPIOs are not defined it's assumed that the board has
+ any missing GPIOs hardwired to match pmic-buck-default-dvs-idx.
+
+- max77802,pmic-buck-selb-gpios: GPIOs to enable DVS-GPIO for BUCKs.
+ Should be five values: 1, 2, 3, 4, 6. It is strongly suggested to include
+ these GPIOs if there's any chance that changing DVS GPIOs one line at a
+ time might glitch your DVS values.
+
+Optional node:
+- voltage-regulators : The regulators of max77802 have to be instantiated
+ under subnode named "voltage-regulators" using the following format.
+
+ regulator_name {
+ regulator-compatible = LDOn/BUCKn
+ standard regulator constraints
+ };
+ refer Documentation/devicetree/bindings/regulator/regulator.txt
+
+ The regulator-compatible property of regulator should initialized with string
+to get matched with their hardware counterparts as follow:
+
+ -LDOn : for LDOs, where n can lie in range 1 to 35.
+ example: LDO1, LDO2, LDO35.
+ -BUCKn : for BUCKs, where n can lie in range 1 to 10.
+ example: BUCK1, BUCK5, BUCK10.
+Example:
+
+ max77802@09 {
+ compatible = "maxim,max77802";
+ interrupt-parent = <_eint>;
+ interrupts = <26 0>;
+ reg = <0x09>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ max77802,pmic-buck-default-dvs-idx = <1>;
+ max77802,pmic-buck-dvs-gpios = < 6 0>,
+ < 2 0>,
+ < 3 0>;
+ max77802,pmic-buck-selb-gpios = < 2 0>,
+ < 3 0>,
+ < 4 0>,
+ < 5 0>,
+ < 6 0>;
+
+ voltage-regulators {
+ ldo11_reg {
+ regulator-compatible = "LDO11";
+ regulator-name = "vdd_ldo11";
+ regulator-min-microvolt = <190>;
+ regulator-max-microvolt = <190>;
+
[PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
Maxim MAX77802 is a power management chip that contains 10 high
efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
to power up application processors and peripherals, a 2-channel
32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
to program the individual regulators, clocks outputs and the RTC.
This patch adds the core support for MAX77802 PMIC and is based
on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree.
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
---
Documentation/devicetree/bindings/mfd/max77802.txt | 88 ++
drivers/mfd/Kconfig| 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77802-irq.c | 332 +
drivers/mfd/max77802.c | 282 +
include/linux/mfd/max77802-private.h | 291 ++
include/linux/mfd/max77802.h | 124
7 files changed, 1131 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/max77802.txt
create mode 100644 drivers/mfd/max77802-irq.c
create mode 100644 drivers/mfd/max77802.c
create mode 100644 include/linux/mfd/max77802-private.h
create mode 100644 include/linux/mfd/max77802.h
diff --git a/Documentation/devicetree/bindings/mfd/max77802.txt
b/Documentation/devicetree/bindings/mfd/max77802.txt
new file mode 100644
index 000..addf02e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max77802.txt
@@ -0,0 +1,88 @@
+Maxim MAX77802 multi-function device
+
+MAX77802 is a Mulitifunction device with PMIC, RTC and Charger on chip. It is
+interfaced to host controller using i2c interface. PMIC, Charger and RTC
+submodules are addressed using same i2c slave address
+
+This document describes the binding for mfd device and PMIC submodule.
+
+Binding for the built-in 32k clock generator block is defined separately
+in bindings/clk/maxim,max77802.txt file.
+
+Required properties:
+- compatible : Must be maxim,max77802;
+- reg : Specifies the i2c slave address of PMIC block.
+- interrupts : This i2c device has an IRQ line connected to the main SoC.
+- interrupt-parent : The parent interrupt controller.
+
+Optional properties:
+- max77802,pmic-buck-default-dvs-idx: We'll always write this DVS index in the
+ PMIC for BUCKs with DVS (Bucks 1-4, 6).
+ NOTE: at the moment these bindings don't include enough details for actual
+ GPIO-DVS--this just lets you choose which single slot to use.
+
+- max77802,pmic-buck-dvs-gpios: The DVS GPIOs. We'll try to set these GPIOs
+ to match pmic-buck-default-dvs-idx at probe time if they are defined. If
+ some or all of these GPIOs are not defined it's assumed that the board has
+ any missing GPIOs hardwired to match pmic-buck-default-dvs-idx.
+
+- max77802,pmic-buck-selb-gpios: GPIOs to enable DVS-GPIO for BUCKs.
+ Should be five values: 1, 2, 3, 4, 6. It is strongly suggested to include
+ these GPIOs if there's any chance that changing DVS GPIOs one line at a
+ time might glitch your DVS values.
+
+Optional node:
+- voltage-regulators : The regulators of max77802 have to be instantiated
+ under subnode named voltage-regulators using the following format.
+
+ regulator_name {
+ regulator-compatible = LDOn/BUCKn
+ standard regulator constraints
+ };
+ refer Documentation/devicetree/bindings/regulator/regulator.txt
+
+ The regulator-compatible property of regulator should initialized with string
+to get matched with their hardware counterparts as follow:
+
+ -LDOn : for LDOs, where n can lie in range 1 to 35.
+ example: LDO1, LDO2, LDO35.
+ -BUCKn : for BUCKs, where n can lie in range 1 to 10.
+ example: BUCK1, BUCK5, BUCK10.
+Example:
+
+ max77802@09 {
+ compatible = maxim,max77802;
+ interrupt-parent = wakeup_eint;
+ interrupts = 26 0;
+ reg = 0x09;
+ #address-cells = 1;
+ #size-cells = 0;
+
+ max77802,pmic-buck-default-dvs-idx = 1;
+ max77802,pmic-buck-dvs-gpios = gpy7 6 0,
+ gpj4 2 0,
+ gpj4 3 0;
+ max77802,pmic-buck-selb-gpios = gph0 2 0,
+ gph0 3 0,
+ gph0 4 0,
+ gph0 5 0,
+ gph0 6 0;
+
+ voltage-regulators {
+ ldo11_reg {
+ regulator-compatible = LDO11;
+ regulator-name = vdd_ldo11;
+ regulator-min-microvolt = 190;
+ regulator-max-microvolt = 190;
+
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
Maxim MAX77802 is a power management chip that contains 10 high
efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
to power up application processors and peripherals, a 2-channel
32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
to program the individual regulators, clocks outputs and the RTC.
This patch adds the core support for MAX77802 PMIC and is based
on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree.
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
(...)
diff --git a/drivers/mfd/max77802-irq.c b/drivers/mfd/max77802-irq.c
new file mode 100644
index 000..38a8ce7
--- /dev/null
+++ b/drivers/mfd/max77802-irq.c
@@ -0,0 +1,332 @@
+/*
+ * max77802-irq.c - Interrupt controller support for MAX77802
+ *
+ * Copyright (C) 2013-2014 Google, Inc
+ *
+ * Copyright (C) 2012 Samsung Electronics Co.Ltd
+ * Chiwoong Byun woong.b...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This driver is based on max8997-irq.c
+ */
+
+#include linux/err.h
+#include linux/irq.h
+#include linux/interrupt.h
+#include linux/gpio.h
+#include linux/mfd/max77802.h
+#include linux/mfd/max77802-private.h
+#include linux/irqdomain.h
+#include linux/regmap.h
+
+enum {
+ MAX77802_DEBUG_IRQ_INFO = 1 0,
+ MAX77802_DEBUG_IRQ_MASK = 1 1,
+ MAX77802_DEBUG_IRQ_INT = 1 2,
+};
+
+static int debug_mask = 0;
+module_param(debug_mask, int, 0);
+MODULE_PARM_DESC(debug_mask, Set debug_mask : 0x0=off 0x1=IRQ_INFO
0x2=IRQ_MASK 0x4=IRQ_INI));
+
+static const u8 max77802_mask_reg[] = {
+ [PMIC_INT1] = MAX77802_REG_INT1MSK,
+ [PMIC_INT2] = MAX77802_REG_INT2MSK,
+ [RTC_INT] = MAX77802_RTC_INTM,
+};
+
+struct max77802_irq_data {
+ int mask;
+ enum max77802_irq_source group;
+};
+
+#define DECLARE_IRQ(idx, _group, _mask) \
+ [(idx)] = { .group = (_group), .mask = (_mask) }
+static const struct max77802_irq_data max77802_irqs[] = {
+ DECLARE_IRQ(MAX77802_PMICIRQ_PWRONF,PMIC_INT1, 1 0),
+ DECLARE_IRQ(MAX77802_PMICIRQ_PWRONR,PMIC_INT1, 1 1),
+ DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBF, PMIC_INT1, 1 2),
+ DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBR, PMIC_INT1, 1 3),
+ DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBF,PMIC_INT1, 1 4),
+ DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBR,PMIC_INT1, 1 5),
+ DECLARE_IRQ(MAX77802_PMICIRQ_ONKEY1S, PMIC_INT1, 1 6),
+ DECLARE_IRQ(MAX77802_PMICIRQ_MRSTB, PMIC_INT1, 1 7),
+ DECLARE_IRQ(MAX77802_PMICIRQ_140C, PMIC_INT2, 1 0),
+ DECLARE_IRQ(MAX77802_PMICIRQ_120C, PMIC_INT2, 1 1),
+ DECLARE_IRQ(MAX77802_RTCIRQ_RTC60S, RTC_INT, 1 0),
+ DECLARE_IRQ(MAX77802_RTCIRQ_RTCA1, RTC_INT, 1 1),
+ DECLARE_IRQ(MAX77802_RTCIRQ_RTCA2, RTC_INT, 1 2),
+ DECLARE_IRQ(MAX77802_RTCIRQ_SMPL, RTC_INT, 1 3),
+ DECLARE_IRQ(MAX77802_RTCIRQ_RTC1S, RTC_INT, 1 4),
+ DECLARE_IRQ(MAX77802_RTCIRQ_WTSR, RTC_INT, 1 5),
+};
Why just not use two regmap_irq_chips (for PMIC and RTC blocks)
replacing whole max77802-irq.c file?
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
On Mon, Jun 09, 2014 at 12:22:41PM +0200, Krzysztof Kozlowski wrote:
On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
+static const struct max77802_irq_data max77802_irqs[] = {
+ DECLARE_IRQ(MAX77802_PMICIRQ_PWRONF,PMIC_INT1, 1 0),
+ DECLARE_IRQ(MAX77802_PMICIRQ_PWRONR,PMIC_INT1, 1 1),
Why just not use two regmap_irq_chips (for PMIC and RTC blocks)
replacing whole max77802-irq.c file?
If they share the same primary IRQ line something like what the arizona
drivers do should handle things well enough.
signature.asc
Description: Digital signature
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
On Mon, Jun 09, 2014 at 11:37:46AM +0200, Javier Martinez Canillas wrote:
+Optional node:
+- voltage-regulators : The regulators of max77802 have to be instantiated
+ under subnode named voltage-regulators using the following format.
Every other PMIC calls this node regulators...
+ regulator_name {
+ regulator-compatible = LDOn/BUCKn
regulator-compatible is deprecated, use the node name instead.
+config MFD_MAX77802
+ bool Maxim Integrated MAX77802 PMIC Support
Why is this bool and not tristate?
+int max77802_irq_resume(struct max77802_dev *max77802)
+{
+ /*
+ * The IRQ that woke us up may still need to be ACK'ed on resume.
+ * If it isn't ever ACK'ed, future IRQs may not be delivered.
+ */
+ if (max77802-irq)
+ max77802_irq_thread(0, max77802);
+
+ return 0;
+}
As covered in another subthread all this code looks like it should be
regmap-irq.
+ if (regmap_read(max77802-regmap,
+ MAX77802_REG_DEVICE_ID, data) 0) {
+ dev_err(max77802-dev,
+ device not found on this channel (this is not an
error)\n);
+ return -ENODEV;
If this is not an error why is it printed as dev_err()? It does look
like an error to me, though.
+ } else {
+ dev_info(max77802-dev, device found\n);
+ }
These sort of prints are just noise, remove this unless there is some
revision information you can display. It's also better practice to
check that the device ID is actually what was expected in case there was
an error in the DT.
+static const struct i2c_device_id max77802_i2c_id[] = {
+ { max77802, TYPE_MAX77802 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max77802_i2c_id);
We have type information here but not in the OF ID table (not that we
ever look at it).
signature.asc
Description: Digital signature
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
Hello Krzysztof,
On 06/09/2014 12:22 PM, Krzysztof Kozlowski wrote:
On pon, 2014-06-09 at 11:37 +0200, Javier Martinez Canillas wrote:
Maxim MAX77802 is a power management chip that contains 10 high
efficiency Buck regulators, 32 Low-dropout (LDO) regulators used
to power up application processors and peripherals, a 2-channel
32kHz clock outputs, a Real-Time-Clock (RTC) and a I2C interface
to program the individual regulators, clocks outputs and the RTC.
This patch adds the core support for MAX77802 PMIC and is based
on a driver added by Simon Glass to the Chrome OS kernel 3.8 tree.
Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
(...)
diff --git a/drivers/mfd/max77802-irq.c b/drivers/mfd/max77802-irq.c
new file mode 100644
index 000..38a8ce7
--- /dev/null
+++ b/drivers/mfd/max77802-irq.c
@@ -0,0 +1,332 @@
+/*
+ * max77802-irq.c - Interrupt controller support for MAX77802
+ *
+ * Copyright (C) 2013-2014 Google, Inc
+ *
+ * Copyright (C) 2012 Samsung Electronics Co.Ltd
+ * Chiwoong Byun woong.b...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This driver is based on max8997-irq.c
+ */
+
+#include linux/err.h
+#include linux/irq.h
+#include linux/interrupt.h
+#include linux/gpio.h
+#include linux/mfd/max77802.h
+#include linux/mfd/max77802-private.h
+#include linux/irqdomain.h
+#include linux/regmap.h
+
+enum {
+MAX77802_DEBUG_IRQ_INFO = 1 0,
+MAX77802_DEBUG_IRQ_MASK = 1 1,
+MAX77802_DEBUG_IRQ_INT = 1 2,
+};
+
+static int debug_mask = 0;
+module_param(debug_mask, int, 0);
+MODULE_PARM_DESC(debug_mask, Set debug_mask : 0x0=off 0x1=IRQ_INFO
0x2=IRQ_MASK 0x4=IRQ_INI));
+
+static const u8 max77802_mask_reg[] = {
+[PMIC_INT1] = MAX77802_REG_INT1MSK,
+[PMIC_INT2] = MAX77802_REG_INT2MSK,
+[RTC_INT] = MAX77802_RTC_INTM,
+};
+
+struct max77802_irq_data {
+int mask;
+enum max77802_irq_source group;
+};
+
+#define DECLARE_IRQ(idx, _group, _mask) \
+[(idx)] = { .group = (_group), .mask = (_mask) }
+static const struct max77802_irq_data max77802_irqs[] = {
+DECLARE_IRQ(MAX77802_PMICIRQ_PWRONF,PMIC_INT1, 1 0),
+DECLARE_IRQ(MAX77802_PMICIRQ_PWRONR,PMIC_INT1, 1 1),
+DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBF, PMIC_INT1, 1 2),
+DECLARE_IRQ(MAX77802_PMICIRQ_JIGONBR, PMIC_INT1, 1 3),
+DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBF,PMIC_INT1, 1 4),
+DECLARE_IRQ(MAX77802_PMICIRQ_ACOKBR,PMIC_INT1, 1 5),
+DECLARE_IRQ(MAX77802_PMICIRQ_ONKEY1S, PMIC_INT1, 1 6),
+DECLARE_IRQ(MAX77802_PMICIRQ_MRSTB, PMIC_INT1, 1 7),
+DECLARE_IRQ(MAX77802_PMICIRQ_140C, PMIC_INT2, 1 0),
+DECLARE_IRQ(MAX77802_PMICIRQ_120C, PMIC_INT2, 1 1),
+DECLARE_IRQ(MAX77802_RTCIRQ_RTC60S, RTC_INT, 1 0),
+DECLARE_IRQ(MAX77802_RTCIRQ_RTCA1, RTC_INT, 1 1),
+DECLARE_IRQ(MAX77802_RTCIRQ_RTCA2, RTC_INT, 1 2),
+DECLARE_IRQ(MAX77802_RTCIRQ_SMPL, RTC_INT, 1 3),
+DECLARE_IRQ(MAX77802_RTCIRQ_RTC1S, RTC_INT, 1 4),
+DECLARE_IRQ(MAX77802_RTCIRQ_WTSR, RTC_INT, 1 5),
+};
Why just not use two regmap_irq_chips (for PMIC and RTC blocks)
replacing whole max77802-irq.c file?
Great, I was not aware about regmap_irq_chip. I'll take a look to it for v2.
Best regards,
Krzysztof
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] mfd: Add driver for Maxim 77802 Power Management IC
Hello Mark,
On 06/09/2014 09:47 PM, Mark Brown wrote:
On Mon, Jun 09, 2014 at 11:37:46AM +0200, Javier Martinez Canillas wrote:
+Optional node:
+- voltage-regulators : The regulators of max77802 have to be instantiated
+ under subnode named voltage-regulators using the following format.
Every other PMIC calls this node regulators...
Ok, I'll change for consistency.
+regulator_name {
+regulator-compatible = LDOn/BUCKn
regulator-compatible is deprecated, use the node name instead.
Ok.
+config MFD_MAX77802
+bool Maxim Integrated MAX77802 PMIC Support
Why is this bool and not tristate?
I noticed that the majority of the mfd PMIC drivers were bool and not tristate
so I thought it was a convention. But nothing prevents this driver to be built
as a module so I'll change it to tristate.
+int max77802_irq_resume(struct max77802_dev *max77802)
+{
+/*
+ * The IRQ that woke us up may still need to be ACK'ed on resume.
+ * If it isn't ever ACK'ed, future IRQs may not be delivered.
+ */
+if (max77802-irq)
+max77802_irq_thread(0, max77802);
+
+return 0;
+}
As covered in another subthread all this code looks like it should be
regmap-irq.
It seems so, I'll take that into account for v2.
+if (regmap_read(max77802-regmap,
+ MAX77802_REG_DEVICE_ID, data) 0) {
+dev_err(max77802-dev,
+device not found on this channel (this is not an
error)\n);
+return -ENODEV;
If this is not an error why is it printed as dev_err()? It does look
like an error to me, though.
Yeah, it is an error so I'll clean that message.
+} else {
+dev_info(max77802-dev, device found\n);
+}
These sort of prints are just noise, remove this unless there is some
revision information you can display. It's also better practice to
check that the device ID is actually what was expected in case there was
an error in the DT.
Ok, will do.
+static const struct i2c_device_id max77802_i2c_id[] = {
+{ max77802, TYPE_MAX77802 },
+{ }
+};
+MODULE_DEVICE_TABLE(i2c, max77802_i2c_id);
We have type information here but not in the OF ID table (not that we
ever look at it).
Yeah, I'll remove the type information here. It is a left over when trying to
combine both max77802 and max77686 drivers since in a combined driver we need
the type information.
Thanks a lot for your feedback.
Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
