Skip to contentSkip to conversation

This patch repairs HTC Magician machine (PXA27x) support

RFC PATCH New Petr Cvek 4 participants 5 messages Aug 12, 2015 – Aug 13, 2015

Subsystems (5)
lore
Share & tools
lore

Petr Cvek

author
#0 lore raw

RFC PATCH 1/1 This patch repairs HTC Magician machine (PXA27x) support

Fixing original code: Problems to successful boot due to the bad LCD power sequence (wrongly configured GPIO).

Add/fix platform data and helper function for various hardware (touchscreen, audio, USB device, ...).

Add new discovered GPIOs and fix names by GPIO context.

Add new LEDs driver.

---
 arch/arm/mach-pxa/Kconfig                 |    1 +
 arch/arm/mach-pxa/include/mach/magician.h |   21 +-
 arch/arm/mach-pxa/magician.c              | 1148 +++++++++++++++++++++++------
 drivers/leds/Kconfig                      |    9 +
 drivers/leds/Makefile                     |    1 +
 drivers/leds/leds-pasic3.c                |  192 +++++
 drivers/mfd/htc-pasic3.c                  |  115 ++-
 include/linux/mfd/htc-pasic3.h            |   69 +-
 8 files changed, 1293 insertions(+), 263 deletions(-)
 create mode 100644 drivers/leds/leds-pasic3.c
arch/arm/mach-pxa/Kconfig +1 -0
index f096836..5c7b6ee 100644
@@ -294,6 +294,7 @@ config MACH_MAGICIAN
 	bool "Enable HTC Magician Support"
 	select IWMMXT
 	select PXA27x
+	select HTC_EGPIO
 
 config MACH_MIOA701
 	bool "Mitac Mio A701 Support"
arch/arm/mach-pxa/include/mach/magician.h +15 -6
index ba6a6e1..8d3936d 100644
@@ -32,6 +32,8 @@
 #define GPIO37_MAGICIAN_KEY_HANGUP		37
 #define GPIO38_MAGICIAN_KEY_CONTACTS		38
 #define GPIO40_MAGICIAN_GSM_OUT2		40
+#define GPIO46_MAGICIAN_IR_RX			46
+#define GPIO47_MAGICIAN_IR_TX			47
 #define GPIO48_MAGICIAN_UNKNOWN			48
 #define GPIO56_MAGICIAN_UNKNOWN			56
 #define GPIO57_MAGICIAN_CAM_RESET		57
@@ -52,14 +54,16 @@
 #define GPIO101_MAGICIAN_KEY_VOL_DOWN 		101
 #define GPIO102_MAGICIAN_KEY_PHONE		102
 #define GPIO103_MAGICIAN_LED_KP			103
-#define GPIO104_MAGICIAN_LCD_POWER_1 		104
-#define GPIO105_MAGICIAN_LCD_POWER_2		105
-#define GPIO106_MAGICIAN_LCD_POWER_3		106
+#define GPIO104_MAGICIAN_LCD_VOFF_EN           104
+#define GPIO105_MAGICIAN_LCD_VON_EN            105
+#define GPIO106_MAGICIAN_LCD_DCDC_NRESET       106
 #define GPIO107_MAGICIAN_DS1WM_IRQ		107
 #define GPIO108_MAGICIAN_GSM_READY		108
 #define GPIO114_MAGICIAN_UNKNOWN		114
 #define GPIO115_MAGICIAN_nPEN_IRQ		115
 #define GPIO116_MAGICIAN_nCAM_EN		116
+#define GPIO117_MAGICIAN_I2C_SCL		117
+#define GPIO118_MAGICIAN_I2C_SDA		118
 #define GPIO119_MAGICIAN_UNKNOWN		119
 #define GPIO120_MAGICIAN_UNKNOWN		120
 
@@ -98,16 +102,19 @@
 #define EGPIO_MAGICIAN_BL_POWER			MAGICIAN_EGPIO(1, 7)
 #define EGPIO_MAGICIAN_SD_POWER			MAGICIAN_EGPIO(2, 0)
 #define EGPIO_MAGICIAN_CARKIT_MIC		MAGICIAN_EGPIO(2, 1)
-#define EGPIO_MAGICIAN_UNKNOWN_WAVEDEV_DLL	MAGICIAN_EGPIO(2, 2)
+#define EGPIO_MAGICIAN_IR_RECEIVE_SHUTDOWN		MAGICIAN_EGPIO(2, 2)
 #define EGPIO_MAGICIAN_FLASH_VPP		MAGICIAN_EGPIO(2, 3)
 #define EGPIO_MAGICIAN_BL_POWER2		MAGICIAN_EGPIO(2, 4)
 #define EGPIO_MAGICIAN_BQ24022_ISET2		MAGICIAN_EGPIO(2, 5)
+#define EGPIO_MAGICIAN_NICD_CHARGE		MAGICIAN_EGPIO(2, 6)
 #define EGPIO_MAGICIAN_GSM_POWER		MAGICIAN_EGPIO(2, 7)
 
 /* input */
 
