* Re: [PATCH v2 10/17] i2c: add locomo i2c driver
From: Wolfram Sang @ 2015-05-12 19:28 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <CALT56yNHvJ1Swj2pp2JrBaJBensSP703JXQKfvivFrJOYf85QA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]
On Tue, May 12, 2015 at 10:27:07PM +0300, Dmitry Eremin-Solenikov wrote:
> 2015-05-12 22:24 GMT+03:00 Wolfram Sang <wsa@the-dreams.de>:
> > On Tue, Apr 28, 2015 at 02:55:47AM +0300, Dmitry Eremin-Solenikov wrote:
> >> LoCoMo chip contains a tiny i2c controller destined to control
> >> M62332 DAC. Provide a separate I2C driver for this cell.
> >>
> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >
> > Thanks for the submission!
> >
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/mfd/locomo.h>
> >> +
> >> +#include <linux/i2c.h>
> >> +#include <linux/i2c-algo-bit.h>
> >
> > To avoid duplicates, I prefer the includes to be sorted. No need to
> > resend because of that, though.
> >
> > Kudos for brushing up old code! Do you want the drivers go via the
> > subsystem trees? Or is there a dependency so it should go in via some
> > other tree in one go?
>
> There is a header dependency -- <linux/mfd/locomo.h> is a new header.
> I'd prefer for these patches to go in via one of MFD, arm-soc or ARM trees
> as a whole series (if that would be ok with respective maintainers).
Fine with me.
Reviewed-by: Wolfram Sang <wsa@the-dreams.de>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 04/17] input: locomokbd: provide an Alt-SysRQ combination
From: Dmitry Torokhov @ 2015-05-12 20:12 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio, linux-i2c,
linux-input, linux-leds, linux-spi, linux-fbdev, alsa-devel
In-Reply-To: <1430178954-11138-5-git-send-email-dbaryshkov@gmail.com>
Hi Dmitry,
On Tue, Apr 28, 2015 at 02:55:41AM +0300, Dmitry Eremin-Solenikov wrote:
> It is usefull sometimes to have an Alt-SysRQ combo on the keyboard to be
> able to trigger sysrq functions directly. Add an option providing sysrq
> mapping for Contact-Home keys.
No, if you want to remap the keys please do it from usepspace; locomokbd
supports it (EVIOCSKEYCODE ioctl).
Thanks.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
> drivers/input/keyboard/Kconfig | 7 +++++++
> drivers/input/keyboard/locomokbd.c | 14 +++++++++++---
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 0a3d875..eb8b11b 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -343,6 +343,13 @@ config KEYBOARD_LOCOMO
> To compile this driver as a module, choose M here: the
> module will be called locomokbd.
>
> +config KEYBOARD_LOCOMO_SYSRQ
> + bool "Provide SysRQ key on LoCoMo keyboard"
> + depends on KEYBOARD_LOCOMO
> + help
> + Say Y here to be able to use Contact-Home as Alt-Sysrq combo.
> + Say N if you want to use them as usual keys.
> +
> config KEYBOARD_LPC32XX
> tristate "LPC32XX matrix key scanner support"
> depends on ARCH_LPC32XX && OF
> diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> index eed0a94..eb8dbe2 100644
> --- a/drivers/input/keyboard/locomokbd.c
> +++ b/drivers/input/keyboard/locomokbd.c
> @@ -40,6 +40,14 @@
> #define KEY_CONTACT KEY_F18
> #define KEY_CENTER KEY_F15
>
> +#ifdef CONFIG_KEYBOARD_LOCOMOKBD_SYSRQ
> +#define LOCOMO_KEY_CONTACT KEY_LEFTALT
> +#define LOCOMO_KEY_HOME KEY_SYSRQ
> +#else
> +#define LOCOMO_KEY_CONTACT KEY_CONTACT
> +#define LOCOMO_KEY_HOME KEY_HOME
> +#endif
> +
> #define KB_ROWS 16
> #define KB_COLS 8
> #define LOCOMOKBD_NUMKEYS (KB_ROWS * KB_COLS)
> @@ -48,7 +56,7 @@
> static const unsigned char
> locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
> 0, KEY_ESC, KEY_ACTIVITY, 0, 0, 0, 0, 0, 0, 0, /* 0 - 9 */
> - 0, 0, 0, 0, 0, 0, 0, KEY_MENU, 0, KEY_CONTACT, /* 10 - 19 */
> + 0, 0, 0, 0, 0, 0, 0, KEY_MENU, 0, LOCOMO_KEY_CONTACT, /* 10 - 19 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 20 - 29 */
> 0, 0, 0, KEY_CENTER, 0, KEY_MAIL, 0, 0, 0, 0, /* 30 - 39 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RIGHT, /* 40 - 49 */
> @@ -280,9 +288,9 @@ static int locomokbd_probe(struct platform_device *dev)
> sizeof(locomokbd->keycode));
>
> if (machine_is_collie())
> - locomokbd->keycode[18] = KEY_HOME;
> + locomokbd->keycode[18] = LOCOMO_KEY_HOME;
> else
> - locomokbd->keycode[3] = KEY_HOME;
> + locomokbd->keycode[3] = LOCOMO_KEY_HOME;
>
> for (i = 0; i < LOCOMOKBD_NUMKEYS; i++)
> input_set_capability(input_dev, EV_KEY, locomokbd->keycode[i]);
> --
> 2.1.4
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 03/17] input: convert LoCoMo keyboard driver to use new locomo core
From: Dmitry Torokhov @ 2015-05-12 20:21 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio, linux-i2c,
linux-input, linux-leds, linux-spi, linux-fbdev, alsa-devel
In-Reply-To: <1430178954-11138-4-git-send-email-dbaryshkov@gmail.com>
Hi Dmitry,
On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:
> As LoCoMo is switching to new device model, adapt keyboard driver to
> support new locomo core driver.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
> drivers/input/keyboard/Kconfig | 1 -
> drivers/input/keyboard/locomokbd.c | 271 +++++++++++++++++++------------------
> 2 files changed, 143 insertions(+), 129 deletions(-)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 106fbac..0a3d875 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -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
>
> diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> index c94d610..eed0a94 100644
> --- a/drivers/input/keyboard/locomokbd.c
> +++ b/drivers/input/keyboard/locomokbd.c
> @@ -23,37 +23,37 @@
> *
> */
>
> -#include <linux/slab.h>
> -#include <linux/module.h>
> +#include <linux/delay.h>
> #include <linux/init.h>
> #include <linux/input.h>
> -#include <linux/delay.h>
> -#include <linux/device.h>
> #include <linux/interrupt.h>
> -#include <linux/ioport.h>
> -
> -#include <asm/hardware/locomo.h>
> -#include <asm/irq.h>
> -
> -MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>");
> -MODULE_DESCRIPTION("LoCoMo keyboard driver");
> -MODULE_LICENSE("GPL");
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/locomo.h>
>
> -#define LOCOMOKBD_NUMKEYS 128
> +/* There is one minor difference between mappings on poodle and collie */
> +#include <asm/mach-types.h>
>
> #define KEY_ACTIVITY KEY_F16
> #define KEY_CONTACT KEY_F18
> #define KEY_CENTER KEY_F15
>
> +#define KB_ROWS 16
> +#define KB_COLS 8
> +#define LOCOMOKBD_NUMKEYS (KB_ROWS * KB_COLS)
> +#define SCANCODE(c, r) (((c)<<4) + (r) + 1)
> +
> static const unsigned char
> locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
> 0, KEY_ESC, KEY_ACTIVITY, 0, 0, 0, 0, 0, 0, 0, /* 0 - 9 */
> - 0, 0, 0, 0, 0, 0, 0, KEY_MENU, KEY_HOME, KEY_CONTACT, /* 10 - 19 */
> + 0, 0, 0, 0, 0, 0, 0, KEY_MENU, 0, KEY_CONTACT, /* 10 - 19 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 20 - 29 */
> 0, 0, 0, KEY_CENTER, 0, KEY_MAIL, 0, 0, 0, 0, /* 30 - 39 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RIGHT, /* 40 - 49 */
> KEY_UP, KEY_LEFT, 0, 0, KEY_P, 0, KEY_O, KEY_I, KEY_Y, KEY_T, /* 50 - 59 */
> - KEY_E, KEY_W, 0, 0, 0, 0, KEY_DOWN, KEY_ENTER, 0, 0, /* 60 - 69 */
> + KEY_E, KEY_W, 0, 0, 0, 0, KEY_DOWN, KEY_KPENTER, 0, 0, /* 60 - 69 */
> KEY_BACKSPACE, 0, KEY_L, KEY_U, KEY_H, KEY_R, KEY_D, KEY_Q, 0, 0, /* 70 - 79 */
> 0, 0, 0, 0, 0, 0, KEY_ENTER, KEY_RIGHTSHIFT, KEY_K, KEY_J, /* 80 - 89 */
> KEY_G, KEY_F, KEY_X, KEY_S, 0, 0, 0, 0, 0, 0, /* 90 - 99 */
> @@ -62,20 +62,14 @@ locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
> KEY_M, KEY_SPACE, KEY_V, KEY_APOSTROPHE, KEY_SLASH, 0, 0, 0 /* 120 - 128 */
> };
>
> -#define KB_ROWS 16
> -#define KB_COLS 8
> -#define KB_ROWMASK(r) (1 << (r))
> -#define SCANCODE(c,r) ( ((c)<<4) + (r) + 1 )
> -
> #define KB_DELAY 8
> -#define SCAN_INTERVAL (HZ/10)
>
> struct locomokbd {
> unsigned char keycode[LOCOMOKBD_NUMKEYS];
> struct input_dev *input;
> - char phys[32];
>
> - unsigned long base;
> + struct regmap *regmap;
> + int irq;
> spinlock_t lock;
>
> struct timer_list timer;
> @@ -84,37 +78,33 @@ struct locomokbd {
> };
>
> /* helper functions for reading the keyboard matrix */
> -static inline void locomokbd_charge_all(unsigned long membase)
> +static inline void locomokbd_charge_all(struct locomokbd *locomokbd)
> {
> - locomo_writel(0x00FF, membase + LOCOMO_KSC);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x00ff);
> }
>
> -static inline void locomokbd_activate_all(unsigned long membase)
> +static inline void locomokbd_activate_all(struct locomokbd *locomokbd)
Drop "inline"s from the .c file please.
> {
> - unsigned long r;
> -
> - locomo_writel(0, membase + LOCOMO_KSC);
> - r = locomo_readl(membase + LOCOMO_KIC);
> - r &= 0xFEFF;
> - locomo_writel(r, membase + LOCOMO_KIC);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> }
>
> -static inline void locomokbd_activate_col(unsigned long membase, int col)
> +static inline void locomokbd_activate_col(struct locomokbd *locomokbd, int col)
> {
> unsigned short nset;
> unsigned short nbset;
>
> - nset = 0xFF & ~(1 << col);
> + nset = 0xFF & ~BIT(col);
> nbset = (nset << 8) + nset;
> - locomo_writel(nbset, membase + LOCOMO_KSC);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, nbset);
> }
>
> -static inline void locomokbd_reset_col(unsigned long membase, int col)
> +static inline void locomokbd_reset_col(struct locomokbd *locomokbd, int col)
> {
> unsigned short nbset;
>
> - nbset = ((0xFF & ~(1 << col)) << 8) + 0xFF;
> - locomo_writel(nbset, membase + LOCOMO_KSC);
> + nbset = ((0xFF & ~BIT(col)) << 8) + 0xFF;
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, nbset);
> }
>
> /*
> @@ -129,24 +119,25 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
> unsigned int row, col, rowd;
> unsigned long flags;
> unsigned int num_pressed;
> - unsigned long membase = locomokbd->base;
> + bool esc_pressed = false;
>
> spin_lock_irqsave(&locomokbd->lock, flags);
>
> - locomokbd_charge_all(membase);
> + locomokbd_charge_all(locomokbd);
>
> num_pressed = 0;
> for (col = 0; col < KB_COLS; col++) {
> -
> - locomokbd_activate_col(membase, col);
> + udelay(KB_DELAY);
> + locomokbd_activate_col(locomokbd, col);
> udelay(KB_DELAY);
>
> - rowd = ~locomo_readl(membase + LOCOMO_KIB);
> + regmap_read(locomokbd->regmap, LOCOMO_KIB, &rowd);
> + rowd = ~rowd;
> for (row = 0; row < KB_ROWS; row++) {
> unsigned int scancode, pressed, key;
>
> scancode = SCANCODE(col, row);
> - pressed = rowd & KB_ROWMASK(row);
> + pressed = rowd & BIT(row);
> key = locomokbd->keycode[scancode];
>
> input_report_key(locomokbd->input, key, pressed);
> @@ -158,29 +149,30 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
> /* The "Cancel/ESC" key is labeled "On/Off" on
> * Collie and Poodle and should suspend the device
> * if it was pressed for more than a second. */
> - if (unlikely(key = KEY_ESC)) {
> - if (!time_after(jiffies,
> - locomokbd->suspend_jiffies + HZ))
> - continue;
> - if (locomokbd->count_cancel++
> - != (HZ/SCAN_INTERVAL + 1))
> - continue;
> - input_event(locomokbd->input, EV_PWR,
> - KEY_SUSPEND, 1);
> - locomokbd->suspend_jiffies = jiffies;
> - } else
> - locomokbd->count_cancel = 0;
> + if (unlikely(key = KEY_ESC))
> + esc_pressed = true;
> }
> - locomokbd_reset_col(membase, col);
> + locomokbd_reset_col(locomokbd, col);
> }
> - locomokbd_activate_all(membase);
> + locomokbd_activate_all(locomokbd);
>
> input_sync(locomokbd->input);
>
> /* if any keys are pressed, enable the timer */
> if (num_pressed)
> - mod_timer(&locomokbd->timer, jiffies + SCAN_INTERVAL);
> + mod_timer(&locomokbd->timer, jiffies + msecs_to_jiffies(100));
> else
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> +
> +
> + if (esc_pressed && time_after(jiffies,
> + locomokbd->suspend_jiffies + msecs_to_jiffies(1000))) {
> + if (locomokbd->count_cancel++ > (20)) {
Why parentheses around 20?
> + input_event(locomokbd->input, EV_PWR,
> + KEY_SUSPEND, 1);
> + locomokbd->suspend_jiffies = jiffies;
> + }
> + } else
> locomokbd->count_cancel = 0;
If one branch has curly braces the other should have them too.
>
> spin_unlock_irqrestore(&locomokbd->lock, flags);
> @@ -192,18 +184,18 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
> static irqreturn_t locomokbd_interrupt(int irq, void *dev_id)
> {
> struct locomokbd *locomokbd = dev_id;
> - u16 r;
> + unsigned int r;
>
> - r = locomo_readl(locomokbd->base + LOCOMO_KIC);
> +
> + regmap_read(locomokbd->regmap, LOCOMO_KIC, &r);
> if ((r & 0x0001) = 0)
> return IRQ_HANDLED;
>
> - locomo_writel(r & ~0x0100, locomokbd->base + LOCOMO_KIC); /* Ack */
> + /* Mask and Ack */
> + regmap_write(locomokbd->regmap, LOCOMO_KIC, r & ~0x110);
>
> - /** wait chattering delay **/
> - udelay(100);
> + mod_timer(&locomokbd->timer, jiffies + msecs_to_jiffies(1));
>
> - locomokbd_scankeyboard(locomokbd);
> return IRQ_HANDLED;
> }
>
> @@ -220,47 +212,37 @@ static void locomokbd_timer_callback(unsigned long data)
> 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);
> - return 0;
> +
> + return regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> }
>
> 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);
> +
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x0);
> }
>
> -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;
>
> - locomokbd = kzalloc(sizeof(struct locomokbd), GFP_KERNEL);
> - input_dev = input_allocate_device();
> - if (!locomokbd || !input_dev) {
> - err = -ENOMEM;
> - goto err_free_mem;
> - }
> + locomokbd = devm_kzalloc(&dev->dev, sizeof(struct locomokbd),
> + GFP_KERNEL);
> + if (!locomokbd)
> + 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;
> - }
> + locomokbd->regmap = dev_get_regmap(dev->dev.parent, NULL);
> + if (!locomokbd->regmap)
> + return -EINVAL;
>
> - locomo_set_drvdata(dev, locomokbd);
> + locomokbd->irq = platform_get_irq(dev, 0);
> + if (locomokbd->irq < 0)
> + return -ENXIO;
>
> - locomokbd->base = (unsigned long) dev->mapbase;
> + platform_set_drvdata(dev, locomokbd);
>
> spin_lock_init(&locomokbd->lock);
>
> @@ -270,11 +252,13 @@ static int locomokbd_probe(struct locomo_dev *dev)
>
> locomokbd->suspend_jiffies = jiffies;
>
> - locomokbd->input = input_dev;
> - strcpy(locomokbd->phys, "locomokbd/input0");
> + input_dev = input_allocate_device();
devm_input_allocate_device()?
> + if (!input_dev)
> + return -ENOMEM;
>
> + locomokbd->input = input_dev;
> input_dev->name = "LoCoMo keyboard";
> - input_dev->phys = locomokbd->phys;
> + input_dev->phys = "locomokbd/input0";
> input_dev->id.bustype = BUS_HOST;
> input_dev->id.vendor = 0x0001;
> input_dev->id.product = 0x0001;
> @@ -291,16 +275,30 @@ static int locomokbd_probe(struct locomo_dev *dev)
>
> input_set_drvdata(input_dev, locomokbd);
>
> - memcpy(locomokbd->keycode, locomokbd_keycode, sizeof(locomokbd->keycode));
> + memcpy(locomokbd->keycode,
> + locomokbd_keycode,
> + sizeof(locomokbd->keycode));
> +
> + if (machine_is_collie())
> + locomokbd->keycode[18] = KEY_HOME;
> + else
> + locomokbd->keycode[3] = KEY_HOME;
This seems like a new addition. Ideally keymap twiddling shoudl be done
from userspace.
> +
> for (i = 0; i < LOCOMOKBD_NUMKEYS; i++)
> - set_bit(locomokbd->keycode[i], input_dev->keybit);
> - clear_bit(0, input_dev->keybit);
> + input_set_capability(input_dev, EV_KEY, locomokbd->keycode[i]);
> + input_set_capability(input_dev, EV_PWR, KEY_SUSPEND);
> + __set_bit(EV_REP, input_dev->evbit);
> +
> + regmap_write(locomokbd->regmap, LOCOMO_KCMD, 1);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x0);
> + regmap_write(locomokbd->regmap, LOCOMO_KIC, 0x0);
>
> /* attempt to get the interrupt */
> - err = request_irq(dev->irq[0], locomokbd_interrupt, 0, "locomokbd", locomokbd);
> + err = request_irq(locomokbd->irq, locomokbd_interrupt, 0,
> + "locomokbd", locomokbd);
devm_request_irq()?
> if (err) {
> - printk(KERN_ERR "locomokbd: Can't get irq for keyboard\n");
> - goto err_release_region;
> + dev_err(&dev->dev, "locomokbd: Can't get irq for keyboard\n");
> + goto err_free_mem;
> }
>
> err = input_register_device(locomokbd->input);
> @@ -309,54 +307,71 @@ static int locomokbd_probe(struct locomo_dev *dev)
>
> return 0;
>
> - err_free_irq:
> - free_irq(dev->irq[0], locomokbd);
> - err_release_region:
> - release_mem_region((unsigned long) dev->mapbase, dev->length);
> - locomo_set_drvdata(dev, NULL);
> - err_free_mem:
> +err_free_irq:
> + free_irq(locomokbd->irq, locomokbd);
> +err_free_mem:
> input_free_device(input_dev);
> - kfree(locomokbd);
>
> return err;
> }
>
> -static int locomokbd_remove(struct locomo_dev *dev)
> +static int locomokbd_remove(struct platform_device *dev)
> {
> - struct locomokbd *locomokbd = locomo_get_drvdata(dev);
> + struct locomokbd *locomokbd = platform_get_drvdata(dev);
>
> - free_irq(dev->irq[0], locomokbd);
> + free_irq(locomokbd->irq, locomokbd);
Is not needed with devm.
>
> del_timer_sync(&locomokbd->timer);
Should likely to go into close().
>
> input_unregister_device(locomokbd->input);
Is not needed with devm.
> - locomo_set_drvdata(dev, NULL);
>
> - release_mem_region((unsigned long) dev->mapbase, dev->length);
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int locomokbd_suspend(struct device *dev)
Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.
> +{
> + struct locomokbd *locomokbd = dev_get_drvdata(dev);
> +
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x0);
>
> - kfree(locomokbd);
> + del_timer_sync(&locomokbd->timer);
>
> return 0;
> }
>
> -static struct locomo_driver keyboard_driver = {
> - .drv = {
> - .name = "locomokbd"
> +static int locomokbd_resume(struct device *dev)
__maybe_unused as well.
> +{
> + struct locomokbd *locomokbd = dev_get_drvdata(dev);
> +
> + regmap_write(locomokbd->regmap, LOCOMO_KCMD, 1);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> +
> + locomokbd_scankeyboard(locomokbd);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(locomo_kbd_pm, locomokbd_suspend, locomokbd_resume);
> +#define LOCOMO_KBD_PM (&locomo_kbd_pm)
> +#else
> +#define LOCOMO_KBD_PM NULL
> +#endif
Just do
static SIMPLE_DEV_PM_OPS(locomo_kbd_pm,
locomokbd_suspend, locomokbd_resume);
outside of #ifdef, it will produce the right thing (an empty structure).
> +
> +static struct platform_driver locomokbd_driver = {
> + .driver = {
> + .name = "locomo-kbd",
> + .pm = LOCOMO_KBD_PM,
.pm = &locomo_kbd_pm;
> },
> - .devid = LOCOMO_DEVID_KEYBOARD,
> .probe = locomokbd_probe,
> .remove = locomokbd_remove,
> };
>
> -static int __init locomokbd_init(void)
> -{
> - return locomo_driver_register(&keyboard_driver);
> -}
> -
> -static void __exit locomokbd_exit(void)
> -{
> - locomo_driver_unregister(&keyboard_driver);
> -}
> +module_platform_driver(locomokbd_driver);
>
> -module_init(locomokbd_init);
> -module_exit(locomokbd_exit);
> +MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>");
> +MODULE_DESCRIPTION("LoCoMo keyboard driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:locomo-kbd");
> --
> 2.1.4
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 01/17] mfd: add new driver for Sharp LoCoMo
From: Dmitry Eremin-Solenikov @ 2015-05-12 20:39 UTC (permalink / raw)
To: Lee Jones
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Dmitry Torokhov, Bryan Wu,
Richard Purdie, Samuel Ortiz, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <20150428184525.GJ9169@x1>
2015-04-28 21:45 GMT+03:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 28 Apr 2015, Dmitry Eremin-Solenikov wrote:
>
>> 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 <dbaryshkov@gmail.com>
>> ---
Thanks for the review. I agree (and have implemented) with most of
your comments.
However I have few questions. See below.
>
>> +/* the following is the overall data for the locomo chip */
>> +struct locomo {
>> + struct device *dev;
>> + unsigned int irq;
>> + spinlock_t lock;
>> + struct irq_domain *domain;
>> + struct regmap *regmap;
>> +};
>> +
>> +static struct resource locomo_kbd_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_KEY),
>> +};
>> +
>> +static struct resource locomo_gpio_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_GPIO),
>> +};
>> +
>> +/* Filled in locomo_probe() function. */
>> +static struct locomo_gpio_platform_data locomo_gpio_pdata;
>
> I'd prefer you didn't use globals for this.
Just for platform data, or for all the structures?
>> +static struct resource locomo_lt_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_LT),
>> +};
>> +
>> +static struct resource locomo_spi_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_SPI),
>> +};
>> +
>> +/* Filled in locomo_probe() function. */
>> +static struct locomo_lcd_platform_data locomo_lcd_pdata;
>> +
>> +static struct mfd_cell locomo_cells[] = {
>> + {
>> + .name = "locomo-kbd",
>> + .resources = locomo_kbd_resources,
>> + .num_resources = ARRAY_SIZE(locomo_kbd_resources),
>> + },
>> + {
>> + .name = "locomo-gpio",
>> + .resources = locomo_gpio_resources,
>> + .num_resources = ARRAY_SIZE(locomo_gpio_resources),
>> + .platform_data = &locomo_gpio_pdata,
>> + .pdata_size = sizeof(locomo_gpio_pdata),
>> + },
>> + {
>> + .name = "locomo-lt", /* Long time timer */
>> + .resources = locomo_lt_resources,
>> + .num_resources = ARRAY_SIZE(locomo_lt_resources),
>> + },
>> + {
>> + .name = "locomo-spi",
>> + .resources = locomo_spi_resources,
>> + .num_resources = ARRAY_SIZE(locomo_spi_resources),
>> + },
>> + {
>> + .name = "locomo-led",
>> + },
>> + {
>> + .name = "locomo-backlight",
>> + },
>
> Please make these:
>
>> + { .name = "locomo-led" },
>> + { .name = "locomo-backlight" },
>
> ... and put them at the bottom.
They will be populated by of_compatible lines, so it makes little sense
to me. What about adding of compatibility lines to this patch?
>> + while (1) {
>> + regmap_read(lchip->regmap, LOCOMO_ICR, &req);
>> + req &= 0x0f00;
>
> What is this magic number? Please #define it.
Adding comments to this function instead.
>
>> + if (!req)
>> + break;
>> +
>> + irq = ffs(req) - 9;
>
> Minus another random number? Either define it or enter a comment.
>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int locomo_suspend(struct device *dev)
>> +{
>> + struct locomo *lchip = dev_get_drvdata(dev);
>> +
>> + /* AUDIO */
>
> WHY ARE YOU SHOUTING? Ironic eh? ;)
>
>> + regmap_write(lchip->regmap, LOCOMO_PAIF, 0x00);
>> +
>> + /*
>> + * Original code disabled the clock depending on leds settings
>> + * However we disable leds before suspend, thus it's safe
>> + * to just assume this setting.
>> + */
>> + /* CLK32 off */
>> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
>> +
>> + /* 22MHz/24MHz clock off */
>> + regmap_write(lchip->regmap, LOCOMO_ACC, 0x00);
>> +
>> + return 0;
>> +}
>> +
>> +static int locomo_resume(struct device *dev)
>> +{
>> + struct locomo *lchip = dev_get_drvdata(dev);
>
> Do audio and clk sort themselves out?
PAIF and ACC registers are used only by audio parts of the device. However
there is no current Linux driver for those parts. The registers are cleared
in case the firmware has set something in them, but in future it will
be the task
of the audio driver to properly clear and restore them.
>> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
>> +
>> + return 0;
>> +}
>> +
[skipped]
>> +
>> + if (pdata) {
>> + locomo_gpio_pdata.gpio_base = pdata->gpio_base;
>> + locomo_lcd_pdata.comadj = pdata->comadj;
>> + } else {
>> + locomo_gpio_pdata.gpio_base = -1;
>> + locomo_lcd_pdata.comadj = 128;
>> + }
>
> struct locomo_gpio_platform_data locomo_gpio_pdata;
>
> locomo_gpio_pdata = devm_kzalloc(<blah>);
>
> locomo_cells[GPIO].platform_data = locomo_gpio_pdata;
I do not quite agree with you at this place. The passed platform_data
will be kmemdup()'ed inside platform core. So the whole struct will be
duplicated twice inside kmallocate'd memory. Ideally I'd like to drop
the whole platform_data busyness, but that requires switching to DTS
first.
>> diff --git a/include/linux/mfd/locomo.h b/include/linux/mfd/locomo.h
>> new file mode 100644
>> index 0000000..6729767
>> --- /dev/null
>> +++ b/include/linux/mfd/locomo.h
>> +/* MCS decoder for boot selecting */
>> +#define LOCOMO_MCSX0 0x10
>> +#define LOCOMO_MCSX1 0x14
>> +#define LOCOMO_MCSX2 0x18
>> +#define LOCOMO_MCSX3 0x1c
>
> These are pretty cryptic. Any way of making them easier to identify.
No way. The names are based on old Sharp code. The drivers do not use
them, but I'd like to still keep the registers for the reference purposes.
>> +struct locomo_gpio_platform_data {
>> + unsigned int gpio_base;
>> +};
>
> A struct for a single int seems overkill.
>
>> +struct locomo_lcd_platform_data {
>> + u8 comadj;
>> +};
>> +
>> +struct locomo_platform_data {
>> + unsigned int gpio_base;
>> + u8 comadj;
>> +};
>
> Why do you need to pass gpio_base twice?
First: machine file -> core driver
Second: core driver -> gpio driver
The other way to do the same would be:
struct locomo_gpio_platform_data {
unsigned int gpio_base;
};
struct locomo_lcd_platform_data {
u8 comadj;
};
struct locomo_platform_data {
struct locomo_gpio_platform_data gpio_pdata;
struct locomo_lcd_platform_data lcd_pdata;
};
And to assign pointers to the passed data in the mfd_cells
during locomo_probe. Does that look better to you?
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 04/17] input: locomokbd: provide an Alt-SysRQ combination
From: Dmitry Eremin-Solenikov @ 2015-05-12 20:40 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-input, linux-leds,
linux-spi
In-Reply-To: <20150512201215.GD20725@dtor-ws>
2015-05-12 23:12 GMT+03:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Dmitry,
>
> On Tue, Apr 28, 2015 at 02:55:41AM +0300, Dmitry Eremin-Solenikov wrote:
>> It is usefull sometimes to have an Alt-SysRQ combo on the keyboard to be
>> able to trigger sysrq functions directly. Add an option providing sysrq
>> mapping for Contact-Home keys.
>
> No, if you want to remap the keys please do it from usepspace; locomokbd
> supports it (EVIOCSKEYCODE ioctl).
OK, this was useful to boot kernels with minimal to no userspace (tiny
initramfs)
and still have the SysRQ at hand. I'll keep this as a local/OE patch instead.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 03/17] input: convert LoCoMo keyboard driver to use new locomo core
From: Dmitry Eremin-Solenikov @ 2015-05-12 21:01 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <20150512202106.GE20725@dtor-ws>
Hello,
2015-05-12 23:21 GMT+03:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Dmitry,
>
> On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:
>> As LoCoMo is switching to new device model, adapt keyboard driver to
>> support new locomo core driver.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
Thanks for the review.
>> /* helper functions for reading the keyboard matrix */
>> -static inline void locomokbd_charge_all(unsigned long membase)
>> +static inline void locomokbd_charge_all(struct locomokbd *locomokbd)
>> {
>> - locomo_writel(0x00FF, membase + LOCOMO_KSC);
>> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x00ff);
>> }
>>
>> -static inline void locomokbd_activate_all(unsigned long membase)
>> +static inline void locomokbd_activate_all(struct locomokbd *locomokbd)
>
> Drop "inline"s from the .c file please.
Why?
>
>> {
>> - unsigned long r;
>> -
>> - locomo_writel(0, membase + LOCOMO_KSC);
>> - r = locomo_readl(membase + LOCOMO_KIC);
>> - r &= 0xFEFF;
>> - locomo_writel(r, membase + LOCOMO_KIC);
>> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
>> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
>> }
>>
[skipped]
>> @@ -291,16 +275,30 @@ static int locomokbd_probe(struct locomo_dev *dev)
>>
>> input_set_drvdata(input_dev, locomokbd);
>>
>> - memcpy(locomokbd->keycode, locomokbd_keycode, sizeof(locomokbd->keycode));
>> + memcpy(locomokbd->keycode,
>> + locomokbd_keycode,
>> + sizeof(locomokbd->keycode));
>> +
>> + if (machine_is_collie())
>> + locomokbd->keycode[18] = KEY_HOME;
>> + else
>> + locomokbd->keycode[3] = KEY_HOME;
>
> This seems like a new addition. Ideally keymap twiddling shoudl be done
> from userspace.
This fixes a hardware issue. Home key is wired differently on two
devices using this driver.
I'd prefer to have such setting in board file or in DTS in future,
however that looks like an
overkill. What would be your suggestion?
>> /* attempt to get the interrupt */
>> - err = request_irq(dev->irq[0], locomokbd_interrupt, 0, "locomokbd", locomokbd);
>> + err = request_irq(locomokbd->irq, locomokbd_interrupt, 0,
>> + "locomokbd", locomokbd);
>
> devm_request_irq()?
>
[skipped]
>> -static int locomokbd_remove(struct locomo_dev *dev)
>> +static int locomokbd_remove(struct platform_device *dev)
>> {
>> - struct locomokbd *locomokbd = locomo_get_drvdata(dev);
>> + struct locomokbd *locomokbd = platform_get_drvdata(dev);
>>
>> - free_irq(dev->irq[0], locomokbd);
>> + free_irq(locomokbd->irq, locomokbd);
>
> Is not needed with devm.
Not quite. There will be a possibility for the IRQ to happen after deleting
a timer in locomokbd_remove() and before freeing the IRQ through the devres
core. Oops.
>
>>
>> del_timer_sync(&locomokbd->timer);
>
> Should likely to go into close().
Hmm. I will rethink this part, thank you.
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int locomokbd_suspend(struct device *dev)
>
> Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.
Fine, however I thought that #ifdef's here are a typical pattern.
[skipped the rest]
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 03/17] input: convert LoCoMo keyboard driver to use new locomo core
From: Dmitry Torokhov @ 2015-05-12 21:13 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <CALT56yOjiBWHH3Uuo82Hqh8Va8V_KCUaaLL7RFtPUjWmfOFbGA@mail.gmail.com>
On Wed, May 13, 2015 at 12:01:23AM +0300, Dmitry Eremin-Solenikov wrote:
> Hello,
>
> 2015-05-12 23:21 GMT+03:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > Hi Dmitry,
> >
> > On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:
> >> As LoCoMo is switching to new device model, adapt keyboard driver to
> >> support new locomo core driver.
> >>
> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >> ---
>
> Thanks for the review.
>
> >> /* helper functions for reading the keyboard matrix */
> >> -static inline void locomokbd_charge_all(unsigned long membase)
> >> +static inline void locomokbd_charge_all(struct locomokbd *locomokbd)
> >> {
> >> - locomo_writel(0x00FF, membase + LOCOMO_KSC);
> >> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x00ff);
> >> }
> >>
> >> -static inline void locomokbd_activate_all(unsigned long membase)
> >> +static inline void locomokbd_activate_all(struct locomokbd *locomokbd)
> >
> > Drop "inline"s from the .c file please.
>
> Why?
Because compiler usually knows better whether a function should be
inlined or not as it keeps track of available registers, so leave the
decision to it.
>
> >
> >> {
> >> - unsigned long r;
> >> -
> >> - locomo_writel(0, membase + LOCOMO_KSC);
> >> - r = locomo_readl(membase + LOCOMO_KIC);
> >> - r &= 0xFEFF;
> >> - locomo_writel(r, membase + LOCOMO_KIC);
> >> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> >> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> >> }
> >>
>
> [skipped]
>
> >> @@ -291,16 +275,30 @@ static int locomokbd_probe(struct locomo_dev *dev)
> >>
> >> input_set_drvdata(input_dev, locomokbd);
> >>
> >> - memcpy(locomokbd->keycode, locomokbd_keycode, sizeof(locomokbd->keycode));
> >> + memcpy(locomokbd->keycode,
> >> + locomokbd_keycode,
> >> + sizeof(locomokbd->keycode));
> >> +
> >> + if (machine_is_collie())
> >> + locomokbd->keycode[18] = KEY_HOME;
> >> + else
> >> + locomokbd->keycode[3] = KEY_HOME;
> >
> > This seems like a new addition. Ideally keymap twiddling shoudl be done
> > from userspace.
>
> This fixes a hardware issue. Home key is wired differently on two
> devices using this driver.
> I'd prefer to have such setting in board file or in DTS in future,
> however that looks like an
> overkill. What would be your suggestion?
I am OK with doing this in driver, just as a separate patch please.
>
> >> /* attempt to get the interrupt */
> >> - err = request_irq(dev->irq[0], locomokbd_interrupt, 0, "locomokbd", locomokbd);
> >> + err = request_irq(locomokbd->irq, locomokbd_interrupt, 0,
> >> + "locomokbd", locomokbd);
> >
> > devm_request_irq()?
> >
> [skipped]
>
> >> -static int locomokbd_remove(struct locomo_dev *dev)
> >> +static int locomokbd_remove(struct platform_device *dev)
> >> {
> >> - struct locomokbd *locomokbd = locomo_get_drvdata(dev);
> >> + struct locomokbd *locomokbd = platform_get_drvdata(dev);
> >>
> >> - free_irq(dev->irq[0], locomokbd);
> >> + free_irq(locomokbd->irq, locomokbd);
> >
> > Is not needed with devm.
>
> Not quite. There will be a possibility for the IRQ to happen after deleting
> a timer in locomokbd_remove() and before freeing the IRQ through the devres
> core. Oops.
Right, but if you make sure that device does not generate interrupts in
probe() until open() is called and do the same in close(), then it
should be OK.
>
> >
> >>
> >> del_timer_sync(&locomokbd->timer);
> >
> > Should likely to go into close().
>
> Hmm. I will rethink this part, thank you.
>
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int locomokbd_suspend(struct device *dev)
> >
> > Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.
>
> Fine, however I thought that #ifdef's here are a typical pattern.
It is up to subsystems, __maybe_unused provides better compile coverage.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 2/3] Phy: DT binding documentation for Broadcom Cygnus USB PHY driver
From: Arun Ramamurthy @ 2015-05-12 22:05 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King, Gregory CLEMENT,
Gabriel FERNANDEZ, Jason Cooper, Thomas Petazzoni,
Greg Kroah-Hartman, Arnd Bergmann
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Dmitry Torokhov,
Anatol Pomazau, Jonathan Richardson, Scott Branden, Ray Jui,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w
In-Reply-To: <5550B878.7060608-l0cyMroinI0@public.gmane.org>
Hi
On 15-05-11 07:11 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 23 April 2015 04:44 AM, Arun Ramamurthy wrote:
>> Broadcom's Cygnus chip has a USB 2.0 host controller connected to
>> three separate phys. One of the phs (port 2) is also connectd to
>> a usb 2.0 device controller
>>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> ---
>> .../bindings/phy/brcm,cygnus-usb-phy.txt | 69
>> ++++++++++++++++++++++
>> include/dt-bindings/phy/phy.h | 2 +
>> 2 files changed, 71 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>> b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>> new file mode 100644
>> index 0000000..ec62044
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>> @@ -0,0 +1,69 @@
>> +BROADCOM CYGNUS USB PHY
>> +
>> +Required Properties:
>> + - compatible: brcm,cygnus-usb-phy
>> + - reg : usbphy_regs - Base address of phy registers
>> + usb2h_idm_regs - Base address of host idm registers
>> + usb2d_idm_regs - Base address of device idm registers
>> + - phy-cells - must be 1 for each port declared. The node
>> + that uses the phy must provide either PHY_CONFIG_DEVICE
>> for device
>> + or PHY_CONFIG_HOST for host
>> +
>> +NOTE: port 0 and port 1 are host only and port 2 can be configured
>> for host or
>> +device.
>> +
>> +Example of phy :
>> + usbphy0: usbphy@0x0301c000 {
>> + compatible = "brcm,cygnus-usb-phy";
>> + reg = <0x0301c000 0x2000>,
>> + <0x18115000 0x1000>,
>> + <0x18111000 0x1000>;
>> + status = "okay";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + usbphy0_0: usbphy0@0 {
>> + #phy-cells = <1>;
>> + reg = <0>;
>> + status = "okay";
>> + phy-supply = <&vbus_p0>;
>> + };
>> +
>> + usbphy0_1: usbphy0@1 {
>> + #phy-cells = <1>;
>> + reg = <1>;
>> + status = "okay";
>> + };
>> +
>> + usbphy0_2: usbphy0@2 {
>> + #phy-cells = <1>;
>> + reg = <2>;
>> + status = "okay";
>> + phy-supply = <&vbus_p2>;
>> + };
>> + };
>> +
>> +Example of node using the phy:
>> +
>> + /* This nodes declares all three ports as host */
>> +
>> + ehci0: usb@0x18048000 {
>> + compatible = "generic-ehci";
>> + reg = <0x18048000 0x100>;
>> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> + phys = <&usbphy0_0 PHY_CONFIG_HOST &usbphy0_1 PHY_CONFIG_HOST
>> &usbphy0_2 PHY_CONFIG_HOST>;
>> + status = "okay";
>> + };
>> +
>> + /*
>> + * This node declares port 2 phy
>> + * and configures it for device
>> + */
>> +
>> + usbd_udc_dwc1: usbd_udc_dwc@0x1804c000 {
>> + compatible = "iproc-udc";
>> + reg = <0x1804c000 0x2000>;
>> + interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
>> + phys = <&usbphy0_2 PHY_CONFIG_DEVICE>;
>> + phy-names = "usb";
>> + };
>> diff --git a/include/dt-bindings/phy/phy.h
>> b/include/dt-bindings/phy/phy.h
>> index 6c90193..3f6b1ac 100644
>> --- a/include/dt-bindings/phy/phy.h
>> +++ b/include/dt-bindings/phy/phy.h
>> @@ -15,5 +15,7 @@
>> #define PHY_TYPE_PCIE 2
>> #define PHY_TYPE_USB2 3
>> #define PHY_TYPE_USB3 4
>> +#define PHY_CONFIG_HOST 1
>> +#define PHY_CONFIG_DEVICE 0
>
> '0' and '1' are already defined for "PHY_NONE" and "PHY_TYPE_SATA". Is
> this for USB2 or for USB3?
>
This is for USB2, do you want me to prefix the defines with USB2?
I think It would be misleading to use PHY_TYPE_SATA or PHY_NONE in this
scenario
> Thanks
> Kishon
^ permalink raw reply
* Re: [PATCH v3 2/3] Phy: DT binding documentation for Broadcom Cygnus USB PHY driver
From: Kishon Vijay Abraham I @ 2015-05-13 6:14 UTC (permalink / raw)
To: Arun Ramamurthy, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King, Gregory CLEMENT,
Gabriel FERNANDEZ, Jason Cooper, Thomas Petazzoni,
Greg Kroah-Hartman, Arnd Bergmann
Cc: devicetree, linux-kernel, linux-fbdev, Dmitry Torokhov,
Anatol Pomazau, Jonathan Richardson, Scott Branden, Ray Jui,
bcm-kernel-feedback-list
In-Reply-To: <55527935.5060907@broadcom.com>
Hi,
On Wednesday 13 May 2015 03:35 AM, Arun Ramamurthy wrote:
> Hi
>
> On 15-05-11 07:11 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 23 April 2015 04:44 AM, Arun Ramamurthy wrote:
>>> Broadcom's Cygnus chip has a USB 2.0 host controller connected to
>>> three separate phys. One of the phs (port 2) is also connectd to
>>> a usb 2.0 device controller
>>>
>>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>>> ---
>>> .../bindings/phy/brcm,cygnus-usb-phy.txt | 69
>>> ++++++++++++++++++++++
>>> include/dt-bindings/phy/phy.h | 2 +
>>> 2 files changed, 71 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>>> b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>>> new file mode 100644
>>> index 0000000..ec62044
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>>> @@ -0,0 +1,69 @@
>>> +BROADCOM CYGNUS USB PHY
>>> +
>>> +Required Properties:
>>> + - compatible: brcm,cygnus-usb-phy
>>> + - reg : usbphy_regs - Base address of phy registers
>>> + usb2h_idm_regs - Base address of host idm registers
>>> + usb2d_idm_regs - Base address of device idm registers
>>> + - phy-cells - must be 1 for each port declared. The node
>>> + that uses the phy must provide either PHY_CONFIG_DEVICE
>>> for device
>>> + or PHY_CONFIG_HOST for host
>>> +
>>> +NOTE: port 0 and port 1 are host only and port 2 can be configured
>>> for host or
>>> +device.
>>> +
>>> +Example of phy :
>>> + usbphy0: usbphy@0x0301c000 {
>>> + compatible = "brcm,cygnus-usb-phy";
>>> + reg = <0x0301c000 0x2000>,
>>> + <0x18115000 0x1000>,
>>> + <0x18111000 0x1000>;
>>> + status = "okay";
>>> +
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + usbphy0_0: usbphy0@0 {
>>> + #phy-cells = <1>;
>>> + reg = <0>;
>>> + status = "okay";
>>> + phy-supply = <&vbus_p0>;
>>> + };
>>> +
>>> + usbphy0_1: usbphy0@1 {
>>> + #phy-cells = <1>;
>>> + reg = <1>;
>>> + status = "okay";
>>> + };
>>> +
>>> + usbphy0_2: usbphy0@2 {
>>> + #phy-cells = <1>;
>>> + reg = <2>;
>>> + status = "okay";
>>> + phy-supply = <&vbus_p2>;
>>> + };
>>> + };
>>> +
>>> +Example of node using the phy:
>>> +
>>> + /* This nodes declares all three ports as host */
>>> +
>>> + ehci0: usb@0x18048000 {
>>> + compatible = "generic-ehci";
>>> + reg = <0x18048000 0x100>;
>>> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>>> + phys = <&usbphy0_0 PHY_CONFIG_HOST &usbphy0_1 PHY_CONFIG_HOST
>>> &usbphy0_2 PHY_CONFIG_HOST>;
>>> + status = "okay";
>>> + };
>>> +
>>> + /*
>>> + * This node declares port 2 phy
>>> + * and configures it for device
>>> + */
>>> +
>>> + usbd_udc_dwc1: usbd_udc_dwc@0x1804c000 {
>>> + compatible = "iproc-udc";
>>> + reg = <0x1804c000 0x2000>;
>>> + interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
>>> + phys = <&usbphy0_2 PHY_CONFIG_DEVICE>;
>>> + phy-names = "usb";
>>> + };
>>> diff --git a/include/dt-bindings/phy/phy.h
>>> b/include/dt-bindings/phy/phy.h
>>> index 6c90193..3f6b1ac 100644
>>> --- a/include/dt-bindings/phy/phy.h
>>> +++ b/include/dt-bindings/phy/phy.h
>>> @@ -15,5 +15,7 @@
>>> #define PHY_TYPE_PCIE 2
>>> #define PHY_TYPE_USB2 3
>>> #define PHY_TYPE_USB3 4
>>> +#define PHY_CONFIG_HOST 1
>>> +#define PHY_CONFIG_DEVICE 0
>>
>> '0' and '1' are already defined for "PHY_NONE" and "PHY_TYPE_SATA". Is
>> this for USB2 or for USB3?
>>
> This is for USB2, do you want me to prefix the defines with USB2?
yes. That will avoid confusion when someone tries to add USB3 HOST PHY or USB3
DEVICE PHY.
> I think It would be misleading to use PHY_TYPE_SATA or PHY_NONE in this
> scenario
I meant to use the next available values like 5 and 6.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH v2 01/17] mfd: add new driver for Sharp LoCoMo
From: Lee Jones @ 2015-05-13 9:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Dmitry Torokhov, Bryan Wu,
Richard Purdie, Samuel Ortiz, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <CALT56yP6FPhGRZFJZkyjjpqGJ6bofg+a1BTC9KcVAgpmt2htoA@mail.gmail.com>
On Tue, 12 May 2015, Dmitry Eremin-Solenikov wrote:
> 2015-04-28 21:45 GMT+03:00 Lee Jones <lee.jones@linaro.org>:
> > On Tue, 28 Apr 2015, Dmitry Eremin-Solenikov wrote:
> >
> >> 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 <dbaryshkov@gmail.com>
> >> ---
>
> Thanks for the review. I agree (and have implemented) with most of
> your comments.
> However I have few questions. See below.
>
> >
> >> +/* the following is the overall data for the locomo chip */
> >> +struct locomo {
> >> + struct device *dev;
> >> + unsigned int irq;
> >> + spinlock_t lock;
> >> + struct irq_domain *domain;
> >> + struct regmap *regmap;
> >> +};
> >> +
> >> +static struct resource locomo_kbd_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_KEY),
> >> +};
> >> +
> >> +static struct resource locomo_gpio_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_GPIO),
> >> +};
> >> +
> >> +/* Filled in locomo_probe() function. */
> >> +static struct locomo_gpio_platform_data locomo_gpio_pdata;
> >
> > I'd prefer you didn't use globals for this.
>
> Just for platform data, or for all the structures?
Just for this. The remainder are standard.
> >> +static struct resource locomo_lt_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_LT),
> >> +};
> >> +
> >> +static struct resource locomo_spi_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_SPI),
> >> +};
> >> +
> >> +/* Filled in locomo_probe() function. */
> >> +static struct locomo_lcd_platform_data locomo_lcd_pdata;
> >> +
> >> +static struct mfd_cell locomo_cells[] = {
> >> + {
> >> + .name = "locomo-kbd",
> >> + .resources = locomo_kbd_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_kbd_resources),
> >> + },
> >> + {
> >> + .name = "locomo-gpio",
> >> + .resources = locomo_gpio_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_gpio_resources),
> >> + .platform_data = &locomo_gpio_pdata,
> >> + .pdata_size = sizeof(locomo_gpio_pdata),
> >> + },
> >> + {
> >> + .name = "locomo-lt", /* Long time timer */
> >> + .resources = locomo_lt_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_lt_resources),
> >> + },
> >> + {
> >> + .name = "locomo-spi",
> >> + .resources = locomo_spi_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_spi_resources),
> >> + },
> >> + {
> >> + .name = "locomo-led",
> >> + },
> >> + {
> >> + .name = "locomo-backlight",
> >> + },
> >
> > Please make these:
> >
> >> + { .name = "locomo-led" },
> >> + { .name = "locomo-backlight" },
> >
> > ... and put them at the bottom.
>
> They will be populated by of_compatible lines, so it makes little sense
> to me. What about adding of compatibility lines to this patch?
Also fine.
Although if you assure me you will do it, you can add them separately.
> >> + while (1) {
> >> + regmap_read(lchip->regmap, LOCOMO_ICR, &req);
> >> + req &= 0x0f00;
> >
> > What is this magic number? Please #define it.
>
> Adding comments to this function instead.
Also acceptable.
> >> + if (!req)
> >> + break;
> >> +
> >> + irq = ffs(req) - 9;
> >
> > Minus another random number? Either define it or enter a comment.
> >
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int locomo_suspend(struct device *dev)
> >> +{
> >> + struct locomo *lchip = dev_get_drvdata(dev);
> >> +
> >> + /* AUDIO */
> >
> > WHY ARE YOU SHOUTING? Ironic eh? ;)
> >
> >> + regmap_write(lchip->regmap, LOCOMO_PAIF, 0x00);
> >> +
> >> + /*
> >> + * Original code disabled the clock depending on leds settings
> >> + * However we disable leds before suspend, thus it's safe
> >> + * to just assume this setting.
> >> + */
> >> + /* CLK32 off */
> >> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
> >> +
> >> + /* 22MHz/24MHz clock off */
> >> + regmap_write(lchip->regmap, LOCOMO_ACC, 0x00);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int locomo_resume(struct device *dev)
> >> +{
> >> + struct locomo *lchip = dev_get_drvdata(dev);
> >
> > Do audio and clk sort themselves out?
>
> PAIF and ACC registers are used only by audio parts of the device. However
> there is no current Linux driver for those parts. The registers are cleared
> in case the firmware has set something in them, but in future it will
> be the task
> of the audio driver to properly clear and restore them.
>
> >> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
> >> +
> >> + return 0;
> >> +}
> >> +
>
> [skipped]
> >> +
> >> + if (pdata) {
> >> + locomo_gpio_pdata.gpio_base = pdata->gpio_base;
> >> + locomo_lcd_pdata.comadj = pdata->comadj;
> >> + } else {
> >> + locomo_gpio_pdata.gpio_base = -1;
> >> + locomo_lcd_pdata.comadj = 128;
> >> + }
> >
> > struct locomo_gpio_platform_data locomo_gpio_pdata;
> >
> > locomo_gpio_pdata = devm_kzalloc(<blah>);
> >
> > locomo_cells[GPIO].platform_data = locomo_gpio_pdata;
>
> I do not quite agree with you at this place. The passed platform_data
> will be kmemdup()'ed inside platform core. So the whole struct will be
> duplicated twice inside kmallocate'd memory. Ideally I'd like to drop
> the whole platform_data busyness, but that requires switching to DTS
> first.
Sounds reasonable.
> >> diff --git a/include/linux/mfd/locomo.h b/include/linux/mfd/locomo.h
> >> new file mode 100644
> >> index 0000000..6729767
> >> --- /dev/null
> >> +++ b/include/linux/mfd/locomo.h
>
> >> +/* MCS decoder for boot selecting */
> >> +#define LOCOMO_MCSX0 0x10
> >> +#define LOCOMO_MCSX1 0x14
> >> +#define LOCOMO_MCSX2 0x18
> >> +#define LOCOMO_MCSX3 0x1c
> >
> > These are pretty cryptic. Any way of making them easier to identify.
>
> No way. The names are based on old Sharp code. The drivers do not use
> them, but I'd like to still keep the registers for the reference purposes.
So they are not used at all? Then why do you want to keep them?
> >> +struct locomo_gpio_platform_data {
> >> + unsigned int gpio_base;
> >> +};
> >
> > A struct for a single int seems overkill.
> >
> >> +struct locomo_lcd_platform_data {
> >> + u8 comadj;
> >> +};
> >> +
> >> +struct locomo_platform_data {
> >> + unsigned int gpio_base;
> >> + u8 comadj;
> >> +};
> >
> > Why do you need to pass gpio_base twice?
>
> First: machine file -> core driver
> Second: core driver -> gpio driver
>
> The other way to do the same would be:
>
> struct locomo_gpio_platform_data {
> unsigned int gpio_base;
> };
>
> struct locomo_lcd_platform_data {
> u8 comadj;
> };
>
> struct locomo_platform_data {
> struct locomo_gpio_platform_data gpio_pdata;
> struct locomo_lcd_platform_data lcd_pdata;
> };
>
> And to assign pointers to the passed data in the mfd_cells
> during locomo_probe. Does that look better to you?
Bingo.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core
From: Jacek Anaszewski @ 2015-05-13 10:31 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Dmitry Torokhov, Bryan Wu,
Richard Purdie, Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-input, linux-leds
In-Reply-To: <CALT56yNJWapNw1XLrzfbUDUz1LF_BB9DfF94H6GhbnBUEP80_w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
> 2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
>> On 04/28/2015 01:55 AM, Dmitry Eremin-Solenikov wrote:
>>>
>>> Adapt locomo leds driver to new locomo core setup.
>>>
>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>> ---
>>> drivers/leds/Kconfig | 1 -
>>> drivers/leds/leds-locomo.c | 119
>>> +++++++++++++++++++++++----------------------
>>> 2 files changed, 61 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 966b960..4b4650b 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -79,7 +79,6 @@ config LEDS_LM3642
>>> config LEDS_LOCOMO
>>> tristate "LED Support for Locomo device"
>>> depends on LEDS_CLASS
>>> - depends on SHARP_LOCOMO
>>
>>
>> Why do you remove this dependency?
>
> Because SHARP_LOCOMO is a Kconfig symbol for the old driver. New driver
> uses MFD_LOCOMO Kconfig entry. Also the driver now uses generic platform
> device and regmap interfaces, so there is no direct dependency on main
> LoCoMo driver. And the policy (IIRC) was not to have such dependencies.
Ack. Shouldn't you also need "select REGMAP_MMIO" ?
>>
>>> help
>>> This option enables support for the LEDs on Sharp Locomo.
>>> Zaurus models SL-5500 and SL-5600.
>>> diff --git a/drivers/leds/leds-locomo.c b/drivers/leds/leds-locomo.c
>>> index 80ba048..7fde812 100644
>>> --- a/drivers/leds/leds-locomo.c
>>> +++ b/drivers/leds/leds-locomo.c
>>> @@ -9,89 +9,92 @@
>>> */
>>>
>>> #include <linux/kernel.h>
>>> -#include <linux/init.h>
>>> -#include <linux/module.h>
>>> -#include <linux/device.h>
>>> #include <linux/leds.h>
>>> -
>>> -#include <mach/hardware.h>
>>> -#include <asm/hardware/locomo.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/locomo.h>
>>
>>
>> Please keep alphabetical order.
>
> Ack
>
>>
>>> +
>>> +struct locomo_led {
>>> + struct led_classdev led;
>>> + struct regmap *regmap;
>>> + unsigned int reg;
>>> +};
>>>
>>> static void locomoled_brightness_set(struct led_classdev *led_cdev,
>>> - enum led_brightness value, int offset)
>>> + enum led_brightness value)
>>> {
>>> - struct locomo_dev *locomo_dev = LOCOMO_DEV(led_cdev->dev->parent);
>>> - unsigned long flags;
>>> + struct locomo_led *led = container_of(led_cdev, struct locomo_led,
>>> led);
>>>
>>> - local_irq_save(flags);
>>> - if (value)
>>> - locomo_writel(LOCOMO_LPT_TOFH, locomo_dev->mapbase +
>>> offset);
>>> - else
>>> - locomo_writel(LOCOMO_LPT_TOFL, locomo_dev->mapbase +
>>> offset);
>>> - local_irq_restore(flags);
>>> + regmap_write(led->regmap, led->reg,
>>> + value ? LOCOMO_LPT_TOFH : LOCOMO_LPT_TOFL);
>>> }
>>
>>
>> Please use work queue for setting brightness. This is required for the
>> driver to be compatible with led triggers. You can refer to the
>> existing LED drivers on how to implement this.
>
> Hmm. Why? The regmap here uses MMIO access, so it is atomic operation
> and doesn't need to be wrapped into work queue, does it?
Triggers can call brightness_set op in the interrupt context, so it
cannot sleep. I see "map->lock(map->lock_arg)" in regmap_write, thus
I am inferring that it can sleep.
I am not sure if regmap implements its 'lock' op when using MMIO.
The best way to figure this out is turning "LED Timer Trigger" on
in the config and execute:
echo "timer" > trigger
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core
From: Dmitry Eremin-Solenikov @ 2015-05-13 14:14 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Dmitry Torokhov, Bryan Wu,
Richard Purdie, Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-input, linux-leds
In-Reply-To: <555327EA.5060402-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2015-05-13 13:31 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
> On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
>>
>> 2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
>>>
>>> On 04/28/2015 01:55 AM, Dmitry Eremin-Solenikov wrote:
>>>>
>>>>
>>>> Adapt locomo leds driver to new locomo core setup.
>>>>
>>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>>> ---
>>>> drivers/leds/Kconfig | 1 -
>>>> drivers/leds/leds-locomo.c | 119
>>>> +++++++++++++++++++++++----------------------
>>>> 2 files changed, 61 insertions(+), 59 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>> index 966b960..4b4650b 100644
>>>> --- a/drivers/leds/Kconfig
>>>> +++ b/drivers/leds/Kconfig
>>>> @@ -79,7 +79,6 @@ config LEDS_LM3642
>>>> config LEDS_LOCOMO
>>>> tristate "LED Support for Locomo device"
>>>> depends on LEDS_CLASS
>>>> - depends on SHARP_LOCOMO
>>>
>>>
>>>
>>> Why do you remove this dependency?
>>
>>
>> Because SHARP_LOCOMO is a Kconfig symbol for the old driver. New driver
>> uses MFD_LOCOMO Kconfig entry. Also the driver now uses generic platform
>> device and regmap interfaces, so there is no direct dependency on main
>> LoCoMo driver. And the policy (IIRC) was not to have such dependencies.
>
>
> Ack. Shouldn't you also need "select REGMAP_MMIO" ?
No. Maybe I should add "select REGMAP" instead.
>>>> static void locomoled_brightness_set(struct led_classdev *led_cdev,
>>>> - enum led_brightness value, int offset)
>>>> + enum led_brightness value)
>>>> {
>>>> - struct locomo_dev *locomo_dev >>>> LOCOMO_DEV(led_cdev->dev->parent);
>>>> - unsigned long flags;
>>>> + struct locomo_led *led = container_of(led_cdev, struct
>>>> locomo_led,
>>>> led);
>>>>
>>>> - local_irq_save(flags);
>>>> - if (value)
>>>> - locomo_writel(LOCOMO_LPT_TOFH, locomo_dev->mapbase +
>>>> offset);
>>>> - else
>>>> - locomo_writel(LOCOMO_LPT_TOFL, locomo_dev->mapbase +
>>>> offset);
>>>> - local_irq_restore(flags);
>>>> + regmap_write(led->regmap, led->reg,
>>>> + value ? LOCOMO_LPT_TOFH : LOCOMO_LPT_TOFL);
>>>> }
>>>
>>>
>>>
>>> Please use work queue for setting brightness. This is required for the
>>> driver to be compatible with led triggers. You can refer to the
>>> existing LED drivers on how to implement this.
>>
>>
>> Hmm. Why? The regmap here uses MMIO access, so it is atomic operation
>> and doesn't need to be wrapped into work queue, does it?
>
>
> Triggers can call brightness_set op in the interrupt context, so it
> cannot sleep. I see "map->lock(map->lock_arg)" in regmap_write, thus
> I am inferring that it can sleep.
>
> I am not sure if regmap implements its 'lock' op when using MMIO.
>
> The best way to figure this out is turning "LED Timer Trigger" on
> in the config and execute:
>
> echo "timer" > trigger
It works without any "might sleep/sleeping in atomic context" warnings.
Technically I'd agree with you. If I'm using regmaps, ideally I should not
depend on the exact backend and put working with it to the work queue.
However as this is a driver for quite old IP block, it is not used with
regmap backends other than MMIO, I'd deduce, it's ok to do things
in a more relaxed way and to call regmap_write even from atomic
contexts.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core
From: Jacek Anaszewski @ 2015-05-13 14:53 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Dmitry Torokhov, Bryan Wu,
Richard Purdie, Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds
In-Reply-To: <CALT56yOJyZvWF9neC2A09Y_w1Ej41uwWvU=aOOM0idbzUkBiFQ@mail.gmail.com>
On 05/13/2015 04:14 PM, Dmitry Eremin-Solenikov wrote:
> 2015-05-13 13:31 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
>> On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
>>>
>>> 2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
>>>>
>>>> On 04/28/2015 01:55 AM, Dmitry Eremin-Solenikov wrote:
>>>>>
>>>>>
>>>>> Adapt locomo leds driver to new locomo core setup.
>>>>>
>>>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>>>> ---
>>>>> drivers/leds/Kconfig | 1 -
>>>>> drivers/leds/leds-locomo.c | 119
>>>>> +++++++++++++++++++++++----------------------
>>>>> 2 files changed, 61 insertions(+), 59 deletions(-)
>>>>>
>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>>> index 966b960..4b4650b 100644
>>>>> --- a/drivers/leds/Kconfig
>>>>> +++ b/drivers/leds/Kconfig
>>>>> @@ -79,7 +79,6 @@ config LEDS_LM3642
>>>>> config LEDS_LOCOMO
>>>>> tristate "LED Support for Locomo device"
>>>>> depends on LEDS_CLASS
>>>>> - depends on SHARP_LOCOMO
>>>>
>>>>
>>>>
>>>> Why do you remove this dependency?
>>>
>>>
>>> Because SHARP_LOCOMO is a Kconfig symbol for the old driver. New driver
>>> uses MFD_LOCOMO Kconfig entry. Also the driver now uses generic platform
>>> device and regmap interfaces, so there is no direct dependency on main
>>> LoCoMo driver. And the policy (IIRC) was not to have such dependencies.
>>
>>
>> Ack. Shouldn't you also need "select REGMAP_MMIO" ?
>
> No. Maybe I should add "select REGMAP" instead.
REGMAP is enabled by default if REGMAP_MMIO is enabled. Having
REGMAP_MMIO selected would also allow to notice at first glance which
regmap backend is used. This would in turn provide a straightforward
explanation on why brightness_set op is not delegated to work queue.
>>>>> static void locomoled_brightness_set(struct led_classdev *led_cdev,
>>>>> - enum led_brightness value, int offset)
>>>>> + enum led_brightness value)
>>>>> {
>>>>> - struct locomo_dev *locomo_dev >>>>> LOCOMO_DEV(led_cdev->dev->parent);
>>>>> - unsigned long flags;
>>>>> + struct locomo_led *led = container_of(led_cdev, struct
>>>>> locomo_led,
>>>>> led);
>>>>>
>>>>> - local_irq_save(flags);
>>>>> - if (value)
>>>>> - locomo_writel(LOCOMO_LPT_TOFH, locomo_dev->mapbase +
>>>>> offset);
>>>>> - else
>>>>> - locomo_writel(LOCOMO_LPT_TOFL, locomo_dev->mapbase +
>>>>> offset);
>>>>> - local_irq_restore(flags);
>>>>> + regmap_write(led->regmap, led->reg,
>>>>> + value ? LOCOMO_LPT_TOFH : LOCOMO_LPT_TOFL);
>>>>> }
>>>>
>>>>
>>>>
>>>> Please use work queue for setting brightness. This is required for the
>>>> driver to be compatible with led triggers. You can refer to the
>>>> existing LED drivers on how to implement this.
>>>
>>>
>>> Hmm. Why? The regmap here uses MMIO access, so it is atomic operation
>>> and doesn't need to be wrapped into work queue, does it?
>>
>>
>> Triggers can call brightness_set op in the interrupt context, so it
>> cannot sleep. I see "map->lock(map->lock_arg)" in regmap_write, thus
>> I am inferring that it can sleep.
>>
>> I am not sure if regmap implements its 'lock' op when using MMIO.
>>
>> The best way to figure this out is turning "LED Timer Trigger" on
>> in the config and execute:
>>
>> echo "timer" > trigger
>
> It works without any "might sleep/sleeping in atomic context" warnings.
>
> Technically I'd agree with you. If I'm using regmaps, ideally I should not
> depend on the exact backend and put working with it to the work queue.
> However as this is a driver for quite old IP block, it is not used with
> regmap backends other than MMIO, I'd deduce, it's ok to do things
> in a more relaxed way and to call regmap_write even from atomic
> contexts.
Ack.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core
From: Dmitry Torokhov @ 2015-05-13 16:42 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Dmitry Eremin-Solenikov, Russell King, Daniel Mack,
Robert Jarzmik, Linus Walleij, Alexandre Courbot, Wolfram Sang,
Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones, Mark Brown,
Jingoo Han, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Liam Girdwood, Andrea Adami, linux-arm-kernel,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-input, linux-leds
In-Reply-To: <5553654F.4010608-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
On Wed, May 13, 2015 at 04:53:03PM +0200, Jacek Anaszewski wrote:
> On 05/13/2015 04:14 PM, Dmitry Eremin-Solenikov wrote:
> >2015-05-13 13:31 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
> >>On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
> >>>
> >>>2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
> >>>>
> >>>>On 04/28/2015 01:55 AM, Dmitry Eremin-Solenikov wrote:
> >>>>>
> >>>>>
> >>>>>Adapt locomo leds driver to new locomo core setup.
> >>>>>
> >>>>>Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >>>>>---
> >>>>> drivers/leds/Kconfig | 1 -
> >>>>> drivers/leds/leds-locomo.c | 119
> >>>>>+++++++++++++++++++++++----------------------
> >>>>> 2 files changed, 61 insertions(+), 59 deletions(-)
> >>>>>
> >>>>>diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >>>>>index 966b960..4b4650b 100644
> >>>>>--- a/drivers/leds/Kconfig
> >>>>>+++ b/drivers/leds/Kconfig
> >>>>>@@ -79,7 +79,6 @@ config LEDS_LM3642
> >>>>> config LEDS_LOCOMO
> >>>>> tristate "LED Support for Locomo device"
> >>>>> depends on LEDS_CLASS
> >>>>>- depends on SHARP_LOCOMO
> >>>>
> >>>>
> >>>>
> >>>>Why do you remove this dependency?
> >>>
> >>>
> >>>Because SHARP_LOCOMO is a Kconfig symbol for the old driver. New driver
> >>>uses MFD_LOCOMO Kconfig entry. Also the driver now uses generic platform
> >>>device and regmap interfaces, so there is no direct dependency on main
> >>>LoCoMo driver. And the policy (IIRC) was not to have such dependencies.
> >>
> >>
> >>Ack. Shouldn't you also need "select REGMAP_MMIO" ?
> >
> >No. Maybe I should add "select REGMAP" instead.
>
> REGMAP is enabled by default if REGMAP_MMIO is enabled. Having
That is unfortunately not how select works: it does not automatically
select parents for the selected symbol.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank
From: Kim, Milo @ 2015-05-13 23:31 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1431376325-25741-1-git-send-email-seanpaul@chromium.org>
Hi Sean,
On 5/14/2015 4:33 AM, Sean Paul wrote:
>
>
> On Mon, May 11, 2015 at 3:48 PM, Kim, Milo <milo.kim@ti.com
> <mailto:milo.kim@ti.com>> wrote:
>
> Hi Sean,
>
> On 5/12/2015 5:32 AM, Sean Paul wrote:
>
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
>
>
> Could you describe what the problem is? I wrote same code in other
> backlight drivers. So maybe I need to look into them as well.
>
>
> Hi Milo,
> Well, I'm not sure what the expected behavior is, but our userspace
> expects the driver to maintain its brightness across suspend/resume. The
> way things are coded now, the driver will clear the brightness value on
> suspend such that when we resume, the backlight is restored to 0.
Got it. Your patch makes sense. Thanks for catching this.
Best regards,
Milo
^ permalink raw reply
* Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank
From: Kim, Milo @ 2015-05-13 23:32 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1431376325-25741-1-git-send-email-seanpaul@chromium.org>
On 5/12/2015 5:32 AM, Sean Paul wrote:
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
>
> Reported-by: Benson Leung <bleung@chromium.org>
> Tested-by: Stephen Barber <smbarber@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Acked-by: Milo Kim <milo.kim@ti.com>
^ permalink raw reply
* Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank
From: Benson Leung @ 2015-05-13 23:40 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1431376325-25741-1-git-send-email-seanpaul@chromium.org>
On Mon, May 11, 2015 at 1:32 PM, Sean Paul <seanpaul@chromium.org> wrote:
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
>
> Reported-by: Benson Leung <bleung@chromium.org>
> Tested-by: Stephen Barber <smbarber@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
--
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
^ permalink raw reply
* Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core
From: Jacek Anaszewski @ 2015-05-14 6:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Dmitry Eremin-Solenikov, Russell King, Daniel Mack,
Robert Jarzmik, Linus Walleij, Alexandre Courbot, Wolfram Sang,
Bryan Wu, Richard Purdie, Samuel Ortiz, Lee Jones, Mark Brown,
Jingoo Han, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Liam Girdwood, Andrea Adami, linux-arm-kernel,
linux-gpio@vger.kernel.org, linux-i2c, linux-input, linux-leds
In-Reply-To: <20150513164248.GB11891@dtor-ws>
On 05/13/2015 06:42 PM, Dmitry Torokhov wrote:
> On Wed, May 13, 2015 at 04:53:03PM +0200, Jacek Anaszewski wrote:
>> On 05/13/2015 04:14 PM, Dmitry Eremin-Solenikov wrote:
>>> 2015-05-13 13:31 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
>>>> On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
>>>>>
>>>>> 2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
>>>>>>
>>>>>> On 04/28/2015 01:55 AM, Dmitry Eremin-Solenikov wrote:
>>>>>>>
>>>>>>>
>>>>>>> Adapt locomo leds driver to new locomo core setup.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>>>>>> ---
>>>>>>> drivers/leds/Kconfig | 1 -
>>>>>>> drivers/leds/leds-locomo.c | 119
>>>>>>> +++++++++++++++++++++++----------------------
>>>>>>> 2 files changed, 61 insertions(+), 59 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>>>>> index 966b960..4b4650b 100644
>>>>>>> --- a/drivers/leds/Kconfig
>>>>>>> +++ b/drivers/leds/Kconfig
>>>>>>> @@ -79,7 +79,6 @@ config LEDS_LM3642
>>>>>>> config LEDS_LOCOMO
>>>>>>> tristate "LED Support for Locomo device"
>>>>>>> depends on LEDS_CLASS
>>>>>>> - depends on SHARP_LOCOMO
>>>>>>
>>>>>>
>>>>>>
>>>>>> Why do you remove this dependency?
>>>>>
>>>>>
>>>>> Because SHARP_LOCOMO is a Kconfig symbol for the old driver. New driver
>>>>> uses MFD_LOCOMO Kconfig entry. Also the driver now uses generic platform
>>>>> device and regmap interfaces, so there is no direct dependency on main
>>>>> LoCoMo driver. And the policy (IIRC) was not to have such dependencies.
>>>>
>>>>
>>>> Ack. Shouldn't you also need "select REGMAP_MMIO" ?
>>>
>>> No. Maybe I should add "select REGMAP" instead.
>>
>> REGMAP is enabled by default if REGMAP_MMIO is enabled. Having
>
> That is unfortunately not how select works: it does not automatically
> select parents for the selected symbol.
Please look at config REGMAP declaration in the file
drivers/base/regmap/Kconfig:
config REGMAP
default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI ||
REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ)
select LZO_COMPRESS
select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
bool
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank
From: Lee Jones @ 2015-05-14 7:25 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1431376325-25741-1-git-send-email-seanpaul@chromium.org>
On Mon, 11 May 2015, Sean Paul wrote:
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
>
> Reported-by: Benson Leung <bleung@chromium.org>
> Tested-by: Stephen Barber <smbarber@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> drivers/video/backlight/lp855x_bl.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
Applied with Acks, thanks.
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 08ae72f..74daf7c 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -257,21 +257,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> static int lp855x_bl_update_status(struct backlight_device *bl)
> {
> struct lp855x *lp = bl_get_data(bl);
> + int brightness = bl->props.brightness;
>
> if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> - bl->props.brightness = 0;
> + brightness = 0;
>
> - if (lp->mode = PWM_BASED) {
> - int br = bl->props.brightness;
> - int max_br = bl->props.max_brightness;
> -
> - lp855x_pwm_ctrl(lp, br, max_br);
> -
> - } else if (lp->mode = REGISTER_BASED) {
> - u8 val = bl->props.brightness;
> -
> - lp855x_write_byte(lp, lp->cfg->reg_brightness, val);
> - }
> + if (lp->mode = PWM_BASED)
> + lp855x_pwm_ctrl(lp, brightness, bl->props.max_brightness);
> + else if (lp->mode = REGISTER_BASED)
> + lp855x_write_byte(lp, lp->cfg->reg_brightness, (u8)brightness);
>
> return 0;
> }
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4] staging: sm750fb: use arch_phys_wc_add() and ioremap_wc()
From: Luis R. Rodriguez @ 2015-05-14 14:06 UTC (permalink / raw)
To: cocci
In-Reply-To: <20150510130903.GA30824@kroah.com>
On Sun, May 10, 2015 at 03:09:03PM +0200, Greg KH wrote:
> On Mon, May 04, 2015 at 05:15:51PM -0700, Luis R. Rodriguez wrote:
> > ---
> > drivers/staging/sm750fb/sm750.c | 36 ++++--------------------------------
> > drivers/staging/sm750fb/sm750.h | 3 ---
> > drivers/staging/sm750fb/sm750_hw.c | 3 +--
> > 3 files changed, 5 insertions(+), 37 deletions(-)
>
> Still doesn't apply to my tree:
I did base it on that day's staging tree, staging is just a fast
moving tree. In the future use of SmPL, when provided, to address merge
conflicts might help us on git.
Meanwhile I've addressed the conflict and will send v5 based on today's
tree outlook.
Luis
^ permalink raw reply
* [PATCH v5] staging: sm750fb: use arch_phys_wc_add() and ioremap_wc()
From: Luis R. Rodriguez @ 2015-05-14 14:07 UTC (permalink / raw)
To: cocci
From: "Luis R. Rodriguez" <mcgrof@suse.com>
The same area used for ioremap() is used for the MTRR area.
Convert the driver from using the x86 specific MTRR code to
the architecture agnostic arch_phys_wc_add(). arch_phys_wc_add()
will avoid MTRR if write-combining is available, in order to
take advantage of that also ensure the ioremap'd area is requested
as write-combining.
There are a few motivations for this:
a) Take advantage of PAT when available
b) Help bury MTRR code away, MTRR is architecture specific and on
x86 its replaced by PAT
c) Help with the goal of eventually using _PAGE_CACHE_UC over
_PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
de33c442e titled "x86 PAT: fix performance drop for glx,
use UC minus for ioremap(), ioremap_nocache() and
pci_mmap_page_range()")
The conversion done is expressed by the following Coccinelle
SmPL patch, it additionally required manual intervention to
address all the #ifdery and removal of redundant things which
arch_phys_wc_add() already addresses such as verbose message
about when MTRR fails and doing nothing when we didn't get
an MTRR.
@ mtrr_found @
expression index, base, size;
@@
-index = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
+index = arch_phys_wc_add(base, size);
@ mtrr_rm depends on mtrr_found @
expression mtrr_found.index, mtrr_found.base, mtrr_found.size;
@@
-mtrr_del(index, base, size);
+arch_phys_wc_del(index);
@ mtrr_rm_zero_arg depends on mtrr_found @
expression mtrr_found.index;
@@
-mtrr_del(index, 0, 0);
+arch_phys_wc_del(index);
@ mtrr_rm_fb_info depends on mtrr_found @
struct fb_info *info;
expression mtrr_found.index;
@@
-mtrr_del(index, info->fix.smem_start, info->fix.smem_len);
+arch_phys_wc_del(index);
@ ioremap_replace_nocache depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@
-info->screen_base = ioremap_nocache(base, size);
+info->screen_base = ioremap_wc(base, size);
@ ioremap_replace_default depends on mtrr_found @
struct fb_info *info;
expression base, size;
@@
-info->screen_base = ioremap(base, size);
+info->screen_base = ioremap_wc(base, size);
Generated-by: Coccinelle SmPL
Cc: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Cc: Teddy Wang <teddy.wang@siliconmotion.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: devel@driverdev.osuosl.org
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
drivers/staging/sm750fb/sm750.c | 36 ++++--------------------------------
drivers/staging/sm750fb/sm750.h | 3 ---
drivers/staging/sm750fb/sm750_hw.c | 3 +--
3 files changed, 5 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 5529905..97fe04a 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -16,9 +16,6 @@
#include<linux/vmalloc.h>
#include<linux/pagemap.h>
#include <linux/console.h>
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
-#endif
#include <asm/fb.h>
#include "sm750.h"
#include "sm750_hw.h"
@@ -47,9 +44,7 @@ typedef int (*PROC_SPEC_INITHW)(struct lynx_share*, struct pci_dev*);
/* common var for all device */
static int g_hwcursor = 1;
static int g_noaccel;
-#ifdef CONFIG_MTRR
static int g_nomtrr;
-#endif
static const char *g_fbmode[] = {NULL, NULL};
static const char *g_def_fbmode = "800x600-16@60";
static char *g_settings = NULL;
@@ -1126,11 +1121,8 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
pr_info("share->revid = %02x\n", share->revid);
share->pdev = pdev;
-#ifdef CONFIG_MTRR
share->mtrr_off = g_nomtrr;
share->mtrr.vram = 0;
- share->mtrr.vram_added = 0;
-#endif
share->accel_off = g_noaccel;
share->dual = g_dualview;
spin_lock_init(&share->slock);
@@ -1158,22 +1150,9 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
goto err_map;
}
-#ifdef CONFIG_MTRR
- if (!share->mtrr_off) {
- pr_info("enable mtrr\n");
- share->mtrr.vram = mtrr_add(share->vidmem_start,
- share->vidmem_size,
- MTRR_TYPE_WRCOMB, 1);
-
- if (share->mtrr.vram < 0) {
- /* don't block driver with the failure of MTRR */
- pr_err("Unable to setup MTRR.\n");
- } else {
- share->mtrr.vram_added = 1;
- pr_info("MTRR added successfully\n");
- }
- }
-#endif
+ if (!share->mtrr_off)
+ share->mtrr.vram = arch_phys_wc_add(share->vidmem_start,
+ share->vidmem_size);
memset_io(share->pvMem, 0, share->vidmem_size);
@@ -1274,12 +1253,7 @@ static void lynxfb_pci_remove(struct pci_dev *pdev)
/* release frame buffer */
framebuffer_release(info);
}
-#ifdef CONFIG_MTRR
- if (share->mtrr.vram_added)
- mtrr_del(share->mtrr.vram,
- share->vidmem_start,
- share->vidmem_size);
-#endif
+ arch_phys_wc_del(share->mtrr.vram);
iounmap(share->pvReg);
iounmap(share->pvMem);
@@ -1321,10 +1295,8 @@ static int __init lynxfb_setup(char *options)
/* options that mean for any lynx chips are configured here */
if (!strncmp(opt, "noaccel", strlen("noaccel")))
g_noaccel = 1;
-#ifdef CONFIG_MTRR
else if (!strncmp(opt, "nomtrr", strlen("nomtrr")))
g_nomtrr = 1;
-#endif
else if (!strncmp(opt, "dual", strlen("dual")))
g_dualview = 1;
else {
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index 0847d2b..5528912 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -51,13 +51,10 @@ struct lynx_share{
struct lynx_accel accel;
int accel_off;
int dual;
-#ifdef CONFIG_MTRR
int mtrr_off;
struct{
int vram;
- int vram_added;
}mtrr;
-#endif
/* all smi graphic adaptor got below attributes */
unsigned long vidmem_start;
unsigned long vidreg_start;
diff --git a/drivers/staging/sm750fb/sm750_hw.c b/drivers/staging/sm750fb/sm750_hw.c
index 9f0d06d..4b77eb1 100644
--- a/drivers/staging/sm750fb/sm750_hw.c
+++ b/drivers/staging/sm750fb/sm750_hw.c
@@ -85,8 +85,7 @@ int hw_sm750_map(struct lynx_share* share, struct pci_dev* pdev)
}
#endif
- share->pvMem = ioremap(share->vidmem_start,
- share->vidmem_size);
+ share->pvMem = ioremap_wc(share->vidmem_start, share->vidmem_size);
if(!share->pvMem){
pr_err("Map video memory failed\n");
--
2.3.2.209.gd67f9d5.dirty
^ permalink raw reply related
* Re: [PATCH v3 00/17] framebuffer: simple conversions to arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-05-14 14:26 UTC (permalink / raw)
To: cocci
In-Reply-To: <CALCETrVdAG1NFA_dYHhOyCbwKR3msHZaaVs0MZMkW33_Sw=Kqg@mail.gmail.com>
On Tue, May 05, 2015 at 06:47:31AM -0700, Andy Lutomirski wrote:
> On Mon, May 4, 2015 at 3:33 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On 29/04/15 22:18, Luis R. Rodriguez wrote:
> >> On Tue, Apr 21, 2015 at 1:16 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >>> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >>>
> >>> This series addresses simple changes to framebuffer drivers to use
> >>> arch_phys_wc_add() and ioremap_wc() as well as fixing gbefb to add
> >>> missing mtrr_del() calls. These changes are pretty straight forward.
> >>>
> >>> Luis R. Rodriguez (17):
> >>> video: fbdev: radeonfb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: gbefb: add missing mtrr_del() calls
> >>> video: fbdev: gbefb: use arch_phys_wc_add() and devm_ioremap_wc()
> >>> video: fbdev: intelfb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: matrox: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: neofb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: nvidia: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: savagefb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: sisfb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: aty: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: i810: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: pm2fb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: pm3fb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: rivafb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: tdfxfb: use arch_phys_wc_add() and ioremap_wc()
> >>> video: fbdev: atmel_lcdfb: use ioremap_wc() for framebuffer
> >>> video: fbdev: geode gxfb: use ioremap_wc() for framebuffer
> >>
> >> Hey folks, these are all pretty straight forward, can anyone take them?
> >
> > I can take these to fbdev tree. Unfortunately I'm not familiar with x86
> > nor mtrr, so I can't really say much about the patches themselves.
> >
>
> I'm on vacation and there's no way I'll be able to usefully review
> these in the next two weeks. I can describe what's going on in case
> it helps:
>
> On x86 there are two ways to get a write-combining MMIO mapping. The
> sane way is ioremap_wc, and the ridiculous way is mtrr_add.
> ioremap_wc does exactly what it appears to do, whereas mtrr_add acts
> on physical addresses, requires power-of-two alignment, and is
> unreliable.
>
> On all recent hardware, ioremap_wc works, and mtrr_add is problematic
> on all hardware even if it often works. The silly thing is that
> mtrr_add pokes at the registers it uses even on hardware with working
> ioremap_wc. This causes problems (those resources are a very limited
> resource).
>
> The solution I came up with a couple years ago is a new API
> arch_phys_wc_add. It is a hint that a physical address range should
> be write-combining. On hardware with working ioremap_wc, it's a
> no-op. On x86 hardware without working ioremap_wc, it just calls
> mtrr_add.
>
> The upshot is that changing mtrr_add with a WC type to
> arch_phys_wc_add is OK as long as the driver also uses ioremap_wc or
> similar _wc APIs. Once everything has been converted, we can unexport
> mtrr_add, which will have all kinds of benefits.
Tomi,
Any chance you can consider this series again? I realize you say you are
not familiar with MTRR but given its prevalence use on framebuffer device
drivers and since you are a framebuffer subsystem maintainer I think its
perhaps fair to ask one of you become familiar with this stuff, or ask
questions if there are any. If its of any help this patch which adds
ioremap_uc() already got merged and the logic to phase MTRR is described
there too:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?idäb6be33c28923d8cde53023e0888b1c5d1a9027
That patch won't even be needed on this series, this series happens to be
straight forward: if you just ioremap_nocache() once and use mtrr_add()
then conver over to ioremap_wc() and use arch_phys_wc_add(). If it already
used ioremap_wc() just convert over from mtrr_add() to arch_phys_wc_add()
Not sure if you will get an Acked-by by x86 folks on this series as they
are busy, but I'm adding bp and hpa just in case.
Luis
^ permalink raw reply
* Re: [PATCHv3 1/4] phy: phy-core: Make GENERIC_PHY an invisible option
From: Mauro Carvalho Chehab @ 2015-05-14 21:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1429743853-10254-2-git-send-email-arun.ramamurthy@broadcom.com>
Em Wed, 22 Apr 2015 16:04:10 -0700
Arun Ramamurthy <arun.ramamurthy@broadcom.com> escreveu:
> Most of the phy providers use "select" to enable GENERIC_PHY. Since select
> is only recommended when the config is not visible, GENERIC_PHY is changed
> an invisible option. To maintain consistency, all phy providers are changed
> to "select" GENERIC_PHY and all non-phy drivers use "depends on" when the
> phy framework is explicity required. USB_MUSB_OMAP2PLUS has a cyclic
> dependency, so it is left as "select".
>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> ---
> drivers/ata/Kconfig | 1 -
> drivers/media/platform/exynos4-is/Kconfig | 2 +-
For media part:
Acked-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> drivers/phy/Kconfig | 4 ++--
> drivers/usb/host/Kconfig | 4 ++--
> drivers/video/fbdev/exynos/Kconfig | 2 +-
> 5 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 5f60155..6d2e881 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -301,7 +301,6 @@ config SATA_MV
> tristate "Marvell SATA support"
> depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
> ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
> - select GENERIC_PHY
> help
> This option enables support for the Marvell Serial ATA family.
> Currently supports 88SX[56]0[48][01] PCI(-X) chips,
> diff --git a/drivers/media/platform/exynos4-is/Kconfig b/drivers/media/platform/exynos4-is/Kconfig
> index b7b2e47..b6f3eaa 100644
> --- a/drivers/media/platform/exynos4-is/Kconfig
> +++ b/drivers/media/platform/exynos4-is/Kconfig
> @@ -31,7 +31,7 @@ config VIDEO_S5P_FIMC
> config VIDEO_S5P_MIPI_CSIS
> tristate "S5P/EXYNOS MIPI-CSI2 receiver (MIPI-CSIS) driver"
> depends on REGULATOR
> - select GENERIC_PHY
> + depends on GENERIC_PHY
> help
> This is a V4L2 driver for Samsung S5P and EXYNOS4 SoC MIPI-CSI2
> receiver (MIPI-CSIS) devices.
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 2962de2..edecdb1 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -5,7 +5,7 @@
> menu "PHY Subsystem"
>
> config GENERIC_PHY
> - bool "PHY Core"
> + bool
> help
> Generic PHY support.
>
> @@ -72,7 +72,7 @@ config PHY_MIPHY365X
> config PHY_RCAR_GEN2
> tristate "Renesas R-Car generation 2 USB PHY driver"
> depends on ARCH_SHMOBILE
> - depends on GENERIC_PHY
> + select GENERIC_PHY
> help
> Support for USB PHY found on Renesas R-Car generation 2 SoCs.
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 5ad60e4..e2197e2 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -182,7 +182,7 @@ config USB_EHCI_HCD_SPEAR
> config USB_EHCI_HCD_STI
> tristate "Support for ST STiHxxx on-chip EHCI USB controller"
> depends on ARCH_STI && OF
> - select GENERIC_PHY
> + depends on GENERIC_PHY
> select USB_EHCI_HCD_PLATFORM
> help
> Enable support for the on-chip EHCI controller found on
> @@ -409,7 +409,7 @@ config USB_OHCI_HCD_SPEAR
> config USB_OHCI_HCD_STI
> tristate "Support for ST STiHxxx on-chip OHCI USB controller"
> depends on ARCH_STI && OF
> - select GENERIC_PHY
> + depends on GENERIC_PHY
> select USB_OHCI_HCD_PLATFORM
> help
> Enable support for the on-chip OHCI controller found on
> diff --git a/drivers/video/fbdev/exynos/Kconfig b/drivers/video/fbdev/exynos/Kconfig
> index 1f16b46..6c53894 100644
> --- a/drivers/video/fbdev/exynos/Kconfig
> +++ b/drivers/video/fbdev/exynos/Kconfig
> @@ -16,7 +16,7 @@ if EXYNOS_VIDEO
>
> config EXYNOS_MIPI_DSI
> bool "EXYNOS MIPI DSI driver support."
> - select GENERIC_PHY
> + depends on GENERIC_PHY
> help
> This enables support for MIPI-DSI device.
>
^ permalink raw reply
* Re: [PATCHv3 1/4] phy: phy-core: Make GENERIC_PHY an invisible option
From: Felipe Balbi @ 2015-05-15 0:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1429743853-10254-2-git-send-email-arun.ramamurthy@broadcom.com>
[-- Attachment #1: Type: text/plain, Size: 3072 bytes --]
Hi,
On Wed, Apr 22, 2015 at 04:04:10PM -0700, Arun Ramamurthy wrote:
> Most of the phy providers use "select" to enable GENERIC_PHY. Since select
> is only recommended when the config is not visible, GENERIC_PHY is changed
> an invisible option. To maintain consistency, all phy providers are changed
> to "select" GENERIC_PHY and all non-phy drivers use "depends on" when the
> phy framework is explicity required. USB_MUSB_OMAP2PLUS has a cyclic
> dependency, so it is left as "select".
>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> ---
> drivers/ata/Kconfig | 1 -
> drivers/media/platform/exynos4-is/Kconfig | 2 +-
> drivers/phy/Kconfig | 4 ++--
> drivers/usb/host/Kconfig | 4 ++--
> drivers/video/fbdev/exynos/Kconfig | 2 +-
> 5 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 5f60155..6d2e881 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -301,7 +301,6 @@ config SATA_MV
> tristate "Marvell SATA support"
> depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
> ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
> - select GENERIC_PHY
> help
> This option enables support for the Marvell Serial ATA family.
> Currently supports 88SX[56]0[48][01] PCI(-X) chips,
> diff --git a/drivers/media/platform/exynos4-is/Kconfig b/drivers/media/platform/exynos4-is/Kconfig
> index b7b2e47..b6f3eaa 100644
> --- a/drivers/media/platform/exynos4-is/Kconfig
> +++ b/drivers/media/platform/exynos4-is/Kconfig
> @@ -31,7 +31,7 @@ config VIDEO_S5P_FIMC
> config VIDEO_S5P_MIPI_CSIS
> tristate "S5P/EXYNOS MIPI-CSI2 receiver (MIPI-CSIS) driver"
> depends on REGULATOR
> - select GENERIC_PHY
> + depends on GENERIC_PHY
> help
> This is a V4L2 driver for Samsung S5P and EXYNOS4 SoC MIPI-CSI2
> receiver (MIPI-CSIS) devices.
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 2962de2..edecdb1 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -5,7 +5,7 @@
> menu "PHY Subsystem"
>
> config GENERIC_PHY
> - bool "PHY Core"
> + bool
> help
> Generic PHY support.
>
> @@ -72,7 +72,7 @@ config PHY_MIPHY365X
> config PHY_RCAR_GEN2
> tristate "Renesas R-Car generation 2 USB PHY driver"
> depends on ARCH_SHMOBILE
> - depends on GENERIC_PHY
> + select GENERIC_PHY
so some you changed from depends to select...
> help
> Support for USB PHY found on Renesas R-Car generation 2 SoCs.
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 5ad60e4..e2197e2 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -182,7 +182,7 @@ config USB_EHCI_HCD_SPEAR
> config USB_EHCI_HCD_STI
> tristate "Support for ST STiHxxx on-chip EHCI USB controller"
> depends on ARCH_STI && OF
> - select GENERIC_PHY
> + depends on GENERIC_PHY
while others you changed from select to depends.
NAK.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] backlight: pwm_bl: Simplify usage of devm_gpiod_get_optional
From: Axel Lin @ 2015-05-16 14:08 UTC (permalink / raw)
To: linux-pwm, linux-fbdev
Cc: Thierry Reding, Alexandre Courbot, Jingoo Han, Lee Jones
Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions),
the gpiod_get* functions take an additional parameter that allows to
specify direction and initial value for output.
Simplify the usage of devm_gpiod_get_optional accordingly.
Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
drivers/video/backlight/pwm_bl.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 3a145a6..57cb9ec 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -241,7 +241,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->dev = &pdev->dev;
pb->enabled = false;
- pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable");
+ pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+ GPIOD_OUT_HIGH);
if (IS_ERR(pb->enable_gpio)) {
ret = PTR_ERR(pb->enable_gpio);
goto err_alloc;
@@ -263,9 +264,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enable_gpio = gpio_to_desc(data->enable_gpio);
}
- if (pb->enable_gpio)
- gpiod_direction_output(pb->enable_gpio, 1);
-
pb->power_supply = devm_regulator_get(&pdev->dev, "power");
if (IS_ERR(pb->power_supply)) {
ret = PTR_ERR(pb->power_supply);
--
2.1.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox