new locomo driver

LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has several design issues (special bus instead of platform bus, doesn't use mfd-core, etc).

Implement 'core' parts of locomo support as an mfd driver.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

drivers/mfd/locomo.c new file +7 -0

+/* DAC send data */
+#define        M62332_SLAVE_ADDR       0x4e    /* Slave address  */
+#define        M62332_W_BIT            0x00    /* W bit (0 only) */
+#define        M62332_SUB_ADDR         0x00    /* Sub address    */
+#define        M62332_A_BIT            0x00    /* A bit (0 only) */
+
+/* DAC setup and hold times (expressed in us) */

drivers/mfd/locomo.c new file +5 -0

+/* IRQ support */
+static void locomo_handler(unsigned int irq, struct irq_desc *desc)
+{
+       struct locomo *lchip = irq_get_handler_data(irq);
+       int req, i;

drivers/mfd/locomo.c new file +16 -0

+       /* Install handlers for IRQ_LOCOMO_* */
+       for (irq = lchip->irq_base;
+                       irq < lchip->irq_base + LOCOMO_NR_IRQS;
+                       irq++) {
+               irq_set_chip_and_handler(irq, &locomo_chip, handle_level_irq);
+               irq_set_chip_data(irq, lchip);
+               set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+       }
+
+       /*
+        * Install handler for IRQ_LOCOMO_HW.
+        */
+       irq_set_irq_type(lchip->irq, IRQ_TYPE_EDGE_FALLING);
+       irq_set_handler_data(lchip->irq, lchip);
+       irq_set_chained_handler(lchip->irq, locomo_handler);
+}

Quoted 7 lines · Dmitry Eremin-Solenikov

LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has several design issues (special bus instead of platform bus, doesn't use mfd-core, etc).

Implement 'core' parts of locomo support as an mfd driver.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

(...)

Quoted 7 lines

drivers/mfd/locomo.c new file +7 -0

+/* DAC send data */
+#define        M62332_SLAVE_ADDR       0x4e    /* Slave address  */
+#define        M62332_W_BIT            0x00    /* W bit (0 only) */
+#define        M62332_SUB_ADDR         0x00    /* Sub address    */
+#define        M62332_A_BIT            0x00    /* A bit (0 only) */
+
+/* DAC setup and hold times (expressed in us) */

drivers/mfd/locomo.c line 151

Quoted 10 lines

drivers/mfd/locomo.c new file +10 -0

+#define DAC_BUS_FREE_TIME      5       /*   4.7 us */
+#define DAC_START_SETUP_TIME   5       /*   4.7 us */
+#define DAC_STOP_SETUP_TIME    4       /*   4.0 us */
+#define DAC_START_HOLD_TIME    5       /*   4.7 us */
+#define DAC_SCL_LOW_HOLD_TIME  5       /*   4.7 us */
+#define DAC_SCL_HIGH_HOLD_TIME 4       /*   4.0 us */
+#define DAC_DATA_SETUP_TIME    1       /*   250 ns */
+#define DAC_DATA_HOLD_TIME     1       /*   300 ns */
+#define DAC_LOW_SETUP_TIME     1       /*   300 ns */
+#define DAC_HIGH_SETUP_TIME    1       /*  1000 ns */

(...)

It seems some DAC handling is part of the MFD driver, and we recently discussed that MFD should not be doing misc stuff but mainly act as arbiter and switching station.

Can you please move the DAC parts of the driver to drivers/iio/dac?

The IIO DAC subsystem will likely add other goodies to the driver for free and give a nice API to consumers.

drivers/mfd/locomo.c line 547

Quoted 6 lines

drivers/mfd/locomo.c new file +6 -0

+       /* Longtime timer */
+       writew(0, lchip->base + LOCOMO_LTINT);
+       /* SPI */
+       writew(0, lchip->base + LOCOMO_SPI + LOCOMO_SPIIE);
+
+       writew(6 + 8 + 320 + 30 - 10, lchip->base + LOCOMO_ASD);

That's a few magic numbers and calculation don't you think?

A comment stating what's going on would be helpful.

2014-10-31 10:42 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 9 lines

It seems some DAC handling is part of the MFD driver, and we recently discussed that MFD should not be doing misc stuff but mainly act as arbiter and switching station.

Can you please move the DAC parts of the driver to drivers/iio/dac?

The IIO DAC subsystem will likely add other goodies to the driver for free and give a nice API to consumers.

I wanted this part to be as simple as possible. I will look into IIO DAC subsystem.
The DAC is as simple 2 channel 8-bit i2c device connected to a separate i2c bus controlled through a register in LoCoMo device. One channel is used for backlight,
other will be used for volume control. So (in theory) I can add the following device
chain: locomo -> i2c-locomo -> m62332 -> IIO DAC client. However isn't that quite an overkill for just backlight & volume control? Please advice me on this.

Quoted 7 lines

Quoted 6 lines

drivers/mfd/locomo.c new file +6 -0

+       /* Longtime timer */
+       writew(0, lchip->base + LOCOMO_LTINT);
+       /* SPI */
+       writew(0, lchip->base + LOCOMO_SPI + LOCOMO_SPIIE);
+
+       writew(6 + 8 + 320 + 30 - 10, lchip->base + LOCOMO_ASD);

Quoted 3 lines

That's a few magic numbers and calculation don't you think?

A comment stating what's going on would be helpful.

Unfortunately little is known here - these values are c&p from old 2.4 Lineo code.
This part is related to generating synchronization pulses for accessing touchscreen.

Quoted 12 lines · Dmitry Eremin-Solenikov

2014-10-31 10:42 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 9 lines

It seems some DAC handling is part of the MFD driver, and we recently discussed that MFD should not be doing misc stuff but mainly act as arbiter and switching station.

Can you please move the DAC parts of the driver to drivers/iio/dac?

The IIO DAC subsystem will likely add other goodies to the driver for free and give a nice API to consumers.

Quoted 9 lines

I wanted this part to be as simple as possible. I will look into IIO DAC subsystem.
The DAC is as simple 2 channel 8-bit i2c device connected to a separate i2c bus controlled through a register in LoCoMo device. One channel is used for backlight,
other will be used for volume control. So (in theory) I can add the following device
chain: locomo -> i2c-locomo -> m62332 -> IIO DAC client. However isn't that quite an overkill for just backlight & volume control? Please advice me on this.

The point is still the same: no unrelated code in drivers/mfd, then either use IIO DAC as a middle layer or sink the DAC handling into respective subdriver, i.e. push it into the backlight or volume directly then.

2014-11-03 16:41 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 4 lines

The point is still the same: no unrelated code in drivers/mfd, then either use IIO DAC as a middle layer or sink the DAC handling into respective subdriver, i.e. push it into the backlight or volume directly then.

The problem is that the DAC is equally used by backlight and by sound device (WIP).
What about true i2c device driver sitting in drivers/misc and exporting a regmap of 2 8-bit registers?

Quoted 6 lines

2014-11-03 16:41 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 4 lines

The point is still the same: no unrelated code in drivers/mfd, then either use IIO DAC as a middle layer or sink the DAC handling into respective subdriver, i.e. push it into the backlight or volume directly then.

Quoted 4 lines

The problem is that the DAC is equally used by backlight and by sound device (WIP).
What about true i2c device driver sitting in drivers/misc and exporting a regmap of 2 8-bit registers?

If it can just export registers that sounds like a MFD. If it needs to export functionality then like Linus says the IIO subsystem abstracts DACs.

I'm actually looking at the regulator interface. Since this DAC serves mostly like a (semi-)constant voltage interface, would it be rather logical to use regulator subsystem to drive it?

Quoted 4 lines

I'm actually looking at the regulator interface. Since this DAC serves mostly like a (semi-)constant voltage interface, would it be rather logical to use regulator subsystem to drive it?

Possibly... it's mostly a function of what the consumers of the functionality look like - if that code looks weird calling into the regulator API then it's bad, if that code looks OK it's fine. I haven't looked at the hardware at all so don't have strong feelings either way so long as the result looks tasteful.

2014-11-03 16:41 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 24 lines

Quoted 21 lines · Dmitry Eremin-Solenikov

2014-10-31 10:42 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 9 lines

It seems some DAC handling is part of the MFD driver, and we recently discussed that MFD should not be doing misc stuff but mainly act as arbiter and switching station.

Can you please move the DAC parts of the driver to drivers/iio/dac?

The IIO DAC subsystem will likely add other goodies to the driver for free and give a nice API to consumers.

I wanted this part to be as simple as possible. I will look into IIO DAC subsystem.
The DAC is as simple 2 channel 8-bit i2c device connected to a separate i2c bus controlled through a register in LoCoMo device. One channel is used for backlight,
other will be used for volume control. So (in theory) I can add the following device
chain: locomo -> i2c-locomo -> m62332 -> IIO DAC client. However isn't that quite an overkill for just backlight & volume control? Please advice me on this.

Quoted 4 lines

The point is still the same: no unrelated code in drivers/mfd, then either use IIO DAC as a middle layer or sink the DAC handling into respective subdriver, i.e. push it into the backlight or volume directly then.

The problem is that the DAC is equally used by backlight and by sound device (WIP).

What about true i2c device driver sitting in drivers/misc and exporting a regmap of 2 8-bit registers?

If it is a generic DAC it should go into drivers/iio/, this will allow code sharing and code re-usability. Given the simplicity of the DAC there might even be other existing drivers that can be used to control it.

Add gpiolib driver for gpio pins placed on the LoCoMo GA.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

Quoted 3 lines · Dmitry Eremin-Solenikov

Add gpiolib driver for gpio pins placed on the LoCoMo GA.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

(etc, everywhere this pattern occurs).

drivers/gpio/gpio-locomo.c line 68

Quoted 11 lines

drivers/gpio/gpio-locomo.c new file +11 -0

+static void locomo_gpio_set(struct gpio_chip *chip,
+               unsigned offset, int value)
+{
+       struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
+       unsigned long flags;
+
+       spin_lock_irqsave(&lg->lock, flags);
+
+       __locomo_gpio_set(chip, offset, value);
+
+       spin_unlock_irqrestore(&lg->lock, flags);

If you actually always have to be getting and releasing a spin lock around the register writes, contemplate using regmap-mmio because that is part of what it does.

But is this locking really necessary?

Quoted 5 lines

drivers/gpio/gpio-locomo.c new file +5 -0

+static int locomo_gpio_remove(struct platform_device *pdev)
+{
+       struct locomo_gpio *lg = platform_get_drvdata(pdev);
+       int ret;
+

drivers/gpio/gpio-locomo.c line 209

Quoted 5 lines

drivers/gpio/gpio-locomo.c new file +5 -0

+       ret = gpiochip_remove(&lg->gpio);
+       if (ret) {
+               dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret);
+               return ret;
+       }

The return value from gpiochip_remove() has been removed in v3.18-rc1 so this will not compile.

2014-10-31 10:48 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 7 lines

Quoted 3 lines · Dmitry Eremin-Solenikov

Add gpiolib driver for gpio pins placed on the LoCoMo GA.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

[skipped]

Quoted 17 lines

(etc, everywhere this pattern occurs).

drivers/gpio/gpio-locomo.c line 68

Quoted 11 lines

drivers/gpio/gpio-locomo.c new file +11 -0

+static void locomo_gpio_set(struct gpio_chip *chip,
+               unsigned offset, int value)
+{
+       struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
+       unsigned long flags;
+
+       spin_lock_irqsave(&lg->lock, flags);
+
+       __locomo_gpio_set(chip, offset, value);
+
+       spin_unlock_irqrestore(&lg->lock, flags);

If you actually always have to be getting and releasing a spin lock around the register writes, contemplate using regmap-mmio because that is part of what it does.

Quoted 1 line

But is this locking really necessary?

I have a custom of doing such locking and never having to think about somebody breaking into RMW cycles.

Also isn't regmap an overkill here? Wouldn't regmap also do a lock/unlock around each register read/write/RMW?

Quoted 30 lines · Dmitry Eremin-Solenikov

2014-10-31 10:48 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 7 lines

Quoted 3 lines · Dmitry Eremin-Solenikov

Add gpiolib driver for gpio pins placed on the LoCoMo GA.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

[skipped]

Quoted 18 lines

(etc, everywhere this pattern occurs).

Review

Quoted 11 lines

(quoted diff) +11 -0

+static void locomo_gpio_set(struct gpio_chip *chip,
+               unsigned offset, int value)
+{
+       struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio);
+       unsigned long flags;
+
+       spin_lock_irqsave(&lg->lock, flags);
+
+       __locomo_gpio_set(chip, offset, value);
+
+       spin_unlock_irqrestore(&lg->lock, flags);

If you actually always have to be getting and releasing a spin lock around the register writes, contemplate using regmap-mmio because that is part of what it does.

But is this locking really necessary?

Quoted 5 lines

I have a custom of doing such locking and never having to think about somebody breaking into RMW cycles.

Also isn't regmap an overkill here? Wouldn't regmap also do a lock/unlock around each register read/write/RMW?

Yes that's the point: if you use regmap mmio you get the RMW-locking for free, with the regmap implementation.

2014-11-03 16:43 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 2 lines

Yes that's the point: if you use regmap mmio you get the RMW-locking for free, with the regmap implementation.

Just to be more concrete. Currently locomo_gpio_ack_irq() function uses one lock and one unlock for doing 3 consecutive RMW I I convert locomo to regmap, will that be 3 lock/unlock calls or still one? (Or maybe I'm trying to be over-protective here and adding more lock/unlock cycles won't matter that much?)

Next question: if I have to export regmap to several subdrivers, is it better to have one big regmap or to have one-map-per-driver approach?

Quoted 10 lines

2014-11-03 16:43 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 2 lines

Yes that's the point: if you use regmap mmio you get the RMW-locking for free, with the regmap implementation.

Just to be more concrete. Currently locomo_gpio_ack_irq() function uses one lock and one unlock for doing 3 consecutive RMW I I convert locomo to regmap, will that be 3 lock/unlock calls or still one? (Or maybe I'm trying to be over-protective here and adding more lock/unlock cycles won't matter that much?)

You'll get three lock/unlocks, we could add an interface for bulk updates under one lock if it's important though.

Quoted 2 lines

Next question: if I have to export regmap to several subdrivers, is it better to have one big regmap or to have one-map-per-driver approach?

One regmap for the physical register map which is shared between all the users.

Just to better understand your suggestions: do you want me to convert to regmap only gpio driver or do you suggest to convert all LoCoMo drivers? That is doable, of course, but the amount of changes is overwhelming. Also I'm concerned about the performance impact from going through regmap layers.

Just to better understand your suggestions: do you want me to convert to regmap only gpio driver or do you suggest to convert all LoCoMo drivers?

That is doable, of course, but the amount of changes is overwhelming. Also I'm concerned about the performance impact from going through regmap layers.

Quoted 2 lines · Dmitry Eremin-Solenikov

Just to better understand your suggestions: do you want me to convert to regmap only gpio driver or do you suggest to convert all LoCoMo drivers?

Um... I was just thinking about this one usecase.

It's no big deal, the other review comments are more important.

Quoted 3 lines

That is doable, of course, but the amount of changes is overwhelming. Also I'm concerned about the performance impact from going through regmap layers.

Is it on a critical path? The current locking isn't any less invasive AFAICT.

Yours,
Linus Walleij

As LoCoMo is switching to new device model, adapt keyboard driver to support new locomo core driver.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/input/keyboard/Kconfig     |   1 -
 drivers/input/keyboard/locomokbd.c | 165 ++++++++++++++++++++-----------------
 2 files changed, 91 insertions(+), 75 deletions(-)

drivers/input/keyboard/Kconfig +0 -1

index a3958c6..b660516 100644
@@ -337,7 +337,6 @@ config KEYBOARD_LM8333
 
 config KEYBOARD_LOCOMO
 	tristate "LoCoMo Keyboard Support"
-	depends on SHARP_LOCOMO
 	help
 	  Say Y here if you are running Linux on a Sharp Zaurus Collie or Poodle based PDA
 

drivers/input/keyboard/locomokbd.c +35 -43

index c94d610..c2fabf3 100644
@@ -28,16 +28,10 @@
 #include <linux/init.h>
 #include <linux/input.h>
 #include <linux/delay.h>
-#include <linux/device.h>
+#include <linux/platform_device.h>
 #include <linux/interrupt.h>
-#include <linux/ioport.h>
-
-#include <asm/hardware/locomo.h>
-#include <asm/irq.h>
-
-MODULE_AUTHOR("John Lenz <[email protected]>");
-MODULE_DESCRIPTION("LoCoMo keyboard driver");
-MODULE_LICENSE("GPL");
+#include <linux/io.h>
+#include <linux/mfd/locomo.h>
 
 #define LOCOMOKBD_NUMKEYS	128
 

@@ -75,7 +69,8 @@ struct locomokbd {
 	struct input_dev *input;
 	char phys[32];
 
-	unsigned long base;
+	void __iomem *base;
+	int irq;
 	spinlock_t lock;
 
 	struct timer_list timer;

@@ -84,37 +79,37 @@ struct locomokbd {
 };
 
 /* helper functions for reading the keyboard matrix */
-static inline void locomokbd_charge_all(unsigned long membase)
+static inline void locomokbd_charge_all(void __iomem *membase)
 {
-	locomo_writel(0x00FF, membase + LOCOMO_KSC);
+	writew(0x00FF, membase + LOCOMO_KSC);
 }
 
-static inline void locomokbd_activate_all(unsigned long membase)
+static inline void locomokbd_activate_all(void __iomem *membase)
 {
 	unsigned long r;
 
-	locomo_writel(0, membase + LOCOMO_KSC);
-	r = locomo_readl(membase + LOCOMO_KIC);
+	writew(0, membase + LOCOMO_KSC);
+	r = readw(membase + LOCOMO_KIC);
 	r &= 0xFEFF;
-	locomo_writel(r, membase + LOCOMO_KIC);
+	writew(r, membase + LOCOMO_KIC);
 }
 
-static inline void locomokbd_activate_col(unsigned long membase, int col)
+static inline void locomokbd_activate_col(void __iomem *membase, int col)
 {
 	unsigned short nset;
 	unsigned short nbset;
 
 	nset = 0xFF & ~(1 << col);
 	nbset = (nset << 8) + nset;
-	locomo_writel(nbset, membase + LOCOMO_KSC);
+	writew(nbset, membase + LOCOMO_KSC);
 }
 
-static inline void locomokbd_reset_col(unsigned long membase, int col)
+static inline void locomokbd_reset_col(void __iomem *membase, int col)
 {
 	unsigned short nbset;
 
 	nbset = ((0xFF & ~(1 << col)) << 8) + 0xFF;
-	locomo_writel(nbset, membase + LOCOMO_KSC);
+	writew(nbset, membase + LOCOMO_KSC);
 }
 
 /*

@@ -129,7 +124,7 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
 	unsigned int row, col, rowd;
 	unsigned long flags;
 	unsigned int num_pressed;
-	unsigned long membase = locomokbd->base;
+	void __iomem *membase = locomokbd->base;
 
 	spin_lock_irqsave(&locomokbd->lock, flags);
 

@@ -141,7 +136,7 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
 		locomokbd_activate_col(membase, col);
 		udelay(KB_DELAY);
 
-		rowd = ~locomo_readl(membase + LOCOMO_KIB);
+		rowd = ~readw(membase + LOCOMO_KIB);
 		for (row = 0; row < KB_ROWS; row++) {
 			unsigned int scancode, pressed, key;
 

@@ -194,11 +189,11 @@ static irqreturn_t locomokbd_interrupt(int irq, void *dev_id)
 	struct locomokbd *locomokbd = dev_id;
 	u16 r;
 
-	r = locomo_readl(locomokbd->base + LOCOMO_KIC);
+	r = readw(locomokbd->base + LOCOMO_KIC);
 	if ((r & 0x0001) == 0)
 		return IRQ_HANDLED;
 
-	locomo_writel(r & ~0x0100, locomokbd->base + LOCOMO_KIC); /* Ack */
+	writew(r & ~0x0100, locomokbd->base + LOCOMO_KIC); /* Ack */
 
 	/** wait chattering delay **/
 	udelay(100);

@@ -222,8 +217,8 @@ static int locomokbd_open(struct input_dev *dev)
 	struct locomokbd *locomokbd = input_get_drvdata(dev);
 	u16 r;
 
-	r = locomo_readl(locomokbd->base + LOCOMO_KIC) | 0x0010;
-	locomo_writel(r, locomokbd->base + LOCOMO_KIC);
+	r = readw(locomokbd->base + LOCOMO_KIC) | 0x0010;
+	writew(r, locomokbd->base + LOCOMO_KIC);
 	return 0;
 }
 

@@ -232,35 +227,39 @@ static void locomokbd_close(struct input_dev *dev)
 	struct locomokbd *locomokbd = input_get_drvdata(dev);
 	u16 r;
 
-	r = locomo_readl(locomokbd->base + LOCOMO_KIC) & ~0x0010;
-	locomo_writel(r, locomokbd->base + LOCOMO_KIC);
+	r = readw(locomokbd->base + LOCOMO_KIC) & ~0x0010;
+	writew(r, locomokbd->base + LOCOMO_KIC);
 }
 
-static int locomokbd_probe(struct locomo_dev *dev)
+static int locomokbd_probe(struct platform_device *dev)
 {
 	struct locomokbd *locomokbd;
 	struct input_dev *input_dev;
 	int i, err;
+	struct resource *res;
+
+	locomokbd = devm_kzalloc(&dev->dev, sizeof(struct locomokbd),
+			GFP_KERNEL);
+	if (!locomokbd)
+		return -ENOMEM;
 
-	locomokbd = kzalloc(sizeof(struct locomokbd), GFP_KERNEL);
 	input_dev = input_allocate_device();
-	if (!locomokbd || !input_dev) {
-		err = -ENOMEM;
-		goto err_free_mem;
-	}
+	if (!input_dev)
+		return -ENOMEM;
 
-	/* try and claim memory region */
-	if (!request_mem_region((unsigned long) dev->mapbase,
-				dev->length,
-				LOCOMO_DRIVER_NAME(dev))) {
-		err = -EBUSY;
-		printk(KERN_ERR "locomokbd: Can't acquire access to io memory for keyboard\n");
-		goto err_free_mem;
-	}
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
 
-	locomo_set_drvdata(dev, locomokbd);

LoCoMo has a possibility to generate per-GPIO edge irqs. Support for that was there in old locomo driver, got 'cleaned up' during old driver IRQ cascading cleanup and is now reimplemented. It is expected that SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

drivers/gpio/gpio-locomo.c +4 -0

+{
+       u16 req;
+       struct locomo_gpio *lg = irq_get_handler_data(irq);
+       int i = lg->irq_base;

drivers/gpio/gpio-locomo.c +12 -0

+static void locomo_gpio_ack_irq(struct irq_data *d)
+{
+       struct locomo_gpio *lg = irq_data_get_irq_chip_data(d);
+       unsigned long flags;
+       u16 r;
+
+       spin_lock_irqsave(&lg->lock, flags);
+
+       r = readw(lg->regs + LOCOMO_GWE);
+       r |= (0x0001 << (d->irq - lg->irq_base));
+       writew(r, lg->regs + LOCOMO_GWE);
+

drivers/gpio/gpio-locomo.c +12 -0

+static void locomo_gpio_setup_irq(struct locomo_gpio *lg)
+{
+       int irq;
+
+       lg->irq_base = irq_alloc_descs(-1, 0, LOCOMO_GPIO_NR_IRQS, -1);
+
+       /* Install handlers for IRQ_LOCOMO_* */
+       for (irq = lg->irq_base;
+                       irq < lg->irq_base + LOCOMO_GPIO_NR_IRQS;
+                       irq++) {
+               irq_set_chip_and_handler(irq, &locomo_gpio_chip,
+                               handle_edge_irq);

Quoted 6 lines · Dmitry Eremin-Solenikov

LoCoMo has a possibility to generate per-GPIO edge irqs. Support for that was there in old locomo driver, got 'cleaned up' during old driver IRQ cascading cleanup and is now reimplemented. It is expected that SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

Please don't use open-coded IRQ handling like this, we are moving away from that.

In Kconfig,

select GPIOLIB_IRQCHIP

and look at the other drivers selecting this for inspiration. There is even some documentation in Documentation/gpio/driver.txt

You will find that it cuts down a lot of overhead from your driver and does everything in the right way in a central place.

Add new simple backlight driver - it cares only about PWM/frontlight part of LoCoMo, it does not touch TFT settings and does not export TFT power control.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/video/backlight/Kconfig     |   6 +-
 drivers/video/backlight/Makefile    |   2 +-
 drivers/video/backlight/locomo_bl.c | 171 ++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+), 4 deletions(-)
 create mode 100644 drivers/video/backlight/locomo_bl.c

drivers/video/backlight/Kconfig +3 -3

index 8d03924..03b77b33 100644
@@ -218,12 +218,12 @@ config BACKLIGHT_LM3533
 	  levels.
 
 config BACKLIGHT_LOCOMO
-	tristate "Sharp LOCOMO LCD/Backlight Driver"
-	depends on SHARP_LOCOMO
+	tristate "Sharp LOCOMO Backlight Driver"
+	depends on MFD_LOCOMO
 	default y
 	help
 	  If you have a Sharp Zaurus SL-5500 (Collie) or SL-5600 (Poodle) say y to
-	  enable the LCD/backlight driver.
+	  enable the backlight driver.
 
 config BACKLIGHT_OMAP1
 	tristate "OMAP1 PWL-based LCD Backlight"

drivers/video/backlight/Makefile +1 -1

index fcd50b73..2a61b7e 100644
@@ -39,7 +39,7 @@ obj-$(CONFIG_BACKLIGHT_IPAQ_MICRO)	+= ipaq_micro_bl.o
 obj-$(CONFIG_BACKLIGHT_LM3533)		+= lm3533_bl.o
 obj-$(CONFIG_BACKLIGHT_LM3630A)		+= lm3630a_bl.o
 obj-$(CONFIG_BACKLIGHT_LM3639)		+= lm3639_bl.o
-obj-$(CONFIG_BACKLIGHT_LOCOMO)		+= locomolcd.o
+obj-$(CONFIG_BACKLIGHT_LOCOMO)		+= locomo_bl.o
 obj-$(CONFIG_BACKLIGHT_LP855X)		+= lp855x_bl.o
 obj-$(CONFIG_BACKLIGHT_LP8788)		+= lp8788_bl.o
 obj-$(CONFIG_BACKLIGHT_LV5207LP)	+= lv5207lp.o

drivers/video/backlight/locomo_bl.c new file +11 -0

new file mode 100644
index 0000000..cec1b51
@@ -0,0 +1,171 @@
+/*
+ * Backlight control code for Sharp Zaurus SL-5500
+ *
+ * Copyright 2005 John Lenz <[email protected]>
+ * Maintainer: Pavel Machek <[email protected]> (unless John wants to :-)
+ * GPL v2
+ *
+ * This driver assumes single CPU. That's okay, because collie is
+ * slightly old hardware, and no one is going to retrofit second CPU to
+ * old PDA.
+ */

drivers/video/backlight/locomo_bl.c new file +26 -0

+
+struct locomo_bl {
+	void __iomem *regs;
+	int current_intensity;
+	int gpio_fl_vr;
+};
+
+static const struct {
+	u16 duty, bpwf;
+	bool vr;
+} locomo_bl_pwm[] = {
+	{ 0, 161, false },
+	{ 117, 161, false },
+	{ 163, 148, false },
+	{ 194, 161, false },
+	{ 194, 161, true },
+};
+
+static int locomo_bl_set_intensity(struct backlight_device *bd)
+{
+	int intensity = bd->props.brightness;
+	struct locomo_bl *bl = dev_get_drvdata(&bd->dev);
+
+	if (bd->props.power != FB_BLANK_UNBLANK)
+		intensity = 0;
+	if (bd->props.fb_blank != FB_BLANK_UNBLANK)

drivers/video/backlight/locomo_bl.c new file +29 -0

+	writew(locomo_bl_pwm[intensity].bpwf | LOCOMO_ALC_EN,
+			bl->regs + LOCOMO_ALS);
 
+
+	bl->current_intensity = intensity;
+	if (bd->props.state & BL_CORE_SUSPENDED)
+		writew(0x00, bl->regs + LOCOMO_ALS);
+
+	return 0;
+}
+
+static int locomo_bl_get_intensity(struct backlight_device *bd)
+{
+	struct locomo_bl *bl = dev_get_drvdata(&bd->dev);
+
+	return bl->current_intensity;
+}
+
+static const struct backlight_ops locomo_bl_ops = {
+	.options	= BL_CORE_SUSPENDRESUME,
+	.get_brightness = locomo_bl_get_intensity,
+	.update_status  = locomo_bl_set_intensity,
+};
+
+static int locomo_bl_probe(struct platform_device *dev)
+{
+	struct backlight_properties props;
+	struct resource *res;
+	struct locomo_bl_platform_data *pdata;
+	struct locomo_bl *bl;

drivers/video/backlight/locomo_bl.c new file +9 -0

+
+	pdata = dev_get_platdata(&dev->dev);
+	if (!pdata)
+		return -EINVAL;
+
+	bl->gpio_fl_vr = pdata->gpio_fl_vr;
+	rc = devm_gpio_request_one(&dev->dev, bl->gpio_fl_vr,
+			GPIOF_OUT_INIT_LOW, "FL VR");
+	if (rc)

drivers/video/backlight/locomo_bl.c new file +13 -0

+						&dev->dev, bl,
+						&locomo_bl_ops, &props);
 
+
+	if (IS_ERR(locomo_bl_device))
+		return PTR_ERR(locomo_bl_device);
+
+	platform_set_drvdata(dev, locomo_bl_device);
+
+	/* Set up frontlight so that screen is readable */
+	backlight_update_status(locomo_bl_device);
+
+	return 0;
+}

drivers/video/backlight/locomo_bl.c new file +17 -0

+
+	return 0;
+}
+
+static void locomo_bl_shutdown(struct platform_device *dev)
+{
+	struct backlight_device *locomo_bl_device = platform_get_drvdata(dev);
+
+	locomo_bl_device->props.brightness = 0;
+	locomo_bl_device->props.power = 0;
+	locomo_bl_set_intensity(locomo_bl_device);
+}
+
+static struct platform_driver locomo_bl_driver = {
+	.driver = {
+		.name	= "locomo-backlight",
+		.owner	= THIS_MODULE,

+MODULE_DESCRIPTION("Collie Backlight driver");

+MODULE_LICENSE("GPL");

+MODULE_ALIAS("platform:locomo-backlight");
--
2.1.1

LoCoMo has some special handling for TFT screens attached to Collie and Poodle. Implement that as a separate driver.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/video/backlight/Kconfig      |   8 ++
 drivers/video/backlight/Makefile     |   1 +
 drivers/video/backlight/locomo_lcd.c | 224 +++++++++++++++++++++++++++++++++++
 3 files changed, 233 insertions(+)
 create mode 100644 drivers/video/backlight/locomo_lcd.c

drivers/video/backlight/Kconfig +8 -0

index 03b77b33..bc5c671 100644
@@ -48,6 +48,14 @@ config LCD_LMS283GF05
 	  SPI driver for Samsung LMS283GF05. This provides basic support
 	  for powering the LCD up/down through a sysfs interface.
 
+config LCD_LOCOMO
+	tristate "Sharp LOCOMO LCD Driver"
+	depends on MFD_LOCOMO
+	default y
+	help
+	  If you have a Sharp Zaurus SL-5500 (Collie) or SL-5600 (Poodle) say y to
+	  enable the LCD driver.
+
 config LCD_LTV350QV
 	tristate "Samsung LTV350QV LCD Panel"
 	depends on SPI_MASTER

drivers/video/backlight/Makefile +0 -0

index 2a61b7e..b2580e7 100644

drivers/video/backlight/Makefile +0 -0

 obj-$(CONFIG_LCD_LMS283GF05)		+= lms283gf05.o
 obj-$(CONFIG_LCD_LMS501KF03)		+= lms501kf03.o
 obj-$(CONFIG_LCD_LTV350QV)		+= ltv350qv.o

drivers/video/backlight/locomo_lcd.c new file +13 -0

new file mode 100644
index 0000000..245efb8
@@ -0,0 +1,224 @@
+/*
+ * Backlight control code for Sharp Zaurus SL-5500
+ *
+ * Copyright 2005 John Lenz <[email protected]>
+ * Maintainer: Pavel Machek <[email protected]> (unless John wants to :-)
+ * GPL v2
+ *
+ * This driver assumes single CPU. That's okay, because collie is
+ * slightly old hardware, and no one is going to retrofit second CPU to
+ * old PDA.
+ */
+
+/* LCD power functions */

drivers/video/backlight/locomo_lcd.c new file +18 -0

+
+static struct platform_device *locomolcd_dev;
+static struct locomo_lcd_platform_data lcd_data;
+static bool locomolcd_is_on;
+static bool locomolcd_is_suspended;
+static void __iomem *locomolcd_regs;
+static struct lcd_device *lcd_dev;
+
+static struct gpio locomo_gpios[] = {
+	{ 0, GPIOF_OUT_INIT_LOW, "LCD VSHA on" },
+	{ 0, GPIOF_OUT_INIT_LOW, "LCD VSHD on" },
+	{ 0, GPIOF_OUT_INIT_LOW, "LCD Vee on" },
+	{ 0, GPIOF_OUT_INIT_LOW, "LCD MOD" },
+};
+
+static void locomolcd_on(void)
+{
+	gpio_set_value(lcd_data.gpio_lcd_vsha_on, 1);

drivers/video/backlight/locomo_lcd.c new file +18 -0

+
+	/* TFTCRST | CPSOUT=0 | CPSEN */
+	writew(0x01, locomolcd_regs + LOCOMO_TC);
+
+	/* Set CPSD */
+	writew(6, locomolcd_regs + LOCOMO_CPSD);
+
+	/* TFTCRST | CPSOUT=0 | CPSEN */
+	writew((0x04 | 0x01), locomolcd_regs + LOCOMO_TC);
+	mdelay(10);
+
+	gpio_set_value(lcd_data.gpio_lcd_mod, 1);
+}
+
+static void locomolcd_off(void)
+{
+	/* TFTCRST=1 | CPSOUT=1 | CPSEN = 0 */
+	writew(0x06, locomolcd_regs + LOCOMO_TC);

drivers/video/backlight/locomo_lcd.c new file +59 -0

+
+	/* TFTCRST=0 | CPSOUT=0 | CPSEN = 0 */
+	writew(0, locomolcd_regs + LOCOMO_TC);
+	gpio_set_value(lcd_data.gpio_lcd_mod, 0);
+	gpio_set_value(lcd_data.gpio_lcd_vshd_on, 0);
+}
+
+int locomo_lcd_set_power(struct lcd_device *lcd, int power)
+{
+	dev_dbg(&lcd->dev, "LCD power %d (is %d)\n", power, locomolcd_is_on);
+	if (power == 0 && !locomolcd_is_on) {
+		locomolcd_is_on = 1;
+		locomolcd_on();
+	}
+	if (power != 0 && locomolcd_is_on) {
+		locomolcd_is_on = 0;
+		locomolcd_off();
+	}
+	return 0;
+}
+
+static int locomo_lcd_get_power(struct lcd_device *lcd)
+{
+	return !locomolcd_is_on;
+}
+
+static struct lcd_ops locomo_lcd_ops = {
+	.set_power = locomo_lcd_set_power,
+	.get_power = locomo_lcd_get_power,
+};
+
+#ifdef CONFIG_PM_SLEEP
+static int locomolcd_suspend(struct device *dev)
+{
+	locomolcd_is_suspended = true;
+	locomolcd_off();
+
+	return 0;
+}
+
+static int locomolcd_resume(struct device *dev)
+{
+	locomolcd_is_suspended = false;
+
+	if (locomolcd_is_on)
+		locomolcd_on();
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(locomolcd_pm, locomolcd_suspend, locomolcd_resume);
+#define LOCOMOLCD_PM	(&locomolcd_pm)
+#else
+#define LOCOMOLCD_PM	NULL
+#endif
+
+static int locomolcd_probe(struct platform_device *dev)
+{
+	unsigned long flags;

drivers/video/backlight/locomo_lcd.c new file +21 -0

+
+	pdata = dev_get_platdata(&dev->dev);
+	if (!pdata)
+		return -EINVAL;
+
+	lcd_data = *pdata;
+
+	locomo_gpios[0].gpio = lcd_data.gpio_lcd_vsha_on;
+	locomo_gpios[1].gpio = lcd_data.gpio_lcd_vshd_on;
+	locomo_gpios[2].gpio = lcd_data.gpio_lcd_vee_on;
+	locomo_gpios[3].gpio = lcd_data.gpio_lcd_mod;
+	dev_info(&dev->dev, "GPIOs: %d %d %d %d\n",
+			locomo_gpios[0].gpio,
+			locomo_gpios[1].gpio,
+			locomo_gpios[2].gpio,
+			locomo_gpios[3].gpio);
+
+	rc = gpio_request_array(locomo_gpios, ARRAY_SIZE(locomo_gpios));
+	if (rc)
+		return rc;
+

drivers/video/backlight/locomo_lcd.c new file +22 -0

+
+	local_irq_save(flags);
+
+	locomolcd_off();
+	locomolcd_dev = NULL;
+
+	local_irq_restore(flags);
+
+	gpio_free_array(locomo_gpios, ARRAY_SIZE(locomo_gpios));
+
+	return 0;
+}
+
+static void locomolcd_shutdown(struct platform_device *dev)
+{
+	locomolcd_off();
+}
+
+static struct platform_driver locomolcd_driver = {
+	.driver = {
+		.name	= "locomo-lcd",
+		.owner	= THIS_MODULE,

drivers/video/backlight/locomo_lcd.c new file +1 -1

+MODULE_ALIAS("platform:locomo-lcd");
--

2.1.1

Quoted 3 lines · Dmitry Eremin-Solenikov

LoCoMo has some special handling for TFT screens attached to Collie and Poodle. Implement that as a separate driver.

As new locomo driver properly supports SPARSE_IRQ, stop playing with NR_IRQS on sa1100 (locomo was the last chip requiring NR_IRQ tricks).

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

Quoted 4 lines · Dmitry Eremin-Solenikov

As new locomo driver properly supports SPARSE_IRQ, stop playing with NR_IRQS on sa1100 (locomo was the last chip requiring NR_IRQ tricks).

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

That's right! By moving to irqdomain things get even better :)

Since LoCoMo driver has been converted to provide proper gpiolib interface, make poodle ASoC platform driver use gpiolib API.

Quoted 2 lines · Dmitry Eremin-Solenikov

Since LoCoMo driver has been converted to provide proper gpiolib interface, make poodle ASoC platform driver use gpiolib API.

Please use subject lines matching the style for the subsystem.

sound/soc/pxa/poodle.c static struct snd_soc_card poodle = { · line 271

Quoted 6 lines

sound/soc/pxa/poodle.c +6 -0

+	ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
+	if (ret) {
+		dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
+				ret);
+		return ret;
+	}

I sense a need for devm_gpio_request_array() here. Otherwise this looks fine - ideally it'd move to gpiod but moving to gpiolib is a clear win so no need to block on this.

Acked-by Mark Brown <[email protected]>

with at least the subject line fixed.

Quoted 2 lines · Dmitry Eremin-Solenikov

Since LoCoMo driver has been converted to provide proper gpiolib interface, make poodle ASoC platform driver use gpiolib API.

Please use subject lines matching the style for the subsystem.

sound/soc/pxa/poodle.c static struct snd_soc_card poodle = { · line 271

Quoted 6 lines

sound/soc/pxa/poodle.c +6 -0

+     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
+     if (ret) {
+             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
+                             ret);
+             return ret;
+     }

I sense a need for devm_gpio_request_array() here. Otherwise this looks fine - ideally it'd move to gpiod but moving to gpiolib is a clear win so no need to block on this.

Quoted 13 lines · Mark Brown

Quoted 2 lines · Dmitry Eremin-Solenikov

Since LoCoMo driver has been converted to provide proper gpiolib interface, make poodle ASoC platform driver use gpiolib API.

Please use subject lines matching the style for the subsystem.

Quoted 6 lines

sound/soc/pxa/poodle.c +6 -0

+     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
+     if (ret) {
+             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
+                             ret);
+             return ret;
+     }

Quoted 3 lines

I sense a need for devm_gpio_request_array() here. Otherwise this looks fine - ideally it'd move to gpiod but moving to gpiolib is a clear win so no need to block on this.

I wish Dmitry took the opportunity to move this driver to the gpiod API, especially since doing so would be trivial for this driver.

Quoted 18 lines · Mark Brown

Quoted 16 lines · Dmitry Eremin-Solenikov

Quoted 2 lines

Since LoCoMo driver has been converted to provide proper gpiolib interface, make poodle ASoC platform driver use gpiolib API.

Please use subject lines matching the style for the subsystem.

sound/soc/pxa/poodle.c static struct snd_soc_card poodle = { · line 271

Quoted 6 lines

sound/soc/pxa/poodle.c +6 -0

+     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
+     if (ret) {
+             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
+                             ret);
+             return ret;
+     }

I sense a need for devm_gpio_request_array() here. Otherwise this looks fine - ideally it'd move to gpiod but moving to gpiolib is a clear win so no need to block on this.

Quoted 2 lines

I wish Dmitry took the opportunity to move this driver to the gpiod API, especially since doing so would be trivial for this driver.

+1 on this.

However this platform is not device tree, so this implies setting up a descriptor table for the affected driver(s) to work properly. See Documentation/gpio/board.txt

2014-10-31 12:52 GMT+03:00 Linus Walleij <[email protected]>:

Quoted 19 lines · Alexandre Courbot

Quoted 18 lines · Mark Brown

Quoted 16 lines · Dmitry Eremin-Solenikov

Quoted 2 lines

Since LoCoMo driver has been converted to provide proper gpiolib interface, make poodle ASoC platform driver use gpiolib API.

Please use subject lines matching the style for the subsystem.

Review

Quoted 6 lines

(quoted diff) +6 -0

+     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
+     if (ret) {
+             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
+                             ret);
+             return ret;
+     }

I sense a need for devm_gpio_request_array() here. Otherwise this looks fine - ideally it'd move to gpiod but moving to gpiolib is a clear win so no need to block on this.

Quoted 8 lines

Quoted 2 lines

I wish Dmitry took the opportunity to move this driver to the gpiod API, especially since doing so would be trivial for this driver.

+1 on this.

However this platform is not device tree, so this implies setting up a descriptor table for the affected driver(s) to work properly. See Documentation/gpio/board.txt

I checked the gpiod interfaces after original suggestion by Alexandre.

Introducing those mapping tables (much like pinctrl tables) look like a duplicate effort if Russell will permit adding a DT support. So I thought that I will reconsider gpiod/pinctrl/etc after fixing LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK and (finally) thinking about device tree support.

Sharp Zaurus SL-5500 and SL-5600 use special companion Gate Array. Current drivers present in Linux kernel has some problems:

* It uses custom bus instead of platform bus/mfd core.

* Device drivers are not well layered/separated.

Quoted 4 lines

Sharp Zaurus SL-5500 and SL-5600 use special companion Gate Array. Current drivers present in Linux kernel has some problems:

* It uses custom bus instead of platform bus/mfd core.

I believe Greg wouldn't see that as a positive point.

Don't think that the platform bus _should_ always be used. It shouldn't (Greg has said he'd like to see the platform bus to be totally killed off.) Instead, custom buses properly suited to the class of device in question is much preferred, especially if it aids in...

Quoted 2 lines

* Device drivers are not well layered/separated.

... better layering or separation of drivers.

So, thinking that converting from a custom bus to a platform bus definitely is /not/ a positive step forward.

(Why mfd was ever allowed to re-use the platform bus stuff is a separate question not relevent to these patches.)

Quoted 4 lines · Dmitry Eremin-Solenikov

Sharp Zaurus SL-5500 and SL-5600 use special companion Gate Array. Current drivers present in Linux kernel has some problems:

* It uses custom bus instead of platform bus/mfd core.

I believe Greg wouldn't see that as a positive point.

Don't think that the platform bus _should_ always be used. It shouldn't (Greg has said he'd like to see the platform bus to be totally killed off.) Instead, custom buses properly suited to the class of device in question is much preferred, especially if it aids in...

Quoted 1 line

* Device drivers are not well layered/separated.

... better layering or separation of drivers.

So, thinking that converting from a custom bus to a platform bus definitely is /not/ a positive step forward.

(Why mfd was ever allowed to re-use the platform bus stuff is a separate question not relevent to these patches.)

Quoted 4 lines

Sharp Zaurus SL-5500 and SL-5600 use special companion Gate Array. Current drivers present in Linux kernel has some problems:

* It uses custom bus instead of platform bus/mfd core.

I believe Greg wouldn't see that as a positive point.

Don't think that the platform bus _should_ always be used. It shouldn't (Greg has said he'd like to see the platform bus to be totally killed off.) Instead, custom buses properly suited to the class of device in question is much preferred, especially if it aids in...

Quoted 2 lines

* Device drivers are not well layered/separated.

... better layering or separation of drivers.

So, thinking that converting from a custom bus to a platform bus definitely is /not/ a positive step forward.

2014-10-28 3:13 GMT+03:00 Russell King - ARM Linux <[email protected]>:

Quoted 8 lines · Dmitry Eremin-Solenikov

Quoted 4 lines

Sharp Zaurus SL-5500 and SL-5600 use special companion Gate Array. Current drivers present in Linux kernel has some problems:

* It uses custom bus instead of platform bus/mfd core.

I believe Greg wouldn't see that as a positive point.

Quoted 4 lines

Don't think that the platform bus _should_ always be used. It shouldn't (Greg has said he'd like to see the platform bus to be totally killed off.) Instead, custom buses properly suited to the class of device in question is much preferred, especially if it aids in...

Greg, Russell, what should I use then for mfd multi-cell devices? Is mfd-core still a permitted API? Should we change mfd-core internals to spawn extra bus (busses?) for the cells?