-#define EGPIO_MAGICIAN_CABLE_STATE_AC		MAGICIAN_EGPIO(4, 0)
-#define EGPIO_MAGICIAN_CABLE_STATE_USB		MAGICIAN_EGPIO(4, 1)
+/* AC=1, USB=0 */
+#define EGPIO_MAGICIAN_CABLE_TYPE		MAGICIAN_EGPIO(4, 0)
+/* =1 when AC or USB cable inserted */
+#define EGPIO_MAGICIAN_CABLE_INSERT1		MAGICIAN_EGPIO(4, 1)
 
 #define EGPIO_MAGICIAN_BOARD_ID0		MAGICIAN_EGPIO(5, 0)
 #define EGPIO_MAGICIAN_BOARD_ID1		MAGICIAN_EGPIO(5, 1)
@@ -116,5 +123,7 @@
 #define EGPIO_MAGICIAN_nSD_READONLY		MAGICIAN_EGPIO(5, 4)
 
 #define EGPIO_MAGICIAN_EP_INSERT		MAGICIAN_EGPIO(6, 1)
+/* FIXME same like 4,1, may differ for host/device */
+#define EGPIO_MAGICIAN_CABLE_INSERT2	MAGICIAN_EGPIO(6, 4)
 
 #endif /* _MAGICIAN_H_ */
arch/arm/mach-pxa/magician.c +924 -224

1,595 lines not on this page · raw message

drivers/leds/Kconfig +9 -0
index 9ad35f7..516ba65 100644
@@ -586,6 +586,15 @@ config LEDS_PM8941_WLED
 	  This option enables support for the 'White' LED block
 	  on Qualcomm PM8941 PMICs.
 
+config LEDS_PASIC3
+	tristate "LED support for the HTC Magician/Alpine PASIC3"
+	depends on LEDS_CLASS
+	depends on HTC_PASIC3
+	select REGMAP
+	help
+	  This option enables support for the PASIC3 chip (different chip
+	  than Compaq ASIC3).
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
drivers/leds/Makefile +1 -0
index 8d6a24a..b1c659c 100644
@@ -65,6 +65,7 @@ obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
 obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
 obj-$(CONFIG_LEDS_PM8941_WLED)		+= leds-pm8941-wled.o
 obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
+obj-$(CONFIG_LEDS_PASIC3)		+= leds-pasic3.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
drivers/leds/leds-pasic3.c new file +192 -0

192 lines not on this page · raw message

drivers/mfd/htc-pasic3.c +102 -13

159 lines not on this page · raw message

include/linux/mfd/htc-pasic3.h +49 -20

85 lines not on this page · raw message

Signature · 1 line
1.7.12.1
author
#1 lore raw

RFC PATCH 1/1 This patch repairs HTC Magician machine (PXA27x) support

Dne 13.8.2015 v 00:51 Petr Cvek napsal(a):

Quoted 15 lines
drivers/mfd/htc-pasic3.c +3 -0
index e88d4f6..3df3f0a 100644
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Philipp Zabel <[email protected]>
  *
+ * LED support:
+ * Copyright (C) 2015 Petr Cvek <[email protected]>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.

drivers/mfd/htc-pasic3.c EXPORT_SYMBOL(pasic3_read_register); /* for leds-pasic3 */ · line 69

Quoted 2 lines
drivers/mfd/htc-pasic3.c +0 -0
@@ -65,8 +68,76 @@ EXPORT_SYMBOL(pasic3_read_register); /* for leds-pasic3 */
  * LEDs
  */

Just some P.S.

Checkpatch warns me about spaces around * in this section. It seems it thinks it is some pointer. Is this behavior OK (I assume it cannot find begining of comment or am I doing something bad?).

Checkpatch also throws warning about new file and MAINTAINERS list updating (for leds-pasic3.c). I assume it falls into LEDs subsystem.

As the patch changes the definitions for many subsystems, are there another people who should be awared of this patch?

Cheers,
Petr Cvek

#2 lore raw

RFC PATCH 1/1 This patch repairs HTC Magician machine (PXA27x) support

In reply to Petr Cvek

Hi Petr,

This is review only of the LED subsystem related part of this patch,.

Quoted 23 lines · Petr Cvek

Fixing original code: Problems to successful boot due to the bad LCD power sequence (wrongly configured GPIO).

Add/fix platform data and helper function for various hardware (touchscreen, audio, USB device, ...).

Add new discovered GPIOs and fix names by GPIO context.

Add new LEDs driver.

---
  arch/arm/mach-pxa/Kconfig                 |    1 +
  arch/arm/mach-pxa/include/mach/magician.h |   21 +-
  arch/arm/mach-pxa/magician.c              | 1148 +++++++++++++++++++++++------
  drivers/leds/Kconfig                      |    9 +
  drivers/leds/Makefile                     |    1 +
  drivers/leds/leds-pasic3.c                |  192 +++++
  drivers/mfd/htc-pasic3.c                  |  115 ++-
  include/linux/mfd/htc-pasic3.h            |   69 +-
  8 files changed, 1293 insertions(+), 263 deletions(-)

