* [PATCH 1/4] video: Add support for the Solomon SSD1307 OLED Controller
From: Maxime Ripard @ 2012-07-31 10:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1343730576-20494-1-git-send-email-maxime.ripard@free-electrons.com>
This patch adds support for the Solomon SSD1307 OLED
controller found on the Crystalfontz CFA10036 board.
This controller can drive a display with a resolution up
to 128x39 and can operate over I2C or SPI.
The current driver has only been tested on the CFA-10036,
that is using this controller over I2C to driver a 96x16
OLED screen.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Brian Lilly <brian@crystalfontz.com>
---
.../devicetree/bindings/video/ssd1307fb.txt | 24 ++
drivers/video/Kconfig | 14 +
drivers/video/Makefile | 1 +
drivers/video/ssd1307fb.c | 418 ++++++++++++++++++++
4 files changed, 457 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/ssd1307fb.txt
create mode 100644 drivers/video/ssd1307fb.c
diff --git a/Documentation/devicetree/bindings/video/ssd1307fb.txt b/Documentation/devicetree/bindings/video/ssd1307fb.txt
new file mode 100644
index 0000000..791e14f
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/ssd1307fb.txt
@@ -0,0 +1,24 @@
+* Solomon SSD1307 Framebuffer Driver
+
+Required properties:
+ - compatible: Should be "solomon,ssd1307fb-<bus>". The only supported bus for
+ now is i2c.
+ - reg: Should contain address of the controller on the I2C bus. Most likely
+ 0x3c or 0x3d
+ - pwm: Should contain the pwm to use according to the OF device tree PWM
+ specification [0]
+ - oled-reset-gpio: Should contain the GPIO used to reset the OLED display
+
+Optional properties:
+ - oled-reset-active-low: Is the reset gpio is active on physical low?
+
+[0]: Documentation/devicetree/bindings/pwm/pwm.txt
+
+Examples:
+ssd1307: oled@3c {
+ compatible = "solomon,ssd1307fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ oled-reset-gpio = <&gpio2 7 1>;
+ oled-reset-active-low;
+};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0217f74..21ae6dd 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2469,4 +2469,18 @@ config FB_SH_MOBILE_MERAM
Up to 4 memory channels can be configured, allowing 4 RGB or
2 YCbCr framebuffers to be configured.
+
+config FB_SSD1307
+ tristate "Solomon SSD1307 framebuffer support"
+ depends on FB && I2C
+ select FB_SYS_FOPS
+ select FB_SYS_FILLRECT
+ select FB_SYS_COPYAREA
+ select FB_SYS_IMAGEBLIT
+ select FB_DEFERRED_IO
+ select PWM
+ help
+ This driver implements support for the Solomon SSD1307
+ OLED controller over I2C.
+
endmenu
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index ee8dafb..6bbb72c 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -164,6 +164,7 @@ obj-$(CONFIG_FB_BFIN_7393) += bfin_adv7393fb.o
obj-$(CONFIG_FB_MX3) += mx3fb.o
obj-$(CONFIG_FB_DA8XX) += da8xx-fb.o
obj-$(CONFIG_FB_MXS) += mxsfb.o
+obj-$(CONFIG_FB_SSD1307) += ssd1307fb.o
# the test framebuffer is last
obj-$(CONFIG_FB_VIRTUAL) += vfb.o
diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
new file mode 100644
index 0000000..c705ab4
--- /dev/null
+++ b/drivers/video/ssd1307fb.c
@@ -0,0 +1,418 @@
+/*
+ * Driver for the Solomon SSD1307 OLED controler
+ *
+ * Copyright 2012 Free Electrons
+ *
+ * Licensed under the GPLv2 or later.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/i2c.h>
+#include <linux/fb.h>
+#include <linux/uaccess.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/pwm.h>
+#include <linux/delay.h>
+
+#define SSD1307FB_WIDTH 96
+#define SSD1307FB_HEIGHT 16
+
+#define SSD1307FB_DATA 0x40
+#define SSD1307FB_COMMAND 0x80
+
+#define SSD1307FB_CONTRAST 0x81
+#define SSD1307FB_SEG_REMAP_ON 0xa1
+#define SSD1307FB_DISPLAY_OFF 0xae
+#define SSD1307FB_DISPLAY_ON 0xaf
+#define SSD1307FB_START_PAGE_ADDRESS 0xb0
+
+struct ssd1307fb_par {
+ struct i2c_client *client;
+ struct fb_info *info;
+ struct pwm_device *pwm;
+ u32 pwm_period;
+ int reset;
+};
+
+static struct fb_fix_screeninfo ssd1307fb_fix __devinitdata = {
+ .id = "Solomon SSD1307",
+ .type = FB_TYPE_PACKED_PIXELS,
+ .visual = FB_VISUAL_MONO10,
+ .xpanstep = 0,
+ .ypanstep = 0,
+ .ywrapstep = 0,
+ .line_length = SSD1307FB_WIDTH / 8,
+ .accel = FB_ACCEL_NONE,
+};
+
+static struct fb_var_screeninfo ssd1307fb_var __devinitdata = {
+ .xres = SSD1307FB_WIDTH,
+ .yres = SSD1307FB_HEIGHT,
+ .xres_virtual = SSD1307FB_WIDTH,
+ .yres_virtual = SSD1307FB_HEIGHT,
+ .bits_per_pixel = 1,
+};
+
+static int ssd1307fb_write_array(struct i2c_client *client, u8 type, u8* cmd, u32 len)
+{
+ u8 *buf;
+ int ret = 0;
+
+ buf = kzalloc(len + 1, GFP_KERNEL);
+ if (!buf) {
+ dev_err(&client->dev, "Couldn't allocate sending buffer.\n");
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ buf[0] = type;
+ memcpy(buf + 1, cmd, len);
+
+ ret = i2c_master_send(client, buf, len + 1);
+ if (ret != len + 1) {
+ dev_err(&client->dev, "Couldn't send I2C command.\n");
+ goto error;
+ }
+
+error:
+ kfree(buf);
+out:
+ return ret;
+}
+
+static inline int ssd1307fb_write_cmd_array(struct i2c_client *client, u8* cmd, u32 len)
+{
+ return ssd1307fb_write_array(client, SSD1307FB_COMMAND, cmd, len);
+}
+
+static inline int ssd1307fb_write_cmd(struct i2c_client *client, u8 cmd)
+{
+ return ssd1307fb_write_cmd_array(client, &cmd, 1);
+}
+
+static inline int ssd1307fb_write_data_array(struct i2c_client *client, u8* cmd, u32 len)
+{
+ return ssd1307fb_write_array(client, SSD1307FB_DATA, cmd, len);
+}
+
+static inline int ssd1307fb_write_data(struct i2c_client *client, u8 data)
+{
+ return ssd1307fb_write_data_array(client, &data, 1);
+}
+
+static int ssd1307fb_set(struct i2c_client *client, u8 value)
+{
+ int i, j, ret;
+
+ for (i = 1; i <= (SSD1307FB_HEIGHT / 8); i++) {
+ ret = ssd1307fb_write_cmd(client, SSD1307FB_START_PAGE_ADDRESS + i);
+ if (ret < 0)
+ goto i2c_error;
+
+ ret = ssd1307fb_write_cmd(client, 0x00);
+ if (ret < 0)
+ goto i2c_error;
+
+ ret = ssd1307fb_write_cmd(client, 0x10);
+ if (ret < 0)
+ goto i2c_error;
+
+ for (j = 0; j < SSD1307FB_WIDTH; j++)
+ ssd1307fb_write_data(client, value);
+ }
+
+ return 0;
+
+i2c_error:
+ dev_err(&client->dev, "Couldn't send i2c command: %d\n", ret);
+ return ret;
+}
+
+static void ssd1307fb_update_display(struct ssd1307fb_par *par)
+{
+ u8 *vmem = par->info->screen_base;
+ int i, j, k;
+
+ /*
+ * The screen is divided in pages, each having a height of 8
+ * pixels, and the width of the screen. When sending a byte of
+ * data to the controller, it gives the 8 bits for the current
+ * column. I.e, the first byte are the 8 bits of the first
+ * column, then the 8 bits for the second column, etc.
+ *
+ *
+ * Representation of the screen, assuming it is 5 bits
+ * wide. Each letter-number combination is a bit that controls
+ * one pixel.
+ *
+ * A0 A1 A2 A3 A4
+ * B0 B1 B2 B3 B4
+ * C0 C1 C2 C3 C4
+ * D0 D1 D2 D3 D4
+ * E0 E1 E2 E3 E4
+ * F0 F1 F2 F3 F4
+ * G0 G1 G2 G3 G4
+ * H0 H1 H2 H3 H4
+ *
+ * If you want to update this screen, you need to send 5 bytes:
+ * (1) A0 B0 C0 D0 E0 F0 G0 H0
+ * (2) A1 B1 C1 D1 E1 F1 G1 H1
+ * (3) A2 B2 C2 D2 E2 F2 G2 H2
+ * (4) A3 B3 C3 D3 E3 F3 G3 H3
+ * (5) A4 B4 C4 D4 E4 F4 G4 H4
+ */
+
+ for (i = 0; i < (SSD1307FB_HEIGHT / 8); i++) {
+ ssd1307fb_write_cmd(par->client, SSD1307FB_START_PAGE_ADDRESS + (i + 1));
+ ssd1307fb_write_cmd(par->client, 0x00);
+ ssd1307fb_write_cmd(par->client, 0x10);
+
+ for (j = 0; j < SSD1307FB_WIDTH; j++) {
+ u8 buf = 0;
+ for (k = 0; k < 8; k++) {
+ u32 page_length = SSD1307FB_WIDTH * i;
+ u32 index = page_length + (SSD1307FB_WIDTH * k + j) / 8;
+ u8 byte = *(vmem + index);
+ u8 bit = byte & (1 << (7 - (j % 8)));
+ bit = bit >> (7 - (j % 8));
+ buf |= bit << k;
+ }
+ ssd1307fb_write_data(par->client, buf);
+ }
+ }
+
+ return;
+}
+
+
+static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct ssd1307fb_par *par = info->par;
+ unsigned long p = *ppos;
+ void *dst;
+ int err = 0;
+
+ dst = (void __force *) (info->screen_base + p);
+
+ if (copy_from_user(dst, buf, count))
+ err = -EFAULT;
+
+ if (!err)
+ *ppos += count;
+
+ ssd1307fb_update_display(par);
+
+ return (err) ? err : count;
+}
+
+static void ssd1307fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
+{
+ struct ssd1307fb_par *par = info->par;
+ sys_fillrect(info, rect);
+ ssd1307fb_update_display(par);
+}
+
+static void ssd1307fb_copyarea(struct fb_info *info, const struct fb_copyarea *area)
+{
+ struct ssd1307fb_par *par = info->par;
+ sys_copyarea(info, area);
+ ssd1307fb_update_display(par);
+}
+
+static void ssd1307fb_imageblit(struct fb_info *info, const struct fb_image *image)
+{
+ struct ssd1307fb_par *par = info->par;
+ sys_imageblit(info, image);
+ ssd1307fb_update_display(par);
+}
+
+static struct fb_ops ssd1307fb_ops = {
+ .owner = THIS_MODULE,
+ .fb_read = fb_sys_read,
+ .fb_write = ssd1307fb_write,
+ .fb_fillrect = ssd1307fb_fillrect,
+ .fb_copyarea = ssd1307fb_copyarea,
+ .fb_imageblit = ssd1307fb_imageblit,
+};
+
+static void ssd1307fb_deferred_io(struct fb_info *info,
+ struct list_head *pagelist)
+{
+ ssd1307fb_update_display(info->par);
+}
+
+static struct fb_deferred_io ssd1307fb_defio = {
+ .delay = HZ,
+ .deferred_io = ssd1307fb_deferred_io,
+};
+
+static int __devinit ssd1307fb_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+ struct fb_info *info;
+ u32 vmem_size = SSD1307FB_WIDTH * SSD1307FB_HEIGHT / 8;
+ struct ssd1307fb_par *par;
+ u8 *vmem;
+ int ret;
+
+ if (!client->dev.of_node) {
+ dev_err(&client->dev, "No device tree data found!\n");
+ ret = -EINVAL;
+ goto generic_error;
+ }
+
+ info = framebuffer_alloc(sizeof(struct ssd1307fb_par), &client->dev);
+ if (!info) {
+ ret = -ENOMEM;
+ goto generic_error;
+ }
+
+ vmem = devm_kzalloc(&client->dev, vmem_size, GFP_KERNEL);
+ if (!vmem) {
+ dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
+ ret = -ENOMEM;
+ goto generic_error;
+ }
+
+ info->fbops = &ssd1307fb_ops;
+ info->fix = ssd1307fb_fix;
+ info->fbdefio = &ssd1307fb_defio;
+
+ info->var = ssd1307fb_var;
+ info->var.red.length = 1;
+ info->var.red.offset = 0;
+ info->var.green.length = 1;
+ info->var.green.offset = 0;
+ info->var.blue.length = 1;
+ info->var.blue.offset = 0;
+
+ info->screen_base = (u8 __force __iomem *)vmem;
+ info->fix.smem_start = (unsigned long)vmem;
+ info->fix.smem_len = vmem_size;
+
+ fb_deferred_io_init(info);
+
+ par = info->par;
+ par->info = info;
+ par->client = client;
+
+ par->reset = of_get_named_gpio(client->dev.of_node,
+ "oled-reset-gpio", 0);
+ if (gpio_is_valid(par->reset)) {
+ int flags = GPIOF_OUT_INIT_HIGH;
+ if (of_get_property(client->dev.of_node,
+ "oled-reset-active-low", NULL))
+ flags = GPIOF_OUT_INIT_LOW;
+ ret = devm_gpio_request_one(&client->dev, par->reset,
+ flags, "oled-reset");
+ if (ret) {
+ dev_err(&client->dev,
+ "failed to request gpio %d: %d\n",
+ par->reset, ret);
+ goto reset_oled_error;
+ }
+ }
+
+ par->pwm = pwm_get(&client->dev, NULL);
+ if (IS_ERR(par->pwm)) {
+ dev_err(&client->dev, "Could not get PWM from device tree!\n");
+ ret = PTR_ERR(par->pwm);
+ goto pwm_error;
+ }
+
+ par->pwm_period = pwm_get_period(par->pwm);
+
+ dev_dbg(&client->dev, "Using PWM%d with a %dns period.\n", par->pwm->pwm, par->pwm_period);
+
+ ret = register_framebuffer(info);
+ if (ret) {
+ dev_err(&client->dev, "Couldn't register the framebuffer\n");
+ goto fbreg_error;
+ }
+
+ i2c_set_clientdata(client, info);
+
+ /* Reset the screen */
+ gpio_set_value(par->reset, 1);
+ udelay(4);
+ gpio_set_value(par->reset, 0);
+ udelay(4);
+
+ /* Enable the PWM */
+ pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
+ pwm_enable(par->pwm);
+
+ /* Map column 127 of the OLED to segment 0 */
+ ret = ssd1307fb_write_cmd(client, SSD1307FB_SEG_REMAP_ON);
+ if (ret < 0) {
+ dev_err(&client->dev, "Couldn't remap the screen.\n");
+ goto remap_error;
+ }
+
+ /* Turn on the display */
+ ret = ssd1307fb_write_cmd(client, SSD1307FB_DISPLAY_ON);
+ if (ret < 0) {
+ dev_err(&client->dev, "Couldn't turn the display on.\n");
+ goto remap_error;
+ }
+
+ dev_info(&client->dev, "fb%d: %s framebuffer device registered, using %d bytes of video memory\n", info->node, info->fix.id, vmem_size);
+
+ return 0;
+
+remap_error:
+ unregister_framebuffer(info);
+ pwm_disable(par->pwm);
+fbreg_error:
+ pwm_put(par->pwm);
+pwm_error:
+reset_oled_error:
+ fb_deferred_io_cleanup(info);
+ framebuffer_release(info);
+generic_error:
+ return ret;
+}
+
+static int __devexit ssd1307fb_remove(struct i2c_client *client)
+{
+ struct fb_info *info = i2c_get_clientdata(client);
+ struct ssd1307fb_par *par = info->par;
+ unregister_framebuffer(info);
+ pwm_disable(par->pwm);
+ pwm_put(par->pwm);
+ fb_deferred_io_cleanup(info);
+ framebuffer_release(info);
+
+ return 0;
+}
+
+static const struct i2c_device_id ssd1307fb_i2c_id[] = {
+ { "ssd1307fb", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ssd1307fb_i2c_id);
+
+static const struct of_device_id ssd1307fb_of_match[] = {
+ { .compatible = "solomon,ssd1307fb-i2c" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ssd1307fb_of_match);
+
+static struct i2c_driver ssd1307fb_driver = {
+ .probe = ssd1307fb_probe,
+ .remove = __devexit_p(ssd1307fb_remove),
+ .id_table = ssd1307fb_i2c_id,
+ .driver = {
+ .name = "ssd1307fb",
+ .of_match_table = of_match_ptr(ssd1307fb_of_match),
+ .owner = THIS_MODULE,
+ },
+};
+
+module_i2c_driver(ssd1307fb_driver);
+
+MODULE_DESCRIPTION("FB driver for the Solomon SSD1307 OLED controler");
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
+MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related
* [PATCHv2 0/4] Add support for the OLED in the CFA10036
From: Maxime Ripard @ 2012-07-31 10:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi everyone,
This patchset adds support for the solomon SSD1307 OLED controller present
in the CFA-10036 board.
It first adds the framebuffer driver for this controller, and then the
needed bits to enable it in the cfa10036 dts.
Thanks,
Maxime
Changes from v1:
* Did some factorisation of the ssd1307fb_write* functions.
* Filled the smem_start field of the fb_fix_screeninfo structure
* Fixed the broken pwm declaration
* Change the names of oled-reset-gpios and reset-active-low properties in the
device tree to oled-reset-gpio and oled-reset-active-low for consistency
Maxime Ripard (4):
video: Add support for the Solomon SSD1307 OLED Controller
ARM: dts: mxs: Add alternative I2C muxing options for imx28
ARM: dts: mxs: Add pwm4 muxing options for imx28
ARM: dts: mxs: add oled support for the cfa-10036
.../devicetree/bindings/video/ssd1307fb.txt | 24 ++
arch/arm/boot/dts/imx28-cfa10036.dts | 20 +
arch/arm/boot/dts/imx28.dtsi | 18 +
drivers/video/Kconfig | 14 +
drivers/video/Makefile | 1 +
drivers/video/ssd1307fb.c | 418 ++++++++++++++++++++
6 files changed, 495 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/ssd1307fb.txt
create mode 100644 drivers/video/ssd1307fb.c
--
1.7.9.5
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 10:19 UTC (permalink / raw)
To: Alex Courbot
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <5017AA87.2040503-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 9833 bytes --]
On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:
> On 07/30/2012 08:33 PM, Thierry Reding wrote:
> >>+You will need an instance of power_seq_resources to keep track of the resources
> >>+that are already allocated. On success, the function returns a devm allocated
> >>+resolved sequence that is ready to be passed to power_seq_run(). In case of
> >>+failure, and error code is returned.
> >
> >I don't quite understand why the struct power_seq_resources is needed.
> >Can this not be stored within power_seq?
>
> power_seq_resources serves two purposes:
> 1) When parsing sequences, it keeps track of the resources we have
> already allocated to avoid getting the same resource twice
> 2) On cleanup, it cleans the resources that needs to be freed (i.e.
> those that are not devm-handled).
>
> 2) can certainly be removed either by enforcing use of devm, or by
> doing reference counting. 1) seems more difficult to avoid - we need
> to keep track of the resources we already own between calls to
> power_seq_build(). I'd certainly be glad to remove that structure
> from public view and simplify the code if that is possible though.
I still don't see the problem. Managing the resources should be part of
the power_seq core and shouldn't be visible to users. Maybe what you are
worried about is that you may need the same resource both for a power-up
and a power-down sequence? I can see how that would require a global
list of resources.
However I still think it would be easier to encapsulate that completely.
Maybe another level of abstraction is required. You could for example
add another type to encapsulate several power sequences and that could
keep a list of used resources. I can't think of a good name, but maybe
the following DT snippet clarifies what I mean:
power-sequences {
#address-cells = <1>;
#size-cells = <0>;
sequence@0 {
name = "up";
#address-cells = <1>;
#size-cells = <0>;
step@0 {
...
};
...
};
sequence@1 {
name = "down";
#address-cells = <1>;
#size-cells = <0>;
step@0 {
...
};
...
};
};
If you add a name property like this, you could extend the API to
support running a named sequence:
power_seq_run(seq, "up");
...
power_seq_run(seq, "down);
> >Also, is there some way we can make the id property for GPIOs not
> >require the -gpio suffix? If the resource type is already GPIO, then it
> >seems redundant to add -gpio to the ID.
>
> There is unfortunately an inconsistency between the way regulators
> and GPIOs are gotten by name. regulator_get(id) will expect to find
> a property named "id-supply", while gpio_request_one(id) expects a
> property named exactly "id". To workaround this we could sprintf the
> correct property name from a non-suffixed property name within the
> driver, but I think this actually speaks more in favor of having
> phandles directly into the sequences.
Yes, if it can be made to work by specifying the phandle directly that
is certainly better.
> >>+static int power_seq_step_run(struct power_seq_step *step)
> >>+{
> >>+ int err = 0;
> >>+
> >>+ if (step->params.pre_delay)
> >>+ mdelay(step->params.pre_delay);
> >>+
> >>+ switch (step->resource->type) {
> >>+#ifdef CONFIG_REGULATOR
> >>+ case POWER_SEQ_REGULATOR:
> >>+ if (step->params.enable)
> >>+ err = regulator_enable(step->resource->regulator);
> >>+ else
> >>+ err = regulator_disable(step->resource->regulator);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_PWM
> >>+ case POWER_SEQ_PWM:
> >>+ if (step->params.enable)
> >>+ err = pwm_enable(step->resource->pwm);
> >>+ else
> >>+ pwm_disable(step->resource->pwm);
> >>+ break;
> >>+#endif
> >>+#ifdef CONFIG_GPIOLIB
> >>+ case POWER_SEQ_GPIO:
> >>+ gpio_set_value_cansleep(step->resource->gpio,
> >>+ step->params.enable);
> >>+ break;
> >>+#endif
> >
> >This kind of #ifdef'ery is quite ugly. I don't know if adding separate
> >*_run() functions for each type of resource would be any better, though.
> >Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
> >GPIOLIB symbols to side-step the issue completely?
>
> If it is not realistic to consider a kernel built without regulator,
> pwm or gpiolib support, then we might as well do that. But isn't
> that a possibility?
I'd say that anything complex enough to make use of power-sequencing
probably has all of these enabled anyway. But maybe I'm not very
qualified to judge.
>
> >>+ if (!seq) return 0;
> >
> >I don't think this is acceptable according to the coding style. Also,
> >perhaps returning -EINVAL would be more meaningful?
>
> I neglected running checkpatch before submitting, apologies for
> that. The return value seems correct to me, a NULL sequence has no
> effect.
But seq == NULL should never happen anyway, right?
>
> >>+
> >>+ while (seq->resource) {
> >
> >Perhaps this should check for POWER_SEQ_STOP instead?
>
> There is no resource for POWER_SEQ_STOP - therefore, a NULL resource
> is used instead.
Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of
a sequence, so intuitively I'd be looking for that as a stop condition.
> >>+typedef struct platform_power_seq_step platform_power_seq;
> >
> >Why are the parameters kept in a separate structure? What are the
> >disadvantages of keeping the in the sequence step structure directly?
>
> This ensures the same parameters are used for the platform data and
> resolved sequences, and also ensures they are all copied correctly
> using memcpy. But maybe I am just making something complex out of
> something that ought to be simpler.
>
> >>+struct power_seq_step {
> >>+ struct power_seq_resource *resource;
> >>+ struct power_step_params params;
> >>+};
> >>+typedef struct power_seq_step power_seq;
> >
> >Would it make sense to make the struct power_seq opaque? I don't see why
> >anyone but the power_seq code should access the internals.
>
> I would like to do that actually. The issue is that it did not work
> go well with the legacy pwm_backlight behavior: a power sequence
> needs to be constructed out of a PWM obtained through
> pwm_request(int pwm_id, char *label) and this behavior cannot be
> emulated using the new platform data interface (which only works
> with pwm_get()). But if I remove this old behavior, then I could
> make power_seq opaque. I don't think many drivers are using it. What
> do you think?
I don't see how that is relevant here, since this power-sequencing code
is supposed to be generic and not tied to any specific implementation.
Can you explain further?
In any case you shouldn't be using pwm_request() in new code.
> >For resource
> >managing it might also be easier to separate struct power_seq_step and
> >struct power_seq, making the power_seq basically something like:
> >
> > struct power_seq {
> > struct power_seq_step *steps;
> > unsigned int num_steps;
> > };
> >
> >Perhaps a name field can be included for diagnostic purposes.
>
> Yes, looks like we are going in that direction. If this can be made
> private then the number of public data structures will not be too
> confusing (platform data only, basically).
Yes, that sounds like a much cleaner approach.
>
> >>+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> >>+ platform_power_seq *pseq);
> >
> >I already mentioned this above: I fail to see why the ress parameter is
> >needed here. It is an internal implementation detail of the power
> >sequence code. Maybe a better place would be to include it within the
> >struct power_seq.
>
> Problem is that I need to track which resources are already
> allocated between calls to power_seq_build(). Even if I attach the
> resources into struct power_seq, they won't be attainable by the
> next call. So I'm afraid we are bound to pass a tracking structure
> at least to power_seq_build.
I think this could be solved nicely by what I proposed earlier.
> >>+/**
> >>+ * Free all the resources previously allocated by power_seq_allocate_resources.
> >>+ */
> >>+void power_seq_free_resources(power_seq_resources *ress);
> >>+
> >>+/**
> >>+ * Run the given power sequence. Returns 0 on success, error code in case of
> >>+ * failure.
> >>+ */
> >>+int power_seq_run(struct device *dev, power_seq *seq);
> >
> >I think the API is too fine grained here. From a user's point of view,
> >I'd expect a sequence like this:
> >
> > seq = power_seq_build(dev, sequence);
> > ...
> > power_seq_run(seq);
> > ...
> > power_seq_free(seq);
> >
> >Perhaps with managed variants where the power_seq_free() is executed
> >automatically:
> >
> > seq = devm_power_seq_build(dev, sequence);
> > ...
> > power_seq_run(seq);
>
> I agree. On top of that, of_parse_power_seq() should directly return
> a resolved power sequence, not the platform data.
I'm not sure. The idiom seems to be to use DT as an alternative source
for platform data, which I guess is due to most drivers already using
platform data. But it has the advantage that after you have the platform
data, be it from DT or directly specified, the subsequent code remains
the same.
Of course you could provide a separate of_build_power_seq() that wraps
both steps for convenience.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* RE: [PATCH] video: da8xx-fb: do clock reset of revision 2 LCDC before enabling
From: Manjunathappa, Prakash @ 2012-07-31 10:18 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1342799471-10729-1-git-send-email-prakash.pm@ti.com>
Hi,
On Mon, Jul 30, 2012 at 02:44:33, Florian Tobias Schandinat wrote:
> On 07/20/2012 03:51 PM, Manjunathappa, Prakash wrote:
> > As in specification software reset should be applied for several
> > cycles before bringing it out of reset. Without this patch
> > particularly during suspend and resume clock reset is not guaranteed
> > to happen.
> >
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
>
> Applied. But it would be better if you made the patch dependencies
> clear, as I didn't consider some of your patches mature enough for this
> merge window. Now I had to figure out why my final build failed and
> applied the patch below.
>
Thanks Florian Tobias Schandinat. I will take care to mention dependencies
in future.
Regards,
Prakash
>
> Best regards,
>
> Florian Tobias Schandinat
>
> ---
> commit a0239073fd75489d25575cf3aaf71ab55b416020
> Author: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Date: Sun Jul 29 16:47:40 2012 +0000
>
> da8xx-fb: fix compile issue due to missing include
>
> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index ca9943a..7ae9d53 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -32,6 +32,7 @@
> #include <linux/console.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> +#include <linux/delay.h>
> #include <linux/lcm.h>
> #include <video/da8xx-fb.h>
> #include <asm/div64.h>
>
>
> > ---
> > drivers/video/da8xx-fb.c | 12 ++++++++----
> > 1 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> > index 3d2d0d1..4440292 100644
> > --- a/drivers/video/da8xx-fb.c
> > +++ b/drivers/video/da8xx-fb.c
> > @@ -262,10 +262,18 @@ static inline void lcd_enable_raster(void)
> > {
> > u32 reg;
> >
> > + /* Put LCDC in reset for several cycles */
> > + if (lcd_revision = LCD_VERSION_2)
> > + /* Write 1 to reset LCDC */
> > + lcdc_write(LCD_CLK_MAIN_RESET, LCD_CLK_RESET_REG);
> > + mdelay(1);
> > +
> > /* Bring LCDC out of reset */
> > if (lcd_revision = LCD_VERSION_2)
> > lcdc_write(0, LCD_CLK_RESET_REG);
> > + mdelay(1);
> >
> > + /* Above reset sequence doesnot reset register context */
> > reg = lcdc_read(LCD_RASTER_CTRL_REG);
> > if (!(reg & LCD_RASTER_ENABLE))
> > lcdc_write(reg | LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
> > @@ -279,10 +287,6 @@ static inline void lcd_disable_raster(void)
> > reg = lcdc_read(LCD_RASTER_CTRL_REG);
> > if (reg & LCD_RASTER_ENABLE)
> > lcdc_write(reg & ~LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
> > -
> > - if (lcd_revision = LCD_VERSION_2)
> > - /* Write 1 to reset LCDC */
> > - lcdc_write(LCD_CLK_MAIN_RESET, LCD_CLK_RESET_REG);
> > }
> >
> > static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
>
>
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 10:15 UTC (permalink / raw)
To: Stephen Warren
Cc: Rob Herring, Stephen Warren, Thierry Reding, Simon Glass,
Grant Likely, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <50170A14.6000201-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
On 07/31/2012 07:26 AM, Stephen Warren wrote:
> On 07/30/2012 09:44 AM, Rob Herring wrote:
>> On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
>>> Some device drivers (panel backlights especially) need to follow precise
>>> sequences for powering on and off, involving gpios, regulators, PWMs
>>> with a precise powering order and delays to respect between each steps.
>>> These sequences are board-specific, and do not belong to a particular
>>> driver - therefore they have been performed by board-specific hook
>>> functions to far.
>>>
>>> With the advent of the device tree and of ARM kernels that are not
>>> board-tied, we cannot rely on these board-specific hooks anymore but
>>> need a way to implement these sequences in a portable manner. This patch
>>> introduces a simple interpreter that can execute such power sequences
>>> encoded either as platform data or within the device tree.
>>>
>>
>> Why not? We'll always have some amount of board code. The key is to
>> limit parts that are just data. I'm not sure this is something that
>> should be in devicetree.
>>
>> Perhaps what is needed is a better way to hook into the driver like
>> notifiers?
>
> I would answer that by asking the reverse question - why should we have
> to put some data in DT, and some data into board files still?
>
> I'd certainly argue that the sequence of which GPIOs/regulators/PWMs to
> manipulate is just data.
>
> To be honest, if we're going to have to put some parts of a board's
> configuration into board files anyway, then the entirety of DT seems
> useless; I'd far rather see all the configuration in one cohesive place
> than arbitrarily split into two/n different locations - that would make
> everything harder to maintain.
Also, having these sequences into the DT would allow an older kernel to
boot on and correctly initialize a newer board with - which is also part
of the DT's purpose if I am not mistaken.
Alex.
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 10:11 UTC (permalink / raw)
To: Thierry Reding
Cc: Simon Glass, Stephen Warren, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <20120731091324.GA15557-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On 07/31/2012 06:13 PM, Thierry Reding wrote:
>> I don't see any need for microseconds myself - anybody sees use for
>> finer-grained delays?
>>
>> Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
>
> You might want to take a look at Documentation/timers/timers-howto.txt.
> msleep() isn't very accurate for periods shorter than 20 ms.
Ok, looks like usleep_range is the way to go here. In that case it would
probably not hurt to specify delays in microseconds in the DT and
platform data as well.
>>>> +Device tree
>>>> +-----------
>>>> +All the same, power sequences can be encoded as device tree nodes. The following
>>>> +properties and nodes are equivalent to the platform data defined previously:
>>>> +
>>>> + power-supply = <&mydevice_reg>;
>>>> + enable-gpio = <&gpio 6 0>;
>>>> +
>>>> + power-on-sequence {
>>>> + regulator@0 {
>>>> + id = "power";
>>>
>>> Is there a reason not to put the phandle here, like:
>>>
>>> id = <&mydevice_reg>;
>>>
>>> (or maybe 'device' instead of id?)
>>
>> There is one reason, but it might be a bad one. On Tegra, PWM
>> phandle uses an extra cell to encode the duty-cycle the PWM should
>> have when we call get_pwm().
>
> This is not only the case on Tegra, but it is the default unless a
> driver specifically overrides it. The second cell specifies the index of
> the PWM device within the PWM chip. The third cell doesn't specify the
> duty cycle but the period of the PWM.
Then I think there is a mistake in
Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt:
"the second cell is the duty cycle in nanoseconds."
>> This makes it possible to address the
>> same PWM with different phandles (and different duty cycles),
>
> How so? A phandle will always refer to a PWM chip. Paired with the
> second cell, of_pwm_request() will resolve that into the correct PWM
> device.
For tegra, we can only address PWMs this way IIRC:
pwm = <&pwm 2 5000000>;
If we had <&pwm 2>, I agree that there would be no problem. But here the
period of the PWM is also given - and in practice, we can request the
same PWM using different phandles. For instance, if the above property
was part of the power-on sequence, and the following
pwm = <&pwm 2 0>;
was part of power-off, how can I know that these two different phandles
refer to the same PWM without calling pwm_get a second time and getting
-EBUSY?
Of course if the same period is specified for both, I will not have this
issue as the phandles will be identical, but the possibility remains
open that we are given a faulty tree here.
More generally speaking, wouldn't it make more sense to have the
period/duty cycle of a PWM encoded into separate properties when needed
and have the phandle just reference the PWM instance? This also seems to
stand from an API point of view, since the period is not specified when
invoking pwm_request or pwm_get, but set by its own pwm_set_period function?
On an unrelated note, I also don't understand why the period is also a
parameter of pwm_config and why pwm_set_period does not do anything
beyond setting a struct member that is returned by pwm_get_period (but
maybe I am missing something here).
Alex.
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 9:51 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <20120730113323.GA7303@avionic-0098.adnet.avionic-design.de>
On 07/30/2012 08:33 PM, Thierry Reding wrote:
>> +You will need an instance of power_seq_resources to keep track of the resources
>> +that are already allocated. On success, the function returns a devm allocated
>> +resolved sequence that is ready to be passed to power_seq_run(). In case of
>> +failure, and error code is returned.
>
> I don't quite understand why the struct power_seq_resources is needed.
> Can this not be stored within power_seq?
power_seq_resources serves two purposes:
1) When parsing sequences, it keeps track of the resources we have
already allocated to avoid getting the same resource twice
2) On cleanup, it cleans the resources that needs to be freed (i.e.
those that are not devm-handled).
2) can certainly be removed either by enforcing use of devm, or by doing
reference counting. 1) seems more difficult to avoid - we need to keep
track of the resources we already own between calls to
power_seq_build(). I'd certainly be glad to remove that structure from
public view and simplify the code if that is possible though.
>> +
>> +A resolved power sequence returned by power_seq_build can be run by
>> +power_run_run():
>> +
>> +int power_seq_run(struct device *dev, power_seq *seq);
>
> Why is the struct device required here? It already is passed during the
> call to pwm_seq_build(), so perhaps you should keep a reference to it
> within struct power_seq?
The device is only needed for printing error messages. But as you point
later, maybe messages should not be printed there at all. I will try to
remove that parameter.
>> +It returns 0 if the sequence has successfully been run, or an error code if a
>> +problem occured.
>> +
>> +Finally, some resources that cannot be allocated through devm need to be freed
>> +manually. Therefore, be sure to call power_seq_free_resources() in your device
>> +remove function:
>> +
>> +void power_seq_free_resources(power_seq_resources *ress);
>
> Could this not also be handled by a managed version? If a power_seq is
> always managed, then I would assume that it also takes care of freeing
> the resources, even if the resources have no managed equivalents.
Right.
> Perhaps it would also make sense to provide non-managed version of these
> functions. I think that would make the managed versions easier and more
> canonical to implement.
A power_seq is a single block of memory, so that should be reasonnably
doable indeed. Let me think a little bit more about that.
>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> + power-supply = <&mydevice_reg>;
>> + enable-gpio = <&gpio 6 0>;
>> +
>> + power-on-sequence {
>> + regulator@0 {
>> + id = "power";
>> + enable;
>> + post-delay = <10>;
>> + };
>> + gpio@1 {
>> + id = "enable-gpio";
>> + enable;
>> + };
>> + };
>> +
>> +Note that first, the phandles of the regulator and gpio used in the sequences
>> +are defined as properties. Then the sequence references them through the id
>> +property of every step. The name of sub-properties defines the type of the step.
>> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
>> +sequentially.
>
> I think there has been quite some discussion regarding the naming of
> subnodes and the conclusion seems to have been to name them uniformly
> after what they represent. As such the power-on-sequence subnodes should
> be called step@0, step@1, etc. However, that will require the addition
> of a property to define the type of resource.
That's fine I guess - just adds some footprint to the DT, but nothing crazy.
> Also, is there some way we can make the id property for GPIOs not
> require the -gpio suffix? If the resource type is already GPIO, then it
> seems redundant to add -gpio to the ID.
There is unfortunately an inconsistency between the way regulators and
GPIOs are gotten by name. regulator_get(id) will expect to find a
property named "id-supply", while gpio_request_one(id) expects a
property named exactly "id". To workaround this we could sprintf the
correct property name from a non-suffixed property name within the
driver, but I think this actually speaks more in favor of having
phandles directly into the sequences.
>> +config POWER_SEQ
>> + bool
>> + default n
>> +
>
> "default n" is already the default, so you can drop that line.
Did that, thanks.
>> +#ifdef CONFIG_OF
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#endif
>
> I think you don't need the CONFIG_OF guard around these. Both of.h and
> of_gpio.h can be included unconditionally and actually contain dummy
> definitions for the public functions in the !OF case.
Fixed, thanks.
>> +static int power_seq_step_run(struct power_seq_step *step)
>> +{
>> + int err = 0;
>> +
>> + if (step->params.pre_delay)
>> + mdelay(step->params.pre_delay);
>> +
>> + switch (step->resource->type) {
>> +#ifdef CONFIG_REGULATOR
>> + case POWER_SEQ_REGULATOR:
>> + if (step->params.enable)
>> + err = regulator_enable(step->resource->regulator);
>> + else
>> + err = regulator_disable(step->resource->regulator);
>> + break;
>> +#endif
>> +#ifdef CONFIG_PWM
>> + case POWER_SEQ_PWM:
>> + if (step->params.enable)
>> + err = pwm_enable(step->resource->pwm);
>> + else
>> + pwm_disable(step->resource->pwm);
>> + break;
>> +#endif
>> +#ifdef CONFIG_GPIOLIB
>> + case POWER_SEQ_GPIO:
>> + gpio_set_value_cansleep(step->resource->gpio,
>> + step->params.enable);
>> + break;
>> +#endif
>
> This kind of #ifdef'ery is quite ugly. I don't know if adding separate
> *_run() functions for each type of resource would be any better, though.
> Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
> GPIOLIB symbols to side-step the issue completely?
If it is not realistic to consider a kernel built without regulator, pwm
or gpiolib support, then we might as well do that. But isn't that a
possibility?
>> + if (!seq) return 0;
>
> I don't think this is acceptable according to the coding style. Also,
> perhaps returning -EINVAL would be more meaningful?
I neglected running checkpatch before submitting, apologies for that.
The return value seems correct to me, a NULL sequence has no effect.
>> +
>> + while (seq->resource) {
>
> Perhaps this should check for POWER_SEQ_STOP instead?
There is no resource for POWER_SEQ_STOP - therefore, a NULL resource is
used instead.
>> + if ((err = power_seq_step_run(seq++))) {
>> + dev_err(dev, "error %d while running power sequence!\n",
>> + err);
>
> For this kind of diagnostics it could be useful to have a name
> associated with the power sequence. But I'm not sure that making the
> power sequence code output an error here is the best solution. I find it
> to be annoying when core code starts outputting too many error codes. In
> this case it's particularily easy to catch the errors in the caller.
Giving names to power sequences sounds like a good idea. Let me see how
this can be done. It might require some more data structuring.
>> +
>> + while ((child = of_get_next_child(node, child)))
>> + cpt++;
>
> for_each_child_of_node()?
>
>> +
>> + /* allocate one more step to signal end of sequence */
>> + ret = devm_kzalloc(dev, sizeof(*ret) * (cpt + 1), GFP_KERNEL);
>> + if (!ret)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + cpt = 0;
>> + while ((child = of_get_next_child(node, child))) {
>
> Here as well.
Ah, didn't know that. Thanks.
>> + /* first pass to count the number of steps to allocate */
>> + for (cpt = 0; pseq[cpt].type != POWER_SEQ_STOP; cpt++);
>
> Wouldn't it be easier to pass around the number of steps in the sequence
> instead of having to count in various places? This would be more along
> the lines of how struct platform_device defines associated resources.
My goal was to limit the number of data structures, but if we add a name
to power sequences, we can add a steps count as well.
>> +
>> + if (!cpt)
>> + return seq;
>
> Perhaps this should return an error-code as well? I find it nice to not
> have to handle NULL specially when using ERR_PTR et al.
Agreed.
>> +typedef enum {
>> + POWER_SEQ_STOP = 0,
>> + POWER_SEQ_REGULATOR,
>> + POWER_SEQ_PWM,
>> + POWER_SEQ_GPIO,
>> + POWER_SEQ_MAX,
>> +} power_res_type;
>
> Maybe the prefix power_seq should be used here as well, so:
> power_seq_res_type.
Definitely.
>> +typedef struct list_head power_seq_resources;
>
> No type definitions like this, please. Also, why define this particular
> type globally?
I will move that into a proper structure with a name and number of steps.
>> +
>> +struct power_step_params {
>> + /* enable the resource if 1, disable if 0 */
>> + bool enable;
>> + /* delay (in ms) to wait before executing the step */
>> + int pre_delay;
>> + /* delay (in ms) to wait after executing the step */
>> + int post_delay;
>
> unsigned int for the delays?
Yup.
>> +typedef struct platform_power_seq_step platform_power_seq;
>
> Why are the parameters kept in a separate structure? What are the
> disadvantages of keeping the in the sequence step structure directly?
This ensures the same parameters are used for the platform data and
resolved sequences, and also ensures they are all copied correctly using
memcpy. But maybe I am just making something complex out of something
that ought to be simpler.
>> +struct power_seq_step {
>> + struct power_seq_resource *resource;
>> + struct power_step_params params;
>> +};
>> +typedef struct power_seq_step power_seq;
>
> Would it make sense to make the struct power_seq opaque? I don't see why
> anyone but the power_seq code should access the internals.
I would like to do that actually. The issue is that it did not work go
well with the legacy pwm_backlight behavior: a power sequence needs to
be constructed out of a PWM obtained through pwm_request(int pwm_id,
char *label) and this behavior cannot be emulated using the new platform
data interface (which only works with pwm_get()). But if I remove this
old behavior, then I could make power_seq opaque. I don't think many
drivers are using it. What do you think?
> For resource
> managing it might also be easier to separate struct power_seq_step and
> struct power_seq, making the power_seq basically something like:
>
> struct power_seq {
> struct power_seq_step *steps;
> unsigned int num_steps;
> };
>
> Perhaps a name field can be included for diagnostic purposes.
Yes, looks like we are going in that direction. If this can be made
private then the number of public data structures will not be too
confusing (platform data only, basically).
>> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
>> + platform_power_seq *pseq);
>
> I already mentioned this above: I fail to see why the ress parameter is
> needed here. It is an internal implementation detail of the power
> sequence code. Maybe a better place would be to include it within the
> struct power_seq.
Problem is that I need to track which resources are already allocated
between calls to power_seq_build(). Even if I attach the resources into
struct power_seq, they won't be attainable by the next call. So I'm
afraid we are bound to pass a tracking structure at least to
power_seq_build.
>> +/**
>> + * Free all the resources previously allocated by power_seq_allocate_resources.
>> + */
>> +void power_seq_free_resources(power_seq_resources *ress);
>> +
>> +/**
>> + * Run the given power sequence. Returns 0 on success, error code in case of
>> + * failure.
>> + */
>> +int power_seq_run(struct device *dev, power_seq *seq);
>
> I think the API is too fine grained here. From a user's point of view,
> I'd expect a sequence like this:
>
> seq = power_seq_build(dev, sequence);
> ...
> power_seq_run(seq);
> ...
> power_seq_free(seq);
>
> Perhaps with managed variants where the power_seq_free() is executed
> automatically:
>
> seq = devm_power_seq_build(dev, sequence);
> ...
> power_seq_run(seq);
I agree. On top of that, of_parse_power_seq() should directly return a
resolved power sequence, not the platform data.
> Generally I really like where this is going.
Thanks - I really appreciate your review.
Alex.
^ permalink raw reply
* inquiry
From: roboth roli company @ 2012-07-31 9:25 UTC (permalink / raw)
i am robothroli, Purchase manager from roli Merchant Ltd. We are
Import/export Company based in taiwan. We are interested in purchasing
your product and I would like to make an inquiry. Please inform me on:
Sample availability and price
Minimum order quantity
FOB Prices
Sincerely
Purchase Manager
robothroli
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 9:16 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Alexandre Courbot, Stephen Warren, Simon Glass,
Grant Likely, Greg Kroah-Hartman, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
In-Reply-To: <20120730154706.GL4468-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 760 bytes --]
On Mon, Jul 30, 2012 at 04:47:06PM +0100, Mark Brown wrote:
> On Mon, Jul 30, 2012 at 10:44:29AM -0500, Rob Herring wrote:
> > On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
>
> > > + power-on-sequence {
> > > + regulator@0 {
> > > + id = "power";
> > > + enable;
>
> > What do this mean? Isn't this implied for a regulator?
>
> I assume you might have some sequences which need some things to be
> turned off for some reason; it at least seems to be something you'd want
> to design for.
Furthermore there is the power-off-sequence equivalent, which you use
for instance when you turn off the panel. Typically they would do the
inverse of the power-on-sequence, so turning off a regulator is
definitiely required.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-07-31 9:13 UTC (permalink / raw)
To: Alex Courbot
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Stephen Warren, Greg Kroah-Hartman, Mark Brown,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <50179933.9090501-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]
On Tue, Jul 31, 2012 at 05:37:07PM +0900, Alex Courbot wrote:
> Hi Simon,
>
> On 07/30/2012 08:00 PM, Simon Glass wrote:
> >For the delay, I think milliseconds is reasonable. I suppose there is
> >no reasonable need for microseconds?
>
> I don't see any need for microseconds myself - anybody sees use for
> finer-grained delays?
>
> Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
You might want to take a look at Documentation/timers/timers-howto.txt.
msleep() isn't very accurate for periods shorter than 20 ms.
> >>+Device tree
> >>+-----------
> >>+All the same, power sequences can be encoded as device tree nodes. The following
> >>+properties and nodes are equivalent to the platform data defined previously:
> >>+
> >>+ power-supply = <&mydevice_reg>;
> >>+ enable-gpio = <&gpio 6 0>;
> >>+
> >>+ power-on-sequence {
> >>+ regulator@0 {
> >>+ id = "power";
> >
> >Is there a reason not to put the phandle here, like:
> >
> > id = <&mydevice_reg>;
> >
> >(or maybe 'device' instead of id?)
>
> There is one reason, but it might be a bad one. On Tegra, PWM
> phandle uses an extra cell to encode the duty-cycle the PWM should
> have when we call get_pwm().
This is not only the case on Tegra, but it is the default unless a
driver specifically overrides it. The second cell specifies the index of
the PWM device within the PWM chip. The third cell doesn't specify the
duty cycle but the period of the PWM.
> This makes it possible to address the
> same PWM with different phandles (and different duty cycles),
How so? A phandle will always refer to a PWM chip. Paired with the
second cell, of_pwm_request() will resolve that into the correct PWM
device.
> which
> causes an issue to know whether a PWM is already used in a sequence
> (potentially resulting in multiple get_pwm calls on the same PWM,
> and also opens the door to ambiguities in behavior (what is the
> correct duty-cycle to use if several different values are passed?)
You can't request a PWM multiple times. The second call well return
-EBUSY, or rather ERR_PTR(-EBUSY).
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: DISPC: Use msleep instead of blocking mdelay
From: Tomi Valkeinen @ 2012-07-31 8:53 UTC (permalink / raw)
To: Jassi Brar; +Cc: a0393947, linux-omap, linux-fbdev
In-Reply-To: <1343138635-2802-1-git-send-email-jaswinder.singh@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 563 bytes --]
On Tue, 2012-07-24 at 19:33 +0530, Jassi Brar wrote:
> We have no reason to block in the error handler workqueue, so use msleep.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> drivers/video/omap2/dss/dispc.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
Thanks, applied.
As a side note, I hate that error handler =). It was a quick emergency
hack to somehow handle certain situations, but generally it sucks.
However, I don't really have a good idea what to do in case we get
errors from DSS HW.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] OMAPDSS: Do not require a VDDS_DSI regulator on am35xx
From: Tomi Valkeinen @ 2012-07-31 8:45 UTC (permalink / raw)
To: Raphael Assenat; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <20120719200429.GD3850@renkinjitsu.usine.8d.com>
[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]
On Thu, 2012-07-19 at 16:04 -0400, Raphael Assenat wrote:
> On our AM3505 based board, dpi.c complains that there is no VDSS_DSI regulator
> and the framebuffer cannot be enabled. However, this check does not seem to
> apply to AM3505/17 chips.
>
> I am not the first facing this issue, see this thread from Nov. 2011:
> http://marc.info/?l=linux-omap&m=132147745930213&w=2
>
> The string 'vdds_dsi' does appear once in the technical reference manual[1]
> but there is no corresponding power pin on the package[2]. I failed to
> locate any signal that could be an equivalent. I am trying to obtain some
> clarifications on TI's forum[3]...
>
> In any case, I am currently running with the patch below. In order to keep
> cpu_is_xx uses to a minimum, I check for am35xx once at init time and allow
> dpi.vdds_dsi_reg to be NULL from then on, getting rid of all the other
> cpu_is_omap34xx uses in the process.
>
> Your comments would be appreciated. Please also consider for merging.
VDDS_DSI is used to power up some of the DSS pins on OMAP3. I don't know
why the HW was designed like that... If you have a correct image without
the power, then obviously it's not needed.
We don't currently deal with AM3xxx SoCs in any way in the driver. It's
difficult enough trying to handle just OMAP DSS versions, and now we
need to add AM3xxx to the mix. Sigh =).
However, I don't want to apply this patch, as we're trying to remove the
cpu_is checks (soc_is goes in the same category).
I guess we need to add entries for the AM3xxx SoCs in the
dss_features.c.
Any idea what other differences AM3xxx has compared to OMAP3?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-07-31 8:37 UTC (permalink / raw)
To: Simon Glass
Cc: Stephen Warren, Thierry Reding, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <CAPnjgZ0H2xrJcL-ytMaX11iYrrhCg7LEM00u_NgEaveM4gHMPw@mail.gmail.com>
Hi Simon,
On 07/30/2012 08:00 PM, Simon Glass wrote:
> For the delay, I think milliseconds is reasonable. I suppose there is
> no reasonable need for microseconds?
I don't see any need for microseconds myself - anybody sees use for
finer-grained delays?
Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.
>> +Device tree
>> +-----------
>> +All the same, power sequences can be encoded as device tree nodes. The following
>> +properties and nodes are equivalent to the platform data defined previously:
>> +
>> + power-supply = <&mydevice_reg>;
>> + enable-gpio = <&gpio 6 0>;
>> +
>> + power-on-sequence {
>> + regulator@0 {
>> + id = "power";
>
> Is there a reason not to put the phandle here, like:
>
> id = <&mydevice_reg>;
>
> (or maybe 'device' instead of id?)
There is one reason, but it might be a bad one. On Tegra, PWM phandle
uses an extra cell to encode the duty-cycle the PWM should have when we
call get_pwm(). This makes it possible to address the same PWM with
different phandles (and different duty cycles), which causes an issue to
know whether a PWM is already used in a sequence (potentially resulting
in multiple get_pwm calls on the same PWM, and also opens the door to
ambiguities in behavior (what is the correct duty-cycle to use if
several different values are passed?)
Maybe the problem lies in how PWM phandles are handled - if duty-cycle
was not part of the information, we would not have this problem.
>> +Note that first, the phandles of the regulator and gpio used in the sequences
>> +are defined as properties. Then the sequence references them through the id
>> +property of every step. The name of sub-properties defines the type of the step.
>> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
>> +sequentially.
>
> For the regulator and gpio types I think you only have an enable. For
> the pwm, what is the intended binding? Is that documented elsewhere?
Same thing, enable/disable which would call pwm_enable and pwm_disable.
One could also image an additional property to set the duty cycle if it
can be taken off the phandle.
> Also it might be worth mentioning how you get a struct power_seq from
> an fdt node, and perhaps given an example of a device which has an
> attached node, so we can see how it is referenced from the device
> (of_parse_power_seq I think). Do put the sequence inside the device
> node or reference it with a phandle?
Yes, this definitely needs more documentation - especially the DT part.
Thanks,
Alex.
^ permalink raw reply
* RE: [PATCH 1/3] Move FIMD register headers to include/video/
From: Marek Szyprowski @ 2012-07-31 8:32 UTC (permalink / raw)
To: 'Jingoo Han', 'Leela Krishna Amudala'
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, kgene.kim, joshi
In-Reply-To: <000701cd6ef5$24315c70$6c941550$%han@samsung.com>
Hello,
On Tuesday, July 31, 2012 10:19 AM Jingoo Han wrote:
> On Tuesday, July 31, 2012 3:28 PM Marek Szyprowski wrote:
> >
> > Hello,
> >
> > On Tuesday, July 31, 2012 2:48 AM Jingoo Han wrote:
> >
> > > On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
> > > >
> > > > Hello Jingoo Han,
> > > >
> > > > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> > > > >>
> > > > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> > > > >> to include/video/samsung_fimd.h
> > > > >>
> > > > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > > > >> ---
> > > > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> > > > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403 -----------------
> > > > >> include/video/samsung_fimd.h | 533 +++++++++++++++++++++++
> > > > >> 3 files changed, 533 insertions(+), 562 deletions(-)
> > > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> > > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> > > > >> create mode 100644 include/video/samsung_fimd.h
> > > > >>
> > > > >> +*/
> > > > >> +
> > > > >> +/*FIMD V8 REG OFFSET */
> > > > >> +#define FIMD_V8_VIDTCON0 (0x20010)
> > > > >> +#define FIMD_V8_VIDTCON1 (0x20014)
> > > > >> +#define FIMD_V8_VIDTCON2 (0x20018)
> > > > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> > > > >> +#define FIMD_V8_VIDCON1 (0x20004)
> > >
> > >
> > > How about using soc_is_exynos5250()?
> > >
> > > +#define VIDTCON0 (soc_is_exynos5250() ? \
> > > + (0x20010) : (0x10))
> > >
> > > In this case, the FIMD driver does not need to change.
> > > Also, one binary is available.
> >
> > Please don't mix two methods of runtime detection. FIMD driver (s3c-fb) already
> > has runtime hw detection based on platform device id. Adding such detection for
> > exynos5 to DRM FIMD driver should not be a big issue too.
>
> Marek,
> Then, do you want use like this?
>
> #define VIDTCON0 (0x10)
> +#define FIMD_V8_VIDTCON0 (0x20010)
>
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -1924,7 +1924,7 @@ static struct s3c_fb_driverdata s3c_fb_data_exynos4 = {
> static struct s3c_fb_driverdata s3c_fb_data_exynos5 = {
> .variant = {
> .nr_windows = 5,
> - .vidtcon = VIDTCON0,
> + .vidtcon = FIMD_V8_VIDTCON0,
Yes, this method looks good imo. Maybe even having something like vidtcon_base in
variant structure will be enough to cover all VIDTCON0-3 registers.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
^ permalink raw reply
* Re: [PATCH 1/3] Move FIMD register headers to include/video/
From: Jingoo Han @ 2012-07-31 8:19 UTC (permalink / raw)
To: 'Marek Szyprowski', 'Leela Krishna Amudala'
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, kgene.kim, joshi, 'Jingoo Han'
In-Reply-To: <032201cd6ee5$ad4a0d50$07de27f0$%szyprowski@samsung.com>
On Tuesday, July 31, 2012 3:28 PM Marek Szyprowski wrote:
>
> Hello,
>
> On Tuesday, July 31, 2012 2:48 AM Jingoo Han wrote:
>
> > On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
> > >
> > > Hello Jingoo Han,
> > >
> > > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> > > >>
> > > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> > > >> to include/video/samsung_fimd.h
> > > >>
> > > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > > >> ---
> > > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> > > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403 -----------------
> > > >> include/video/samsung_fimd.h | 533 +++++++++++++++++++++++
> > > >> 3 files changed, 533 insertions(+), 562 deletions(-)
> > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> > > >> create mode 100644 include/video/samsung_fimd.h
> > > >>
> > > >> +*/
> > > >> +
> > > >> +/*FIMD V8 REG OFFSET */
> > > >> +#define FIMD_V8_VIDTCON0 (0x20010)
> > > >> +#define FIMD_V8_VIDTCON1 (0x20014)
> > > >> +#define FIMD_V8_VIDTCON2 (0x20018)
> > > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> > > >> +#define FIMD_V8_VIDCON1 (0x20004)
> >
> >
> > How about using soc_is_exynos5250()?
> >
> > +#define VIDTCON0 (soc_is_exynos5250() ? \
> > + (0x20010) : (0x10))
> >
> > In this case, the FIMD driver does not need to change.
> > Also, one binary is available.
>
> Please don't mix two methods of runtime detection. FIMD driver (s3c-fb) already
> has runtime hw detection based on platform device id. Adding such detection for
> exynos5 to DRM FIMD driver should not be a big issue too.
Marek,
Then, do you want use like this?
#define VIDTCON0 (0x10)
+#define FIMD_V8_VIDTCON0 (0x20010)
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1924,7 +1924,7 @@ static struct s3c_fb_driverdata s3c_fb_data_exynos4 = {
static struct s3c_fb_driverdata s3c_fb_data_exynos5 = {
.variant = {
.nr_windows = 5,
- .vidtcon = VIDTCON0,
+ .vidtcon = FIMD_V8_VIDTCON0,
>
> Best regards
> --
> Marek Szyprowski
> Samsung Poland R&D Center
^ permalink raw reply
* Re: [PATCH 1/3] Move FIMD register headers to include/video/
From: Jingoo Han @ 2012-07-31 8:09 UTC (permalink / raw)
To: 'Tomasz Figa'
Cc: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc, dri-devel, linux-fbdev, ben-linux, inki.dae,
kgene.kim, joshi, 'Marek Szyprowski'
In-Reply-To: <2073177.5bSgcKjVcU@easynote>
On Tuesday, July 31, 2012 3:37 PM, Tomasz Figa wrote:
>
> Hi,
>
> On Tuesday 31 of July 2012 at 09:47:57, Jingoo Han wrote:
> > On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
> > > Hello Jingoo Han,
> > >
> > > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> > > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> > > >> to include/video/samsung_fimd.h
> > > >>
> > > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > > >> ---
> > > >>
> > > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> > > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403
> > > >> -----------------
> > > >> include/video/samsung_fimd.h | 533
> > > >> +++++++++++++++++++++++ 3 files changed, 533 insertions(+), 562
> > > >> deletions(-)
> > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> > > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> > > >> create mode 100644 include/video/samsung_fimd.h
> > > >>
> > > >> +*/
> > > >> +
> > > >> +/*FIMD V8 REG OFFSET */
> > > >> +#define FIMD_V8_VIDTCON0 (0x20010)
> > > >> +#define FIMD_V8_VIDTCON1 (0x20014)
> > > >> +#define FIMD_V8_VIDTCON2 (0x20018)
> > > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> > > >> +#define FIMD_V8_VIDCON1 (0x20004)
> >
> > How about using soc_is_exynos5250()?
> >
> > +#define VIDTCON0 (soc_is_exynos5250() ? \
> > + (0x20010) : (0x10))
> >
> > In this case, the FIMD driver does not need to change.
> > Also, one binary is available.
> >
>
> This would look good indeed, but there are some drawbacks:
> - it would not scale nicely for future SoCs using the new FIMD
I don't think so.
Currently, one clear thing is that only Exynos5250 FIMD has VIDTCON0 offset
with 0x20000.
We cannot know whether future SoCs have VIDTCON0 offset with 0x20000.
Also, VIDTCON offset is not relevant to FIMD version.
There was the case that FIMD version 7 has VIDTCON0 offset with 0x20000.
> - it would add the overhead of checking SoC ID for every access to affected
> registers (at least 1 load, 1 AND, 1 compare, 1 move and 1 conditional OR, so
> 5 instructions in total, possibly even more, as opposed to a single load from
> a variant struct).
I don't think that it's critical.
VIDTCON registers are used for controlling LCD timing values.
These registers are just used for probing and resuming.
It is not used at running time.
Anyway, your point would be considered.
Thank you.
>
> I would stay with the way used in s3c-fb driver, using variant structs
> describing FIMD revisions.
>
> Best regards,
> Tomasz Figa
>
> >
> > Best regards,
> > Jingoo Han
> >
> > > > CC'ed Marek.
> > > >
> > > > To Leela Krishna Amudala,
> > > >
> > > > Don't add these definitions for FIMD_V8_xxx registers, which arenot
> > > > related to current "regs-fb-v4.h>
> > > and regs-fb.h".
> > >
> > > > Just "move" and "merge" regs-fb-v4.h and regs-fb.h to one header file,
> > > > not "add" new definitions. If you want to add these definitions, please
> > > > make new patch for this.>
> > > Will do it in the suggested way,
> > >
> > > > Also, "#define FIMD_V8_xxx" is ugly.
> > > > I think that there is better way.
> > > > Please, find other way.
> > >
> > > I used FIMD_V8_xxx instead of EXYNOS5_FIMD_*, because in future,
> > > there is a possibility that version 8 FIMD can be used in other
> > > application processors also.
> > > Thanks for reviewing the patch.
> > >
> > > Best Wishes,
> > > Leela Krishna.
> > >
> > > >> --
> > > >> 1.7.0.4
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-fbdev"
> > > > in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> > in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-07-31 7:59 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, linux-kernel, Jiri Kosina, Jaya Kumar, linux-fbdev
In-Reply-To: <CANq1E4Tb1Er+O54rN3OteMXqeXAXDr4Z-dvY+RQxZW_NdAfifw@mail.gmail.com>
Hi David,
On Tue, 31 Jul 2012 09:26:07 David Herrmann wrote:
> This is not directly related to this patchset, but did you fix the
> locking issue with hid-core? It is still on my todo-list but I haven't
> gotten around fixing it, yet. However, I plan on fixing it this
> summer, but if picolcd does not require it, it's probably not worth
> the work.
I worked around it with patch 6/7 as in disabling the version check.
The other probing steps don't expect a response from the hardware, thus
are not affected.
But it would definitely be helpful to be able to revert that one.
Understanding and fixing the FB-related misbehavior on quick bind/unbind
seems more important though.
Not related with this, getting a return value from
usbhid_submit_report() as in -EAGAIN or -ENODEV would help in order
to avoid the ugly
hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -19
lines in kernel log and stopping interaction with hardware when it's
gone already or giving it time to consume its queue when there is lots
of data being sent.
Regards,
Bruno
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: David Herrmann @ 2012-07-31 7:26 UTC (permalink / raw)
To: Bruno Prémont
Cc: linux-input, linux-kernel, Jiri Kosina, Jaya Kumar, linux-fbdev
In-Reply-To: <20120730213656.0a9f6d30@neptune.home>
Hi Bruno
On Mon, Jul 30, 2012 at 9:36 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> Hi,
>
> This series updates picoLCD driver:
> - split the driver functions into separate files which get included
> depending on Kconfig selection
> (implementation for CIR using RC_CORE will follow later)
> - drop private framebuffer refcounting in favor of refcounting added
> to fb_info some time ago
> - fix various bugs issues
> - disabled firmware version checking in probe() as it does not work
> anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> [HID: Fix race condition between driver core and ll-driver]
>
> Note: I still get weird behavior on quick unbind/bind sequences
> issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
> support and apparently more specifically fb_defio part of it.
>
> Unfortunately I'm out of ideas as to how to track down the problem which
> shows either as SLAB corruption (detected with SLUB debugging, e.g.
>
> [ 6383.521833] ======================================> [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
> [ 6383.530020] -----------------------------------------------------------------------------
> [ 6383.530020]
> [ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
> [ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
> [ 6383.530020]
> [ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
> [ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
> [ 6383.530020] Redzone cef511d0: bb bb bb bb ....
> [ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
> [ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
> [ 6383.530020] Call Trace:
> [ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
> [ 6383.530020] [<c10bd415>] object_err+0x35/0x40
> [ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
> [ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
> [ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
> [ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
> [ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
> [ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
> [ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
> [ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
> [ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
> [ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
> [ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
> [ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
> [ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
> [ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
> [ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
> [ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
> [ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
> [ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
> [ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
> [ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
> [ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
> [ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
> [ 6383.530020] FIX kmalloc-64: Object at 0xcef51190 not freed
>
> or worse spontaneous reboot of the system without any trace on netconsole or
> serial console.
>
> echo $devid > bind; echo $devid > unbind
> or
> echo $devid > bind; echo $devid > unbind; sleep 0.2; echo $devid > bind; echo $devid > unbind
>
> is sufficient to trigger the above issue while waiting a few seconds between bind and unbind
> shows no sign of trouble.
>
> Suggestions as to how to debug this and fix it are welcome!
This is not directly related to this patchset, but did you fix the
locking issue with hid-core? It is still on my todo-list but I haven't
gotten around fixing it, yet. However, I plan on fixing it this
summer, but if picolcd does not require it, it's probably not worth
the work.
Regards
David
^ permalink raw reply
* Re: [PATCH 1/3] Move FIMD register headers to include/video/
From: Leela Krishna Amudala @ 2012-07-31 7:24 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Jingoo Han, linux-arm-kernel, linux-samsung-soc, dri-devel,
linux-fbdev, ben-linux, inki.dae, kgene.kim, joshi
In-Reply-To: <032201cd6ee5$ad4a0d50$07de27f0$%szyprowski@samsung.com>
Hello Marek,
On Tue, Jul 31, 2012 at 11:58 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hello,
>
> On Tuesday, July 31, 2012 2:48 AM Jingoo Han wrote:
>
>> On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
>> >
>> > Hello Jingoo Han,
>> >
>> > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
>> > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
>> > >>
>> > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
>> > >> to include/video/samsung_fimd.h
>> > >>
>> > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> > >> ---
>> > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
>> > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403 -----------------
>> > >> include/video/samsung_fimd.h | 533 +++++++++++++++++++++++
>> > >> 3 files changed, 533 insertions(+), 562 deletions(-)
>> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
>> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
>> > >> create mode 100644 include/video/samsung_fimd.h
>> > >>
>> > >> +*/
>> > >> +
>> > >> +/*FIMD V8 REG OFFSET */
>> > >> +#define FIMD_V8_VIDTCON0 (0x20010)
>> > >> +#define FIMD_V8_VIDTCON1 (0x20014)
>> > >> +#define FIMD_V8_VIDTCON2 (0x20018)
>> > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
>> > >> +#define FIMD_V8_VIDCON1 (0x20004)
>>
>>
>> How about using soc_is_exynos5250()?
>>
>> +#define VIDTCON0 (soc_is_exynos5250() ? \
>> + (0x20010) : (0x10))
>>
>> In this case, the FIMD driver does not need to change.
>> Also, one binary is available.
>
> Please don't mix two methods of runtime detection. FIMD driver (s3c-fb) already
> has runtime hw detection based on platform device id. Adding such detection for
> exynos5 to DRM FIMD driver should not be a big issue too.
>
I have code ready for DRM-FIMD driver with the above approach (getting
FIMD version by runtime detection)
will post the patches soon along with arch side patches once the arch
changes are ready.
and I'll continue the same name FIMD_V8_xxx for macro names.
> Best regards
> --
> Marek Szyprowski
> Samsung Poland R&D Center
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/3] Move FIMD register headers to include/video/
From: Tomasz Figa @ 2012-07-31 6:37 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc, dri-devel, linux-fbdev, ben-linux, inki.dae,
kgene.kim, joshi, 'Marek Szyprowski'
In-Reply-To: <000a01cd6eb6$2113a050$633ae0f0$%han@samsung.com>
Hi,
On Tuesday 31 of July 2012 at 09:47:57, Jingoo Han wrote:
> On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
> > Hello Jingoo Han,
> >
> > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> > >> to include/video/samsung_fimd.h
> > >>
> > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > >> ---
> > >>
> > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403
> > >> -----------------
> > >> include/video/samsung_fimd.h | 533
> > >> +++++++++++++++++++++++ 3 files changed, 533 insertions(+), 562
> > >> deletions(-)
> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> > >> create mode 100644 include/video/samsung_fimd.h
> > >>
> > >> +*/
> > >> +
> > >> +/*FIMD V8 REG OFFSET */
> > >> +#define FIMD_V8_VIDTCON0 (0x20010)
> > >> +#define FIMD_V8_VIDTCON1 (0x20014)
> > >> +#define FIMD_V8_VIDTCON2 (0x20018)
> > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> > >> +#define FIMD_V8_VIDCON1 (0x20004)
>
> How about using soc_is_exynos5250()?
>
> +#define VIDTCON0 (soc_is_exynos5250() ? \
> + (0x20010) : (0x10))
>
> In this case, the FIMD driver does not need to change.
> Also, one binary is available.
>
This would look good indeed, but there are some drawbacks:
- it would not scale nicely for future SoCs using the new FIMD
- it would add the overhead of checking SoC ID for every access to affected
registers (at least 1 load, 1 AND, 1 compare, 1 move and 1 conditional OR, so
5 instructions in total, possibly even more, as opposed to a single load from
a variant struct).
I would stay with the way used in s3c-fb driver, using variant structs
describing FIMD revisions.
Best regards,
Tomasz Figa
>
> Best regards,
> Jingoo Han
>
> > > CC'ed Marek.
> > >
> > > To Leela Krishna Amudala,
> > >
> > > Don't add these definitions for FIMD_V8_xxx registers, which arenot
> > > related to current "regs-fb-v4.h>
> > and regs-fb.h".
> >
> > > Just "move" and "merge" regs-fb-v4.h and regs-fb.h to one header file,
> > > not "add" new definitions. If you want to add these definitions, please
> > > make new patch for this.>
> > Will do it in the suggested way,
> >
> > > Also, "#define FIMD_V8_xxx" is ugly.
> > > I think that there is better way.
> > > Please, find other way.
> >
> > I used FIMD_V8_xxx instead of EXYNOS5_FIMD_*, because in future,
> > there is a possibility that version 8 FIMD can be used in other
> > application processors also.
> > Thanks for reviewing the patch.
> >
> > Best Wishes,
> > Leela Krishna.
> >
> > >> --
> > >> 1.7.0.4
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-fbdev"
> > > in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH 1/3] Move FIMD register headers to include/video/
From: Marek Szyprowski @ 2012-07-31 6:28 UTC (permalink / raw)
To: 'Jingoo Han', 'Leela Krishna Amudala'
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, kgene.kim, joshi
In-Reply-To: <000a01cd6eb6$2113a050$633ae0f0$%han@samsung.com>
Hello,
On Tuesday, July 31, 2012 2:48 AM Jingoo Han wrote:
> On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
> >
> > Hello Jingoo Han,
> >
> > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> > >>
> > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> > >> to include/video/samsung_fimd.h
> > >>
> > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > >> ---
> > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403 -----------------
> > >> include/video/samsung_fimd.h | 533 +++++++++++++++++++++++
> > >> 3 files changed, 533 insertions(+), 562 deletions(-)
> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> > >> create mode 100644 include/video/samsung_fimd.h
> > >>
> > >> +*/
> > >> +
> > >> +/*FIMD V8 REG OFFSET */
> > >> +#define FIMD_V8_VIDTCON0 (0x20010)
> > >> +#define FIMD_V8_VIDTCON1 (0x20014)
> > >> +#define FIMD_V8_VIDTCON2 (0x20018)
> > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> > >> +#define FIMD_V8_VIDCON1 (0x20004)
>
>
> How about using soc_is_exynos5250()?
>
> +#define VIDTCON0 (soc_is_exynos5250() ? \
> + (0x20010) : (0x10))
>
> In this case, the FIMD driver does not need to change.
> Also, one binary is available.
Please don't mix two methods of runtime detection. FIMD driver (s3c-fb) already
has runtime hw detection based on platform device id. Adding such detection for
exynos5 to DRM FIMD driver should not be a big issue too.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
^ permalink raw reply
* RE: [PATCH 1/3] Move FIMD register headers to include/video/
From: Jingoo Han @ 2012-07-31 0:47 UTC (permalink / raw)
To: 'Leela Krishna Amudala'
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, kgene.kim, joshi, 'Marek Szyprowski',
'Jingoo Han'
In-Reply-To: <CAL1wa8e7Joghf9S=SgbQLJFWcYOrP5uK6qYXsV_N6rX1MVEALg@mail.gmail.com>
On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
>
> Hello Jingoo Han,
>
> On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> >>
> >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> >> to include/video/samsung_fimd.h
> >>
> >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >> ---
> >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403 -----------------
> >> include/video/samsung_fimd.h | 533 +++++++++++++++++++++++
> >> 3 files changed, 533 insertions(+), 562 deletions(-)
> >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> >> create mode 100644 include/video/samsung_fimd.h
> >>
> >> +*/
> >> +
> >> +/*FIMD V8 REG OFFSET */
> >> +#define FIMD_V8_VIDTCON0 (0x20010)
> >> +#define FIMD_V8_VIDTCON1 (0x20014)
> >> +#define FIMD_V8_VIDTCON2 (0x20018)
> >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> >> +#define FIMD_V8_VIDCON1 (0x20004)
How about using soc_is_exynos5250()?
+#define VIDTCON0 (soc_is_exynos5250() ? \
+ (0x20010) : (0x10))
In this case, the FIMD driver does not need to change.
Also, one binary is available.
Best regards,
Jingoo Han
> >
> >
> > CC'ed Marek.
> >
> > To Leela Krishna Amudala,
> >
> > Don't add these definitions for FIMD_V8_xxx registers, which arenot related to current "regs-fb-v4.h
> and regs-fb.h".
> > Just "move" and "merge" regs-fb-v4.h and regs-fb.h to one header file, not "add" new definitions.
> > If you want to add these definitions, please make new patch for this.
> >
>
> Will do it in the suggested way,
>
>
> > Also, "#define FIMD_V8_xxx" is ugly.
> > I think that there is better way.
> > Please, find other way.
> >
> >
>
> I used FIMD_V8_xxx instead of EXYNOS5_FIMD_*, because in future,
> there is a possibility that version 8 FIMD can be used in other
> application processors also.
> Thanks for reviewing the patch.
>
> Best Wishes,
> Leela Krishna.
>
> >> --
> >> 1.7.0.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Stephen Warren @ 2012-07-30 22:45 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
In-Reply-To: <1343390750-3642-2-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 07/27/2012 06:05 AM, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
> diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt
> +Sequences Format
> +----------------
> +Power sequences are a series of sequential steps during which an action is
> +performed on a resource. The supported resources so far are:
> +- GPIOs
> +- Regulators
> +- PWMs
> +
> +Each step designates a resource and the following parameters:
> +- Whether the step should enable or disable the resource,
At the highest level, i.e. what's being describe here, I wouldn't even
talk about enable/disable, but rather the action to perform on the
resource. The set of legal actions may be specific to the type of
resource in question. Admittedly enable/disable are common though.
> +- Delay to wait before performing the action,
> +- Delay to wait after performing the action.
I don't see a need to have a delay both before and after an action;
except at the start of the sequence, step n's post-delay is at the same
point in the sequence as step n+1's pre-delay. Perhaps make a "delay"
step type?
> +Both new resources and parameters can be introduced, but the goal is of course
> +to keep things as simple and compact as possible.
> +The platform data is a simple array of platform_power_seq_step instances, each
Rather than jumping straight into platform data here, I'd expect an
enumeration of legal resource types, and what actions can be performed
on each, followed by a description of a sequence (very simply, just a
list of actions and their parameters). This could be followed by a
section describing the mapping of the abstract concepts to concrete
platform data representation (and concrete device tree representation).
> +instance describing a step. The type as well as one of id or gpio members
> +(depending on the type) must be specified. The last step must be of type
> +POWER_SEQ_STOP.
I'd certainly suggest having a step count rather than a sentinel value
in the list.
> Regulator and PWM resources are identified by name. GPIO are
> +identified by number.
That's a little implementation-specific. I guess it's entirely true for
a platform data representation, but not when mapping this into device tree.
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function takes care of resolving the resources as
> +they are met in the sequence and to allocate them if needed:
> +
> +power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> + platform_power_seq *pseq);
> +
> +You will need an instance of power_seq_resources to keep track of the resources
> +that are already allocated. On success, the function returns a devm allocated
> +resolved sequence that is ready to be passed to power_seq_run(). In case of
> +failure, and error code is returned.
If the result is devm-allocated, the function probably should be named
devm_power_seq_build().
I wonder if using the same structure/array as input and output would
simplify the API; the platform data would fill in the fields mentioned
above, and power_seq_build() would parse those, then set other fields in
the same structs to the looked-up handle values?
> +Finally, some resources that cannot be allocated through devm need to be freed
> +manually. Therefore, be sure to call power_seq_free_resources() in your device
> +remove function:
> +
> +void power_seq_free_resources(power_seq_resources *ress);
You can make a custom devm free routine for the power_seq_resources
itself, so the overall free'ing of the content can be triggered by devm,
but the free'ing function can then call whatever non-devm APIs it wants
for the non-devm-allocated members.
> +Device tree
> +-----------
> +All the same, power sequences can be encoded as device tree nodes. The following
> +properties and nodes are equivalent to the platform data defined previously:
> +
> + power-supply = <&mydevice_reg>;
> + enable-gpio = <&gpio 6 0>;
> +
> + power-on-sequence {
> + regulator@0 {
As Thierry mentioned, the step nodes should be named for the type of
object they are (a "step") not the type or name of resource they act
upon ("regulator" or "gpio").
If the nodes have a unit address (i.e. end in "@n"), which they will
have to if all named "step" and there's more than one of them, then they
will need a matching reg property. Equally, the parent node will need
#address-cells and #size-cells too. So, the last couple lines would be:
power-on-sequence {
#address-cells = <1>;
#size-cells = <0>;
step@0 {
reg = <0>;
> + id = "power";
"id" is usually a name or identifier. I think you mean "type" or perhaps
"action" here:
type = "regulator";
action = "enable";
or:
action = "enable-regulator";
I wish we had integer constants in DT so we didn't have to do all this
with strings. Sigh.
> + enable;
> + post-delay = <10>;
> + };
> + gpio@1 {
> + id = "enable-gpio";
> + enable;
> + };
> + };
> +
> +Note that first, the phandles of the regulator and gpio used in the sequences
> +are defined as properties. Then the sequence references them through the id
> +property of every step. The name of sub-properties defines the type of the step.
> +Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
> +sequentially.
Oh I see. That's a little confusing. Why not just reference the relevant
resources directly in each step; something more like:
gpio@1 {
action = "enable-gpio";
gpio = <&gpio 1 0>;
};
I guess that might make parsing/building a little harder, since you'd
have to detect when you'd already done gpio_request() on a given GPIO
and not repeat it or something like that, but to me this makes the DT a
lot easier to comprehend.
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Stephen Warren @ 2012-07-30 22:26 UTC (permalink / raw)
To: Rob Herring
Cc: Alexandre Courbot, Stephen Warren, Thierry Reding, Simon Glass,
Grant Likely, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
In-Reply-To: <5016ABDD.5010809-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 07/30/2012 09:44 AM, Rob Herring wrote:
> On 07/27/2012 07:05 AM, Alexandre Courbot wrote:
>> Some device drivers (panel backlights especially) need to follow precise
>> sequences for powering on and off, involving gpios, regulators, PWMs
>> with a precise powering order and delays to respect between each steps.
>> These sequences are board-specific, and do not belong to a particular
>> driver - therefore they have been performed by board-specific hook
>> functions to far.
>>
>> With the advent of the device tree and of ARM kernels that are not
>> board-tied, we cannot rely on these board-specific hooks anymore but
>> need a way to implement these sequences in a portable manner. This patch
>> introduces a simple interpreter that can execute such power sequences
>> encoded either as platform data or within the device tree.
>>
>
> Why not? We'll always have some amount of board code. The key is to
> limit parts that are just data. I'm not sure this is something that
> should be in devicetree.
>
> Perhaps what is needed is a better way to hook into the driver like
> notifiers?
I would answer that by asking the reverse question - why should we have
to put some data in DT, and some data into board files still?
I'd certainly argue that the sequence of which GPIOs/regulators/PWMs to
manipulate is just data.
To be honest, if we're going to have to put some parts of a board's
configuration into board files anyway, then the entirety of DT seems
useless; I'd far rather see all the configuration in one cohesive place
than arbitrarily split into two/n different locations - that would make
everything harder to maintain.
^ permalink raw reply
* Re: Gethering power management/policy hw drivers under drivers/power/? (Re: [RFC][PATCH v3 1/3] runt
From: Rafael J. Wysocki @ 2012-07-30 20:59 UTC (permalink / raw)
To: Anton Vorontsov
Cc: Alex Courbot, Jean Pihet, Greg Kroah-Hartman, David Woodhouse,
Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Liam Girdwood, MyungJoo Ham, linux-pm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120730024049.GA10442@lizard>
On Monday, July 30, 2012, Anton Vorontsov wrote:
> On Mon, Jul 30, 2012 at 10:51:42AM +0900, Alex Courbot wrote:
> [...]
> > On the other hand I have just noticed that the apparently unrelated
> > Adaptive Voltage Scaling driver just appeared in drivers/power/avs.
> > So if Anton and David are ok with this, maybe I could put the power
> > sequences code in its own subdirectory within drivers/power.
>
> Well, currently drivers/power/ is indeed just for power supply class
> subsystem and drivers. But if the trend is to gather power management
> ("policy") stuff under one directory, i.e.
>
> drivers/
> power/
> supplies/ <- former "power supply class and drivers"
> regulators/
> idle/
> cpuidle/
> cpufreq/
> devfreq/
> avs/
> ...
>
> That would probably make sense, we could easily see the big picture.
> But if we're not going to do this long-term,
Yes, we may do this eventually, but surely not right now.
Thanks,
Rafael
^ permalink raw reply
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