This patch-series adds support for Device tree to 88PM800 mfd driver.
It also enabled configuration of irq clear method through DT.
Testing::
- Boot tested on PXA1928 based platform.
- probe of mfd, rtc and regulator function passing successfully.
- Basic read operations on registers
V2 => V3
=======
Link to V2: https://www.mail-archive.com/[email protected]/msg914299.html
- Replaced deprecated "regulator-compatible" property with "regulator-name".
- Added Rob's Acked-by to [PATCH 3/3]
V1 => V2
=======
Link to V1: http://lkml.iu.edu/hypermail/linux/kernel/1505.3/04386.html
- Split binding changes from original commit
- Updated binding info as per Rob's suggestion
- Dropped PATCH 4/4, as discussed during review
- Dropped PATCH 3/4, as it is independent RTC code change,
so will submit it separately to ease merging.
- Fixed all other minor comments
Attempt has been made to push some of the patches to the list sometime
back in 2013.
Link to previous patch submission:
https://lkml.org/lkml/2013/8/14/86
TODO:
=====
- init config for 88PM860 device
- Rgulator driver changes to add support for 88PM860 device
Vaibhav Hiremath (3):
mfd: 88pm800: Add device tree support
mfd: 88pm800: Allow configuration of interrupt clear method
mfd: devicetree: bindings: Add new 88pm800 mfd binding
Documentation/devicetree/bindings/mfd/88pm800.txt | 60 +++++++++++++++++++++++
drivers/mfd/88pm800.c | 40 +++++++++++++--
include/linux/mfd/88pm80x.h | 6 ++-
3 files changed, 99 insertions(+), 7 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
--
1.9.1
Add DT support to the 88pm800 driver, along with compatible
field for it's sub-devices (rtc, onkey and regulator)
Signed-off-by: Chao Xie <[email protected]>
Signed-off-by: Vaibhav Hiremath <[email protected]>
---
drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 841717a..059f01a 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -27,6 +27,7 @@
#include <linux/mfd/core.h>
#include <linux/mfd/88pm80x.h>
#include <linux/slab.h>
+#include <linux/of_device.h>
/* Interrupt Registers */
#define PM800_INT_STATUS1 (0x05)
@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
};
MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
+static const struct of_device_id pm80x_of_match_table[] = {
+ { .compatible = "marvell,88pm800", },
+ {},
+};
+
static struct resource rtc_resources[] = {
{
.name = "88pm80x-rtc",
@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
static struct mfd_cell rtc_devs[] = {
{
.name = "88pm80x-rtc",
+ .of_compatible = "marvell,88pm80x-rtc",
.num_resources = ARRAY_SIZE(rtc_resources),
.resources = &rtc_resources[0],
.id = -1,
@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
static const struct mfd_cell onkey_devs[] = {
{
.name = "88pm80x-onkey",
+ .of_compatible = "marvell,88pm80x-onkey",
.num_resources = 1,
.resources = &onkey_resources[0],
.id = -1,
@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
static const struct mfd_cell regulator_devs[] = {
{
.name = "88pm80x-regulator",
+ .of_compatible = "marvell,88pm80x-regulator",
.id = -1,
},
};
@@ -544,8 +553,23 @@ static int pm800_probe(struct i2c_client *client,
int ret = 0;
struct pm80x_chip *chip;
struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
+ struct device_node *np = client->dev.of_node;
struct pm80x_subchip *subchip;
+ if (!pdata && !np) {
+ dev_err(&client->dev,
+ "pm80x requires platform data or of_node\n");
+ return -EINVAL;
+ }
+
+ if (!pdata) {
+ pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(&client->dev, "failed to allocaate memory\n");
+ return -ENOMEM;
+ }
+ }
+
ret = pm80x_init(client);
if (ret) {
dev_err(&client->dev, "pm800_init fail\n");
@@ -611,6 +635,7 @@ static struct i2c_driver pm800_driver = {
.name = "88PM800",
.owner = THIS_MODULE,
.pm = &pm80x_pm_ops,
+ .of_match_table = pm80x_of_match_table,
},
.probe = pm800_probe,
.remove = pm800_remove,
--
1.9.1
As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
(page 0) controls the method of clearing interrupt
status of 88pm800 family of devices;
0: clear on read
1: clear on write
This patch allows to configure this field, through DT.
Also, as suggested by "Lee Jones" renaming DT property and variable
field to appropriate name.
Signed-off-by: Zhao Ye <[email protected]>
Signed-off-by: Vaibhav Hiremath <[email protected]>
---
drivers/mfd/88pm800.c | 15 ++++++++++-----
include/linux/mfd/88pm80x.h | 6 ++++--
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 059f01a..c1a6306 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
{
struct regmap *map = chip->regmap;
unsigned long flags = IRQF_ONESHOT;
- int data, mask, ret = -EINVAL;
+ int irq_clr_mode, mask, ret = -EINVAL;
if (!map || !chip->irq) {
dev_err(chip->dev, "incorrect parameters\n");
@@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
}
/*
- * irq_mode defines the way of clearing interrupt. it's read-clear by
- * default.
+ * irq_clr_on_wr defines the way of clearing interrupt by
+ * read/write(0/1). It's read-clear by default.
*/
mask =
PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
PM800_WAKEUP2_INT_MASK;
- data = PM800_WAKEUP2_INT_CLEAR;
- ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
+ irq_clr_mode = (chip->irq_clr_on_wr) ?
+ PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
+ ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
if (ret < 0)
goto out;
@@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
}
chip->regmap_irq_chip = &pm800_irq_chip;
+ chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
ret = device_irq_init_800(chip);
if (ret < 0) {
@@ -568,6 +570,9 @@ static int pm800_probe(struct i2c_client *client,
dev_err(&client->dev, "failed to allocaate memory\n");
return -ENOMEM;
}
+
+ pdata->irq_clr_on_wr = of_property_read_bool(np,
+ "marvell,irq-clr-on-write");
}
ret = pm80x_init(client);
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 97cb283..94b3dcd 100644
--- a/include/linux/mfd/88pm80x.h
+++ b/include/linux/mfd/88pm80x.h
@@ -77,6 +77,8 @@ enum {
#define PM800_WAKEUP2 (0x0E)
#define PM800_WAKEUP2_INV_INT (1 << 0)
#define PM800_WAKEUP2_INT_CLEAR (1 << 1)
+#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
#define PM800_WAKEUP2_INT_MASK (1 << 2)
#define PM800_POWER_UP_LOG (0x10)
@@ -300,7 +302,7 @@ struct pm80x_chip {
struct regmap_irq_chip_data *irq_data;
int type;
int irq;
- int irq_mode;
+ int irq_clr_on_wr; /* '1': Clear on write, '0': Clear on read*/
unsigned long wu_flag;
spinlock_t lock;
};
@@ -315,7 +317,7 @@ struct pm80x_platform_data {
*/
struct regulator_init_data *regulators[PM800_ID_RG_MAX];
unsigned int num_regulators;
- int irq_mode; /* Clear interrupt by read/write(0/1) */
+ int irq_clr_on_wr; /* Clear interrupt by read/write(0/1) */
int batt_det; /* enable/disable */
int (*plat_config)(struct pm80x_chip *chip,
struct pm80x_platform_data *pdata);
--
1.9.1
With addition of DT support to 88pm800 mfd driver, this patch
adds new DT binding documentation along with respective properties.
Signed-off-by: Vaibhav Hiremath <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/mfd/88pm800.txt | 60 +++++++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
new file mode 100644
index 0000000..6897119
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -0,0 +1,60 @@
+* Marvell 88PM8xx Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
+- reg : the I2C slave address for the 88pm8xx chip
+- interrupts : IRQ line for the 88pm8xx chip
+- interrupt-controller: describes the 88pm8xx as an interrupt controller
+- #interrupt-cells : should be 1.
+ - The cell is the 88pm8xx local IRQ number
+
+Optional parent device properties:
+- marvell,irq-clr-on-write: indicates whether interrupt status is cleared
+ by write or read.
+ If enabled, interrupt is cleared by write else just read would do.
+
+88pm8xx family of devices consists of varied group of sub-devices:
+
+Device Supply Names Description
+------ ------------ -----------
+88pm80x-onkey : : On key
+88pm80x-rtc : : RTC
+88pm80x-regulator : : Regulators
+
+Note: More device list will follow
+
+Example:
+
+ pmic: 88pm800@30 {
+ compatible = "marvell,88pm800";
+ reg = <0x30>;
+ interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-parent = <&gic>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ marvell,irq-clr-on-write;
+
+ regulators {
+ compatible = "marvell,88pm80x-regulator";
+
+ buck1a: BUCK1A {
+ regulator-name = "BUCK1A";
+ regulator-min-microvolt = <600000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ ldo1: LDO1 {
+ regulator-name = "LDO1";
+ regulator-min-microvolt = <1700000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ };
+
+ rtc {
+ compatible = "marvell,88pm80x-rtc";
+ };
+ };
--
1.9.1
2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> Add DT support to the 88pm800 driver, along with compatible
> field for it's sub-devices (rtc, onkey and regulator)
>
> Signed-off-by: Chao Xie <[email protected]>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
> ---
> drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 841717a..059f01a 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -27,6 +27,7 @@
> #include <linux/mfd/core.h>
> #include <linux/mfd/88pm80x.h>
> #include <linux/slab.h>
> +#include <linux/of_device.h>
>
> /* Interrupt Registers */
> #define PM800_INT_STATUS1 (0x05)
> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> };
> MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>
> +static const struct of_device_id pm80x_of_match_table[] = {
> + { .compatible = "marvell,88pm800", },
> + {},
> +};
> +
> static struct resource rtc_resources[] = {
> {
> .name = "88pm80x-rtc",
> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> static struct mfd_cell rtc_devs[] = {
> {
> .name = "88pm80x-rtc",
> + .of_compatible = "marvell,88pm80x-rtc",
> .num_resources = ARRAY_SIZE(rtc_resources),
> .resources = &rtc_resources[0],
> .id = -1,
> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> static const struct mfd_cell onkey_devs[] = {
> {
> .name = "88pm80x-onkey",
> + .of_compatible = "marvell,88pm80x-onkey",
> .num_resources = 1,
> .resources = &onkey_resources[0],
> .id = -1,
> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> static const struct mfd_cell regulator_devs[] = {
> {
> .name = "88pm80x-regulator",
> + .of_compatible = "marvell,88pm80x-regulator",
> .id = -1,
> },
> };
> @@ -544,8 +553,23 @@ static int pm800_probe(struct i2c_client *client,
> int ret = 0;
> struct pm80x_chip *chip;
> struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> + struct device_node *np = client->dev.of_node;
> struct pm80x_subchip *subchip;
>
> + if (!pdata && !np) {
> + dev_err(&client->dev,
> + "pm80x requires platform data or of_node\n");
> + return -EINVAL;
> + }
> +
> + if (!pdata) {
> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_err(&client->dev, "failed to allocaate memory\n");
Generic error message for ENOMEM is not needed. Just return ENOMEM and
the core code will print the error.
Rest looks fine,
Best regards,
Krzysztof
2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath <[email protected]>:
> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> (page 0) controls the method of clearing interrupt
> status of 88pm800 family of devices;
>
> 0: clear on read
> 1: clear on write
>
> This patch allows to configure this field, through DT.
>
> Also, as suggested by "Lee Jones" renaming DT property and variable
> field to appropriate name.
>
> Signed-off-by: Zhao Ye <[email protected]>
> Signed-off-by: Vaibhav Hiremath <[email protected]>
It does not look like a property of the board. Instead it looks like a
runtime configuration so it should not be part of DT bindings.
I understand that previously this was configured by platform data and
now you want to move everything to DT. But this does not belong to
DT...
Best regards,
Krzysztof
> ---
> drivers/mfd/88pm800.c | 15 ++++++++++-----
> include/linux/mfd/88pm80x.h | 6 ++++--
> 2 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 059f01a..c1a6306 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> {
> struct regmap *map = chip->regmap;
> unsigned long flags = IRQF_ONESHOT;
> - int data, mask, ret = -EINVAL;
> + int irq_clr_mode, mask, ret = -EINVAL;
>
> if (!map || !chip->irq) {
> dev_err(chip->dev, "incorrect parameters\n");
> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> }
>
> /*
> - * irq_mode defines the way of clearing interrupt. it's read-clear by
> - * default.
> + * irq_clr_on_wr defines the way of clearing interrupt by
> + * read/write(0/1). It's read-clear by default.
> */
> mask =
> PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
> PM800_WAKEUP2_INT_MASK;
>
> - data = PM800_WAKEUP2_INT_CLEAR;
> - ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> + irq_clr_mode = (chip->irq_clr_on_wr) ?
> + PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> + ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>
> if (ret < 0)
> goto out;
> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
> }
>
> chip->regmap_irq_chip = &pm800_irq_chip;
> + chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>
> ret = device_irq_init_800(chip);
> if (ret < 0) {
> @@ -568,6 +570,9 @@ static int pm800_probe(struct i2c_client *client,
> dev_err(&client->dev, "failed to allocaate memory\n");
> return -ENOMEM;
> }
> +
> + pdata->irq_clr_on_wr = of_property_read_bool(np,
> + "marvell,irq-clr-on-write");
> }
>
> ret = pm80x_init(client);
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 97cb283..94b3dcd 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -77,6 +77,8 @@ enum {
> #define PM800_WAKEUP2 (0x0E)
> #define PM800_WAKEUP2_INV_INT (1 << 0)
> #define PM800_WAKEUP2_INT_CLEAR (1 << 1)
> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1)
> #define PM800_WAKEUP2_INT_MASK (1 << 2)
>
> #define PM800_POWER_UP_LOG (0x10)
> @@ -300,7 +302,7 @@ struct pm80x_chip {
> struct regmap_irq_chip_data *irq_data;
> int type;
> int irq;
> - int irq_mode;
> + int irq_clr_on_wr; /* '1': Clear on write, '0': Clear on read*/
> unsigned long wu_flag;
> spinlock_t lock;
> };
> @@ -315,7 +317,7 @@ struct pm80x_platform_data {
> */
> struct regulator_init_data *regulators[PM800_ID_RG_MAX];
> unsigned int num_regulators;
> - int irq_mode; /* Clear interrupt by read/write(0/1) */
> + int irq_clr_on_wr; /* Clear interrupt by read/write(0/1) */
> int batt_det; /* enable/disable */
> int (*plat_config)(struct pm80x_chip *chip,
> struct pm80x_platform_data *pdata);
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thursday 25 June 2015 05:33 AM, Krzysztof Kozlowski wrote:
> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath <[email protected]>:
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>> 0: clear on read
>> 1: clear on write
>>
>> This patch allows to configure this field, through DT.
>>
>> Also, as suggested by "Lee Jones" renaming DT property and variable
>> field to appropriate name.
>>
>> Signed-off-by: Zhao Ye <[email protected]>
>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>
> It does not look like a property of the board. Instead it looks like a
> runtime configuration so it should not be part of DT bindings.
>
Why do you say that?
It is very well feature of 88PM860 device, where you can control irq
clear operation (either read/write).
Thanks,
Vaibhav
> I understand that previously this was configured by platform data and
> now you want to move everything to DT. But this does not belong to
> DT...
>
Thats not completely true.
I think DT is the right place for this configuration.
Thanks,
Vaibhav
On Thursday 25 June 2015 05:27 AM, Krzysztof Kozlowski wrote:
> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath <[email protected]>:
>> Add DT support to the 88pm800 driver, along with compatible
>> field for it's sub-devices (rtc, onkey and regulator)
>>
>> Signed-off-by: Chao Xie <[email protected]>
>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>> ---
>> drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 841717a..059f01a 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -27,6 +27,7 @@
>> #include <linux/mfd/core.h>
>> #include <linux/mfd/88pm80x.h>
>> #include <linux/slab.h>
>> +#include <linux/of_device.h>
>>
>> /* Interrupt Registers */
>> #define PM800_INT_STATUS1 (0x05)
>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>> };
>> MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>
>> +static const struct of_device_id pm80x_of_match_table[] = {
>> + { .compatible = "marvell,88pm800", },
>> + {},
>> +};
>> +
>> static struct resource rtc_resources[] = {
>> {
>> .name = "88pm80x-rtc",
>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>> static struct mfd_cell rtc_devs[] = {
>> {
>> .name = "88pm80x-rtc",
>> + .of_compatible = "marvell,88pm80x-rtc",
>> .num_resources = ARRAY_SIZE(rtc_resources),
>> .resources = &rtc_resources[0],
>> .id = -1,
>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>> static const struct mfd_cell onkey_devs[] = {
>> {
>> .name = "88pm80x-onkey",
>> + .of_compatible = "marvell,88pm80x-onkey",
>> .num_resources = 1,
>> .resources = &onkey_resources[0],
>> .id = -1,
>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>> static const struct mfd_cell regulator_devs[] = {
>> {
>> .name = "88pm80x-regulator",
>> + .of_compatible = "marvell,88pm80x-regulator",
>> .id = -1,
>> },
>> };
>> @@ -544,8 +553,23 @@ static int pm800_probe(struct i2c_client *client,
>> int ret = 0;
>> struct pm80x_chip *chip;
>> struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>> + struct device_node *np = client->dev.of_node;
>> struct pm80x_subchip *subchip;
>>
>> + if (!pdata && !np) {
>> + dev_err(&client->dev,
>> + "pm80x requires platform data or of_node\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!pdata) {
>> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + dev_err(&client->dev, "failed to allocaate memory\n");
>
> Generic error message for ENOMEM is not needed. Just return ENOMEM and
> the core code will print the error.
>
> Rest looks fine,
Ok, will remove it.
Should I add your reviewed-by in V4 for this patch?
Thanks,
Vaibhav
On 25.06.2015 14:26, Vaibhav Hiremath wrote:
>
>
> On Thursday 25 June 2015 05:33 AM, Krzysztof Kozlowski wrote:
>> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath
>> <[email protected]>:
>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>> (page 0) controls the method of clearing interrupt
>>> status of 88pm800 family of devices;
>>>
>>> 0: clear on read
>>> 1: clear on write
>>>
>>> This patch allows to configure this field, through DT.
>>>
>>> Also, as suggested by "Lee Jones" renaming DT property and variable
>>> field to appropriate name.
>>>
>>> Signed-off-by: Zhao Ye <[email protected]>
>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>
>> It does not look like a property of the board. Instead it looks like a
>> runtime configuration so it should not be part of DT bindings.
>>
>
> Why do you say that?
>
> It is very well feature of 88PM860 device, where you can control irq
> clear operation (either read/write).
>
>
> Thanks,
> Vaibhav
>
>> I understand that previously this was configured by platform data and
>> now you want to move everything to DT. But this does not belong to
>> DT...
>>
>
> Thats not completely true.
> I think DT is the right place for this configuration.
DT and its bindings describe the specific board or device. Let me quote:
<<The "Open Firmware Device Tree", or simply Device Tree (DT), is a data
structure and language for describing hardware. More specifically, it
is a description of hardware that is readable by an operating system...>>
Whether you clear interrupts by writing or reading is configured during
runtime and it is completely independent to wiring. Each board with
88pm800 would allow both methods. So this is not a property of hardware
in the terms of open firmware. This is a runtime configuration.
Description of hardware would be a property which specifies whether a
88pm800-like device or a board using 88pm800 device ALLOWS choosing
different interrupt clearing.
Best regards,
Krzysztof
On 25.06.2015 14:27, Vaibhav Hiremath wrote:
>
>
> On Thursday 25 June 2015 05:27 AM, Krzysztof Kozlowski wrote:
>> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath
>> <[email protected]>:
>>> Add DT support to the 88pm800 driver, along with compatible
>>> field for it's sub-devices (rtc, onkey and regulator)
>>>
>>> Signed-off-by: Chao Xie <[email protected]>
>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>> ---
>>> drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
>>> 1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>> index 841717a..059f01a 100644
>>> --- a/drivers/mfd/88pm800.c
>>> +++ b/drivers/mfd/88pm800.c
>>> @@ -27,6 +27,7 @@
>>> #include <linux/mfd/core.h>
>>> #include <linux/mfd/88pm80x.h>
>>> #include <linux/slab.h>
>>> +#include <linux/of_device.h>
>>>
>>> /* Interrupt Registers */
>>> #define PM800_INT_STATUS1 (0x05)
>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id
>>> pm80x_id_table[] = {
>>> };
>>> MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>
>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>> + { .compatible = "marvell,88pm800", },
>>> + {},
>>> +};
>>> +
>>> static struct resource rtc_resources[] = {
>>> {
>>> .name = "88pm80x-rtc",
>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>> static struct mfd_cell rtc_devs[] = {
>>> {
>>> .name = "88pm80x-rtc",
>>> + .of_compatible = "marvell,88pm80x-rtc",
>>> .num_resources = ARRAY_SIZE(rtc_resources),
>>> .resources = &rtc_resources[0],
>>> .id = -1,
>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>> static const struct mfd_cell onkey_devs[] = {
>>> {
>>> .name = "88pm80x-onkey",
>>> + .of_compatible = "marvell,88pm80x-onkey",
>>> .num_resources = 1,
>>> .resources = &onkey_resources[0],
>>> .id = -1,
>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>> static const struct mfd_cell regulator_devs[] = {
>>> {
>>> .name = "88pm80x-regulator",
>>> + .of_compatible = "marvell,88pm80x-regulator",
>>> .id = -1,
>>> },
>>> };
>>> @@ -544,8 +553,23 @@ static int pm800_probe(struct i2c_client *client,
>>> int ret = 0;
>>> struct pm80x_chip *chip;
>>> struct pm80x_platform_data *pdata =
>>> dev_get_platdata(&client->dev);
>>> + struct device_node *np = client->dev.of_node;
>>> struct pm80x_subchip *subchip;
>>>
>>> + if (!pdata && !np) {
>>> + dev_err(&client->dev,
>>> + "pm80x requires platform data or of_node\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!pdata) {
>>> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata),
>>> GFP_KERNEL);
>>> + if (!pdata) {
>>> + dev_err(&client->dev, "failed to allocaate
>>> memory\n");
>>
>> Generic error message for ENOMEM is not needed. Just return ENOMEM and
>> the core code will print the error.
>>
>> Rest looks fine,
>
>
> Ok, will remove it.
>
> Should I add your reviewed-by in V4 for this patch?
No, not yet. :)
I would put such tag in my reply if I had that intention.
Best regards,
Krzysztof
On Thursday 25 June 2015 11:02 AM, Krzysztof Kozlowski wrote:
> On 25.06.2015 14:26, Vaibhav Hiremath wrote:
>>
>>
>> On Thursday 25 June 2015 05:33 AM, Krzysztof Kozlowski wrote:
>>> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath
>>> <[email protected]>:
>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>> (page 0) controls the method of clearing interrupt
>>>> status of 88pm800 family of devices;
>>>>
>>>> 0: clear on read
>>>> 1: clear on write
>>>>
>>>> This patch allows to configure this field, through DT.
>>>>
>>>> Also, as suggested by "Lee Jones" renaming DT property and variable
>>>> field to appropriate name.
>>>>
>>>> Signed-off-by: Zhao Ye <[email protected]>
>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>
>>> It does not look like a property of the board. Instead it looks like a
>>> runtime configuration so it should not be part of DT bindings.
>>>
>>
>> Why do you say that?
>>
>> It is very well feature of 88PM860 device, where you can control irq
>> clear operation (either read/write).
>>
>>
>> Thanks,
>> Vaibhav
>>
>>> I understand that previously this was configured by platform data and
>>> now you want to move everything to DT. But this does not belong to
>>> DT...
>>>
>>
>> Thats not completely true.
>> I think DT is the right place for this configuration.
>
> DT and its bindings describe the specific board or device. Let me quote:
> <<The "Open Firmware Device Tree", or simply Device Tree (DT), is a data
> structure and language for describing hardware. More specifically, it
> is a description of hardware that is readable by an operating system...>>
>
> Whether you clear interrupts by writing or reading is configured during
> runtime and it is completely independent to wiring. Each board with
> 88pm800 would allow both methods. So this is not a property of hardware
> in the terms of open firmware. This is a runtime configuration.
>
Yes,
Fair enough...
I see very little value in runtime configuration, why not just do it
only way (either read or write)?
I would prefer to just set it by default (during init), to clear irq on
write.
Thanks,
Vaibhav
On 25.06.2015 14:44, Vaibhav Hiremath wrote:
>
>
> On Thursday 25 June 2015 11:02 AM, Krzysztof Kozlowski wrote:
>> On 25.06.2015 14:26, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Thursday 25 June 2015 05:33 AM, Krzysztof Kozlowski wrote:
>>>> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath
>>>> <[email protected]>:
>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>> (page 0) controls the method of clearing interrupt
>>>>> status of 88pm800 family of devices;
>>>>>
>>>>> 0: clear on read
>>>>> 1: clear on write
>>>>>
>>>>> This patch allows to configure this field, through DT.
>>>>>
>>>>> Also, as suggested by "Lee Jones" renaming DT property and variable
>>>>> field to appropriate name.
>>>>>
>>>>> Signed-off-by: Zhao Ye <[email protected]>
>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>
>>>> It does not look like a property of the board. Instead it looks like a
>>>> runtime configuration so it should not be part of DT bindings.
>>>>
>>>
>>> Why do you say that?
>>>
>>> It is very well feature of 88PM860 device, where you can control irq
>>> clear operation (either read/write).
>>>
>>>
>>> Thanks,
>>> Vaibhav
>>>
>>>> I understand that previously this was configured by platform data and
>>>> now you want to move everything to DT. But this does not belong to
>>>> DT...
>>>>
>>>
>>> Thats not completely true.
>>> I think DT is the right place for this configuration.
>>
>> DT and its bindings describe the specific board or device. Let me quote:
>> <<The "Open Firmware Device Tree", or simply Device Tree (DT), is a data
>> structure and language for describing hardware. More specifically, it
>> is a description of hardware that is readable by an operating system...>>
>>
>> Whether you clear interrupts by writing or reading is configured during
>> runtime and it is completely independent to wiring. Each board with
>> 88pm800 would allow both methods. So this is not a property of hardware
>> in the terms of open firmware. This is a runtime configuration.
>>
>
> Yes,
> Fair enough...
>
> I see very little value in runtime configuration, why not just do it
> only way (either read or write)?
> I would prefer to just set it by default (during init), to clear irq on
> write.
Hard-coding a default value, if board files are not present, looks OK to me.
Best regards,
Krzysztof
On Thursday 25 June 2015 11:20 AM, Krzysztof Kozlowski wrote:
> On 25.06.2015 14:44, Vaibhav Hiremath wrote:
>>
>>
>> On Thursday 25 June 2015 11:02 AM, Krzysztof Kozlowski wrote:
>>> On 25.06.2015 14:26, Vaibhav Hiremath wrote:
>>>>
>>>>
>>>> On Thursday 25 June 2015 05:33 AM, Krzysztof Kozlowski wrote:
>>>>> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath
>>>>> <[email protected]>:
>>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>>> (page 0) controls the method of clearing interrupt
>>>>>> status of 88pm800 family of devices;
>>>>>>
>>>>>> 0: clear on read
>>>>>> 1: clear on write
>>>>>>
>>>>>> This patch allows to configure this field, through DT.
>>>>>>
>>>>>> Also, as suggested by "Lee Jones" renaming DT property and variable
>>>>>> field to appropriate name.
>>>>>>
>>>>>> Signed-off-by: Zhao Ye <[email protected]>
>>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>>
>>>
<snip>
>>
>> Yes,
>> Fair enough...
>>
>> I see very little value in runtime configuration, why not just do it
>> only way (either read or write)?
>> I would prefer to just set it by default (during init), to clear irq on
>> write.
>
> Hard-coding a default value, if board files are not present, looks OK to me.
>
This is how it will look, I will also update the binding information
with this.
hvaibhav@hvaibhav-ThinkPad-T440p:~/projects/mainline/linux$ git diff
--cached
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 0a417ac..e415a06 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -645,9 +645,8 @@ static int pm800_probe(struct i2c_client *client,
dev_err(&client->dev, "failed to allocaate
memory\n");
return -ENOMEM;
}
-
- pdata->irq_clr_on_wr = of_property_read_bool(np,
- "marvell,irq-clr-on-write");
+ /* Setting irq clear method on write */
+ pdata->irq_clr_on_wr = true;
}
ret = pm80x_init(client);
Thanks,
Vaibhav
On 25.06.2015 14:57, Vaibhav Hiremath wrote:
>
>
> On Thursday 25 June 2015 11:20 AM, Krzysztof Kozlowski wrote:
>> On 25.06.2015 14:44, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Thursday 25 June 2015 11:02 AM, Krzysztof Kozlowski wrote:
>>>> On 25.06.2015 14:26, Vaibhav Hiremath wrote:
>>>>>
>>>>>
>>>>> On Thursday 25 June 2015 05:33 AM, Krzysztof Kozlowski wrote:
>>>>>> 2015-06-24 18:21 GMT+09:00 Vaibhav Hiremath
>>>>>> <[email protected]>:
>>>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>>>> (page 0) controls the method of clearing interrupt
>>>>>>> status of 88pm800 family of devices;
>>>>>>>
>>>>>>> 0: clear on read
>>>>>>> 1: clear on write
>>>>>>>
>>>>>>> This patch allows to configure this field, through DT.
>>>>>>>
>>>>>>> Also, as suggested by "Lee Jones" renaming DT property and variable
>>>>>>> field to appropriate name.
>>>>>>>
>>>>>>> Signed-off-by: Zhao Ye <[email protected]>
>>>>>>> Signed-off-by: Vaibhav Hiremath <[email protected]>
>>>>>>
>>>>
>
> <snip>
>
>>>
>>> Yes,
>>> Fair enough...
>>>
>>> I see very little value in runtime configuration, why not just do it
>>> only way (either read or write)?
>>> I would prefer to just set it by default (during init), to clear irq on
>>> write.
>>
>> Hard-coding a default value, if board files are not present, looks OK
>> to me.
>>
>
> This is how it will look, I will also update the binding information
> with this.
>
>
> hvaibhav@hvaibhav-ThinkPad-T440p:~/projects/mainline/linux$ git diff
> --cached
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 0a417ac..e415a06 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -645,9 +645,8 @@ static int pm800_probe(struct i2c_client *client,
> dev_err(&client->dev, "failed to allocaate
> memory\n");
> return -ENOMEM;
> }
> -
> - pdata->irq_clr_on_wr = of_property_read_bool(np,
> - "marvell,irq-clr-on-write");
> + /* Setting irq clear method on write */
> + pdata->irq_clr_on_wr = true;
> }
>
> ret = pm80x_init(client);
IMHO it is good.
Best regards,
Krzysztof