create mode 100644 drivers/leds/leds-pasic3.c

[...]

Quoted 46 lines
drivers/leds/Kconfig +9 -0
index 9ad35f7..516ba65 100644
@@ -586,6 +586,15 @@ config LEDS_PM8941_WLED
  	  This option enables support for the 'White' LED block
  	  on Qualcomm PM8941 PMICs.
 
+config LEDS_PASIC3
+	tristate "LED support for the HTC Magician/Alpine PASIC3"
+	depends on LEDS_CLASS
+	depends on HTC_PASIC3
+	select REGMAP
+	help
+	  This option enables support for the PASIC3 chip (different chip
+	  than Compaq ASIC3).
+
  comment "LED Triggers"
  source "drivers/leds/trigger/Kconfig"
 
drivers/leds/Makefile +1 -0
index 8d6a24a..b1c659c 100644
@@ -65,6 +65,7 @@ obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
  obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
  obj-$(CONFIG_LEDS_PM8941_WLED)		+= leds-pm8941-wled.o
  obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
+obj-$(CONFIG_LEDS_PASIC3)		+= leds-pasic3.o
 
  # LED SPI Drivers
  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
drivers/leds/leds-pasic3.c new file +8 -0
new file mode 100644
index 0000000..ecb0557
@@ -0,0 +1,192 @@
+/*
+ * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)
+ *
+ * Copyright (c) 2015 Petr Cvek <[email protected]>
+ *
+ * Based on leds-asic3.c driver
+ *
+ * This program is free software; you can redistribute it and/or modify

drivers/leds/leds-pasic3.c line 18

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+

This empty line is not required.

drivers/leds/leds-pasic3.c line 21

Quoted 3 lines
drivers/leds/leds-pasic3.c new file +3 -0
+#include <linux/mfd/htc-pasic3.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>

Please keep alphabetical order across all includes.

Quoted 26 lines
drivers/leds/leds-pasic3.c new file +26 -0
+/*
+ * 1 tick is around 62-63 ms, blinking settings (on+off):
+ *	Min: 1+1 ticks ~125ms
+ *	Max: 127+127 ticks ~15?875ms
+ * Sometimes takes time to change after write (counter overflow?)
+ */
+
+#define MS_TO_CLK(ms)	DIV_ROUND_CLOSEST(((ms)*1024), 64000)
+#define CLK_TO_MS(clk)	(((clk)*64000)/1024)
+#define MAX_CLK		254		/* 127 + 127 (2x 7 bit reg) */
+#define MAX_MS		CLK_TO_MS(MAX_CLK)
+
+static void brightness_set(struct led_classdev *cdev,
+	enum led_brightness value)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct pasic3_led *led;
+	struct device *dev;
+	u8 val;
+
+	if (!cell->platform_data) {
+		pr_err("No platform data\n");
+		return;
+	}
+

drivers/leds/leds-pasic3.c line 58

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+	led = cell->platform_data;
+	dev = pdev->dev.parent;
+
+	if (value == LED_OFF) {
+		val = pasic3_read_register(dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(dev, PASIC3_CH_CONTROL,
+			val & ~(led->bit_blink_en | led->bit_force_on));
+
+		val = pasic3_read_register(dev, PASIC3_MASK_A);
+		pasic3_write_register(dev, PASIC3_MASK_A, val & ~led->bit_mask);

I think that you need spin_lock protection as setting brightness is not an atomic operation here. Moreover pasic3_write_register calls __raw_writeb twice, without spin_lock protection. This function is used also in drivers/mfd/htc-pasic3.c - shouldn't it be fixed?

Quoted 38 lines
drivers/leds/leds-pasic3.c new file +38 -0
+	} else {
+		val = pasic3_read_register(dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(dev, PASIC3_CH_CONTROL,
+			val | led->bit_force_on);
+	}
+}
+
+static int blink_set(struct led_classdev *cdev,
+	unsigned long *delay_on,
+	unsigned long *delay_off)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct device *dev;
+	struct pasic3_led *led;
+	u32 on, off;
+	u8 val;
+
+	if (!cell->platform_data) {
+		pr_err("No platform data\n");
+		return -EINVAL;
+	}
+
+	if (*delay_on > MAX_MS || *delay_off > MAX_MS)
+		return -EINVAL;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		/* If both are zero then a sensible default should be chosen */
+		on = MS_TO_CLK(500);
+		off = MS_TO_CLK(500);
+	} else {
+		on = MS_TO_CLK(*delay_on);
+		off = MS_TO_CLK(*delay_off);
+		if ((on + off) > MAX_CLK)
+			return -EINVAL;
+		/* Minimal value must be 1 */
+		on = on ? on : 1;
+		off = off ? off : 1;

drivers/leds/leds-pasic3.c line 106

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+	}
+
+	led = cell->platform_data;
+	dev = pdev->dev.parent;
+
+	pasic3_write_register(dev, led->reg_delay_on, on);
+	pasic3_write_register(dev, led->reg_delay_off, off);
+
+	val = pasic3_read_register(dev, PASIC3_CH_CONTROL);
+	pasic3_write_register(dev, PASIC3_CH_CONTROL, val | led->bit_blink_en);

