[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

Rather than hard-coding the quirk topology, which stopped scaling,
parse the information from DT. The code looks for all compatible
PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
to the same pin. If so, the code sends a matching sequence to the
PMIC to deassert the IRQ.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-renesas-soc@vger.kernel.org
---
V2: - Replace the DT shared IRQ check loop with memcmp()
    - Send the I2C message to deassert the IRQ line to all PMICs
      in the list with shared IRQ line instead of just one
    - Add comment that this works only in case all the PMICs are
      on the same I2C bus
V3: - Drop the addr = 0x00 init
    - Drop reinit of argsa in rcar_gen2_regulator_quirk
---
 arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++-----
 1 file changed, 90 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 93f628acfd94..b919073aa27e 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -31,8 +31,10 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/list.h>
 #include <linux/notifier.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/mfd/da9063/registers.h>
 
 
@@ -44,34 +46,45 @@
 /* start of DA9210 System Control and Event Registers */
 #define DA9210_REG_MASK_A		0x54
 
+struct regulator_quirk {
+	struct list_head		list;
+	const struct of_device_id	*id;
+	struct of_phandle_args		irq_args;
+	struct i2c_msg			i2c_msg;
+	bool				shared;	/* IRQ line is shared */
+};
+
+static LIST_HEAD(quirk_list);
 static void __iomem *irqc;
 
 /* first byte sets the memory pointer, following are consecutive reg values */
 static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
 static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
 
-static struct i2c_msg da9xxx_msgs[3] = {
-	{
-		.addr = 0x58,
-		.len = ARRAY_SIZE(da9063_irq_clr),
-		.buf = da9063_irq_clr,
-	}, {
-		.addr = 0x68,
-		.len = ARRAY_SIZE(da9210_irq_clr),
-		.buf = da9210_irq_clr,
-	}, {
-		.addr = 0x70,
-		.len = ARRAY_SIZE(da9210_irq_clr),
-		.buf = da9210_irq_clr,
-	},
+static struct i2c_msg da9063_msgs = {
+	.len = ARRAY_SIZE(da9063_irq_clr),
+	.buf = da9063_irq_clr,
+};
+
+static struct i2c_msg da9210_msgs = {
+	.len = ARRAY_SIZE(da9210_irq_clr),
+	.buf = da9210_irq_clr,
+};
+
+static const struct of_device_id rcar_gen2_quirk_match[] = {
+	{ .compatible = "dlg,da9063", .data = &da9063_msgs },
+	{ .compatible = "dlg,da9210", .data = &da9210_msgs },
+	{},
 };
 
 static int regulator_quirk_notify(struct notifier_block *nb,
 				  unsigned long action, void *data)
 {
+	struct regulator_quirk *pos, *tmp;
 	struct device *dev = data;
 	struct i2c_client *client;
 	static bool done;
+	int ret;
 	u32 mon;
 
 	if (done)
@@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb,
 	client = to_i2c_client(dev);
 	dev_dbg(dev, "Detected %s\n", client->name);
 
-	if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
-	    (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
-	    (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
-		int ret, len;
+	/*
+	 * Send message to all PMICs that share an IRQ line to deassert it.
+	 *
+	 * WARNING: This works only if all the PMICs are on the same I2C bus.
+	 */
+	list_for_each_entry(pos, &quirk_list, list) {
+		if (!pos->shared)
+			continue;
 
-		/* There are two DA9210 on Stout, one on the other boards. */
-		len = of_machine_is_compatible("renesas,stout") ? 3 : 2;
+		dev_info(&client->dev, "clearing %s@0x%02x interrupts\n",
+			 pos->id->compatible, pos->i2c_msg.addr);
 
-		dev_info(&client->dev, "clearing da9063/da9210 interrupts\n");
-		ret = i2c_transfer(client->adapter, da9xxx_msgs, len);
-		if (ret != len)
+		ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1);
+		if (ret != 1)
 			dev_err(&client->dev, "i2c error %d\n", ret);
 	}
 
@@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb,
 remove:
 	dev_info(dev, "IRQ2 is not asserted, removing quirk\n");
 
+	list_for_each_entry_safe(pos, tmp, &quirk_list, list) {
+		list_del(&pos->list);
+		kfree(pos);
+	}
+
 	done = true;
 	iounmap(irqc);
 	return 0;
@@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
 
 static int __init rcar_gen2_regulator_quirk(void)
 {
-	u32 mon;
+	struct device_node *np;
+	const struct of_device_id *id;
+	struct regulator_quirk *quirk;
+	struct regulator_quirk *pos;
+	struct of_phandle_args *argsa, *argsb;
+	u32 mon, addr;
+	int ret;
 
 	if (!of_machine_is_compatible("renesas,koelsch") &&
 	    !of_machine_is_compatible("renesas,lager") &&
@@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
 	    !of_machine_is_compatible("renesas,gose"))
 		return -ENODEV;
 
+	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
+		if (!np || !of_device_is_available(np))
+			break;
+
+		quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
+
+		argsa = &quirk->irq_args;
+		memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
+
+		ret = of_property_read_u32(np, "reg", &addr);
+		if (ret)
+			return ret;
+
+		quirk->id = id;
+		quirk->i2c_msg.addr = addr;
+		quirk->shared = false;
+
+		ret = of_irq_parse_one(np, 0, &quirk->irq_args);
+		if (ret)
+			return ret;
+
+		list_for_each_entry(pos, &quirk_list, list) {
+			argsb = &pos->irq_args;
+
+			if (argsa->args_count != argsb->args_count)
+				continue;
+
+			ret = memcmp(argsa->args, argsb->args,
+				     argsa->args_count *
+				     sizeof(argsa->args[0]));
+			if (!ret) {
+				pos->shared = true;
+				quirk->shared = true;
+			}
+		}
+
+		list_add_tail(&quirk->list, &quirk_list);
+	}
+
 	irqc = ioremap(IRQC_BASE, PAGE_SIZE);
 	if (!irqc)
 		return -ENOMEM;
-- 
2.16.2

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

Rather than hard-coding the quirk topology, which stopped scaling,
parse the information from DT. The code looks for all compatible
PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
to the same pin. If so, the code sends a matching sequence to the
PMIC to deassert the IRQ.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-renesas-soc at vger.kernel.org
---
V2: - Replace the DT shared IRQ check loop with memcmp()
    - Send the I2C message to deassert the IRQ line to all PMICs
      in the list with shared IRQ line instead of just one
    - Add comment that this works only in case all the PMICs are
      on the same I2C bus
V3: - Drop the addr = 0x00 init
    - Drop reinit of argsa in rcar_gen2_regulator_quirk
---
 arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++-----
 1 file changed, 90 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 93f628acfd94..b919073aa27e 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -31,8 +31,10 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/list.h>
 #include <linux/notifier.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/mfd/da9063/registers.h>
 
 
@@ -44,34 +46,45 @@
 /* start of DA9210 System Control and Event Registers */
 #define DA9210_REG_MASK_A		0x54
 
+struct regulator_quirk {
+	struct list_head		list;
+	const struct of_device_id	*id;
+	struct of_phandle_args		irq_args;
+	struct i2c_msg			i2c_msg;
+	bool				shared;	/* IRQ line is shared */
+};
+
+static LIST_HEAD(quirk_list);
 static void __iomem *irqc;
 
 /* first byte sets the memory pointer, following are consecutive reg values */
 static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
 static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
 
-static struct i2c_msg da9xxx_msgs[3] = {
-	{
-		.addr = 0x58,
-		.len = ARRAY_SIZE(da9063_irq_clr),
-		.buf = da9063_irq_clr,
-	}, {
-		.addr = 0x68,
-		.len = ARRAY_SIZE(da9210_irq_clr),
-		.buf = da9210_irq_clr,
-	}, {
-		.addr = 0x70,
-		.len = ARRAY_SIZE(da9210_irq_clr),
-		.buf = da9210_irq_clr,
-	},
+static struct i2c_msg da9063_msgs = {
+	.len = ARRAY_SIZE(da9063_irq_clr),
+	.buf = da9063_irq_clr,
+};
+
+static struct i2c_msg da9210_msgs = {
+	.len = ARRAY_SIZE(da9210_irq_clr),
+	.buf = da9210_irq_clr,
+};
+
+static const struct of_device_id rcar_gen2_quirk_match[] = {
+	{ .compatible = "dlg,da9063", .data = &da9063_msgs },
+	{ .compatible = "dlg,da9210", .data = &da9210_msgs },
+	{},
 };
 
 static int regulator_quirk_notify(struct notifier_block *nb,
 				  unsigned long action, void *data)
 {
+	struct regulator_quirk *pos, *tmp;
 	struct device *dev = data;
 	struct i2c_client *client;
 	static bool done;
+	int ret;
 	u32 mon;
 
 	if (done)
@@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb,
 	client = to_i2c_client(dev);
 	dev_dbg(dev, "Detected %s\n", client->name);
 
-	if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
-	    (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
-	    (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
-		int ret, len;
+	/*
+	 * Send message to all PMICs that share an IRQ line to deassert it.
+	 *
+	 * WARNING: This works only if all the PMICs are on the same I2C bus.
+	 */
+	list_for_each_entry(pos, &quirk_list, list) {
+		if (!pos->shared)
+			continue;
 
-		/* There are two DA9210 on Stout, one on the other boards. */
-		len = of_machine_is_compatible("renesas,stout") ? 3 : 2;
+		dev_info(&client->dev, "clearing %s at 0x%02x interrupts\n",
+			 pos->id->compatible, pos->i2c_msg.addr);
 
-		dev_info(&client->dev, "clearing da9063/da9210 interrupts\n");
-		ret = i2c_transfer(client->adapter, da9xxx_msgs, len);
-		if (ret != len)
+		ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1);
+		if (ret != 1)
 			dev_err(&client->dev, "i2c error %d\n", ret);
 	}
 
@@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb,
 remove:
 	dev_info(dev, "IRQ2 is not asserted, removing quirk\n");
 
+	list_for_each_entry_safe(pos, tmp, &quirk_list, list) {
+		list_del(&pos->list);
+		kfree(pos);
+	}
+
 	done = true;
 	iounmap(irqc);
 	return 0;
@@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
 
 static int __init rcar_gen2_regulator_quirk(void)
 {
-	u32 mon;
+	struct device_node *np;
+	const struct of_device_id *id;
+	struct regulator_quirk *quirk;
+	struct regulator_quirk *pos;
+	struct of_phandle_args *argsa, *argsb;
+	u32 mon, addr;
+	int ret;
 
 	if (!of_machine_is_compatible("renesas,koelsch") &&
 	    !of_machine_is_compatible("renesas,lager") &&
@@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
 	    !of_machine_is_compatible("renesas,gose"))
 		return -ENODEV;
 
+	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
+		if (!np || !of_device_is_available(np))
+			break;
+
+		quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
+
+		argsa = &quirk->irq_args;
+		memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
+
+		ret = of_property_read_u32(np, "reg", &addr);
+		if (ret)
+			return ret;
+
+		quirk->id = id;
+		quirk->i2c_msg.addr = addr;
+		quirk->shared = false;
+
+		ret = of_irq_parse_one(np, 0, &quirk->irq_args);
+		if (ret)
+			return ret;
+
+		list_for_each_entry(pos, &quirk_list, list) {
+			argsb = &pos->irq_args;
+
+			if (argsa->args_count != argsb->args_count)
+				continue;
+
+			ret = memcmp(argsa->args, argsb->args,
+				     argsa->args_count *
+				     sizeof(argsa->args[0]));
+			if (!ret) {
+				pos->shared = true;
+				quirk->shared = true;
+			}
+		}
+
+		list_add_tail(&quirk->list, &quirk_list);
+	}
+
 	irqc = ioremap(IRQC_BASE, PAGE_SIZE);
 	if (!irqc)
 		return -ENOMEM;
-- 
2.16.2

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Simon Horman]

On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
> Rather than hard-coding the quirk topology, which stopped scaling,
> parse the information from DT. The code looks for all compatible
> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> to the same pin. If so, the code sends a matching sequence to the
> PMIC to deassert the IRQ.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: - Replace the DT shared IRQ check loop with memcmp()
>     - Send the I2C message to deassert the IRQ line to all PMICs
>       in the list with shared IRQ line instead of just one
>     - Add comment that this works only in case all the PMICs are
>       on the same I2C bus
> V3: - Drop the addr = 0x00 init
>     - Drop reinit of argsa in rcar_gen2_regulator_quirk
> ---
>  arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++-----
>  1 file changed, 90 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> index 93f628acfd94..b919073aa27e 100644
> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> @@ -31,8 +31,10 @@
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/list.h>
>  #include <linux/notifier.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/mfd/da9063/registers.h>
>  
>  
> @@ -44,34 +46,45 @@
>  /* start of DA9210 System Control and Event Registers */
>  #define DA9210_REG_MASK_A		0x54
>  
> +struct regulator_quirk {
> +	struct list_head		list;
> +	const struct of_device_id	*id;
> +	struct of_phandle_args		irq_args;
> +	struct i2c_msg			i2c_msg;
> +	bool				shared;	/* IRQ line is shared */
> +};
> +
> +static LIST_HEAD(quirk_list);
>  static void __iomem *irqc;
>  
>  /* first byte sets the memory pointer, following are consecutive reg values */
>  static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
>  static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
>  
> -static struct i2c_msg da9xxx_msgs[3] = {
> -	{
> -		.addr = 0x58,
> -		.len = ARRAY_SIZE(da9063_irq_clr),
> -		.buf = da9063_irq_clr,
> -	}, {
> -		.addr = 0x68,
> -		.len = ARRAY_SIZE(da9210_irq_clr),
> -		.buf = da9210_irq_clr,
> -	}, {
> -		.addr = 0x70,
> -		.len = ARRAY_SIZE(da9210_irq_clr),
> -		.buf = da9210_irq_clr,
> -	},
> +static struct i2c_msg da9063_msgs = {
> +	.len = ARRAY_SIZE(da9063_irq_clr),
> +	.buf = da9063_irq_clr,
> +};
> +
> +static struct i2c_msg da9210_msgs = {
> +	.len = ARRAY_SIZE(da9210_irq_clr),
> +	.buf = da9210_irq_clr,
> +};
> +
> +static const struct of_device_id rcar_gen2_quirk_match[] = {
> +	{ .compatible = "dlg,da9063", .data = &da9063_msgs },
> +	{ .compatible = "dlg,da9210", .data = &da9210_msgs },
> +	{},
>  };
>  
>  static int regulator_quirk_notify(struct notifier_block *nb,
>  				  unsigned long action, void *data)
>  {
> +	struct regulator_quirk *pos, *tmp;
>  	struct device *dev = data;
>  	struct i2c_client *client;
>  	static bool done;
> +	int ret;
>  	u32 mon;
>  
>  	if (done)
> @@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>  	client = to_i2c_client(dev);
>  	dev_dbg(dev, "Detected %s\n", client->name);
>  
> -	if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
> -	    (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
> -	    (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
> -		int ret, len;
> +	/*
> +	 * Send message to all PMICs that share an IRQ line to deassert it.
> +	 *
> +	 * WARNING: This works only if all the PMICs are on the same I2C bus.
> +	 */
> +	list_for_each_entry(pos, &quirk_list, list) {
> +		if (!pos->shared)
> +			continue;
>  
> -		/* There are two DA9210 on Stout, one on the other boards. */
> -		len = of_machine_is_compatible("renesas,stout") ? 3 : 2;
> +		dev_info(&client->dev, "clearing %s@0x%02x interrupts\n",
> +			 pos->id->compatible, pos->i2c_msg.addr);
>  
> -		dev_info(&client->dev, "clearing da9063/da9210 interrupts\n");
> -		ret = i2c_transfer(client->adapter, da9xxx_msgs, len);
> -		if (ret != len)
> +		ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1);
> +		if (ret != 1)
>  			dev_err(&client->dev, "i2c error %d\n", ret);
>  	}
>  
> @@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>  remove:
>  	dev_info(dev, "IRQ2 is not asserted, removing quirk\n");
>  
> +	list_for_each_entry_safe(pos, tmp, &quirk_list, list) {
> +		list_del(&pos->list);
> +		kfree(pos);
> +	}
> +
>  	done = true;
>  	iounmap(irqc);
>  	return 0;
> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>  
>  static int __init rcar_gen2_regulator_quirk(void)
>  {
> -	u32 mon;
> +	struct device_node *np;
> +	const struct of_device_id *id;
> +	struct regulator_quirk *quirk;
> +	struct regulator_quirk *pos;
> +	struct of_phandle_args *argsa, *argsb;
> +	u32 mon, addr;
> +	int ret;
>  
>  	if (!of_machine_is_compatible("renesas,koelsch") &&
>  	    !of_machine_is_compatible("renesas,lager") &&
> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>  	    !of_machine_is_compatible("renesas,gose"))
>  		return -ENODEV;
>  
> +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
> +		if (!np || !of_device_is_available(np))
> +			break;
> +
> +		quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
> +
> +		argsa = &quirk->irq_args;
> +		memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
> +
> +		ret = of_property_read_u32(np, "reg", &addr);
> +		if (ret)
> +			return ret;
> +
> +		quirk->id = id;
> +		quirk->i2c_msg.addr = addr;
> +		quirk->shared = false;
> +
> +		ret = of_irq_parse_one(np, 0, &quirk->irq_args);

As per my comment on v2,
&quirk->irq_args is assigned to argsa above and used directly here.

> +		if (ret)
> +			return ret;
> +
> +		list_for_each_entry(pos, &quirk_list, list) {
> +			argsb = &pos->irq_args;
> +
> +			if (argsa->args_count != argsb->args_count)
> +				continue;
> +
> +			ret = memcmp(argsa->args, argsb->args,
> +				     argsa->args_count *
> +				     sizeof(argsa->args[0]));
> +			if (!ret) {
> +				pos->shared = true;
> +				quirk->shared = true;
> +			}
> +		}
> +
> +		list_add_tail(&quirk->list, &quirk_list);
> +	}
> +
>  	irqc = ioremap(IRQC_BASE, PAGE_SIZE);
>  	if (!irqc)
>  		return -ENOMEM;
> -- 
> 2.16.2
> 

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Simon Horman]

On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
> Rather than hard-coding the quirk topology, which stopped scaling,
> parse the information from DT. The code looks for all compatible
> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> to the same pin. If so, the code sends a matching sequence to the
> PMIC to deassert the IRQ.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: linux-renesas-soc at vger.kernel.org
> ---
> V2: - Replace the DT shared IRQ check loop with memcmp()
>     - Send the I2C message to deassert the IRQ line to all PMICs
>       in the list with shared IRQ line instead of just one
>     - Add comment that this works only in case all the PMICs are
>       on the same I2C bus
> V3: - Drop the addr = 0x00 init
>     - Drop reinit of argsa in rcar_gen2_regulator_quirk
> ---
>  arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++-----
>  1 file changed, 90 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> index 93f628acfd94..b919073aa27e 100644
> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> @@ -31,8 +31,10 @@
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/list.h>
>  #include <linux/notifier.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/mfd/da9063/registers.h>
>  
>  
> @@ -44,34 +46,45 @@
>  /* start of DA9210 System Control and Event Registers */
>  #define DA9210_REG_MASK_A		0x54
>  
> +struct regulator_quirk {
> +	struct list_head		list;
> +	const struct of_device_id	*id;
> +	struct of_phandle_args		irq_args;
> +	struct i2c_msg			i2c_msg;
> +	bool				shared;	/* IRQ line is shared */
> +};
> +
> +static LIST_HEAD(quirk_list);
>  static void __iomem *irqc;
>  
>  /* first byte sets the memory pointer, following are consecutive reg values */
>  static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
>  static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
>  
> -static struct i2c_msg da9xxx_msgs[3] = {
> -	{
> -		.addr = 0x58,
> -		.len = ARRAY_SIZE(da9063_irq_clr),
> -		.buf = da9063_irq_clr,
> -	}, {
> -		.addr = 0x68,
> -		.len = ARRAY_SIZE(da9210_irq_clr),
> -		.buf = da9210_irq_clr,
> -	}, {
> -		.addr = 0x70,
> -		.len = ARRAY_SIZE(da9210_irq_clr),
> -		.buf = da9210_irq_clr,
> -	},
> +static struct i2c_msg da9063_msgs = {
> +	.len = ARRAY_SIZE(da9063_irq_clr),
> +	.buf = da9063_irq_clr,
> +};
> +
> +static struct i2c_msg da9210_msgs = {
> +	.len = ARRAY_SIZE(da9210_irq_clr),
> +	.buf = da9210_irq_clr,
> +};
> +
> +static const struct of_device_id rcar_gen2_quirk_match[] = {
> +	{ .compatible = "dlg,da9063", .data = &da9063_msgs },
> +	{ .compatible = "dlg,da9210", .data = &da9210_msgs },
> +	{},
>  };
>  
>  static int regulator_quirk_notify(struct notifier_block *nb,
>  				  unsigned long action, void *data)
>  {
> +	struct regulator_quirk *pos, *tmp;
>  	struct device *dev = data;
>  	struct i2c_client *client;
>  	static bool done;
> +	int ret;
>  	u32 mon;
>  
>  	if (done)
> @@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>  	client = to_i2c_client(dev);
>  	dev_dbg(dev, "Detected %s\n", client->name);
>  
> -	if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
> -	    (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
> -	    (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
> -		int ret, len;
> +	/*
> +	 * Send message to all PMICs that share an IRQ line to deassert it.
> +	 *
> +	 * WARNING: This works only if all the PMICs are on the same I2C bus.
> +	 */
> +	list_for_each_entry(pos, &quirk_list, list) {
> +		if (!pos->shared)
> +			continue;
>  
> -		/* There are two DA9210 on Stout, one on the other boards. */
> -		len = of_machine_is_compatible("renesas,stout") ? 3 : 2;
> +		dev_info(&client->dev, "clearing %s at 0x%02x interrupts\n",
> +			 pos->id->compatible, pos->i2c_msg.addr);
>  
> -		dev_info(&client->dev, "clearing da9063/da9210 interrupts\n");
> -		ret = i2c_transfer(client->adapter, da9xxx_msgs, len);
> -		if (ret != len)
> +		ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1);
> +		if (ret != 1)
>  			dev_err(&client->dev, "i2c error %d\n", ret);
>  	}
>  
> @@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>  remove:
>  	dev_info(dev, "IRQ2 is not asserted, removing quirk\n");
>  
> +	list_for_each_entry_safe(pos, tmp, &quirk_list, list) {
> +		list_del(&pos->list);
> +		kfree(pos);
> +	}
> +
>  	done = true;
>  	iounmap(irqc);
>  	return 0;
> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>  
>  static int __init rcar_gen2_regulator_quirk(void)
>  {
> -	u32 mon;
> +	struct device_node *np;
> +	const struct of_device_id *id;
> +	struct regulator_quirk *quirk;
> +	struct regulator_quirk *pos;
> +	struct of_phandle_args *argsa, *argsb;
> +	u32 mon, addr;
> +	int ret;
>  
>  	if (!of_machine_is_compatible("renesas,koelsch") &&
>  	    !of_machine_is_compatible("renesas,lager") &&
> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>  	    !of_machine_is_compatible("renesas,gose"))
>  		return -ENODEV;
>  
> +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
> +		if (!np || !of_device_is_available(np))
> +			break;
> +
> +		quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
> +
> +		argsa = &quirk->irq_args;
> +		memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
> +
> +		ret = of_property_read_u32(np, "reg", &addr);
> +		if (ret)
> +			return ret;
> +
> +		quirk->id = id;
> +		quirk->i2c_msg.addr = addr;
> +		quirk->shared = false;
> +
> +		ret = of_irq_parse_one(np, 0, &quirk->irq_args);

As per my comment on v2,
&quirk->irq_args is assigned to argsa above and used directly here.

> +		if (ret)
> +			return ret;
> +
> +		list_for_each_entry(pos, &quirk_list, list) {
> +			argsb = &pos->irq_args;
> +
> +			if (argsa->args_count != argsb->args_count)
> +				continue;
> +
> +			ret = memcmp(argsa->args, argsb->args,
> +				     argsa->args_count *
> +				     sizeof(argsa->args[0]));
> +			if (!ret) {
> +				pos->shared = true;
> +				quirk->shared = true;
> +			}
> +		}
> +
> +		list_add_tail(&quirk->list, &quirk_list);
> +	}
> +
>  	irqc = ioremap(IRQC_BASE, PAGE_SIZE);
>  	if (!irqc)
>  		return -ENOMEM;
> -- 
> 2.16.2
> 

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

Hi Marek,

On Tue, Jun 05, 2018 at 10:07:28AM +0200, Simon Horman wrote:
> On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
> > Rather than hard-coding the quirk topology, which stopped scaling,
> > parse the information from DT. The code looks for all compatible
> > PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> > to the same pin. If so, the code sends a matching sequence to the
> > PMIC to deassert the IRQ.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > Cc: Simon Horman <horms+renesas@verge.net.au>
> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>

From an I2C point of view:

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Minor nits:

> > @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
> >  
> >  static int __init rcar_gen2_regulator_quirk(void)
> >  {
> > -	u32 mon;
> > +	struct device_node *np;
> > +	const struct of_device_id *id;
> > +	struct regulator_quirk *quirk;
> > +	struct regulator_quirk *pos;

Merge the last two lines into one?

> > +	struct of_phandle_args *argsa, *argsb;
> > +	u32 mon, addr;
> > +	int ret;
> >  
> >  	if (!of_machine_is_compatible("renesas,koelsch") &&
> >  	    !of_machine_is_compatible("renesas,lager") &&
> > @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
> >  	    !of_machine_is_compatible("renesas,gose"))
> >  		return -ENODEV;
> >  
> > +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
> > +		if (!np || !of_device_is_available(np))

Can '!np' actually happen? This is the exit condition of the for-loop,
or am I overlooking something?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Wolfram Sang]

Hi Marek,

On Tue, Jun 05, 2018 at 10:07:28AM +0200, Simon Horman wrote:
> On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
> > Rather than hard-coding the quirk topology, which stopped scaling,
> > parse the information from DT. The code looks for all compatible
> > PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> > to the same pin. If so, the code sends a matching sequence to the
> > PMIC to deassert the IRQ.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > Cc: Simon Horman <horms+renesas@verge.net.au>
> > Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>

>From an I2C point of view:

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Minor nits:

> > @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
> >  
> >  static int __init rcar_gen2_regulator_quirk(void)
> >  {
> > -	u32 mon;
> > +	struct device_node *np;
> > +	const struct of_device_id *id;
> > +	struct regulator_quirk *quirk;
> > +	struct regulator_quirk *pos;

Merge the last two lines into one?

> > +	struct of_phandle_args *argsa, *argsb;
> > +	u32 mon, addr;
> > +	int ret;
> >  
> >  	if (!of_machine_is_compatible("renesas,koelsch") &&
> >  	    !of_machine_is_compatible("renesas,lager") &&
> > @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
> >  	    !of_machine_is_compatible("renesas,gose"))
> >  		return -ENODEV;
> >  
> > +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
> > +		if (!np || !of_device_is_available(np))

Can '!np' actually happen? This is the exit condition of the for-loop,
or am I overlooking something?

Regards,

   Wolfram

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180605/35898b42/attachment.sig>

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/05/2018 10:07 AM, Simon Horman wrote:
> On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
>> Rather than hard-coding the quirk topology, which stopped scaling,
>> parse the information from DT. The code looks for all compatible
>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>> to the same pin. If so, the code sends a matching sequence to the
>> PMIC to deassert the IRQ.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>> V2: - Replace the DT shared IRQ check loop with memcmp()
>>     - Send the I2C message to deassert the IRQ line to all PMICs
>>       in the list with shared IRQ line instead of just one
>>     - Add comment that this works only in case all the PMICs are
>>       on the same I2C bus
>> V3: - Drop the addr = 0x00 init
>>     - Drop reinit of argsa in rcar_gen2_regulator_quirk
>> ---
>>  arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++-----
>>  1 file changed, 90 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> index 93f628acfd94..b919073aa27e 100644
>> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> @@ -31,8 +31,10 @@
>>  #include <linux/i2c.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>> +#include <linux/list.h>
>>  #include <linux/notifier.h>
>>  #include <linux/of.h>
>> +#include <linux/of_irq.h>
>>  #include <linux/mfd/da9063/registers.h>
>>  
>>  
>> @@ -44,34 +46,45 @@
>>  /* start of DA9210 System Control and Event Registers */
>>  #define DA9210_REG_MASK_A		0x54
>>  
>> +struct regulator_quirk {
>> +	struct list_head		list;
>> +	const struct of_device_id	*id;
>> +	struct of_phandle_args		irq_args;
>> +	struct i2c_msg			i2c_msg;
>> +	bool				shared;	/* IRQ line is shared */
>> +};
>> +
>> +static LIST_HEAD(quirk_list);
>>  static void __iomem *irqc;
>>  
>>  /* first byte sets the memory pointer, following are consecutive reg values */
>>  static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
>>  static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
>>  
>> -static struct i2c_msg da9xxx_msgs[3] = {
>> -	{
>> -		.addr = 0x58,
>> -		.len = ARRAY_SIZE(da9063_irq_clr),
>> -		.buf = da9063_irq_clr,
>> -	}, {
>> -		.addr = 0x68,
>> -		.len = ARRAY_SIZE(da9210_irq_clr),
>> -		.buf = da9210_irq_clr,
>> -	}, {
>> -		.addr = 0x70,
>> -		.len = ARRAY_SIZE(da9210_irq_clr),
>> -		.buf = da9210_irq_clr,
>> -	},
>> +static struct i2c_msg da9063_msgs = {
>> +	.len = ARRAY_SIZE(da9063_irq_clr),
>> +	.buf = da9063_irq_clr,
>> +};
>> +
>> +static struct i2c_msg da9210_msgs = {
>> +	.len = ARRAY_SIZE(da9210_irq_clr),
>> +	.buf = da9210_irq_clr,
>> +};
>> +
>> +static const struct of_device_id rcar_gen2_quirk_match[] = {
>> +	{ .compatible = "dlg,da9063", .data = &da9063_msgs },
>> +	{ .compatible = "dlg,da9210", .data = &da9210_msgs },
>> +	{},
>>  };
>>  
>>  static int regulator_quirk_notify(struct notifier_block *nb,
>>  				  unsigned long action, void *data)
>>  {
>> +	struct regulator_quirk *pos, *tmp;
>>  	struct device *dev = data;
>>  	struct i2c_client *client;
>>  	static bool done;
>> +	int ret;
>>  	u32 mon;
>>  
>>  	if (done)
>> @@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>>  	client = to_i2c_client(dev);
>>  	dev_dbg(dev, "Detected %s\n", client->name);
>>  
>> -	if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
>> -	    (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
>> -	    (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
>> -		int ret, len;
>> +	/*
>> +	 * Send message to all PMICs that share an IRQ line to deassert it.
>> +	 *
>> +	 * WARNING: This works only if all the PMICs are on the same I2C bus.
>> +	 */
>> +	list_for_each_entry(pos, &quirk_list, list) {
>> +		if (!pos->shared)
>> +			continue;
>>  
>> -		/* There are two DA9210 on Stout, one on the other boards. */
>> -		len = of_machine_is_compatible("renesas,stout") ? 3 : 2;
>> +		dev_info(&client->dev, "clearing %s@0x%02x interrupts\n",
>> +			 pos->id->compatible, pos->i2c_msg.addr);
>>  
>> -		dev_info(&client->dev, "clearing da9063/da9210 interrupts\n");
>> -		ret = i2c_transfer(client->adapter, da9xxx_msgs, len);
>> -		if (ret != len)
>> +		ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1);
>> +		if (ret != 1)
>>  			dev_err(&client->dev, "i2c error %d\n", ret);
>>  	}
>>  
>> @@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>>  remove:
>>  	dev_info(dev, "IRQ2 is not asserted, removing quirk\n");
>>  
>> +	list_for_each_entry_safe(pos, tmp, &quirk_list, list) {
>> +		list_del(&pos->list);
>> +		kfree(pos);
>> +	}
>> +
>>  	done = true;
>>  	iounmap(irqc);
>>  	return 0;
>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>  
>>  static int __init rcar_gen2_regulator_quirk(void)
>>  {
>> -	u32 mon;
>> +	struct device_node *np;
>> +	const struct of_device_id *id;
>> +	struct regulator_quirk *quirk;
>> +	struct regulator_quirk *pos;
>> +	struct of_phandle_args *argsa, *argsb;
>> +	u32 mon, addr;
>> +	int ret;
>>  
>>  	if (!of_machine_is_compatible("renesas,koelsch") &&
>>  	    !of_machine_is_compatible("renesas,lager") &&
>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>  	    !of_machine_is_compatible("renesas,gose"))
>>  		return -ENODEV;
>>  
>> +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
>> +		if (!np || !of_device_is_available(np))
>> +			break;
>> +
>> +		quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
>> +
>> +		argsa = &quirk->irq_args;
>> +		memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
>> +
>> +		ret = of_property_read_u32(np, "reg", &addr);
>> +		if (ret)
>> +			return ret;
>> +
>> +		quirk->id = id;
>> +		quirk->i2c_msg.addr = addr;
>> +		quirk->shared = false;
>> +
>> +		ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> 
> As per my comment on v2,
> &quirk->irq_args is assigned to argsa above and used directly here.
> 

Hum, OK

-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/05/2018 10:07 AM, Simon Horman wrote:
> On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
>> Rather than hard-coding the quirk topology, which stopped scaling,
>> parse the information from DT. The code looks for all compatible
>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>> to the same pin. If so, the code sends a matching sequence to the
>> PMIC to deassert the IRQ.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Cc: linux-renesas-soc at vger.kernel.org
>> ---
>> V2: - Replace the DT shared IRQ check loop with memcmp()
>>     - Send the I2C message to deassert the IRQ line to all PMICs
>>       in the list with shared IRQ line instead of just one
>>     - Add comment that this works only in case all the PMICs are
>>       on the same I2C bus
>> V3: - Drop the addr = 0x00 init
>>     - Drop reinit of argsa in rcar_gen2_regulator_quirk
>> ---
>>  arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c | 114 ++++++++++++++++-----
>>  1 file changed, 90 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> index 93f628acfd94..b919073aa27e 100644
>> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> @@ -31,8 +31,10 @@
>>  #include <linux/i2c.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>> +#include <linux/list.h>
>>  #include <linux/notifier.h>
>>  #include <linux/of.h>
>> +#include <linux/of_irq.h>
>>  #include <linux/mfd/da9063/registers.h>
>>  
>>  
>> @@ -44,34 +46,45 @@
>>  /* start of DA9210 System Control and Event Registers */
>>  #define DA9210_REG_MASK_A		0x54
>>  
>> +struct regulator_quirk {
>> +	struct list_head		list;
>> +	const struct of_device_id	*id;
>> +	struct of_phandle_args		irq_args;
>> +	struct i2c_msg			i2c_msg;
>> +	bool				shared;	/* IRQ line is shared */
>> +};
>> +
>> +static LIST_HEAD(quirk_list);
>>  static void __iomem *irqc;
>>  
>>  /* first byte sets the memory pointer, following are consecutive reg values */
>>  static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
>>  static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
>>  
>> -static struct i2c_msg da9xxx_msgs[3] = {
>> -	{
>> -		.addr = 0x58,
>> -		.len = ARRAY_SIZE(da9063_irq_clr),
>> -		.buf = da9063_irq_clr,
>> -	}, {
>> -		.addr = 0x68,
>> -		.len = ARRAY_SIZE(da9210_irq_clr),
>> -		.buf = da9210_irq_clr,
>> -	}, {
>> -		.addr = 0x70,
>> -		.len = ARRAY_SIZE(da9210_irq_clr),
>> -		.buf = da9210_irq_clr,
>> -	},
>> +static struct i2c_msg da9063_msgs = {
>> +	.len = ARRAY_SIZE(da9063_irq_clr),
>> +	.buf = da9063_irq_clr,
>> +};
>> +
>> +static struct i2c_msg da9210_msgs = {
>> +	.len = ARRAY_SIZE(da9210_irq_clr),
>> +	.buf = da9210_irq_clr,
>> +};
>> +
>> +static const struct of_device_id rcar_gen2_quirk_match[] = {
>> +	{ .compatible = "dlg,da9063", .data = &da9063_msgs },
>> +	{ .compatible = "dlg,da9210", .data = &da9210_msgs },
>> +	{},
>>  };
>>  
>>  static int regulator_quirk_notify(struct notifier_block *nb,
>>  				  unsigned long action, void *data)
>>  {
>> +	struct regulator_quirk *pos, *tmp;
>>  	struct device *dev = data;
>>  	struct i2c_client *client;
>>  	static bool done;
>> +	int ret;
>>  	u32 mon;
>>  
>>  	if (done)
>> @@ -88,17 +101,20 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>>  	client = to_i2c_client(dev);
>>  	dev_dbg(dev, "Detected %s\n", client->name);
>>  
>> -	if ((client->addr == 0x58 && !strcmp(client->name, "da9063")) ||
>> -	    (client->addr == 0x68 && !strcmp(client->name, "da9210")) ||
>> -	    (client->addr == 0x70 && !strcmp(client->name, "da9210"))) {
>> -		int ret, len;
>> +	/*
>> +	 * Send message to all PMICs that share an IRQ line to deassert it.
>> +	 *
>> +	 * WARNING: This works only if all the PMICs are on the same I2C bus.
>> +	 */
>> +	list_for_each_entry(pos, &quirk_list, list) {
>> +		if (!pos->shared)
>> +			continue;
>>  
>> -		/* There are two DA9210 on Stout, one on the other boards. */
>> -		len = of_machine_is_compatible("renesas,stout") ? 3 : 2;
>> +		dev_info(&client->dev, "clearing %s at 0x%02x interrupts\n",
>> +			 pos->id->compatible, pos->i2c_msg.addr);
>>  
>> -		dev_info(&client->dev, "clearing da9063/da9210 interrupts\n");
>> -		ret = i2c_transfer(client->adapter, da9xxx_msgs, len);
>> -		if (ret != len)
>> +		ret = i2c_transfer(client->adapter, &pos->i2c_msg, 1);
>> +		if (ret != 1)
>>  			dev_err(&client->dev, "i2c error %d\n", ret);
>>  	}
>>  
>> @@ -111,6 +127,11 @@ static int regulator_quirk_notify(struct notifier_block *nb,
>>  remove:
>>  	dev_info(dev, "IRQ2 is not asserted, removing quirk\n");
>>  
>> +	list_for_each_entry_safe(pos, tmp, &quirk_list, list) {
>> +		list_del(&pos->list);
>> +		kfree(pos);
>> +	}
>> +
>>  	done = true;
>>  	iounmap(irqc);
>>  	return 0;
>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>  
>>  static int __init rcar_gen2_regulator_quirk(void)
>>  {
>> -	u32 mon;
>> +	struct device_node *np;
>> +	const struct of_device_id *id;
>> +	struct regulator_quirk *quirk;
>> +	struct regulator_quirk *pos;
>> +	struct of_phandle_args *argsa, *argsb;
>> +	u32 mon, addr;
>> +	int ret;
>>  
>>  	if (!of_machine_is_compatible("renesas,koelsch") &&
>>  	    !of_machine_is_compatible("renesas,lager") &&
>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>  	    !of_machine_is_compatible("renesas,gose"))
>>  		return -ENODEV;
>>  
>> +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
>> +		if (!np || !of_device_is_available(np))
>> +			break;
>> +
>> +		quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
>> +
>> +		argsa = &quirk->irq_args;
>> +		memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
>> +
>> +		ret = of_property_read_u32(np, "reg", &addr);
>> +		if (ret)
>> +			return ret;
>> +
>> +		quirk->id = id;
>> +		quirk->i2c_msg.addr = addr;
>> +		quirk->shared = false;
>> +
>> +		ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> 
> As per my comment on v2,
> &quirk->irq_args is assigned to argsa above and used directly here.
> 

Hum, OK

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/05/2018 11:21 AM, Wolfram Sang wrote:
> Hi Marek,
> 
> On Tue, Jun 05, 2018 at 10:07:28AM +0200, Simon Horman wrote:
>> On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>> parse the information from DT. The code looks for all compatible
>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>> to the same pin. If so, the code sends a matching sequence to the
>>> PMIC to deassert the IRQ.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>> Cc: Simon Horman <horms+renesas@verge.net.au>
>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> From an I2C point of view:
> 
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Minor nits:
> 
>>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>>  
>>>  static int __init rcar_gen2_regulator_quirk(void)
>>>  {
>>> -	u32 mon;
>>> +	struct device_node *np;
>>> +	const struct of_device_id *id;
>>> +	struct regulator_quirk *quirk;
>>> +	struct regulator_quirk *pos;
> 
> Merge the last two lines into one?
> 
>>> +	struct of_phandle_args *argsa, *argsb;
>>> +	u32 mon, addr;
>>> +	int ret;
>>>  
>>>  	if (!of_machine_is_compatible("renesas,koelsch") &&
>>>  	    !of_machine_is_compatible("renesas,lager") &&
>>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>>  	    !of_machine_is_compatible("renesas,gose"))
>>>  		return -ENODEV;
>>>  
>>> +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
>>> +		if (!np || !of_device_is_available(np))
> 
> Can '!np' actually happen? This is the exit condition of the for-loop,
> or am I overlooking something?

I had to take a look again, no, it's not needed.

-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/05/2018 11:21 AM, Wolfram Sang wrote:
> Hi Marek,
> 
> On Tue, Jun 05, 2018 at 10:07:28AM +0200, Simon Horman wrote:
>> On Mon, Jun 04, 2018 at 07:59:11PM +0200, Marek Vasut wrote:
>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>> parse the information from DT. The code looks for all compatible
>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>> to the same pin. If so, the code sends a matching sequence to the
>>> PMIC to deassert the IRQ.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>> Cc: Simon Horman <horms+renesas@verge.net.au>
>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> From an I2C point of view:
> 
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Minor nits:
> 
>>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>>  
>>>  static int __init rcar_gen2_regulator_quirk(void)
>>>  {
>>> -	u32 mon;
>>> +	struct device_node *np;
>>> +	const struct of_device_id *id;
>>> +	struct regulator_quirk *quirk;
>>> +	struct regulator_quirk *pos;
> 
> Merge the last two lines into one?
> 
>>> +	struct of_phandle_args *argsa, *argsb;
>>> +	u32 mon, addr;
>>> +	int ret;
>>>  
>>>  	if (!of_machine_is_compatible("renesas,koelsch") &&
>>>  	    !of_machine_is_compatible("renesas,lager") &&
>>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>>  	    !of_machine_is_compatible("renesas,gose"))
>>>  		return -ENODEV;
>>>  
>>> +	for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
>>> +		if (!np || !of_device_is_available(np))
> 
> Can '!np' actually happen? This is the exit condition of the for-loop,
> or am I overlooking something?

I had to take a look again, no, it's not needed.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> Rather than hard-coding the quirk topology, which stopped scaling,
> parse the information from DT. The code looks for all compatible
> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied

da9063

> to the same pin. If so, the code sends a matching sequence to the
> PMIC to deassert the IRQ.

Note that not all R-Car Gen2 boards have all regulators described in DT yet.
E.g. gose lacks da9210. So that has to be fixed first.

>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: - Replace the DT shared IRQ check loop with memcmp()
>     - Send the I2C message to deassert the IRQ line to all PMICs
>       in the list with shared IRQ line instead of just one
>     - Add comment that this works only in case all the PMICs are
>       on the same I2C bus
> V3: - Drop the addr = 0x00 init
>     - Drop reinit of argsa in rcar_gen2_regulator_quirk

Thanks for the update!

> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c

> @@ -44,34 +46,45 @@
>  /* start of DA9210 System Control and Event Registers */
>  #define DA9210_REG_MASK_A              0x54
>
> +struct regulator_quirk {
> +       struct list_head                list;
> +       const struct of_device_id       *id;
> +       struct of_phandle_args          irq_args;
> +       struct i2c_msg                  i2c_msg;
> +       bool                            shared; /* IRQ line is shared */
> +};
> +
> +static LIST_HEAD(quirk_list);
>  static void __iomem *irqc;
>
>  /* first byte sets the memory pointer, following are consecutive reg values */
>  static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
>  static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
>
> -static struct i2c_msg da9xxx_msgs[3] = {
> -       {
> -               .addr = 0x58,
> -               .len = ARRAY_SIZE(da9063_irq_clr),
> -               .buf = da9063_irq_clr,
> -       }, {
> -               .addr = 0x68,
> -               .len = ARRAY_SIZE(da9210_irq_clr),
> -               .buf = da9210_irq_clr,
> -       }, {
> -               .addr = 0x70,
> -               .len = ARRAY_SIZE(da9210_irq_clr),
> -               .buf = da9210_irq_clr,
> -       },
> +static struct i2c_msg da9063_msgs = {

da9063_msg? (it's a single message)

> +       .len = ARRAY_SIZE(da9063_irq_clr),
> +       .buf = da9063_irq_clr,
> +};
> +
> +static struct i2c_msg da9210_msgs = {

da9210_msg?

> +       .len = ARRAY_SIZE(da9210_irq_clr),
> +       .buf = da9210_irq_clr,
> +};

> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>
>  static int __init rcar_gen2_regulator_quirk(void)
>  {
> -       u32 mon;
> +       struct device_node *np;
> +       const struct of_device_id *id;
> +       struct regulator_quirk *quirk;
> +       struct regulator_quirk *pos;
> +       struct of_phandle_args *argsa, *argsb;
> +       u32 mon, addr;
> +       int ret;

Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.

>
>         if (!of_machine_is_compatible("renesas,koelsch") &&
>             !of_machine_is_compatible("renesas,lager") &&
> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>             !of_machine_is_compatible("renesas,gose"))
>                 return -ENODEV;

I think the board checks above can be removed. That will auto-enable the
fix on e.g. Porter (once its regulators have ended up in DTS, of course).

>
> +       for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
> +               if (!np || !of_device_is_available(np))

!np cannot happen

> +                       break;
> +
> +               quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);

Missing NULL check

> +
> +               argsa = &quirk->irq_args;
> +               memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
> +
> +               ret = of_property_read_u32(np, "reg", &addr);
> +               if (ret)
> +                       return ret;

I think it's safer to skip this entry and continue, after calling
kfree(quirk), of course.

> +
> +               quirk->id = id;
> +               quirk->i2c_msg.addr = addr;
> +               quirk->shared = false;

No need to clear shared, it was cleared by kzalloc().

> +
> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> +               if (ret)
> +                       return ret;

kfree(quirk) and continue...

Works fine on Koelsch, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> Rather than hard-coding the quirk topology, which stopped scaling,
> parse the information from DT. The code looks for all compatible
> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied

da9063

> to the same pin. If so, the code sends a matching sequence to the
> PMIC to deassert the IRQ.

Note that not all R-Car Gen2 boards have all regulators described in DT yet.
E.g. gose lacks da9210. So that has to be fixed first.

>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: linux-renesas-soc at vger.kernel.org
> ---
> V2: - Replace the DT shared IRQ check loop with memcmp()
>     - Send the I2C message to deassert the IRQ line to all PMICs
>       in the list with shared IRQ line instead of just one
>     - Add comment that this works only in case all the PMICs are
>       on the same I2C bus
> V3: - Drop the addr = 0x00 init
>     - Drop reinit of argsa in rcar_gen2_regulator_quirk

Thanks for the update!

> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c

> @@ -44,34 +46,45 @@
>  /* start of DA9210 System Control and Event Registers */
>  #define DA9210_REG_MASK_A              0x54
>
> +struct regulator_quirk {
> +       struct list_head                list;
> +       const struct of_device_id       *id;
> +       struct of_phandle_args          irq_args;
> +       struct i2c_msg                  i2c_msg;
> +       bool                            shared; /* IRQ line is shared */
> +};
> +
> +static LIST_HEAD(quirk_list);
>  static void __iomem *irqc;
>
>  /* first byte sets the memory pointer, following are consecutive reg values */
>  static u8 da9063_irq_clr[] = { DA9063_REG_IRQ_MASK_A, 0xff, 0xff, 0xff, 0xff };
>  static u8 da9210_irq_clr[] = { DA9210_REG_MASK_A, 0xff, 0xff };
>
> -static struct i2c_msg da9xxx_msgs[3] = {
> -       {
> -               .addr = 0x58,
> -               .len = ARRAY_SIZE(da9063_irq_clr),
> -               .buf = da9063_irq_clr,
> -       }, {
> -               .addr = 0x68,
> -               .len = ARRAY_SIZE(da9210_irq_clr),
> -               .buf = da9210_irq_clr,
> -       }, {
> -               .addr = 0x70,
> -               .len = ARRAY_SIZE(da9210_irq_clr),
> -               .buf = da9210_irq_clr,
> -       },
> +static struct i2c_msg da9063_msgs = {

da9063_msg? (it's a single message)

> +       .len = ARRAY_SIZE(da9063_irq_clr),
> +       .buf = da9063_irq_clr,
> +};
> +
> +static struct i2c_msg da9210_msgs = {

da9210_msg?

> +       .len = ARRAY_SIZE(da9210_irq_clr),
> +       .buf = da9210_irq_clr,
> +};

> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>
>  static int __init rcar_gen2_regulator_quirk(void)
>  {
> -       u32 mon;
> +       struct device_node *np;
> +       const struct of_device_id *id;
> +       struct regulator_quirk *quirk;
> +       struct regulator_quirk *pos;
> +       struct of_phandle_args *argsa, *argsb;
> +       u32 mon, addr;
> +       int ret;

Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.

>
>         if (!of_machine_is_compatible("renesas,koelsch") &&
>             !of_machine_is_compatible("renesas,lager") &&
> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>             !of_machine_is_compatible("renesas,gose"))
>                 return -ENODEV;

I think the board checks above can be removed. That will auto-enable the
fix on e.g. Porter (once its regulators have ended up in DTS, of course).

>
> +       for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
> +               if (!np || !of_device_is_available(np))

!np cannot happen

> +                       break;
> +
> +               quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);

Missing NULL check

> +
> +               argsa = &quirk->irq_args;
> +               memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
> +
> +               ret = of_property_read_u32(np, "reg", &addr);
> +               if (ret)
> +                       return ret;

I think it's safer to skip this entry and continue, after calling
kfree(quirk), of course.

> +
> +               quirk->id = id;
> +               quirk->i2c_msg.addr = addr;
> +               quirk->shared = false;

No need to clear shared, it was cleared by kzalloc().

> +
> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> +               if (ret)
> +                       return ret;

kfree(quirk) and continue...

Works fine on Koelsch, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> Rather than hard-coding the quirk topology, which stopped scaling,
>> parse the information from DT. The code looks for all compatible
>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> 
> da9063
> 
>> to the same pin. If so, the code sends a matching sequence to the
>> PMIC to deassert the IRQ.
> 
> Note that not all R-Car Gen2 boards have all regulators described in DT yet.
> E.g. gose lacks da9210. So that has to be fixed first.

[...]

> da9210_msg?

Fixed

> 
>> +       .len = ARRAY_SIZE(da9210_irq_clr),
>> +       .buf = da9210_irq_clr,
>> +};
> 
>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>
>>  static int __init rcar_gen2_regulator_quirk(void)
>>  {
>> -       u32 mon;
>> +       struct device_node *np;
>> +       const struct of_device_id *id;
>> +       struct regulator_quirk *quirk;
>> +       struct regulator_quirk *pos;
>> +       struct of_phandle_args *argsa, *argsb;
>> +       u32 mon, addr;
>> +       int ret;
> 
> Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.
> 
>>
>>         if (!of_machine_is_compatible("renesas,koelsch") &&
>>             !of_machine_is_compatible("renesas,lager") &&
>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>             !of_machine_is_compatible("renesas,gose"))
>>                 return -ENODEV;
> 
> I think the board checks above can be removed. That will auto-enable the
> fix on e.g. Porter (once its regulators have ended up in DTS, of course).

Removing the check would also enable it on boards where we don't want
this enabled, so I'd prefer to keep the check to avoid strange surprises.

>>
>> +       for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
>> +               if (!np || !of_device_is_available(np))
> 
> !np cannot happen

Right

>> +                       break;
>> +
>> +               quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
> 
> Missing NULL check
> 
>> +
>> +               argsa = &quirk->irq_args;
>> +               memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
>> +
>> +               ret = of_property_read_u32(np, "reg", &addr);
>> +               if (ret)
>> +                       return ret;
> 
> I think it's safer to skip this entry and continue, after calling
> kfree(quirk), of course.
> 
>> +
>> +               quirk->id = id;
>> +               quirk->i2c_msg.addr = addr;
>> +               quirk->shared = false;
> 
> No need to clear shared, it was cleared by kzalloc().
> 
>> +
>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>> +               if (ret)
>> +                       return ret;
> 
> kfree(quirk) and continue...
>

I wonder if it shouldn't rather free the entire list and abort ?

> Works fine on Koelsch, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> Rather than hard-coding the quirk topology, which stopped scaling,
>> parse the information from DT. The code looks for all compatible
>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> 
> da9063
> 
>> to the same pin. If so, the code sends a matching sequence to the
>> PMIC to deassert the IRQ.
> 
> Note that not all R-Car Gen2 boards have all regulators described in DT yet.
> E.g. gose lacks da9210. So that has to be fixed first.

[...]

> da9210_msg?

Fixed

> 
>> +       .len = ARRAY_SIZE(da9210_irq_clr),
>> +       .buf = da9210_irq_clr,
>> +};
> 
>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>
>>  static int __init rcar_gen2_regulator_quirk(void)
>>  {
>> -       u32 mon;
>> +       struct device_node *np;
>> +       const struct of_device_id *id;
>> +       struct regulator_quirk *quirk;
>> +       struct regulator_quirk *pos;
>> +       struct of_phandle_args *argsa, *argsb;
>> +       u32 mon, addr;
>> +       int ret;
> 
> Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.
> 
>>
>>         if (!of_machine_is_compatible("renesas,koelsch") &&
>>             !of_machine_is_compatible("renesas,lager") &&
>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>             !of_machine_is_compatible("renesas,gose"))
>>                 return -ENODEV;
> 
> I think the board checks above can be removed. That will auto-enable the
> fix on e.g. Porter (once its regulators have ended up in DTS, of course).

Removing the check would also enable it on boards where we don't want
this enabled, so I'd prefer to keep the check to avoid strange surprises.

>>
>> +       for_each_matching_node_and_match(np, rcar_gen2_quirk_match, &id) {
>> +               if (!np || !of_device_is_available(np))
> 
> !np cannot happen

Right

>> +                       break;
>> +
>> +               quirk = kzalloc(sizeof(*quirk), GFP_KERNEL);
> 
> Missing NULL check
> 
>> +
>> +               argsa = &quirk->irq_args;
>> +               memcpy(&quirk->i2c_msg, id->data, sizeof(quirk->i2c_msg));
>> +
>> +               ret = of_property_read_u32(np, "reg", &addr);
>> +               if (ret)
>> +                       return ret;
> 
> I think it's safer to skip this entry and continue, after calling
> kfree(quirk), of course.
> 
>> +
>> +               quirk->id = id;
>> +               quirk->i2c_msg.addr = addr;
>> +               quirk->shared = false;
> 
> No need to clear shared, it was cleared by kzalloc().
> 
>> +
>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>> +               if (ret)
>> +                       return ret;
> 
> kfree(quirk) and continue...
>

I wonder if it shouldn't rather free the entire list and abort ?

> Works fine on Koelsch, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> > On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> Rather than hard-coding the quirk topology, which stopped scaling,
> >> parse the information from DT. The code looks for all compatible
> >> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >
> > da9063
> >
> >> to the same pin. If so, the code sends a matching sequence to the
> >> PMIC to deassert the IRQ.

> >> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
> >>
> >>  static int __init rcar_gen2_regulator_quirk(void)
> >>  {
> >> -       u32 mon;
> >> +       struct device_node *np;
> >> +       const struct of_device_id *id;
> >> +       struct regulator_quirk *quirk;
> >> +       struct regulator_quirk *pos;
> >> +       struct of_phandle_args *argsa, *argsb;
> >> +       u32 mon, addr;
> >> +       int ret;
> >
> > Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.
> >
> >>
> >>         if (!of_machine_is_compatible("renesas,koelsch") &&
> >>             !of_machine_is_compatible("renesas,lager") &&
> >> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
> >>             !of_machine_is_compatible("renesas,gose"))
> >>                 return -ENODEV;
> >
> > I think the board checks above can be removed. That will auto-enable the
> > fix on e.g. Porter (once its regulators have ended up in DTS, of course).
>
> Removing the check would also enable it on boards where we don't want
> this enabled, so I'd prefer to keep the check to avoid strange surprises.

Like, Porter? ;-)

> >> +               ret = of_property_read_u32(np, "reg", &addr);
> >> +               if (ret)
> >> +                       return ret;
> >
> > I think it's safer to skip this entry and continue, after calling
> > kfree(quirk), of course.
> >
> >> +
> >> +               quirk->id = id;
> >> +               quirk->i2c_msg.addr = addr;
> >> +               quirk->shared = false;
> >> +
> >> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >> +               if (ret)
> >> +                       return ret;
> >
> > kfree(quirk) and continue...
>
> I wonder if it shouldn't rather free the entire list and abort ?

"Be strict when sending, be liberal when receiving."

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> > On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> Rather than hard-coding the quirk topology, which stopped scaling,
> >> parse the information from DT. The code looks for all compatible
> >> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >
> > da9063
> >
> >> to the same pin. If so, the code sends a matching sequence to the
> >> PMIC to deassert the IRQ.

> >> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
> >>
> >>  static int __init rcar_gen2_regulator_quirk(void)
> >>  {
> >> -       u32 mon;
> >> +       struct device_node *np;
> >> +       const struct of_device_id *id;
> >> +       struct regulator_quirk *quirk;
> >> +       struct regulator_quirk *pos;
> >> +       struct of_phandle_args *argsa, *argsb;
> >> +       u32 mon, addr;
> >> +       int ret;
> >
> > Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.
> >
> >>
> >>         if (!of_machine_is_compatible("renesas,koelsch") &&
> >>             !of_machine_is_compatible("renesas,lager") &&
> >> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
> >>             !of_machine_is_compatible("renesas,gose"))
> >>                 return -ENODEV;
> >
> > I think the board checks above can be removed. That will auto-enable the
> > fix on e.g. Porter (once its regulators have ended up in DTS, of course).
>
> Removing the check would also enable it on boards where we don't want
> this enabled, so I'd prefer to keep the check to avoid strange surprises.

Like, Porter? ;-)

> >> +               ret = of_property_read_u32(np, "reg", &addr);
> >> +               if (ret)
> >> +                       return ret;
> >
> > I think it's safer to skip this entry and continue, after calling
> > kfree(quirk), of course.
> >
> >> +
> >> +               quirk->id = id;
> >> +               quirk->i2c_msg.addr = addr;
> >> +               quirk->shared = false;
> >> +
> >> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >> +               if (ret)
> >> +                       return ret;
> >
> > kfree(quirk) and continue...
>
> I wonder if it shouldn't rather free the entire list and abort ?

"Be strict when sending, be liberal when receiving."

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>> parse the information from DT. The code looks for all compatible
>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>
>>> da9063
>>>
>>>> to the same pin. If so, the code sends a matching sequence to the
>>>> PMIC to deassert the IRQ.
> 
>>>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>>>
>>>>  static int __init rcar_gen2_regulator_quirk(void)
>>>>  {
>>>> -       u32 mon;
>>>> +       struct device_node *np;
>>>> +       const struct of_device_id *id;
>>>> +       struct regulator_quirk *quirk;
>>>> +       struct regulator_quirk *pos;
>>>> +       struct of_phandle_args *argsa, *argsb;
>>>> +       u32 mon, addr;
>>>> +       int ret;
>>>
>>> Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.
>>>
>>>>
>>>>         if (!of_machine_is_compatible("renesas,koelsch") &&
>>>>             !of_machine_is_compatible("renesas,lager") &&
>>>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>>>             !of_machine_is_compatible("renesas,gose"))
>>>>                 return -ENODEV;
>>>
>>> I think the board checks above can be removed. That will auto-enable the
>>> fix on e.g. Porter (once its regulators have ended up in DTS, of course).
>>
>> Removing the check would also enable it on boards where we don't want
>> this enabled, so I'd prefer to keep the check to avoid strange surprises.
> 
> Like, Porter? ;-)

I'm adding Porter in a separate patch.

>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>> +               if (ret)
>>>> +                       return ret;
>>>
>>> I think it's safer to skip this entry and continue, after calling
>>> kfree(quirk), of course.
>>>
>>>> +
>>>> +               quirk->id = id;
>>>> +               quirk->i2c_msg.addr = addr;
>>>> +               quirk->shared = false;
>>>> +
>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>> +               if (ret)
>>>> +                       return ret;
>>>
>>> kfree(quirk) and continue...
>>
>> I wonder if it shouldn't rather free the entire list and abort ?
> 
> "Be strict when sending, be liberal when receiving."

Meaning ? I think "the language barrier is protecting me" (TM)

-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>> parse the information from DT. The code looks for all compatible
>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>
>>> da9063
>>>
>>>> to the same pin. If so, the code sends a matching sequence to the
>>>> PMIC to deassert the IRQ.
> 
>>>> @@ -122,7 +143,13 @@ static struct notifier_block regulator_quirk_nb = {
>>>>
>>>>  static int __init rcar_gen2_regulator_quirk(void)
>>>>  {
>>>> -       u32 mon;
>>>> +       struct device_node *np;
>>>> +       const struct of_device_id *id;
>>>> +       struct regulator_quirk *quirk;
>>>> +       struct regulator_quirk *pos;
>>>> +       struct of_phandle_args *argsa, *argsb;
>>>> +       u32 mon, addr;
>>>> +       int ret;
>>>
>>> Some people prefer "Reverse Christmas Tree Ordering", i.e. longest line first.
>>>
>>>>
>>>>         if (!of_machine_is_compatible("renesas,koelsch") &&
>>>>             !of_machine_is_compatible("renesas,lager") &&
>>>> @@ -130,6 +157,45 @@ static int __init rcar_gen2_regulator_quirk(void)
>>>>             !of_machine_is_compatible("renesas,gose"))
>>>>                 return -ENODEV;
>>>
>>> I think the board checks above can be removed. That will auto-enable the
>>> fix on e.g. Porter (once its regulators have ended up in DTS, of course).
>>
>> Removing the check would also enable it on boards where we don't want
>> this enabled, so I'd prefer to keep the check to avoid strange surprises.
> 
> Like, Porter? ;-)

I'm adding Porter in a separate patch.

>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>> +               if (ret)
>>>> +                       return ret;
>>>
>>> I think it's safer to skip this entry and continue, after calling
>>> kfree(quirk), of course.
>>>
>>>> +
>>>> +               quirk->id = id;
>>>> +               quirk->i2c_msg.addr = addr;
>>>> +               quirk->shared = false;
>>>> +
>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>> +               if (ret)
>>>> +                       return ret;
>>>
>>> kfree(quirk) and continue...
>>
>> I wonder if it shouldn't rather free the entire list and abort ?
> 
> "Be strict when sending, be liberal when receiving."

Meaning ? I think "the language barrier is protecting me" (TM)

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>> parse the information from DT. The code looks for all compatible
> >>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>> to the same pin. If so, the code sends a matching sequence to the
> >>>> PMIC to deassert the IRQ.

> >>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>> +               if (ret)
> >>>> +                       return ret;
> >>>
> >>> I think it's safer to skip this entry and continue, after calling
> >>> kfree(quirk), of course.
> >>>
> >>>> +
> >>>> +               quirk->id = id;
> >>>> +               quirk->i2c_msg.addr = addr;
> >>>> +               quirk->shared = false;
> >>>> +
> >>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>> +               if (ret)
> >>>> +                       return ret;
> >>>
> >>> kfree(quirk) and continue...
> >>
> >> I wonder if it shouldn't rather free the entire list and abort ?
> >
> > "Be strict when sending, be liberal when receiving."
>
> Meaning ? I think "the language barrier is protecting me" (TM)

Do the best you can, given the buggy DT you received.
I.e. don't fail completely, just ignore the bad device node, and continue.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>> parse the information from DT. The code looks for all compatible
> >>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>> to the same pin. If so, the code sends a matching sequence to the
> >>>> PMIC to deassert the IRQ.

> >>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>> +               if (ret)
> >>>> +                       return ret;
> >>>
> >>> I think it's safer to skip this entry and continue, after calling
> >>> kfree(quirk), of course.
> >>>
> >>>> +
> >>>> +               quirk->id = id;
> >>>> +               quirk->i2c_msg.addr = addr;
> >>>> +               quirk->shared = false;
> >>>> +
> >>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>> +               if (ret)
> >>>> +                       return ret;
> >>>
> >>> kfree(quirk) and continue...
> >>
> >> I wonder if it shouldn't rather free the entire list and abort ?
> >
> > "Be strict when sending, be liberal when receiving."
>
> Meaning ? I think "the language barrier is protecting me" (TM)

Do the best you can, given the buggy DT you received.
I.e. don't fail completely, just ignore the bad device node, and continue.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>>>> parse the information from DT. The code looks for all compatible
>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>>>> to the same pin. If so, the code sends a matching sequence to the
>>>>>> PMIC to deassert the IRQ.
> 
>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>>>> +               if (ret)
>>>>>> +                       return ret;
>>>>>
>>>>> I think it's safer to skip this entry and continue, after calling
>>>>> kfree(quirk), of course.
>>>>>
>>>>>> +
>>>>>> +               quirk->id = id;
>>>>>> +               quirk->i2c_msg.addr = addr;
>>>>>> +               quirk->shared = false;
>>>>>> +
>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>>>> +               if (ret)
>>>>>> +                       return ret;
>>>>>
>>>>> kfree(quirk) and continue...
>>>>
>>>> I wonder if it shouldn't rather free the entire list and abort ?
>>>
>>> "Be strict when sending, be liberal when receiving."
>>
>> Meaning ? I think "the language barrier is protecting me" (TM)
> 
> Do the best you can, given the buggy DT you received.
> I.e. don't fail completely, just ignore the bad device node, and continue.

But if you ignore node, you might as well ignore one which is shared and
then the system crashes due to IRQ storm anyway. So hum, what can we do ?

-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>>>> parse the information from DT. The code looks for all compatible
>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>>>> to the same pin. If so, the code sends a matching sequence to the
>>>>>> PMIC to deassert the IRQ.
> 
>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>>>> +               if (ret)
>>>>>> +                       return ret;
>>>>>
>>>>> I think it's safer to skip this entry and continue, after calling
>>>>> kfree(quirk), of course.
>>>>>
>>>>>> +
>>>>>> +               quirk->id = id;
>>>>>> +               quirk->i2c_msg.addr = addr;
>>>>>> +               quirk->shared = false;
>>>>>> +
>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>>>> +               if (ret)
>>>>>> +                       return ret;
>>>>>
>>>>> kfree(quirk) and continue...
>>>>
>>>> I wonder if it shouldn't rather free the entire list and abort ?
>>>
>>> "Be strict when sending, be liberal when receiving."
>>
>> Meaning ? I think "the language barrier is protecting me" (TM)
> 
> Do the best you can, given the buggy DT you received.
> I.e. don't fail completely, just ignore the bad device node, and continue.

But if you ignore node, you might as well ignore one which is shared and
then the system crashes due to IRQ storm anyway. So hum, what can we do ?

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>>>> parse the information from DT. The code looks for all compatible
> >>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>>>> to the same pin. If so, the code sends a matching sequence to the
> >>>>>> PMIC to deassert the IRQ.
> >
> >>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>>>> +               if (ret)
> >>>>>> +                       return ret;
> >>>>>
> >>>>> I think it's safer to skip this entry and continue, after calling
> >>>>> kfree(quirk), of course.
> >>>>>
> >>>>>> +
> >>>>>> +               quirk->id = id;
> >>>>>> +               quirk->i2c_msg.addr = addr;
> >>>>>> +               quirk->shared = false;
> >>>>>> +
> >>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>>>> +               if (ret)
> >>>>>> +                       return ret;
> >>>>>
> >>>>> kfree(quirk) and continue...
> >>>>
> >>>> I wonder if it shouldn't rather free the entire list and abort ?
> >>>
> >>> "Be strict when sending, be liberal when receiving."
> >>
> >> Meaning ? I think "the language barrier is protecting me" (TM)
> >
> > Do the best you can, given the buggy DT you received.
> > I.e. don't fail completely, just ignore the bad device node, and continue.
>
> But if you ignore node, you might as well ignore one which is shared and
> then the system crashes due to IRQ storm anyway. So hum, what can we do ?

Correct. If it's a critical node, it will crash regardless.
If it's a non-critical node, you have the choice between aborting and crashing,
or ignoring and keeping the system alive. Your call.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>>>> parse the information from DT. The code looks for all compatible
> >>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>>>> to the same pin. If so, the code sends a matching sequence to the
> >>>>>> PMIC to deassert the IRQ.
> >
> >>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>>>> +               if (ret)
> >>>>>> +                       return ret;
> >>>>>
> >>>>> I think it's safer to skip this entry and continue, after calling
> >>>>> kfree(quirk), of course.
> >>>>>
> >>>>>> +
> >>>>>> +               quirk->id = id;
> >>>>>> +               quirk->i2c_msg.addr = addr;
> >>>>>> +               quirk->shared = false;
> >>>>>> +
> >>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>>>> +               if (ret)
> >>>>>> +                       return ret;
> >>>>>
> >>>>> kfree(quirk) and continue...
> >>>>
> >>>> I wonder if it shouldn't rather free the entire list and abort ?
> >>>
> >>> "Be strict when sending, be liberal when receiving."
> >>
> >> Meaning ? I think "the language barrier is protecting me" (TM)
> >
> > Do the best you can, given the buggy DT you received.
> > I.e. don't fail completely, just ignore the bad device node, and continue.
>
> But if you ignore node, you might as well ignore one which is shared and
> then the system crashes due to IRQ storm anyway. So hum, what can we do ?

Correct. If it's a critical node, it will crash regardless.
If it's a non-critical node, you have the choice between aborting and crashing,
or ignoring and keeping the system alive. Your call.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>>>>>> parse the information from DT. The code looks for all compatible
>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>>>>>> to the same pin. If so, the code sends a matching sequence to the
>>>>>>>> PMIC to deassert the IRQ.
>>>
>>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>>>>>> +               if (ret)
>>>>>>>> +                       return ret;
>>>>>>>
>>>>>>> I think it's safer to skip this entry and continue, after calling
>>>>>>> kfree(quirk), of course.
>>>>>>>
>>>>>>>> +
>>>>>>>> +               quirk->id = id;
>>>>>>>> +               quirk->i2c_msg.addr = addr;
>>>>>>>> +               quirk->shared = false;
>>>>>>>> +
>>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>>>>>> +               if (ret)
>>>>>>>> +                       return ret;
>>>>>>>
>>>>>>> kfree(quirk) and continue...
>>>>>>
>>>>>> I wonder if it shouldn't rather free the entire list and abort ?
>>>>>
>>>>> "Be strict when sending, be liberal when receiving."
>>>>
>>>> Meaning ? I think "the language barrier is protecting me" (TM)
>>>
>>> Do the best you can, given the buggy DT you received.
>>> I.e. don't fail completely, just ignore the bad device node, and continue.
>>
>> But if you ignore node, you might as well ignore one which is shared and
>> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
> 
> Correct. If it's a critical node, it will crash regardless.
> If it's a non-critical node, you have the choice between aborting and crashing,
> or ignoring and keeping the system alive. Your call.

But wait, since we control which machines this code runs on , can't we
assure they have valid DTs ? This situation with invalid DT starts to
look a bit hypothetical to me.

-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>>>>>> parse the information from DT. The code looks for all compatible
>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>>>>>> to the same pin. If so, the code sends a matching sequence to the
>>>>>>>> PMIC to deassert the IRQ.
>>>
>>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>>>>>> +               if (ret)
>>>>>>>> +                       return ret;
>>>>>>>
>>>>>>> I think it's safer to skip this entry and continue, after calling
>>>>>>> kfree(quirk), of course.
>>>>>>>
>>>>>>>> +
>>>>>>>> +               quirk->id = id;
>>>>>>>> +               quirk->i2c_msg.addr = addr;
>>>>>>>> +               quirk->shared = false;
>>>>>>>> +
>>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>>>>>> +               if (ret)
>>>>>>>> +                       return ret;
>>>>>>>
>>>>>>> kfree(quirk) and continue...
>>>>>>
>>>>>> I wonder if it shouldn't rather free the entire list and abort ?
>>>>>
>>>>> "Be strict when sending, be liberal when receiving."
>>>>
>>>> Meaning ? I think "the language barrier is protecting me" (TM)
>>>
>>> Do the best you can, given the buggy DT you received.
>>> I.e. don't fail completely, just ignore the bad device node, and continue.
>>
>> But if you ignore node, you might as well ignore one which is shared and
>> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
> 
> Correct. If it's a critical node, it will crash regardless.
> If it's a non-critical node, you have the choice between aborting and crashing,
> or ignoring and keeping the system alive. Your call.

But wait, since we control which machines this code runs on , can't we
assure they have valid DTs ? This situation with invalid DT starts to
look a bit hypothetical to me.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> >>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>>>>>> parse the information from DT. The code looks for all compatible
> >>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>>>>>> to the same pin. If so, the code sends a matching sequence to the
> >>>>>>>> PMIC to deassert the IRQ.
> >>>
> >>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>>>>>> +               if (ret)
> >>>>>>>> +                       return ret;
> >>>>>>>
> >>>>>>> I think it's safer to skip this entry and continue, after calling
> >>>>>>> kfree(quirk), of course.
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +               quirk->id = id;
> >>>>>>>> +               quirk->i2c_msg.addr = addr;
> >>>>>>>> +               quirk->shared = false;
> >>>>>>>> +
> >>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>>>>>> +               if (ret)
> >>>>>>>> +                       return ret;
> >>>>>>>
> >>>>>>> kfree(quirk) and continue...
> >>>>>>
> >>>>>> I wonder if it shouldn't rather free the entire list and abort ?
> >>>>>
> >>>>> "Be strict when sending, be liberal when receiving."
> >>>>
> >>>> Meaning ? I think "the language barrier is protecting me" (TM)
> >>>
> >>> Do the best you can, given the buggy DT you received.
> >>> I.e. don't fail completely, just ignore the bad device node, and continue.
> >>
> >> But if you ignore node, you might as well ignore one which is shared and
> >> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
> >
> > Correct. If it's a critical node, it will crash regardless.
> > If it's a non-critical node, you have the choice between aborting and crashing,
> > or ignoring and keeping the system alive. Your call.
>
> But wait, since we control which machines this code runs on , can't we
> assure they have valid DTs ? This situation with invalid DT starts to
> look a bit hypothetical to me.

That assumes you keep the list of machines to check, and don't want to fix the
issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
you still need to check for r8a779[0-4] and r8a774[23457]).

Anyway, as we care about booting old DTBs on new kernels (for a while), we
have a few more release cycles to bikeshed ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> >>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>>>>>> parse the information from DT. The code looks for all compatible
> >>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>>>>>> to the same pin. If so, the code sends a matching sequence to the
> >>>>>>>> PMIC to deassert the IRQ.
> >>>
> >>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>>>>>> +               if (ret)
> >>>>>>>> +                       return ret;
> >>>>>>>
> >>>>>>> I think it's safer to skip this entry and continue, after calling
> >>>>>>> kfree(quirk), of course.
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +               quirk->id = id;
> >>>>>>>> +               quirk->i2c_msg.addr = addr;
> >>>>>>>> +               quirk->shared = false;
> >>>>>>>> +
> >>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>>>>>> +               if (ret)
> >>>>>>>> +                       return ret;
> >>>>>>>
> >>>>>>> kfree(quirk) and continue...
> >>>>>>
> >>>>>> I wonder if it shouldn't rather free the entire list and abort ?
> >>>>>
> >>>>> "Be strict when sending, be liberal when receiving."
> >>>>
> >>>> Meaning ? I think "the language barrier is protecting me" (TM)
> >>>
> >>> Do the best you can, given the buggy DT you received.
> >>> I.e. don't fail completely, just ignore the bad device node, and continue.
> >>
> >> But if you ignore node, you might as well ignore one which is shared and
> >> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
> >
> > Correct. If it's a critical node, it will crash regardless.
> > If it's a non-critical node, you have the choice between aborting and crashing,
> > or ignoring and keeping the system alive. Your call.
>
> But wait, since we control which machines this code runs on , can't we
> assure they have valid DTs ? This situation with invalid DT starts to
> look a bit hypothetical to me.

That assumes you keep the list of machines to check, and don't want to fix the
issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
you still need to check for r8a779[0-4] and r8a774[23457]).

Anyway, as we care about booting old DTBs on new kernels (for a while), we
have a few more release cycles to bikeshed ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 04:30 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
>>>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
>>>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>>>>>>>> parse the information from DT. The code looks for all compatible
>>>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>>>>>>>> to the same pin. If so, the code sends a matching sequence to the
>>>>>>>>>> PMIC to deassert the IRQ.
>>>>>
>>>>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>>>>>>>> +               if (ret)
>>>>>>>>>> +                       return ret;
>>>>>>>>>
>>>>>>>>> I think it's safer to skip this entry and continue, after calling
>>>>>>>>> kfree(quirk), of course.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +               quirk->id = id;
>>>>>>>>>> +               quirk->i2c_msg.addr = addr;
>>>>>>>>>> +               quirk->shared = false;
>>>>>>>>>> +
>>>>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>>>>>>>> +               if (ret)
>>>>>>>>>> +                       return ret;
>>>>>>>>>
>>>>>>>>> kfree(quirk) and continue...
>>>>>>>>
>>>>>>>> I wonder if it shouldn't rather free the entire list and abort ?
>>>>>>>
>>>>>>> "Be strict when sending, be liberal when receiving."
>>>>>>
>>>>>> Meaning ? I think "the language barrier is protecting me" (TM)
>>>>>
>>>>> Do the best you can, given the buggy DT you received.
>>>>> I.e. don't fail completely, just ignore the bad device node, and continue.
>>>>
>>>> But if you ignore node, you might as well ignore one which is shared and
>>>> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
>>>
>>> Correct. If it's a critical node, it will crash regardless.
>>> If it's a non-critical node, you have the choice between aborting and crashing,
>>> or ignoring and keeping the system alive. Your call.
>>
>> But wait, since we control which machines this code runs on , can't we
>> assure they have valid DTs ? This situation with invalid DT starts to
>> look a bit hypothetical to me.
> 
> That assumes you keep the list of machines to check, and don't want to fix the
> issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
> you still need to check for r8a779[0-4] and r8a774[23457]).

Yes, I want to keep a list of machines to check, to be _sure_ some
machine doesn't randomly blow up.

> Anyway, as we care about booting old DTBs on new kernels (for a while), we
> have a few more release cycles to bikeshed ;-)

I was about to ask if this patch then makes any sense or not.

-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/11/2018 04:30 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
>>> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
>>>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
>>>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
>>>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
>>>>>>>>>> parse the information from DT. The code looks for all compatible
>>>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
>>>>>>>>>> to the same pin. If so, the code sends a matching sequence to the
>>>>>>>>>> PMIC to deassert the IRQ.
>>>>>
>>>>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
>>>>>>>>>> +               if (ret)
>>>>>>>>>> +                       return ret;
>>>>>>>>>
>>>>>>>>> I think it's safer to skip this entry and continue, after calling
>>>>>>>>> kfree(quirk), of course.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +               quirk->id = id;
>>>>>>>>>> +               quirk->i2c_msg.addr = addr;
>>>>>>>>>> +               quirk->shared = false;
>>>>>>>>>> +
>>>>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
>>>>>>>>>> +               if (ret)
>>>>>>>>>> +                       return ret;
>>>>>>>>>
>>>>>>>>> kfree(quirk) and continue...
>>>>>>>>
>>>>>>>> I wonder if it shouldn't rather free the entire list and abort ?
>>>>>>>
>>>>>>> "Be strict when sending, be liberal when receiving."
>>>>>>
>>>>>> Meaning ? I think "the language barrier is protecting me" (TM)
>>>>>
>>>>> Do the best you can, given the buggy DT you received.
>>>>> I.e. don't fail completely, just ignore the bad device node, and continue.
>>>>
>>>> But if you ignore node, you might as well ignore one which is shared and
>>>> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
>>>
>>> Correct. If it's a critical node, it will crash regardless.
>>> If it's a non-critical node, you have the choice between aborting and crashing,
>>> or ignoring and keeping the system alive. Your call.
>>
>> But wait, since we control which machines this code runs on , can't we
>> assure they have valid DTs ? This situation with invalid DT starts to
>> look a bit hypothetical to me.
> 
> That assumes you keep the list of machines to check, and don't want to fix the
> issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
> you still need to check for r8a779[0-4] and r8a774[23457]).

Yes, I want to keep a list of machines to check, to be _sure_ some
machine doesn't randomly blow up.

> Anyway, as we care about booting old DTBs on new kernels (for a while), we
> have a few more release cycles to bikeshed ;-)

I was about to ask if this patch then makes any sense or not.

-- 
Best regards,
Marek Vasut

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 5:26 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 04:30 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> >>>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> >>>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>>>>>>>> parse the information from DT. The code looks for all compatible
> >>>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>>>>>>>> to the same pin. If so, the code sends a matching sequence to the
> >>>>>>>>>> PMIC to deassert the IRQ.
> >>>>>
> >>>>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>>>>>>>> +               if (ret)
> >>>>>>>>>> +                       return ret;
> >>>>>>>>>
> >>>>>>>>> I think it's safer to skip this entry and continue, after calling
> >>>>>>>>> kfree(quirk), of course.
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +               quirk->id = id;
> >>>>>>>>>> +               quirk->i2c_msg.addr = addr;
> >>>>>>>>>> +               quirk->shared = false;
> >>>>>>>>>> +
> >>>>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>>>>>>>> +               if (ret)
> >>>>>>>>>> +                       return ret;
> >>>>>>>>>
> >>>>>>>>> kfree(quirk) and continue...
> >>>>>>>>
> >>>>>>>> I wonder if it shouldn't rather free the entire list and abort ?
> >>>>>>>
> >>>>>>> "Be strict when sending, be liberal when receiving."
> >>>>>>
> >>>>>> Meaning ? I think "the language barrier is protecting me" (TM)
> >>>>>
> >>>>> Do the best you can, given the buggy DT you received.
> >>>>> I.e. don't fail completely, just ignore the bad device node, and continue.
> >>>>
> >>>> But if you ignore node, you might as well ignore one which is shared and
> >>>> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
> >>>
> >>> Correct. If it's a critical node, it will crash regardless.
> >>> If it's a non-critical node, you have the choice between aborting and crashing,
> >>> or ignoring and keeping the system alive. Your call.
> >>
> >> But wait, since we control which machines this code runs on , can't we
> >> assure they have valid DTs ? This situation with invalid DT starts to
> >> look a bit hypothetical to me.
> >
> > That assumes you keep the list of machines to check, and don't want to fix the
> > issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
> > you still need to check for r8a779[0-4] and r8a774[23457]).
>
> Yes, I want to keep a list of machines to check, to be _sure_ some
> machine doesn't randomly blow up.

Just checking for the presence of a "renesas,irqc" node should be sufficient.
Using that node would also get rid of the hardcoded IRQC_BASE address.
Note that the code assumes IRQ2. If another IRQ is used, that won't harm
much though (as in: if it didn't blow up before, it won't blow up now).

> > Anyway, as we care about booting old DTBs on new kernels (for a while), we
> > have a few more release cycles to bikeshed ;-)
>
> I was about to ask if this patch then makes any sense or not.

Sure. Less hard-coding is always better.
Especially if it means we can make it work on more machines automatically :-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 11, 2018 at 5:26 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 04:30 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> >>>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> >>>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>>>>>>>> parse the information from DT. The code looks for all compatible
> >>>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>>>>>>>> to the same pin. If so, the code sends a matching sequence to the
> >>>>>>>>>> PMIC to deassert the IRQ.
> >>>>>
> >>>>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>>>>>>>> +               if (ret)
> >>>>>>>>>> +                       return ret;
> >>>>>>>>>
> >>>>>>>>> I think it's safer to skip this entry and continue, after calling
> >>>>>>>>> kfree(quirk), of course.
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +               quirk->id = id;
> >>>>>>>>>> +               quirk->i2c_msg.addr = addr;
> >>>>>>>>>> +               quirk->shared = false;
> >>>>>>>>>> +
> >>>>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>>>>>>>> +               if (ret)
> >>>>>>>>>> +                       return ret;
> >>>>>>>>>
> >>>>>>>>> kfree(quirk) and continue...
> >>>>>>>>
> >>>>>>>> I wonder if it shouldn't rather free the entire list and abort ?
> >>>>>>>
> >>>>>>> "Be strict when sending, be liberal when receiving."
> >>>>>>
> >>>>>> Meaning ? I think "the language barrier is protecting me" (TM)
> >>>>>
> >>>>> Do the best you can, given the buggy DT you received.
> >>>>> I.e. don't fail completely, just ignore the bad device node, and continue.
> >>>>
> >>>> But if you ignore node, you might as well ignore one which is shared and
> >>>> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
> >>>
> >>> Correct. If it's a critical node, it will crash regardless.
> >>> If it's a non-critical node, you have the choice between aborting and crashing,
> >>> or ignoring and keeping the system alive. Your call.
> >>
> >> But wait, since we control which machines this code runs on , can't we
> >> assure they have valid DTs ? This situation with invalid DT starts to
> >> look a bit hypothetical to me.
> >
> > That assumes you keep the list of machines to check, and don't want to fix the
> > issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
> > you still need to check for r8a779[0-4] and r8a774[23457]).
>
> Yes, I want to keep a list of machines to check, to be _sure_ some
> machine doesn't randomly blow up.

Just checking for the presence of a "renesas,irqc" node should be sufficient.
Using that node would also get rid of the hardcoded IRQC_BASE address.
Note that the code assumes IRQ2. If another IRQ is used, that won't harm
much though (as in: if it didn't blow up before, it won't blow up now).

> > Anyway, as we care about booting old DTBs on new kernels (for a while), we
> > have a few more release cycles to bikeshed ;-)
>
> I was about to ask if this patch then makes any sense or not.

Sure. Less hard-coding is always better.
Especially if it means we can make it work on more machines automatically :-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/13/2018 01:28 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

[...]

>>>> But wait, since we control which machines this code runs on , can't we
>>>> assure they have valid DTs ? This situation with invalid DT starts to
>>>> look a bit hypothetical to me.
>>>
>>> That assumes you keep the list of machines to check, and don't want to fix the
>>> issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
>>> you still need to check for r8a779[0-4] and r8a774[23457]).
>>
>> Yes, I want to keep a list of machines to check, to be _sure_ some
>> machine doesn't randomly blow up.
> 
> Just checking for the presence of a "renesas,irqc" node should be sufficient.

How so? Any other R-Car machine can have the irqc node too. That's
fragile at best.

> Using that node would also get rid of the hardcoded IRQC_BASE address.
> Note that the code assumes IRQ2. If another IRQ is used, that won't harm
> much though (as in: if it didn't blow up before, it won't blow up now).

We could/should fix up the irqc detection though.

>>> Anyway, as we care about booting old DTBs on new kernels (for a while), we
>>> have a few more release cycles to bikeshed ;-)
>>
>> I was about to ask if this patch then makes any sense or not.
> 
> Sure. Less hard-coding is always better.
> Especially if it means we can make it work on more machines automatically :-)

I prefer to be in control of that.

-- 
Best regards,
Marek Vasut

[PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk

[Marek Vasut]

On 06/13/2018 01:28 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

[...]

>>>> But wait, since we control which machines this code runs on , can't we
>>>> assure they have valid DTs ? This situation with invalid DT starts to
>>>> look a bit hypothetical to me.
>>>
>>> That assumes you keep the list of machines to check, and don't want to fix the
>>> issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
>>> you still need to check for r8a779[0-4] and r8a774[23457]).
>>
>> Yes, I want to keep a list of machines to check, to be _sure_ some
>> machine doesn't randomly blow up.
> 
> Just checking for the presence of a "renesas,irqc" node should be sufficient.

How so? Any other R-Car machine can have the irqc node too. That's
fragile at best.

> Using that node would also get rid of the hardcoded IRQC_BASE address.
> Note that the code assumes IRQ2. If another IRQ is used, that won't harm
> much though (as in: if it didn't blow up before, it won't blow up now).

We could/should fix up the irqc detection though.

>>> Anyway, as we care about booting old DTBs on new kernels (for a while), we
>>> have a few more release cycles to bikeshed ;-)
>>
>> I was about to ask if this patch then makes any sense or not.
> 
> Sure. Less hard-coding is always better.
> Especially if it means we can make it work on more machines automatically :-)

I prefer to be in control of that.

-- 
Best regards,
Marek Vasut