linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF
@ 2013-08-17  5:46 Alexander Shiyan
  2013-08-17  5:46 ` [PATCH v3 RESEND 2/3] input: mc13783: Add MC13892 support Alexander Shiyan
  2013-08-17  6:20 ` [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF Michael Grzeschik
  0 siblings, 2 replies; 5+ messages in thread
From: Alexander Shiyan @ 2013-08-17  5:46 UTC (permalink / raw)
  To: linux-input; +Cc: Dmitry Torokhov, Sascha Hauer, Alexander Shiyan

This patch is a preparation mc13xxx powerbutton driver to support
MC13892 and support the probe through the DT.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/mach-imx/mach-mx31moboard.c   |   9 +-
 drivers/input/misc/mc13783-pwrbutton.c | 332 +++++++++++++--------------------
 include/linux/mfd/mc13xxx.h            |  28 +--
 3 files changed, 151 insertions(+), 218 deletions(-)

diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
index 6f424ec..2a8aa43 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -276,9 +276,12 @@ static struct mc13xxx_leds_platform_data moboard_leds = {
 };
 
 static struct mc13xxx_buttons_platform_data moboard_buttons = {
-	.b1on_flags = MC13783_BUTTON_DBNC_750MS | MC13783_BUTTON_ENABLE |
-			MC13783_BUTTON_POL_INVERT,
-	.b1on_key = KEY_POWER,
+	.buttons[0] = {
+		.keycode	= KEY_POWER,
+		.flags		= MC13XXX_BUTTON_ENABLE |
+				  MC13XXX_BUTTON_DBNC_750MS |
+				  MC13XXX_BUTTON_POL_INVERT,
+	},
 };
 
 static struct mc13xxx_codec_platform_data moboard_codec = {
diff --git a/drivers/input/misc/mc13783-pwrbutton.c b/drivers/input/misc/mc13783-pwrbutton.c
index d0277a7..aaaacef 100644
--- a/drivers/input/misc/mc13783-pwrbutton.c
+++ b/drivers/input/misc/mc13783-pwrbutton.c
@@ -24,248 +24,176 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/input.h>
-#include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/mfd/mc13783.h>
-#include <linux/sched.h>
-#include <linux/slab.h>
-
-struct mc13783_pwrb {
-	struct input_dev *pwr;
-	struct mc13xxx *mc13783;
-#define MC13783_PWRB_B1_POL_INVERT	(1 << 0)
-#define MC13783_PWRB_B2_POL_INVERT	(1 << 1)
-#define MC13783_PWRB_B3_POL_INVERT	(1 << 2)
-	int flags;
-	unsigned short keymap[3];
+
+struct mc13xxx_button_def {
+	unsigned int	irq;
+	unsigned int	sense_bit;
 };
 
-#define MC13783_REG_INTERRUPT_SENSE_1		5
-#define MC13783_IRQSENSE1_ONOFD1S		(1 << 3)
-#define MC13783_IRQSENSE1_ONOFD2S		(1 << 4)
-#define MC13783_IRQSENSE1_ONOFD3S		(1 << 5)
+struct mc13xxx_pwrb_devtype {
+	struct mc13xxx_button_def	btn_def[MAX13XXX_NUM_BUTTONS];
+};
 
-#define MC13783_REG_POWER_CONTROL_2		15
-#define MC13783_POWER_CONTROL_2_ON1BDBNC	4
-#define MC13783_POWER_CONTROL_2_ON2BDBNC	6
-#define MC13783_POWER_CONTROL_2_ON3BDBNC	8
-#define MC13783_POWER_CONTROL_2_ON1BRSTEN	(1 << 1)
-#define MC13783_POWER_CONTROL_2_ON2BRSTEN	(1 << 2)
-#define MC13783_POWER_CONTROL_2_ON3BRSTEN	(1 << 3)
+struct mc13xxx_pwrb {
+	struct mc13xxx_pwrb_devtype	*devtype;
+	unsigned int			enabled;
+	unsigned int			inverted;
+	u16				btn_code[MAX13XXX_NUM_BUTTONS];
+	struct input_dev		*input;
+	struct mc13xxx			*mc13xxx;
+};
 
-static irqreturn_t button_irq(int irq, void *_priv)
-{
-	struct mc13783_pwrb *priv = _priv;
-	int val;
-
-	mc13xxx_irq_ack(priv->mc13783, irq);
-	mc13xxx_reg_read(priv->mc13783, MC13783_REG_INTERRUPT_SENSE_1, &val);
-
-	switch (irq) {
-	case MC13783_IRQ_ONOFD1:
-		val = val & MC13783_IRQSENSE1_ONOFD1S ? 1 : 0;
-		if (priv->flags & MC13783_PWRB_B1_POL_INVERT)
-			val ^= 1;
-		input_report_key(priv->pwr, priv->keymap[0], val);
-		break;
-
-	case MC13783_IRQ_ONOFD2:
-		val = val & MC13783_IRQSENSE1_ONOFD2S ? 1 : 0;
-		if (priv->flags & MC13783_PWRB_B2_POL_INVERT)
-			val ^= 1;
-		input_report_key(priv->pwr, priv->keymap[1], val);
-		break;
-
-	case MC13783_IRQ_ONOFD3:
-		val = val & MC13783_IRQSENSE1_ONOFD3S ? 1 : 0;
-		if (priv->flags & MC13783_PWRB_B3_POL_INVERT)
-			val ^= 1;
-		input_report_key(priv->pwr, priv->keymap[2], val);
-		break;
-	}
+#define MC13XXX_REG_INTERRUPT_SENSE_1	5
+#define MC13XXX_REG_POWER_CONTROL_2	15
 
-	input_sync(priv->pwr);
+static irqreturn_t mc13xxx_pwrbutton_irq(int irq, void *data)
+{
+	struct mc13xxx_pwrb *priv = data;
+	unsigned int i, val;
+
+	mc13xxx_irq_ack(priv->mc13xxx, irq);
+	mc13xxx_reg_read(priv->mc13xxx, MC13XXX_REG_INTERRUPT_SENSE_1, &val);
+
+	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++)
+		if (irq == priv->devtype->btn_def[i].irq) {
+			val = !!(val & priv->devtype->btn_def[i].sense_bit);
+			if (priv->inverted & BIT(i))
+				val = !val;
+			input_report_key(priv->input, priv->btn_code[i], val);
+			input_sync(priv->input);
+			break;
+		}
 
 	return IRQ_HANDLED;
 }
 
-static int mc13783_pwrbutton_probe(struct platform_device *pdev)
+static int __init mc13xxx_pwrbutton_probe(struct platform_device *pdev)
 {
-	const struct mc13xxx_buttons_platform_data *pdata;
-	struct mc13xxx *mc13783 = dev_get_drvdata(pdev->dev.parent);
-	struct input_dev *pwr;
-	struct mc13783_pwrb *priv;
-	int err = 0;
-	int reg = 0;
-
-	pdata = dev_get_platdata(&pdev->dev);
+	struct mc13xxx_buttons_platform_data *pdata =
+		dev_get_platdata(&pdev->dev);
+	struct mc13xxx *mc13xxx = dev_get_drvdata(pdev->dev.parent);
+	struct mc13xxx_pwrb_devtype *devtype =
+		(struct mc13xxx_pwrb_devtype *)pdev->id_entry->driver_data;
+	struct mc13xxx_pwrb *priv;
+	int i, reg = 0, ret = -EINVAL;
+
 	if (!pdata) {
-		dev_err(&pdev->dev, "missing platform data\n");
+		dev_err(&pdev->dev, "Missing platform data\n");
 		return -ENODEV;
 	}
 
-	pwr = input_allocate_device();
-	if (!pwr) {
-		dev_dbg(&pdev->dev, "Can't allocate power button\n");
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
-	}
-
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		err = -ENOMEM;
-		dev_dbg(&pdev->dev, "Can't allocate power button\n");
-		goto free_input_dev;
-	}
-
-	reg |= (pdata->b1on_flags & 0x3) << MC13783_POWER_CONTROL_2_ON1BDBNC;
-	reg |= (pdata->b2on_flags & 0x3) << MC13783_POWER_CONTROL_2_ON2BDBNC;
-	reg |= (pdata->b3on_flags & 0x3) << MC13783_POWER_CONTROL_2_ON3BDBNC;
-
-	priv->pwr = pwr;
-	priv->mc13783 = mc13783;
 
-	mc13xxx_lock(mc13783);
-
-	if (pdata->b1on_flags & MC13783_BUTTON_ENABLE) {
-		priv->keymap[0] = pdata->b1on_key;
-		if (pdata->b1on_key != KEY_RESERVED)
-			__set_bit(pdata->b1on_key, pwr->keybit);
-
-		if (pdata->b1on_flags & MC13783_BUTTON_POL_INVERT)
-			priv->flags |= MC13783_PWRB_B1_POL_INVERT;
+	priv->input = devm_input_allocate_device(&pdev->dev);
+	if (!priv->input)
+		return -ENOMEM;
 
-		if (pdata->b1on_flags & MC13783_BUTTON_RESET_EN)
-			reg |= MC13783_POWER_CONTROL_2_ON1BRSTEN;
+	priv->mc13xxx = mc13xxx;
+	priv->devtype = devtype;
+	platform_set_drvdata(pdev, priv);
 
-		err = mc13xxx_irq_request(mc13783, MC13783_IRQ_ONOFD1,
-					  button_irq, "b1on", priv);
-		if (err) {
-			dev_dbg(&pdev->dev, "Can't request irq\n");
-			goto free_priv;
-		}
+	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++) {
+		u16 code, invert, reset, debounce;
+
+		if (!(pdata->buttons[i].flags & MC13XXX_BUTTON_ENABLE))
+			continue;
+		code = pdata->buttons[i].keycode;
+		invert = !!(pdata->buttons[i].flags &
+			    MC13XXX_BUTTON_POL_INVERT);
+		reset = !!(pdata->buttons[i].flags &
+			   MC13XXX_BUTTON_RESET_EN);
+		debounce = pdata->buttons[i].flags;
+
+		priv->btn_code[i] = code;
+		if (code != KEY_RESERVED)
+			__set_bit(code, priv->input->keybit);
+
+		priv->enabled |= BIT(i);
+		priv->inverted |= invert << i;
+		reg |= reset << (i + 1);
+		reg |= (debounce & 0x03) << (4 + i * 2);
 	}
 
-	if (pdata->b2on_flags & MC13783_BUTTON_ENABLE) {
-		priv->keymap[1] = pdata->b2on_key;
-		if (pdata->b2on_key != KEY_RESERVED)
-			__set_bit(pdata->b2on_key, pwr->keybit);
-
-		if (pdata->b2on_flags & MC13783_BUTTON_POL_INVERT)
-			priv->flags |= MC13783_PWRB_B2_POL_INVERT;
-
-		if (pdata->b2on_flags & MC13783_BUTTON_RESET_EN)
-			reg |= MC13783_POWER_CONTROL_2_ON2BRSTEN;
-
-		err = mc13xxx_irq_request(mc13783, MC13783_IRQ_ONOFD2,
-					  button_irq, "b2on", priv);
-		if (err) {
-			dev_dbg(&pdev->dev, "Can't request irq\n");
-			goto free_irq_b1;
+	mc13xxx_lock(mc13xxx);
+
+	mc13xxx_reg_rmw(mc13xxx, MC13XXX_REG_POWER_CONTROL_2, 0x3fe, reg);
+
+	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++)
+		if (priv->enabled & BIT(i)) {
+			ret = mc13xxx_irq_request(mc13xxx,
+						  devtype->btn_def[i].irq,
+						  mc13xxx_pwrbutton_irq, NULL,
+						  priv);
+			if (ret) {
+				dev_err(&pdev->dev, "Can't request IRQ%i: %i\n",
+					devtype->btn_def[i].irq, ret);
+				priv->enabled &= ~BIT(i);
+			}
 		}
-	}
 
-	if (pdata->b3on_flags & MC13783_BUTTON_ENABLE) {
-		priv->keymap[2] = pdata->b3on_key;
-		if (pdata->b3on_key != KEY_RESERVED)
-			__set_bit(pdata->b3on_key, pwr->keybit);
+	mc13xxx_unlock(mc13xxx);
 
-		if (pdata->b3on_flags & MC13783_BUTTON_POL_INVERT)
-			priv->flags |= MC13783_PWRB_B3_POL_INVERT;
+	priv->input->name		= "mc13xxx_pwrbutton";
+	priv->input->phys		= "mc13xxx_pwrbutton/input0";
+	priv->input->id.bustype		= BUS_HOST;
+	priv->input->id.vendor		= 0x0001;
+	priv->input->id.product		= 0x0001;
+	priv->input->id.version		= 0x0100;
+	priv->input->keycode		= priv->btn_code;
+	priv->input->keycodemax		= ARRAY_SIZE(priv->btn_code);
+	priv->input->keycodesize	= sizeof(priv->btn_code[0]);
+	__set_bit(EV_KEY, priv->input->evbit);
 
-		if (pdata->b3on_flags & MC13783_BUTTON_RESET_EN)
-			reg |= MC13783_POWER_CONTROL_2_ON3BRSTEN;
-
-		err = mc13xxx_irq_request(mc13783, MC13783_IRQ_ONOFD3,
-					  button_irq, "b3on", priv);
-		if (err) {
-			dev_dbg(&pdev->dev, "Can't request irq: %d\n", err);
-			goto free_irq_b2;
-		}
-	}
+	input_set_drvdata(priv->input, priv);
 
-	mc13xxx_reg_rmw(mc13783, MC13783_REG_POWER_CONTROL_2, 0x3FE, reg);
+	ret = input_register_device(priv->input);
+	if (ret)
+		dev_err(&pdev->dev, "Can't register input device: %i\n", ret);
 
-	mc13xxx_unlock(mc13783);
-
-	pwr->name = "mc13783_pwrbutton";
-	pwr->phys = "mc13783_pwrbutton/input0";
-	pwr->dev.parent = &pdev->dev;
-
-	pwr->keycode = priv->keymap;
-	pwr->keycodemax = ARRAY_SIZE(priv->keymap);
-	pwr->keycodesize = sizeof(priv->keymap[0]);
-	__set_bit(EV_KEY, pwr->evbit);
-
-	err = input_register_device(pwr);
-	if (err) {
-		dev_dbg(&pdev->dev, "Can't register power button: %d\n", err);
-		goto free_irq;
-	}
-
-	platform_set_drvdata(pdev, priv);
-
-	return 0;
-
-free_irq:
-	mc13xxx_lock(mc13783);
-
-	if (pdata->b3on_flags & MC13783_BUTTON_ENABLE)
-		mc13xxx_irq_free(mc13783, MC13783_IRQ_ONOFD3, priv);
-
-free_irq_b2:
-	if (pdata->b2on_flags & MC13783_BUTTON_ENABLE)
-		mc13xxx_irq_free(mc13783, MC13783_IRQ_ONOFD2, priv);
-
-free_irq_b1:
-	if (pdata->b1on_flags & MC13783_BUTTON_ENABLE)
-		mc13xxx_irq_free(mc13783, MC13783_IRQ_ONOFD1, priv);
-
-free_priv:
-	mc13xxx_unlock(mc13783);
-	kfree(priv);
-
-free_input_dev:
-	input_free_device(pwr);
-
-	return err;
+	return ret;
 }
 
-static int mc13783_pwrbutton_remove(struct platform_device *pdev)
+static int mc13xxx_pwrbutton_remove(struct platform_device *pdev)
 {
-	struct mc13783_pwrb *priv = platform_get_drvdata(pdev);
-	const struct mc13xxx_buttons_platform_data *pdata;
-
-	pdata = dev_get_platdata(&pdev->dev);
-
-	mc13xxx_lock(priv->mc13783);
-
-	if (pdata->b3on_flags & MC13783_BUTTON_ENABLE)
-		mc13xxx_irq_free(priv->mc13783, MC13783_IRQ_ONOFD3, priv);
-	if (pdata->b2on_flags & MC13783_BUTTON_ENABLE)
-		mc13xxx_irq_free(priv->mc13783, MC13783_IRQ_ONOFD2, priv);
-	if (pdata->b1on_flags & MC13783_BUTTON_ENABLE)
-		mc13xxx_irq_free(priv->mc13783, MC13783_IRQ_ONOFD1, priv);
+	struct mc13xxx_pwrb *priv = platform_get_drvdata(pdev);
+	int i;
 
-	mc13xxx_unlock(priv->mc13783);
-
-	input_unregister_device(priv->pwr);
-	kfree(priv);
+	mc13xxx_lock(priv->mc13xxx);
+	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++)
+		if (priv->enabled & BIT(i))
+			mc13xxx_irq_free(priv->mc13xxx,
+					 priv->devtype->btn_def[i].irq, priv);
+	mc13xxx_unlock(priv->mc13xxx);
 
 	return 0;
 }
 
-static struct platform_driver mc13783_pwrbutton_driver = {
-	.probe		= mc13783_pwrbutton_probe,
-	.remove		= mc13783_pwrbutton_remove,
-	.driver		= {
-		.name	= "mc13783-pwrbutton",
+static const struct mc13xxx_pwrb_devtype mc13783_pwrb_devtype = {
+	.btn_def[0] = { MC13783_IRQ_ONOFD1, BIT(3) },
+	.btn_def[1] = { MC13783_IRQ_ONOFD2, BIT(4) },
+	.btn_def[2] = { MC13783_IRQ_ONOFD3, BIT(5) }
+};
+
+static const struct platform_device_id mc13xxx_pwrbutton_id_table[] = {
+	{ "mc13783-pwrbutton", (kernel_ulong_t)&mc13783_pwrb_devtype },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, mc13xxx_pwrbutton_id_table);
+
+static struct platform_driver mc13xxx_pwrbutton_driver = {
+	.driver	= {
+		.name	= "mc13xxx-pwrbutton",
 		.owner	= THIS_MODULE,
 	},
+	.remove		= mc13xxx_pwrbutton_remove,
+	.id_table	= mc13xxx_pwrbutton_id_table,
 };
+module_platform_driver_probe(mc13xxx_pwrbutton_driver, mc13xxx_pwrbutton_probe);
 
-module_platform_driver(mc13783_pwrbutton_driver);
-
-MODULE_ALIAS("platform:mc13783-pwrbutton");
 MODULE_DESCRIPTION("MC13783 Power Button");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Philippe Retornaz");
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index 41ed592..b895538 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -142,20 +142,22 @@ struct mc13xxx_leds_platform_data {
 	u32 led_control[MAX_LED_CONTROL_REGS];
 };
 
+#define MAX13XXX_NUM_BUTTONS	3
+
+struct mc13xxx_button {
+	u16		keycode;
+	unsigned int	flags;
+#define MC13XXX_BUTTON_DBNC_0MS		0
+#define MC13XXX_BUTTON_DBNC_30MS	1
+#define MC13XXX_BUTTON_DBNC_150MS	2
+#define MC13XXX_BUTTON_DBNC_750MS	3
+#define MC13XXX_BUTTON_ENABLE		(1 << 2)
+#define MC13XXX_BUTTON_POL_INVERT	(1 << 3)
+#define MC13XXX_BUTTON_RESET_EN		(1 << 4)
+};
+
 struct mc13xxx_buttons_platform_data {
-#define MC13783_BUTTON_DBNC_0MS		0
-#define MC13783_BUTTON_DBNC_30MS	1
-#define MC13783_BUTTON_DBNC_150MS	2
-#define MC13783_BUTTON_DBNC_750MS	3
-#define MC13783_BUTTON_ENABLE		(1 << 2)
-#define MC13783_BUTTON_POL_INVERT	(1 << 3)
-#define MC13783_BUTTON_RESET_EN		(1 << 4)
-	int b1on_flags;
-	unsigned short b1on_key;
-	int b2on_flags;
-	unsigned short b2on_key;
-	int b3on_flags;
-	unsigned short b3on_key;
+	struct mc13xxx_button	buttons[MAX13XXX_NUM_BUTTONS];
 };
 
 struct mc13xxx_ts_platform_data {
-- 
1.8.1.5


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 RESEND 2/3] input: mc13783: Add MC13892 support
  2013-08-17  5:46 [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF Alexander Shiyan
@ 2013-08-17  5:46 ` Alexander Shiyan
  2013-08-17  6:20 ` [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF Michael Grzeschik
  1 sibling, 0 replies; 5+ messages in thread
From: Alexander Shiyan @ 2013-08-17  5:46 UTC (permalink / raw)
  To: linux-input; +Cc: Dmitry Torokhov, Sascha Hauer, Alexander Shiyan

This patch adds support for MC13892 PMIC in mc13783-pwrbutton driver.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/input/misc/Kconfig             |  6 +++---
 drivers/input/misc/mc13783-pwrbutton.c | 10 +++++++++-
 include/linux/mfd/mc13892.h            |  4 ++++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 0b541cd..0d83653 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -167,10 +167,10 @@ config INPUT_MAX8997_HAPTIC
 	  module will be called max8997-haptic.
 
 config INPUT_MC13783_PWRBUTTON
-	tristate "MC13783 ON buttons"
-	depends on MFD_MC13783
+	tristate "MC13783/MC13892 ON buttons"
+	depends on MFD_MC13XXX
 	help
-	  Support the ON buttons of MC13783 PMIC as an input device
+	  Support the ON buttons of MC13783/MC13892 PMIC as an input device
 	  reporting power button status.
 
 	  To compile this driver as a module, choose M here: the module
diff --git a/drivers/input/misc/mc13783-pwrbutton.c b/drivers/input/misc/mc13783-pwrbutton.c
index aaaacef..2e21f19 100644
--- a/drivers/input/misc/mc13783-pwrbutton.c
+++ b/drivers/input/misc/mc13783-pwrbutton.c
@@ -26,6 +26,7 @@
 #include <linux/input.h>
 #include <linux/platform_device.h>
 #include <linux/mfd/mc13783.h>
+#include <linux/mfd/mc13892.h>
 
 struct mc13xxx_button_def {
 	unsigned int	irq;
@@ -178,8 +179,15 @@ static const struct mc13xxx_pwrb_devtype mc13783_pwrb_devtype = {
 	.btn_def[2] = { MC13783_IRQ_ONOFD3, BIT(5) }
 };
 
+static const struct mc13xxx_pwrb_devtype mc13892_pwrb_devtype = {
+	.btn_def[0] = { MC13892_IRQ_ONOFD1, BIT(3) },
+	.btn_def[1] = { MC13892_IRQ_ONOFD2, BIT(4) },
+	.btn_def[2] = { MC13892_IRQ_ONOFD3, BIT(2) }
+};
+
 static const struct platform_device_id mc13xxx_pwrbutton_id_table[] = {
 	{ "mc13783-pwrbutton", (kernel_ulong_t)&mc13783_pwrb_devtype },
+	{ "mc13892-pwrbutton", (kernel_ulong_t)&mc13892_pwrb_devtype },
 	{ }
 };
 MODULE_DEVICE_TABLE(platform, mc13xxx_pwrbutton_id_table);
@@ -194,6 +202,6 @@ static struct platform_driver mc13xxx_pwrbutton_driver = {
 };
 module_platform_driver_probe(mc13xxx_pwrbutton_driver, mc13xxx_pwrbutton_probe);
 
-MODULE_DESCRIPTION("MC13783 Power Button");
+MODULE_DESCRIPTION("MC13XXX Power Button");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Philippe Retornaz");
diff --git a/include/linux/mfd/mc13892.h b/include/linux/mfd/mc13892.h
index a00f2be..bdc3baf 100644
--- a/include/linux/mfd/mc13892.h
+++ b/include/linux/mfd/mc13892.h
@@ -36,4 +36,8 @@
 #define MC13892_PWGT2SPI	22
 #define MC13892_VCOINCELL	23
 
+#define MC13892_IRQ_ONOFD3	26
+#define MC13892_IRQ_ONOFD1	27
+#define MC13892_IRQ_ONOFD2	28
+
 #endif
-- 
1.8.1.5


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF
  2013-08-17  5:46 [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF Alexander Shiyan
  2013-08-17  5:46 ` [PATCH v3 RESEND 2/3] input: mc13783: Add MC13892 support Alexander Shiyan
@ 2013-08-17  6:20 ` Michael Grzeschik
  2013-08-17  6:53   ` Alexander Shiyan
  1 sibling, 1 reply; 5+ messages in thread
From: Michael Grzeschik @ 2013-08-17  6:20 UTC (permalink / raw)
  To: Alexander Shiyan; +Cc: linux-input, Dmitry Torokhov, Sascha Hauer

Hi Alexander,

On Sat, Aug 17, 2013 at 09:46:20AM +0400, Alexander Shiyan wrote:
> This patch is a preparation mc13xxx powerbutton driver to support
> MC13892 and support the probe through the DT.

As this patch already mention by itself, it's doing to much at once.
And by looking at it, it realy does more than it tells here.

Can you rework that into seperate readable tasks. In this patch you
nearly rewrite the whole file. That runs the review impossible.

> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  arch/arm/mach-imx/mach-mx31moboard.c   |   9 +-
>  drivers/input/misc/mc13783-pwrbutton.c | 332 +++++++++++++--------------------
>  include/linux/mfd/mc13xxx.h            |  28 +--
>  3 files changed, 151 insertions(+), 218 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
> index 6f424ec..2a8aa43 100644
> --- a/arch/arm/mach-imx/mach-mx31moboard.c
> +++ b/arch/arm/mach-imx/mach-mx31moboard.c
> @@ -276,9 +276,12 @@ static struct mc13xxx_leds_platform_data moboard_leds = {
>  };
>  
>  static struct mc13xxx_buttons_platform_data moboard_buttons = {
> -	.b1on_flags = MC13783_BUTTON_DBNC_750MS | MC13783_BUTTON_ENABLE |
> -			MC13783_BUTTON_POL_INVERT,
> -	.b1on_key = KEY_POWER,
> +	.buttons[0] = {
> +		.keycode	= KEY_POWER,
> +		.flags		= MC13XXX_BUTTON_ENABLE |
> +				  MC13XXX_BUTTON_DBNC_750MS |
> +				  MC13XXX_BUTTON_POL_INVERT,
> +	},
>  };
>  
>  static struct mc13xxx_codec_platform_data moboard_codec = {
> diff --git a/drivers/input/misc/mc13783-pwrbutton.c b/drivers/input/misc/mc13783-pwrbutton.c
> index d0277a7..aaaacef 100644
> --- a/drivers/input/misc/mc13783-pwrbutton.c
> +++ b/drivers/input/misc/mc13783-pwrbutton.c
> @@ -24,248 +24,176 @@
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
>  #include <linux/input.h>
> -#include <linux/interrupt.h>
>  #include <linux/platform_device.h>
>  #include <linux/mfd/mc13783.h>
> -#include <linux/sched.h>
> -#include <linux/slab.h>
> -
> -struct mc13783_pwrb {
> -	struct input_dev *pwr;
> -	struct mc13xxx *mc13783;
> -#define MC13783_PWRB_B1_POL_INVERT	(1 << 0)
> -#define MC13783_PWRB_B2_POL_INVERT	(1 << 1)
> -#define MC13783_PWRB_B3_POL_INVERT	(1 << 2)
> -	int flags;
> -	unsigned short keymap[3];
> +
> +struct mc13xxx_button_def {
> +	unsigned int	irq;
> +	unsigned int	sense_bit;
>  };
>  
> -#define MC13783_REG_INTERRUPT_SENSE_1		5
> -#define MC13783_IRQSENSE1_ONOFD1S		(1 << 3)
> -#define MC13783_IRQSENSE1_ONOFD2S		(1 << 4)
> -#define MC13783_IRQSENSE1_ONOFD3S		(1 << 5)
> +struct mc13xxx_pwrb_devtype {
> +	struct mc13xxx_button_def	btn_def[MAX13XXX_NUM_BUTTONS];
> +};
>  
> -#define MC13783_REG_POWER_CONTROL_2		15
> -#define MC13783_POWER_CONTROL_2_ON1BDBNC	4
> -#define MC13783_POWER_CONTROL_2_ON2BDBNC	6
> -#define MC13783_POWER_CONTROL_2_ON3BDBNC	8
> -#define MC13783_POWER_CONTROL_2_ON1BRSTEN	(1 << 1)
> -#define MC13783_POWER_CONTROL_2_ON2BRSTEN	(1 << 2)
> -#define MC13783_POWER_CONTROL_2_ON3BRSTEN	(1 << 3)
> +struct mc13xxx_pwrb {
> +	struct mc13xxx_pwrb_devtype	*devtype;
> +	unsigned int			enabled;
> +	unsigned int			inverted;
> +	u16				btn_code[MAX13XXX_NUM_BUTTONS];
> +	struct input_dev		*input;
> +	struct mc13xxx			*mc13xxx;
> +};
>  
> -static irqreturn_t button_irq(int irq, void *_priv)
> -{
> -	struct mc13783_pwrb *priv = _priv;
> -	int val;
> -
> -	mc13xxx_irq_ack(priv->mc13783, irq);
> -	mc13xxx_reg_read(priv->mc13783, MC13783_REG_INTERRUPT_SENSE_1, &val);
> -
> -	switch (irq) {
> -	case MC13783_IRQ_ONOFD1:
> -		val = val & MC13783_IRQSENSE1_ONOFD1S ? 1 : 0;
> -		if (priv->flags & MC13783_PWRB_B1_POL_INVERT)
> -			val ^= 1;
> -		input_report_key(priv->pwr, priv->keymap[0], val);
> -		break;
> -
> -	case MC13783_IRQ_ONOFD2:
> -		val = val & MC13783_IRQSENSE1_ONOFD2S ? 1 : 0;
> -		if (priv->flags & MC13783_PWRB_B2_POL_INVERT)
> -			val ^= 1;
> -		input_report_key(priv->pwr, priv->keymap[1], val);
> -		break;
> -
> -	case MC13783_IRQ_ONOFD3:
> -		val = val & MC13783_IRQSENSE1_ONOFD3S ? 1 : 0;
> -		if (priv->flags & MC13783_PWRB_B3_POL_INVERT)
> -			val ^= 1;
> -		input_report_key(priv->pwr, priv->keymap[2], val);
> -		break;
> -	}
> +#define MC13XXX_REG_INTERRUPT_SENSE_1	5
> +#define MC13XXX_REG_POWER_CONTROL_2	15
>  
> -	input_sync(priv->pwr);
> +static irqreturn_t mc13xxx_pwrbutton_irq(int irq, void *data)
> +{
> +	struct mc13xxx_pwrb *priv = data;
> +	unsigned int i, val;
> +
> +	mc13xxx_irq_ack(priv->mc13xxx, irq);
> +	mc13xxx_reg_read(priv->mc13xxx, MC13XXX_REG_INTERRUPT_SENSE_1, &val);
> +
> +	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++)
> +		if (irq == priv->devtype->btn_def[i].irq) {
> +			val = !!(val & priv->devtype->btn_def[i].sense_bit);
> +			if (priv->inverted & BIT(i))
> +				val = !val;
> +			input_report_key(priv->input, priv->btn_code[i], val);
> +			input_sync(priv->input);
> +			break;
> +		}
>  
>  	return IRQ_HANDLED;
>  }
>  
> -static int mc13783_pwrbutton_probe(struct platform_device *pdev)
> +static int __init mc13xxx_pwrbutton_probe(struct platform_device *pdev)
>  {
> -	const struct mc13xxx_buttons_platform_data *pdata;
> -	struct mc13xxx *mc13783 = dev_get_drvdata(pdev->dev.parent);
> -	struct input_dev *pwr;
> -	struct mc13783_pwrb *priv;
> -	int err = 0;
> -	int reg = 0;
> -
> -	pdata = dev_get_platdata(&pdev->dev);
> +	struct mc13xxx_buttons_platform_data *pdata =
> +		dev_get_platdata(&pdev->dev);
> +	struct mc13xxx *mc13xxx = dev_get_drvdata(pdev->dev.parent);
> +	struct mc13xxx_pwrb_devtype *devtype =
> +		(struct mc13xxx_pwrb_devtype *)pdev->id_entry->driver_data;
> +	struct mc13xxx_pwrb *priv;
> +	int i, reg = 0, ret = -EINVAL;
> +
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "missing platform data\n");
> +		dev_err(&pdev->dev, "Missing platform data\n");
>  		return -ENODEV;
>  	}
>  
> -	pwr = input_allocate_device();
> -	if (!pwr) {
> -		dev_dbg(&pdev->dev, "Can't allocate power button\n");
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
> -	}
> -
> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> -	if (!priv) {
> -		err = -ENOMEM;
> -		dev_dbg(&pdev->dev, "Can't allocate power button\n");
> -		goto free_input_dev;
> -	}
> -
> -	reg |= (pdata->b1on_flags & 0x3) << MC13783_POWER_CONTROL_2_ON1BDBNC;
> -	reg |= (pdata->b2on_flags & 0x3) << MC13783_POWER_CONTROL_2_ON2BDBNC;
> -	reg |= (pdata->b3on_flags & 0x3) << MC13783_POWER_CONTROL_2_ON3BDBNC;
> -
> -	priv->pwr = pwr;
> -	priv->mc13783 = mc13783;
>  
> -	mc13xxx_lock(mc13783);
> -
> -	if (pdata->b1on_flags & MC13783_BUTTON_ENABLE) {
> -		priv->keymap[0] = pdata->b1on_key;
> -		if (pdata->b1on_key != KEY_RESERVED)
> -			__set_bit(pdata->b1on_key, pwr->keybit);
> -
> -		if (pdata->b1on_flags & MC13783_BUTTON_POL_INVERT)
> -			priv->flags |= MC13783_PWRB_B1_POL_INVERT;
> +	priv->input = devm_input_allocate_device(&pdev->dev);
> +	if (!priv->input)
> +		return -ENOMEM;
>  
> -		if (pdata->b1on_flags & MC13783_BUTTON_RESET_EN)
> -			reg |= MC13783_POWER_CONTROL_2_ON1BRSTEN;
> +	priv->mc13xxx = mc13xxx;
> +	priv->devtype = devtype;
> +	platform_set_drvdata(pdev, priv);
>  
> -		err = mc13xxx_irq_request(mc13783, MC13783_IRQ_ONOFD1,
> -					  button_irq, "b1on", priv);
> -		if (err) {
> -			dev_dbg(&pdev->dev, "Can't request irq\n");
> -			goto free_priv;
> -		}
> +	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++) {
> +		u16 code, invert, reset, debounce;
> +
> +		if (!(pdata->buttons[i].flags & MC13XXX_BUTTON_ENABLE))
> +			continue;
> +		code = pdata->buttons[i].keycode;
> +		invert = !!(pdata->buttons[i].flags &
> +			    MC13XXX_BUTTON_POL_INVERT);
> +		reset = !!(pdata->buttons[i].flags &
> +			   MC13XXX_BUTTON_RESET_EN);
> +		debounce = pdata->buttons[i].flags;
> +
> +		priv->btn_code[i] = code;
> +		if (code != KEY_RESERVED)
> +			__set_bit(code, priv->input->keybit);
> +
> +		priv->enabled |= BIT(i);
> +		priv->inverted |= invert << i;
> +		reg |= reset << (i + 1);
> +		reg |= (debounce & 0x03) << (4 + i * 2);
>  	}
>  
> -	if (pdata->b2on_flags & MC13783_BUTTON_ENABLE) {
> -		priv->keymap[1] = pdata->b2on_key;
> -		if (pdata->b2on_key != KEY_RESERVED)
> -			__set_bit(pdata->b2on_key, pwr->keybit);
> -
> -		if (pdata->b2on_flags & MC13783_BUTTON_POL_INVERT)
> -			priv->flags |= MC13783_PWRB_B2_POL_INVERT;
> -
> -		if (pdata->b2on_flags & MC13783_BUTTON_RESET_EN)
> -			reg |= MC13783_POWER_CONTROL_2_ON2BRSTEN;
> -
> -		err = mc13xxx_irq_request(mc13783, MC13783_IRQ_ONOFD2,
> -					  button_irq, "b2on", priv);
> -		if (err) {
> -			dev_dbg(&pdev->dev, "Can't request irq\n");
> -			goto free_irq_b1;
> +	mc13xxx_lock(mc13xxx);
> +
> +	mc13xxx_reg_rmw(mc13xxx, MC13XXX_REG_POWER_CONTROL_2, 0x3fe, reg);
> +
> +	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++)
> +		if (priv->enabled & BIT(i)) {
> +			ret = mc13xxx_irq_request(mc13xxx,
> +						  devtype->btn_def[i].irq,
> +						  mc13xxx_pwrbutton_irq, NULL,
> +						  priv);
> +			if (ret) {
> +				dev_err(&pdev->dev, "Can't request IRQ%i: %i\n",
> +					devtype->btn_def[i].irq, ret);
> +				priv->enabled &= ~BIT(i);
> +			}
>  		}
> -	}
>  
> -	if (pdata->b3on_flags & MC13783_BUTTON_ENABLE) {
> -		priv->keymap[2] = pdata->b3on_key;
> -		if (pdata->b3on_key != KEY_RESERVED)
> -			__set_bit(pdata->b3on_key, pwr->keybit);
> +	mc13xxx_unlock(mc13xxx);
>  
> -		if (pdata->b3on_flags & MC13783_BUTTON_POL_INVERT)
> -			priv->flags |= MC13783_PWRB_B3_POL_INVERT;
> +	priv->input->name		= "mc13xxx_pwrbutton";
> +	priv->input->phys		= "mc13xxx_pwrbutton/input0";
> +	priv->input->id.bustype		= BUS_HOST;
> +	priv->input->id.vendor		= 0x0001;
> +	priv->input->id.product		= 0x0001;
> +	priv->input->id.version		= 0x0100;
> +	priv->input->keycode		= priv->btn_code;
> +	priv->input->keycodemax		= ARRAY_SIZE(priv->btn_code);
> +	priv->input->keycodesize	= sizeof(priv->btn_code[0]);
> +	__set_bit(EV_KEY, priv->input->evbit);
>  
> -		if (pdata->b3on_flags & MC13783_BUTTON_RESET_EN)
> -			reg |= MC13783_POWER_CONTROL_2_ON3BRSTEN;
> -
> -		err = mc13xxx_irq_request(mc13783, MC13783_IRQ_ONOFD3,
> -					  button_irq, "b3on", priv);
> -		if (err) {
> -			dev_dbg(&pdev->dev, "Can't request irq: %d\n", err);
> -			goto free_irq_b2;
> -		}
> -	}
> +	input_set_drvdata(priv->input, priv);
>  
> -	mc13xxx_reg_rmw(mc13783, MC13783_REG_POWER_CONTROL_2, 0x3FE, reg);
> +	ret = input_register_device(priv->input);
> +	if (ret)
> +		dev_err(&pdev->dev, "Can't register input device: %i\n", ret);
>  
> -	mc13xxx_unlock(mc13783);
> -
> -	pwr->name = "mc13783_pwrbutton";
> -	pwr->phys = "mc13783_pwrbutton/input0";
> -	pwr->dev.parent = &pdev->dev;
> -
> -	pwr->keycode = priv->keymap;
> -	pwr->keycodemax = ARRAY_SIZE(priv->keymap);
> -	pwr->keycodesize = sizeof(priv->keymap[0]);
> -	__set_bit(EV_KEY, pwr->evbit);
> -
> -	err = input_register_device(pwr);
> -	if (err) {
> -		dev_dbg(&pdev->dev, "Can't register power button: %d\n", err);
> -		goto free_irq;
> -	}
> -
> -	platform_set_drvdata(pdev, priv);
> -
> -	return 0;
> -
> -free_irq:
> -	mc13xxx_lock(mc13783);
> -
> -	if (pdata->b3on_flags & MC13783_BUTTON_ENABLE)
> -		mc13xxx_irq_free(mc13783, MC13783_IRQ_ONOFD3, priv);
> -
> -free_irq_b2:
> -	if (pdata->b2on_flags & MC13783_BUTTON_ENABLE)
> -		mc13xxx_irq_free(mc13783, MC13783_IRQ_ONOFD2, priv);
> -
> -free_irq_b1:
> -	if (pdata->b1on_flags & MC13783_BUTTON_ENABLE)
> -		mc13xxx_irq_free(mc13783, MC13783_IRQ_ONOFD1, priv);
> -
> -free_priv:
> -	mc13xxx_unlock(mc13783);
> -	kfree(priv);
> -
> -free_input_dev:
> -	input_free_device(pwr);
> -
> -	return err;
> +	return ret;
>  }
>  
> -static int mc13783_pwrbutton_remove(struct platform_device *pdev)
> +static int mc13xxx_pwrbutton_remove(struct platform_device *pdev)
>  {
> -	struct mc13783_pwrb *priv = platform_get_drvdata(pdev);
> -	const struct mc13xxx_buttons_platform_data *pdata;
> -
> -	pdata = dev_get_platdata(&pdev->dev);
> -
> -	mc13xxx_lock(priv->mc13783);
> -
> -	if (pdata->b3on_flags & MC13783_BUTTON_ENABLE)
> -		mc13xxx_irq_free(priv->mc13783, MC13783_IRQ_ONOFD3, priv);
> -	if (pdata->b2on_flags & MC13783_BUTTON_ENABLE)
> -		mc13xxx_irq_free(priv->mc13783, MC13783_IRQ_ONOFD2, priv);
> -	if (pdata->b1on_flags & MC13783_BUTTON_ENABLE)
> -		mc13xxx_irq_free(priv->mc13783, MC13783_IRQ_ONOFD1, priv);
> +	struct mc13xxx_pwrb *priv = platform_get_drvdata(pdev);
> +	int i;
>  
> -	mc13xxx_unlock(priv->mc13783);
> -
> -	input_unregister_device(priv->pwr);
> -	kfree(priv);
> +	mc13xxx_lock(priv->mc13xxx);
> +	for (i = 0; i < MAX13XXX_NUM_BUTTONS; i++)
> +		if (priv->enabled & BIT(i))
> +			mc13xxx_irq_free(priv->mc13xxx,
> +					 priv->devtype->btn_def[i].irq, priv);
> +	mc13xxx_unlock(priv->mc13xxx);
>  
>  	return 0;
>  }
>  
> -static struct platform_driver mc13783_pwrbutton_driver = {
> -	.probe		= mc13783_pwrbutton_probe,
> -	.remove		= mc13783_pwrbutton_remove,
> -	.driver		= {
> -		.name	= "mc13783-pwrbutton",
> +static const struct mc13xxx_pwrb_devtype mc13783_pwrb_devtype = {
> +	.btn_def[0] = { MC13783_IRQ_ONOFD1, BIT(3) },
> +	.btn_def[1] = { MC13783_IRQ_ONOFD2, BIT(4) },
> +	.btn_def[2] = { MC13783_IRQ_ONOFD3, BIT(5) }
> +};
> +
> +static const struct platform_device_id mc13xxx_pwrbutton_id_table[] = {
> +	{ "mc13783-pwrbutton", (kernel_ulong_t)&mc13783_pwrb_devtype },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, mc13xxx_pwrbutton_id_table);
> +
> +static struct platform_driver mc13xxx_pwrbutton_driver = {
> +	.driver	= {
> +		.name	= "mc13xxx-pwrbutton",
>  		.owner	= THIS_MODULE,
>  	},
> +	.remove		= mc13xxx_pwrbutton_remove,
> +	.id_table	= mc13xxx_pwrbutton_id_table,
>  };
> +module_platform_driver_probe(mc13xxx_pwrbutton_driver, mc13xxx_pwrbutton_probe);
>  
> -module_platform_driver(mc13783_pwrbutton_driver);
> -
> -MODULE_ALIAS("platform:mc13783-pwrbutton");
>  MODULE_DESCRIPTION("MC13783 Power Button");
>  MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Philippe Retornaz");
> diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
> index 41ed592..b895538 100644
> --- a/include/linux/mfd/mc13xxx.h
> +++ b/include/linux/mfd/mc13xxx.h
> @@ -142,20 +142,22 @@ struct mc13xxx_leds_platform_data {
>  	u32 led_control[MAX_LED_CONTROL_REGS];
>  };
>  
> +#define MAX13XXX_NUM_BUTTONS	3
> +
> +struct mc13xxx_button {
> +	u16		keycode;
> +	unsigned int	flags;
> +#define MC13XXX_BUTTON_DBNC_0MS		0
> +#define MC13XXX_BUTTON_DBNC_30MS	1
> +#define MC13XXX_BUTTON_DBNC_150MS	2
> +#define MC13XXX_BUTTON_DBNC_750MS	3
> +#define MC13XXX_BUTTON_ENABLE		(1 << 2)
> +#define MC13XXX_BUTTON_POL_INVERT	(1 << 3)
> +#define MC13XXX_BUTTON_RESET_EN		(1 << 4)
> +};
> +
>  struct mc13xxx_buttons_platform_data {
> -#define MC13783_BUTTON_DBNC_0MS		0
> -#define MC13783_BUTTON_DBNC_30MS	1
> -#define MC13783_BUTTON_DBNC_150MS	2
> -#define MC13783_BUTTON_DBNC_750MS	3
> -#define MC13783_BUTTON_ENABLE		(1 << 2)
> -#define MC13783_BUTTON_POL_INVERT	(1 << 3)
> -#define MC13783_BUTTON_RESET_EN		(1 << 4)
> -	int b1on_flags;
> -	unsigned short b1on_key;
> -	int b2on_flags;
> -	unsigned short b2on_key;
> -	int b3on_flags;
> -	unsigned short b3on_key;
> +	struct mc13xxx_button	buttons[MAX13XXX_NUM_BUTTONS];
>  };
>  
>  struct mc13xxx_ts_platform_data {
> -- 
> 1.8.1.5
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF
  2013-08-17  6:20 ` [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF Michael Grzeschik
@ 2013-08-17  6:53   ` Alexander Shiyan
  2013-08-17  9:23     ` Sascha Hauer
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Shiyan @ 2013-08-17  6:53 UTC (permalink / raw)
  To: Michael Grzeschik; +Cc: linux-input, Dmitry Torokhov, Sascha Hauer

On Sat, 17 Aug 2013 08:20:12 +0200
Michael Grzeschik <mgr@pengutronix.de> wrote:

> On Sat, Aug 17, 2013 at 09:46:20AM +0400, Alexander Shiyan wrote:
> > This patch is a preparation mc13xxx powerbutton driver to support
> > MC13892 and support the probe through the DT.
> 
> As this patch already mention by itself, it's doing to much at once.
> And by looking at it, it realy does more than it tells here.
> 
> Can you rework that into seperate readable tasks. In this patch you
> nearly rewrite the whole file. That runs the review impossible.

All changes in this patch are addicted to each other, can not be divided.
As an alternative, I can do all as a new driver.
Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF
  2013-08-17  6:53   ` Alexander Shiyan
@ 2013-08-17  9:23     ` Sascha Hauer
  0 siblings, 0 replies; 5+ messages in thread
From: Sascha Hauer @ 2013-08-17  9:23 UTC (permalink / raw)
  To: Alexander Shiyan
  Cc: Michael Grzeschik, linux-input, Dmitry Torokhov, Sascha Hauer

On Sat, Aug 17, 2013 at 10:53:05AM +0400, Alexander Shiyan wrote:
> On Sat, 17 Aug 2013 08:20:12 +0200
> Michael Grzeschik <mgr@pengutronix.de> wrote:
> 
> > On Sat, Aug 17, 2013 at 09:46:20AM +0400, Alexander Shiyan wrote:
> > > This patch is a preparation mc13xxx powerbutton driver to support
> > > MC13892 and support the probe through the DT.
> > 
> > As this patch already mention by itself, it's doing to much at once.
> > And by looking at it, it realy does more than it tells here.
> > 
> > Can you rework that into seperate readable tasks. In this patch you
> > nearly rewrite the whole file. That runs the review impossible.
> 
> All changes in this patch are addicted to each other, can not be divided.

I find this hard to believe. Here are some changes that can be separated
easily:

- Rename defines from MC13XXX_BUTTON_ to MC13783_BUTTON_
- convert module_platform_driver to module_platform_driver_probe, why do
  you change this anyway?
- convert from kzalloc to devm_kzalloc
- rewrite error message from "missing platform data" to "Missing
  platform data"

Drop these changes or make them separate patches and your original patch
would be smaller already.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2013-08-17  9:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-17  5:46 [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF Alexander Shiyan
2013-08-17  5:46 ` [PATCH v3 RESEND 2/3] input: mc13783: Add MC13892 support Alexander Shiyan
2013-08-17  6:20 ` [PATCH v3 RESEND 1/3] input: mc13783: Prepare driver to support MC13892 and OF Michael Grzeschik
2013-08-17  6:53   ` Alexander Shiyan
2013-08-17  9:23     ` Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).