Ditto.

Quoted 8 lines
drivers/leds/leds-pasic3.c new file +8 -0
+	*delay_on = CLK_TO_MS(on);
+	*delay_off = CLK_TO_MS(off);
+
+	return 0;
+}
+
+static int pasic3_led_probe(struct platform_device *pdev)
+{

drivers/leds/leds-pasic3.c line 125

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+	struct pasic3_led *led = dev_get_platdata(&pdev->dev);
+	int ret;
+
+	ret = mfd_cell_enable(pdev);
+	if (ret < 0)
+		return ret;
+
+	led->cdev.flags = LED_CORE_SUSPENDRESUME;
+	led->cdev.brightness_set = brightness_set;
+	led->cdev.blink_set = blink_set;

You need to set also led->cdev.name. What is the name of your LED in the sysfs now?

drivers/leds/leds-pasic3.c line 129

Quoted 3 lines
drivers/leds/leds-pasic3.c new file +3 -0
+	ret = led_classdev_register(&pdev->dev, &led->cdev);
+	if (ret < 0)
+		goto out;

Please use devm_led_classdev_register.

Quoted 62 lines
drivers/leds/leds-pasic3.c new file +62 -0
+	return 0;
+
+out:
+	(void) mfd_cell_disable(pdev);
+	return ret;
+}
+
+static int pasic3_led_remove(struct platform_device *pdev)
+{
+	struct pasic3_led *led = dev_get_platdata(&pdev->dev);
+
+	led_classdev_unregister(&led->cdev);
+
+	return mfd_cell_disable(pdev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pasic3_led_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	int ret;
+
+	ret = 0;
+	if (cell->suspend)
+		ret = (*cell->suspend)(pdev);
+
+	return ret;
+}
+
+static int pasic3_led_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	int ret;
+
+	ret = 0;
+	if (cell->resume)
+		ret = (*cell->resume)(pdev);
+
+	return ret;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pasic3_led_pm_ops, pasic3_led_suspend,
+	pasic3_led_resume);
+
+static struct platform_driver pasic3_led_driver = {
+	.probe		= pasic3_led_probe,
+	.remove		= pasic3_led_remove,
+	.driver		= {
+		.name	= "leds-pasic3",
+		.pm	= &pasic3_led_pm_ops,
+	},
+};
+
+module_platform_driver(pasic3_led_driver);
+
+MODULE_AUTHOR("Petr Cvek <[email protected]>");
+MODULE_DESCRIPTION("HTC Magician PASIC3 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-pasic3");

[...]

Signature · 2 lines
Best Regards,
Jacek Anaszewski
#3 lore raw

RFC PATCH 1/1 This patch repairs HTC Magician machine (PXA27x) support

In reply to Petr Cvek
Quoted 11 lines · Petr Cvek

Fixing original code: Problems to successful boot due to the bad LCD power sequence (wrongly configured GPIO).

Add/fix platform data and helper function for various hardware (touchscreen, audio, USB device, ...).

Add new discovered GPIOs and fix names by GPIO context.

Add new LEDs driver.

Quoted 11 lines
---
 arch/arm/mach-pxa/Kconfig                 |    1 +
 arch/arm/mach-pxa/include/mach/magician.h |   21 +-
 arch/arm/mach-pxa/magician.c              | 1148 +++++++++++++++++++++++------
 drivers/leds/Kconfig                      |    9 +
 drivers/leds/Makefile                     |    1 +
 drivers/leds/leds-pasic3.c                |  192 +++++
 drivers/mfd/htc-pasic3.c                  |  115 ++-
 include/linux/mfd/htc-pasic3.h            |   69 +-
 8 files changed, 1293 insertions(+), 263 deletions(-)
 create mode 100644 drivers/leds/leds-pasic3.c

I'm pretty sure there aren't strong enough dependence to warrant all of this code being shoved into a single patch. Please do what you can to break it up into simple, manageable pieces which will make the review and maintenance (patch acceptance) process easier.

[...]

Quoted 35 lines
drivers/leds/leds-pasic3.c new file +29 -0
new file mode 100644
index 0000000..ecb0557
@@ -0,0 +1,192 @@
+/*
+ * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)
+ *
+ * Copyright (c) 2015 Petr Cvek <[email protected]>
+ *
+ * Based on leds-asic3.c driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+
+#include <linux/mfd/htc-pasic3.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+
+/*
+ * 1 tick is around 62-63 ms, blinking settings (on+off):
+ *	Min: 1+1 ticks ~125ms
+ *	Max: 127+127 ticks ~15?875ms
+ * Sometimes takes time to change after write (counter overflow?)
+ */
+

drivers/leds/leds-pasic3.c line 39

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+#define MS_TO_CLK(ms)	DIV_ROUND_CLOSEST(((ms)*1024), 64000)
+#define CLK_TO_MS(clk)	(((clk)*64000)/1024)
+#define MAX_CLK		254		/* 127 + 127 (2x 7 bit reg) */
+#define MAX_MS		CLK_TO_MS(MAX_CLK)
+
+static void brightness_set(struct led_classdev *cdev,
+	enum led_brightness value)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);

This device should have no idea it's part of an MFD.

Just fetch platform data in the normal way.

Quoted 22 lines
drivers/leds/leds-pasic3.c new file +22 -0
+	struct pasic3_led *led;
+	struct device *dev;
+	u8 val;
+
+	if (!cell->platform_data) {
+		pr_err("No platform data\n");
+		return;
+	}
+
+	led = cell->platform_data;
+	dev = pdev->dev.parent;
+
+	if (value == LED_OFF) {
+		val = pasic3_read_register(dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(dev, PASIC3_CH_CONTROL,
+			val & ~(led->bit_blink_en | led->bit_force_on));
+
+		val = pasic3_read_register(dev, PASIC3_MASK_A);
+		pasic3_write_register(dev, PASIC3_MASK_A, val & ~led->bit_mask);
+	} else {
+		val = pasic3_read_register(dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(dev, PASIC3_CH_CONTROL,

drivers/leds/leds-pasic3.c line 71

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+			val | led->bit_force_on);
+	}
+}
+
+static int blink_set(struct led_classdev *cdev,
+	unsigned long *delay_on,
+	unsigned long *delay_off)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);

As above.

Quoted 40 lines
drivers/leds/leds-pasic3.c new file +40 -0
+	struct device *dev;
+	struct pasic3_led *led;
+	u32 on, off;
+	u8 val;
+
+	if (!cell->platform_data) {
+		pr_err("No platform data\n");
+		return -EINVAL;
+	}
+
+	if (*delay_on > MAX_MS || *delay_off > MAX_MS)
+		return -EINVAL;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		/* If both are zero then a sensible default should be chosen */
+		on = MS_TO_CLK(500);
+		off = MS_TO_CLK(500);
+	} else {
+		on = MS_TO_CLK(*delay_on);
+		off = MS_TO_CLK(*delay_off);
+		if ((on + off) > MAX_CLK)
+			return -EINVAL;
+		/* Minimal value must be 1 */
+		on = on ? on : 1;
+		off = off ? off : 1;
+	}
+
+	led = cell->platform_data;
+	dev = pdev->dev.parent;
+
+	pasic3_write_register(dev, led->reg_delay_on, on);
+	pasic3_write_register(dev, led->reg_delay_off, off);
+
+	val = pasic3_read_register(dev, PASIC3_CH_CONTROL);
+	pasic3_write_register(dev, PASIC3_CH_CONTROL, val | led->bit_blink_en);
+
+	*delay_on = CLK_TO_MS(on);
+	*delay_off = CLK_TO_MS(off);
+
+	return 0;

drivers/leds/leds-pasic3.c line 121

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+}
+
+static int pasic3_led_probe(struct platform_device *pdev)
+{
+	struct pasic3_led *led = dev_get_platdata(&pdev->dev);
+	int ret;
+
+	ret = mfd_cell_enable(pdev);
+	if (ret < 0)
+		return ret;

That is not what .enable is for, please don't use it.

Quoted 12 lines
drivers/leds/leds-pasic3.c new file +12 -0
+	led->cdev.flags = LED_CORE_SUSPENDRESUME;
+	led->cdev.brightness_set = brightness_set;
+	led->cdev.blink_set = blink_set;
+
+	ret = led_classdev_register(&pdev->dev, &led->cdev);
+	if (ret < 0)
+		goto out;
+
+	return 0;
+
+out:
+	(void) mfd_cell_disable(pdev);

drivers/leds/leds-pasic3.c line 144

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+	return ret;
+}
+
+static int pasic3_led_remove(struct platform_device *pdev)
+{
+	struct pasic3_led *led = dev_get_platdata(&pdev->dev);
+
+	led_classdev_unregister(&led->cdev);
+
+	return mfd_cell_disable(pdev);

Likewise.

Quoted 15 lines
drivers/leds/leds-pasic3.c new file +15 -0
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pasic3_led_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	int ret;
+
+	ret = 0;
+	if (cell->suspend)
+		ret = (*cell->suspend)(pdev);
+
+	return ret;
+}

drivers/leds/leds-pasic3.c line 169

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+
+static int pasic3_led_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	int ret;
+
+	ret = 0;
+	if (cell->resume)
+		ret = (*cell->resume)(pdev);

Err, no. Move the cell->resume() code into here.

Quoted 11 lines
drivers/leds/leds-pasic3.c new file +11 -0
+	return ret;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pasic3_led_pm_ops, pasic3_led_suspend,
+	pasic3_led_resume);
+
+static struct platform_driver pasic3_led_driver = {
+	.probe		= pasic3_led_probe,
+	.remove		= pasic3_led_remove,
+	.driver		= {

drivers/leds/leds-pasic3.c line 191

Quoted 10 lines
drivers/leds/leds-pasic3.c new file +10 -0
+		.name	= "leds-pasic3",
+		.pm	= &pasic3_led_pm_ops,
+	},
+};
+
+module_platform_driver(pasic3_led_driver);
+
+MODULE_AUTHOR("Petr Cvek <[email protected]>");
+MODULE_DESCRIPTION("HTC Magician PASIC3 LED driver");
+MODULE_LICENSE("GPL");

v2

drivers/mfd/htc-pasic3.c line 8

Quoted 12 lines
drivers/leds/leds-pasic3.c new file +1 -0
+MODULE_ALIAS("platform:leds-pasic3");
drivers/mfd/htc-pasic3.c +3 -0
index e88d4f6..3df3f0a 100644
@@ -3,6 +3,9 @@
  *
  * Copyright (C) 2006 Philipp Zabel <[email protected]>
  *
+ * LED support:
+ * Copyright (C) 2015 Petr Cvek <[email protected]>
+ *

You don't need to break up the copyright statements. Remove the "LED support" line and the blank line above it.

Quoted 13 lines
drivers/mfd/htc-pasic3.c +4 -2
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.
@@ -65,8 +68,76 @@ EXPORT_SYMBOL(pasic3_read_register); /* for leds-pasic3 */
  * LEDs
  */
 
-static struct mfd_cell led_cell __initdata = {
-	.name = "leds-pasic3",
+
+static int pasic3_leds_enable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);

drivers/mfd/htc-pasic3.c EXPORT_SYMBOL(pasic3_read_register); /* for leds-pasic3 */ · line 84

Quoted 10 lines
drivers/mfd/htc-pasic3.c +10 -0
+	struct pasic3_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct pasic3_led *led;
+
+	led = cell->platform_data;
+	pdata = led->pdata;
+
+	gpio_set_value(pdata->power_gpio, 1);
+
+	return 0;
+}

You need to re-think this function (you're doing some pretty wacky stuff in there) and move it into the LED driver.

drivers/mfd/htc-pasic3.c EXPORT_SYMBOL(pasic3_read_register); /* for leds-pasic3 */ · line 98

Quoted 13 lines
drivers/mfd/htc-pasic3.c +13 -0
+static int pasic3_leds_disable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct pasic3_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct pasic3_led *led;
+
+	led = cell->platform_data;
+	pdata = led->pdata;
+
+	gpio_set_value(pdata->power_gpio, 0);
+
+	return 0;
+}

And this one.

drivers/mfd/htc-pasic3.c EXPORT_SYMBOL(pasic3_read_register); /* for leds-pasic3 */ · line 112

Quoted 13 lines
drivers/mfd/htc-pasic3.c +13 -0
+static int pasic3_leds_suspend(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct pasic3_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct pasic3_led *led;
+
+	led = cell->platform_data;
+	pdata = led->pdata;
+
+	gpio_set_value(pdata->power_gpio, 0);
+
+	return 0;
+}

And this one.

Quoted 18 lines
drivers/mfd/htc-pasic3.c +18 -0
+#define PASIC3_NUM_LEDS 3
+
+static struct mfd_cell pasic3_cell_leds[PASIC3_NUM_LEDS] = {
+	[0] = {
+		.name		= "leds-pasic3",
+		.id			= 0,
+		.enable		= pasic3_leds_enable,
+		.disable	= pasic3_leds_disable,
+		.suspend	= pasic3_leds_suspend,
+		.resume		= pasic3_leds_enable,
+	},
+	[1] = {
+		.name		= "leds-pasic3",
+		.id			= 1,
+		.enable		= pasic3_leds_enable,
+		.disable	= pasic3_leds_disable,
+		.suspend	= pasic3_leds_suspend,
+		.resume		= pasic3_leds_enable,

drivers/mfd/htc-pasic3.c EXPORT_SYMBOL(pasic3_read_register); /* for leds-pasic3 */ · line 141

Quoted 10 lines
drivers/mfd/htc-pasic3.c +9 -0
+	},
+	[2] = {
+		.name		= "leds-pasic3",
+		.id			= 2,
+		.enable		= pasic3_leds_enable,
+		.disable	= pasic3_leds_disable,
+		.suspend	= pasic3_leds_suspend,
+		.resume		= pasic3_leds_enable,
+	},
 };

Why do you need to explicitly set the .ids?

drivers/mfd/htc-pasic3.c static int ds1wm_enable(struct platform_device *pdev) · line 153

Quoted 9 lines
drivers/mfd/htc-pasic3.c +2 -2
 /*
@@ -78,10 +149,10 @@ static int ds1wm_enable(struct platform_device *pdev)
 	struct device *dev = pdev->dev.parent;
 	int c;
 
-	c = pasic3_read_register(dev, 0x28);
-	pasic3_write_register(dev, 0x28, c & 0x7f);
+	c = pasic3_read_register(dev, PASIC3_GPIO);
+	pasic3_write_register(dev, PASIC3_GPIO, c & ~DS1WM_nEN);

Not keen on camel case defines.

Quoted 26 lines
drivers/mfd/htc-pasic3.c +4 -7
-	dev_dbg(dev, "DS1WM OWM_EN low (active) %02x\n", c & 0x7f);
+	dev_dbg(dev, "DS1WM OWM_EN low (active) %02x\n", c & ~DS1WM_nEN);
 	return 0;
 }
 
@@ -90,10 +161,10 @@ static int ds1wm_disable(struct platform_device *pdev)
 	struct device *dev = pdev->dev.parent;
 	int c;
 
-	c = pasic3_read_register(dev, 0x28);
-	pasic3_write_register(dev, 0x28, c | 0x80);
+	c = pasic3_read_register(dev, PASIC3_GPIO);
+	pasic3_write_register(dev, PASIC3_GPIO, c | DS1WM_nEN);
 
-	dev_dbg(dev, "DS1WM OWM_EN high (inactive) %02x\n", c | 0x80);
+	dev_dbg(dev, "DS1WM OWM_EN high (inactive) %02x\n", c | DS1WM_nEN);
 	return 0;
 }
 
@@ -172,13 +243,27 @@ static int __init pasic3_probe(struct platform_device *pdev)
 			dev_warn(dev, "failed to register DS1WM\n");
 	}
 
-	if (pdata && pdata->led_pdata) {
-		led_cell.platform_data = pdata->led_pdata;
-		led_cell.pdata_size = sizeof(struct pasic3_leds_machinfo);

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 253

Quoted 10 lines
drivers/mfd/htc-pasic3.c +8 -2
-		ret = mfd_add_devices(&pdev->dev, pdev->id, &led_cell, 1, r,
-				      0, NULL);
+	if (pdata && pdata->leds) {
+		int i;
+
+		for (i = 0; i < PASIC3_NUM_LEDS; ++i) {
+			pdata->leds[i].pdata = pdata;
+			pasic3_cell_leds[i].platform_data = &pdata->leds[i];
+			pasic3_cell_leds[i].pdata_size = sizeof(pdata->leds[i]);
+		}

Where is the platform data passed in from?

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 254

Quoted 1 line
drivers/mfd/htc-pasic3.c +1 -0
+		ret = mfd_add_devices(&pdev->dev, 0,

Are you sure you don't want the this to be automatically done for you?

See: include/linux/platform_device.h

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 264

Quoted 10 lines
drivers/mfd/htc-pasic3.c +8 -0
+			pasic3_cell_leds, PASIC3_NUM_LEDS, NULL, 0, NULL);
+
 		if (ret < 0)
 			dev_warn(dev, "failed to register LED device\n");
+
+		if (pdata->power_gpio) {
+			ret = gpio_request(pdata->power_gpio,
+				"Red-Blue LED Power");
+			if (ret < 0)
+				dev_warn(dev, "failed to request power GPIO\n");

Why are you doing this in here? Doesn't this belong in the LED driver?

Quoted 7 lines
drivers/mfd/htc-pasic3.c +2 -0
+		}
+
 	}
 
 	return 0;
@@ -187,10 +272,14 @@ static int __init pasic3_probe(struct platform_device *pdev)
 static int pasic3_remove(struct platform_device *pdev)

drivers/mfd/htc-pasic3.c static int __init pasic3_probe(struct platform_device *pdev) · line 282

Quoted 10 lines
drivers/mfd/htc-pasic3.c +4 -0
 {
 	struct pasic3_data *asic = platform_get_drvdata(pdev);
+	struct pasic3_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct resource *r;
 
 	mfd_remove_devices(&pdev->dev);
 
+	if (pdata->power_gpio)
+		gpio_free(pdata->power_gpio);
+

Move somewhere else.

Quoted 22 lines
drivers/mfd/htc-pasic3.c +0 -0
 	iounmap(asic->mapping);
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(r->start, resource_size(r));
include/linux/mfd/htc-pasic3.h +5 -6
index 3d3ed67..e8e4cd2 100644
@@ -18,36 +18,65 @@
 extern void pasic3_write_register(struct device *dev, u32 reg, u8 val);
 extern u8 pasic3_read_register(struct device *dev, u32 reg);
 
-/*
- * mask for registers 0x20,0x21,0x22
- */
-#define PASIC3_MASK_LED0 0x04
-#define PASIC3_MASK_LED1 0x08
-#define PASIC3_MASK_LED2 0x40
+#define PASIC3_CH0_DELAY_ON		0x00
+#define PASIC3_CH0_DELAY_OFF	0x01
+#define PASIC3_CH1_DELAY_ON		0x02
+#define PASIC3_CH1_DELAY_OFF	0x03
+#define PASIC3_CH2_DELAY_ON		0x04

include/linux/mfd/htc-pasic3.h line 35

Quoted 10 lines
include/linux/mfd/htc-pasic3.h +10 -0
+#define PASIC3_CH2_DELAY_OFF	0x05
+#define PASIC3_DELAY_MASK		0x7f
+
+#define PASIC3_CH_CONTROL		0x06
+#define		R06_CH0_EN			(1<<0)
+#define		R06_CH1_EN			(1<<1)
+#define		R06_CH2_EN			(1<<2)
+#define		R06_CH0_FORCE_ON	(1<<3)
+#define		R06_CH1_FORCE_ON	(1<<4)
+#define		R06_CH2_FORCE_ON	(1<<5)

Use BIT().

Quoted 6 lines
include/linux/mfd/htc-pasic3.h +3 -1
 /*
- * bits in register 0x06
+ * pwm_ch0_out | force_on | (bit0 & bit1 & bit2)
+ * pwm_ch1_out | force_on | (bit0 & bit1 & bit2)
+ * pwm_ch2_out | force_on | (bit2?bit0:(bit0 & ! bit1))
  */

include/linux/mfd/htc-pasic3.h line 48

Quoted 10 lines
include/linux/mfd/htc-pasic3.h +7 -3
-#define PASIC3_BIT2_LED0 0x08
-#define PASIC3_BIT2_LED1 0x10
-#define PASIC3_BIT2_LED2 0x20
+
+#define PASIC3_MASK_A	0x20
+#define PASIC3_MASK_B	0x21
+#define PASIC3_MASK_C	0x22
+#define  MASK_CH0	(1<<2)
+#define  MASK_CH1	(1<<3)
+#define  MASK_CH2	(1<<6)

As above and for all other "1 <<" logic.

Quoted 19 lines
include/linux/mfd/htc-pasic3.h +15 -2
+#define  PASIC3_GPIO	0x28
+
+#define	DS1WM_nEN	(1<<7)
+#define  PASIC3_SYS	0x2a
+/* NORMAL_RST, CAM_PWR_RST and UNKNOWN will autoclear, set STATUS */
+#define  NORMAL_RST		(1<<0)
+#define  CAM_PWR_RST	(1<<1)
+#define  UNKNOWN		(1<<2)
+#define  STATUS_NORMAL_RST	(1<<4)
+#define  STATUS_CAM_PWR_RST	(1<<5)
+#define  STATUS_UNKNOWN		(1<<6)
+
+#define PASIC3_RST_EN		0x2c
+#define  EN_NORMAL_RST	0x40
+#define  EN_DOOR_RST	0x42
 
 struct pasic3_led {
-	struct led_classdev         led;
-	unsigned int                hw_num;

include/linux/mfd/htc-pasic3.h line 72

Quoted 10 lines
include/linux/mfd/htc-pasic3.h +7 -3
-	unsigned int                bit2;
-	unsigned int                mask;
-	struct pasic3_leds_machinfo *pdata;
+	struct led_classdev		cdev;
+	unsigned int	hw_num;
+	unsigned char	bit_blink_en;
+	unsigned char	bit_force_on;
+	unsigned char	bit_mask;
+	unsigned char	reg_delay_on;
+	unsigned char	reg_delay_off;

Are you sure these shouldn't be bools?

Quoted 15 lines
include/linux/mfd/htc-pasic3.h +2 -5
+	struct pasic3_platform_data *pdata;
 };
 
-struct pasic3_leds_machinfo {
+struct pasic3_platform_data {
 	unsigned int      num_leds;
 	unsigned int      power_gpio;
 	struct pasic3_led *leds;
-};
-
-struct pasic3_platform_data {
-	struct pasic3_leds_machinfo *led_pdata;
 	unsigned int                 clock_rate;
 };
 
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
maintainer
#4 lore raw

RFC PATCH 1/1 This patch repairs HTC Magician machine (PXA27x) support

In reply to Petr Cvek
Quoted 11 lines · Petr Cvek

Fixing original code: Problems to successful boot due to the bad LCD power sequence (wrongly configured GPIO).

Add/fix platform data and helper function for various hardware (touchscreen, audio, USB device, ...).

Add new discovered GPIOs and fix names by GPIO context.

Add new LEDs driver.

Hi Petr,

Just as Lee, I want the serie to be split per-subsystem.

For the mach-pxa part, I need you to split the patch into several chunks to review.
At least please split :
- a first patch for cleanup (identation and whitespaces)
- a second patch for cleanup (comments changes, debug messages)
- a serie of fix patches
- one for the power sequence
- one for GPIO fixes
- a serie of evolution patches
- new gpio_keys
- new backlight stuff
- one per new device declared
- new flash stuff
- etc ...

Please keep in mind that in order to pick the patches, I need to split them in 3 groups :
- cleanups
- fixes
- evolutions / new stuff
Moreover I want to be able to revert one functionality (for example new power sequence) without reverting the whole serie.

You probably know that the pxa patches need the subject prefix to be "ARM: pxa:
magician: " in your case. Other subsystem maintainers have similar requirements I believe.

When that's done, I'll begin the review, and Philipp will probably too as he is the maintainer.

Cheers.

--
Robert