* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Greg KH @ 2013-02-04 1:14 UTC (permalink / raw)
To: Alexander Holler
Cc: Andrew Morton, linux-kernel, linux-fbdev,
Florian Tobias Schandinat, Bernie Thompson, Steve Glendinning,
stable
In-Reply-To: <5108329E.2050802@ahsoftware.de>
On Tue, Jan 29, 2013 at 09:35:42PM +0100, Alexander Holler wrote:
> Am 29.01.2013 16:51, schrieb Alexander Holler:
> >Am 29.01.2013 12:11, schrieb Alexander Holler:
> >
> >>
> >>To explain the problem on shutdown a bit further, I think the following
> >>happens (usb and driver are statically linked and started by the kernel):
> >>
> >>shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
> >>for a kill or an urb which it doesn't get.
> >
> >Having a second look at what I've written above, I'm not even sure if
> >the kernel sends one or more fatal signals on shutdown at all. I've just
> >assumed it because otherwise down_interruptible() wouldn't have worked
> >before (it would have stalled on shutdown too (if an urb got missed),
> >not only on disconnect).
> >
> >Sounds like an interesting question I should read about (if/when fatal
> >signals are issued by the kernel). ;)
> >
> >>Maybe the sequence is different if the usb-stack and udlfb are used as a
> >>module and/or udlfb is used only for X/fb. I'm not sure what actually
> >>does shut down the usb-stack in such a case, but maybe more than one
> >>kill signal might be thrown around.
>
> If anyone still follows my monologue: The question was interesting
> enough that I couldn't resist for long. ;)
>
> (all pasted => format broken)
>
> In drivers/tty/sysrq.c there is
>
> ------
> static void send_sig_all(int sig)
> {
> struct task_struct *p;
>
> read_lock(&tasklist_lock);
> for_each_process(p) {
> if (p->flags & PF_KTHREAD)
> continue;
> if (is_global_init(p))
> continue;
>
> do_send_sig_info(sig, SEND_SIG_FORCED, p, true);
> }
> read_unlock(&tasklist_lock);
> }
>
> static void sysrq_handle_term(int key)
> {
> send_sig_all(SIGTERM);
> console_loglevel = 8;
> }
>
> (...)
>
> static void sysrq_handle_kill(int key)
> {
> send_sig_all(SIGKILL);
> console_loglevel = 8;
> }
> ------
>
> Now I've done some learning by doing (kernel 3.7.5 + some patches):
>
> ------
> diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
> index df249f3..db8a86c 100644
> --- a/drivers/video/udlfb.c
> +++ b/drivers/video/udlfb.c
> @@ -1876,14 +1876,18 @@ static void dlfb_free_urb_list(struct dlfb_data
> *dev)
> unsigned long flags;
>
> pr_notice("Freeing all render urbs\n");
> + if (current->flags & PF_KTHREAD)
> + pr_info("AHO: I'm a kernel thread\n");
>
> /* keep waiting and freeing, until we've got 'em all */
> while (count--) {
>
> /* Timeout likely occurs at disconnect (resulting in a
> leak) */
> ret = down_timeout_killable(&dev->urbs.limit_sem,
> FREE_URB_TIMEOUT);
> - if (ret)
> + if (ret) {
> + pr_info("AHO: ret %d\n", ret);
> break;
> + }
>
> spin_lock_irqsave(&dev->urbs.lock, flags);
> ------
>
> Now I've disconnected the display. And, as send_sig_all() already
> suggests, the result was (besides discovering an oops in
> call_timer_fn.isra (once)):
>
> ------
> [ 120.963010] udlfb: AHO: I'm a kernel thread
> [ 122.957024] udlfb: AHO: ret -62
> ------
> (-62 is -ETIME)
>
> So, if the above down_timeout_killable() is only
> down_interruptible(), as in kernel 3.7.5, the box would not
> shutdown afterwards, because on shutdown no signal would be send to
> that kernel-thread which called dlfb_free_urb_list().
>
> A last note: dlfb_usb_disconnect() (thus dlfb_free_urb_list()) isn't
> called on shutdown if the device would still be connected. So the
> problem only might happen, if the screen will be disconnected before
> shutdown (and an urb gets missed). So the subject of my patch is
> correct. ;)
Yes, we don't disconnect all devices from the USB bus on shutdown
because, I think, we didn't tear down all of the PCI devices originally,
so your USB bus never knew it was going to be shutdown.
This is how things have always worked, and shutting down PCI devices in
the past have been known to cause problems. I think. I vaguely
remember some issues when I tried to do this 10+ years or so ago in the
2.5 kernel days, but I could be totally wrong given that I can't
remember what I was working on last month at times...
So you are right in that your driver will wait for forever for a
disconnect() to happen, as it will never be called. I don't understand
the problem that this is causing when it happens. What's wrong with
udlfb that having the cpu suddently reset as the powerdown happened
without it knowing about it?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] video: add ili922x lcd driver
From: Jingoo Han @ 2013-02-04 5:45 UTC (permalink / raw)
To: 'Anatolij Gustschin'
Cc: 'Andrew Morton', 'LKML', linux-fbdev,
'Stefano Babic', 'Richard Purdie',
'Florian Tobias Schandinat', 'Jingoo Han'
In-Reply-To: <1359729703-24915-1-git-send-email-agust@denx.de>
On Friday, February 01, 2013 11:42 PM, Anatolij Gustschin wrote
CC'ed Andrew Morton
>
> From: Stefano Babic <sbabic@denx.de>
>
> Add LCD driver for Ilitek ILI9221/ILI9222 controller.
>
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> ---
> drivers/video/backlight/Kconfig | 7 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/ili922x.c | 586 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 594 insertions(+), 0 deletions(-)
> create mode 100644 drivers/video/backlight/ili922x.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 765a945..97b4672 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -59,6 +59,13 @@ config LCD_LTV350QV
>
> The LTV350QV panel is present on all ATSTK1000 boards.
>
> +config LCD_ILI922X
> + tristate "ILI Technology ILI9221/ILI9222 support"
> + depends on SPI
> + help
> + If you have a panel based on the ILI9221/9222 controller
> + chip then say y to include a driver for it.
> +
> config LCD_ILI9320
> tristate "ILI Technology ILI9320 controller support"
> depends on SPI
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index e7ce729..3cfd901 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_LCD_HP700) += jornada720_lcd.o
> obj-$(CONFIG_LCD_L4F00242T03) += l4f00242t03.o
> obj-$(CONFIG_LCD_LMS283GF05) += lms283gf05.o
> obj-$(CONFIG_LCD_LTV350QV) += ltv350qv.o
> +obj-$(CONFIG_LCD_ILI922X) += ili922x.o
> obj-$(CONFIG_LCD_ILI9320) += ili9320.o
> obj-$(CONFIG_LCD_PLATFORM) += platform_lcd.o
> obj-$(CONFIG_LCD_VGG2432A4) += vgg2432a4.o
> diff --git a/drivers/video/backlight/ili922x.c b/drivers/video/backlight/ili922x.c
> new file mode 100644
> index 0000000..18c33df
> --- /dev/null
> +++ b/drivers/video/backlight/ili922x.c
> @@ -0,0 +1,586 @@
> +/*
> + * (C) Copyright 2008
> + * Stefano Babic, DENX Software Engineering, sbabic@denx.de.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
Please remove this comment. It is hard to keep track of the address of
Free Software Foundation.
Also, above mentioned address is not the same with the current address.
> + *
> + * This driver implements a lcd device for the ILITEK 922x display
> + * controller. The interface to the display is SPI and the display's
> + * memory is cyclically updated
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/fb.h>
> +#include <linux/init.h>
> +#include <linux/lcd.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
Would you order inclusions of <linux/xxx.h> according to alphabetical
ordering, for readability?
> +
> +/* Register offset, see manual section 8.2 */
> +#define REG_START_OSCILLATION 0x00
> +#define REG_DRIVER_CODE_READ 0x00
> +#define REG_DRIVER_OUTPUT_CONTROL 0x01
> +#define REG_LCD_AC_DRIVEING_CONTROL 0x02
> +#define REG_ENTRY_MODE 0x03
> +#define REG_COMPARE_1 0x04
> +#define REG_COMPARE_2 0x05
> +#define REG_DISPLAY_CONTROL_1 0x07
> +#define REG_DISPLAY_CONTROL_2 0x08
> +#define REG_DISPLAY_CONTROL_3 0x09
> +#define REG_FRAME_CYCLE_CONTROL 0x0B
> +#define REG_EXT_INTF_CONTROL 0x0C
> +#define REG_POWER_CONTROL_1 0x10
> +#define REG_POWER_CONTROL_2 0x11
> +#define REG_POWER_CONTROL_3 0x12
> +#define REG_POWER_CONTROL_4 0x13
> +#define REG_RAM_ADDRESS_SET 0x21
> +#define REG_WRITE_DATA_TO_GRAM 0x22
> +#define REG_RAM_WRITE_MASK1 0x23
> +#define REG_RAM_WRITE_MASK2 0x24
> +#define REG_GAMMA_CONTROL_1 0x30
> +#define REG_GAMMA_CONTROL_2 0x31
> +#define REG_GAMMA_CONTROL_3 0x32
> +#define REG_GAMMA_CONTROL_4 0x33
> +#define REG_GAMMA_CONTROL_5 0x34
> +#define REG_GAMMA_CONTROL_6 0x35
> +#define REG_GAMMA_CONTROL_7 0x36
> +#define REG_GAMMA_CONTROL_8 0x37
> +#define REG_GAMMA_CONTROL_9 0x38
> +#define REG_GAMMA_CONTROL_10 0x39
> +#define REG_GATE_SCAN_CONTROL 0x40
> +#define REG_VERT_SCROLL_CONTROL 0x41
> +#define REG_FIRST_SCREEN_DRIVE_POS 0x42
> +#define REG_SECOND_SCREEN_DRIVE_POS 0x43
> +#define REG_RAM_ADDR_POS_H 0x44
> +#define REG_RAM_ADDR_POS_V 0x45
> +#define REG_OSCILLATOR_CONTROL 0x4F
> +#define REG_GPIO 0x60
> +#define REG_OTP_VCM_PROGRAMMING 0x61
> +#define REG_OTP_VCM_STATUS_ENABLE 0x62
> +#define REG_OTP_PROGRAMMING_ID_KEY 0x65
> +
> +/*
> + * maximum frequency for register access
> + * (not for the GRAM access)
> + */
> +#define ILITEK_MAX_FREQ_REG 4000000
> +
> +/*
> + * Device ID as found in the datasheet (supports 9221 and 9222)
> + */
> +#define ILITEK_DEVICE_ID 0x9220
> +#define ILITEK_DEVICE_ID_MASK 0xFFF0
> +
> +/* Last two bits in the START BYTE */
> +#define START_RS_INDEX 0
> +#define START_RS_REG 1
> +#define START_RW_WRITE 0
> +#define START_RW_READ 1
> +
> +/**
> + * START_BYTE(id, rs, rw)
> + *
> + * Set the start byte according to the required operation.
> + * The start byte is defined as:
> + * ----------------------------------
> + * | 0 | 1 | 1 | 1 | 0 | ID | RS | RW |
> + * ----------------------------------
> + * @id: display's id as set by the manufacturer
> + * @rs: operation type bit, one of:
> + * - START_RS_INDEX set the index register
> + * - START_RS_REG write/read registers/GRAM
> + * @rw: read/write operation
> + * - START_RW_WRITE write
> + * - START_RW_READ read
> + */
> +#define START_BYTE(id, rs, rw) \
> + (0x70 | (((id) & 0x01) << 2) | (((rs) & 0x01) << 1) | ((rw) & 0x01))
> +
> +/**
> + * CHECK_FREQ_REG(spi_device s, spi_transfer x) - Check the frequency
> + * for the SPI transfer. According to the datasheet, the controller
> + * accept higher frequency for the GRAM transfer, but it requires
> + * lower frequency when the registers are read/written.
> + * The macro sets the frequency in the spi_transfer structure if
> + * the frequency exceeds the maximum value.
> + */
> +#define CHECK_FREQ_REG(s, x) \
> + do { \
> + if (s->max_speed_hz > ILITEK_MAX_FREQ_REG) \
> + ((struct spi_transfer *)x)->speed_hz = \
> + ILITEK_MAX_FREQ_REG; \
> + } while (0)
> +
> +#define CMD_BUFSIZE 16
> +
> +#define POWER_IS_ON(pwr) ((pwr) <= FB_BLANK_NORMAL)
> +
> +#define set_tx_byte(b) (tx_invert ? ~(b) : b)
> +
> +/**
> + * ili922x_id - id as set by manufacturer
> + */
> +static int ili922x_id = 1;
> +module_param(ili922x_id, int, 0);
> +
> +static int tx_invert;
> +module_param(tx_invert, int, 0);
> +
> +/**
> + * driver's private structure
> + */
> +struct ili922x {
> + struct spi_device *spi;
> + struct lcd_device *ld;
> + int power;
> +};
> +
> +#define NUM_DUMMY_BYTES 1
> +static void send_dummy(struct spi_device *spi)
> +{
> + struct spi_message m;
> + struct spi_transfer xfer;
> + unsigned char tbuf[CMD_BUFSIZE];
> + unsigned char rbuf[CMD_BUFSIZE];
> + int ret = 0, i;
> +
> + return;
> +
> + memset(&xfer, 0, sizeof(struct spi_transfer));
> + spi_message_init(&m);
> + xfer.tx_buf = tbuf;
> + xfer.rx_buf = rbuf;
> + xfer.cs_change = 1;
> + CHECK_FREQ_REG(spi, &xfer);
> +
> + for (i = 0; i < NUM_DUMMY_BYTES; i++)
> + tbuf[i] = set_tx_byte(0xFF);
> +
> + xfer.bits_per_word = 8;
> + xfer.len = NUM_DUMMY_BYTES;
> + spi_message_add_tail(&xfer, &m);
> + ret = spi_sync(spi, &m);
> +
> + udelay(10);
How about replacing udelay() with usleep_range()?
> +}
> +
> +/**
> + * read_status - read status register from display
> + * @spi: spi device
> + */
> +static u16 read_status(struct spi_device *spi)
> +{
> + struct spi_message m;
Would you replace 'm' with 'msg' or 'message' for the readability?
It's too short, even though 'm' is used in include/linux/spi/spi.h.
struct spi_message msg;
> + struct spi_transfer xfer;
> + unsigned char tbuf[CMD_BUFSIZE];
> + unsigned char rbuf[CMD_BUFSIZE];
> + int ret = 0, i;
> +
> + send_dummy(spi);
> +
> + memset(&xfer, 0, sizeof(struct spi_transfer));
> + spi_message_init(&m);
> + xfer.tx_buf = tbuf;
> + xfer.rx_buf = rbuf;
> + xfer.cs_change = 1;
> + CHECK_FREQ_REG(spi, &xfer);
> +
> + tbuf[0] = set_tx_byte(START_BYTE(ili922x_id, START_RS_INDEX,
> + START_RW_READ));
> + for (i = 1; i < 4; i++)
> + tbuf[i] = set_tx_byte(0); /* dummy */
> +
> + xfer.bits_per_word = 8;
> + xfer.len = 4;
> + spi_message_add_tail(&xfer, &m);
> + ret = spi_sync(spi, &m);
> +
> + return (rbuf[2] << 8) + rbuf[3];
> +}
> +
> +/**
> + * read_reg - read register from display
> + * @spi: spi device
> + * @reg: offset of the register to be read
> + * @rx: output value
> + */
> +static int read_reg(struct spi_device *spi, u8 reg, u16 *rx)
> +{
> + struct spi_message m;
> + struct spi_transfer xfer_regindex, xfer_regvalue;
> + unsigned char tbuf[CMD_BUFSIZE];
> + unsigned char rbuf[CMD_BUFSIZE];
> + int ret = 0, len = 0, i, send_bytes;
> +
> + send_dummy(spi);
> +
> + memset(&xfer_regindex, 0, sizeof(struct spi_transfer));
> + memset(&xfer_regvalue, 0, sizeof(struct spi_transfer));
> + spi_message_init(&m);
> + xfer_regindex.tx_buf = tbuf;
> + xfer_regindex.rx_buf = rbuf;
> + xfer_regindex.cs_change = 1;
> + CHECK_FREQ_REG(spi, &xfer_regindex);
> +
> + tbuf[0] = set_tx_byte(START_BYTE(ili922x_id, START_RS_INDEX,
> + START_RW_WRITE));
> + tbuf[1] = set_tx_byte(0);
> + tbuf[2] = set_tx_byte(reg);
> + xfer_regindex.bits_per_word = 8;
> + len = xfer_regindex.len = 3;
> + spi_message_add_tail(&xfer_regindex, &m);
> +
> + send_bytes = len;
> +
> + tbuf[len++] = set_tx_byte(START_BYTE(ili922x_id, START_RS_REG,
> + START_RW_READ));
> + for (i = len; i < CMD_BUFSIZE; i++)
> + tbuf[i] = set_tx_byte(0); /* dummy */
> +
> + xfer_regvalue.cs_change = 1;
> + xfer_regvalue.len = 4;
I don't understand why length 4 is necessary.
In my opinion, length 3 seems to be enough.
- tbuf[4] is used for sending 'START_BYTE(ili922x_id, START_RS_REG, START_RW_READ)'.
- rbuf[5] and rbuf[6] are used for receiving value as below.
*rx = (rbuf[1 + send_bytes] << 8) + rbuf[2 + send_bytes];
However, tbuf[7] or rbuf[7] seems to be unnecessary.
If I'm wrong, please let me know kindly.
> + xfer_regvalue.tx_buf = &tbuf[send_bytes];
> + xfer_regvalue.rx_buf = &rbuf[send_bytes];
> + CHECK_FREQ_REG(spi, &xfer_regvalue);
> +
> + spi_message_add_tail(&xfer_regvalue, &m);
> + ret = spi_sync(spi, &m);
> + if (ret < 0) {
> + dev_dbg(&spi->dev, "Error sending SPI message 0x%x", ret);
> + return ret;
> + }
> +
> + *rx = (rbuf[1 + send_bytes] << 8) + rbuf[2 + send_bytes];
> + return 0;
> +}
> +
> +/**
> + * write_reg - write a controller register
> + * @spi: struct spi_device *
> + * @reg: offset of the register to be written
> + * @value: value to be written
> + */
> +static int write_reg(struct spi_device *spi, u8 reg, u16 value)
> +{
> + struct spi_message m;
> + struct spi_transfer xfer_regindex, xfer_regvalue;
> + unsigned char tbuf[CMD_BUFSIZE];
> + unsigned char rbuf[CMD_BUFSIZE];
> + int ret = 0, len = 0;
> +
> + send_dummy(spi);
> +
> + memset(&xfer_regindex, 0, sizeof(struct spi_transfer));
> + memset(&xfer_regvalue, 0, sizeof(struct spi_transfer));
> +
> + spi_message_init(&m);
> + xfer_regindex.tx_buf = tbuf;
> + xfer_regindex.rx_buf = rbuf;
> + xfer_regindex.cs_change = 1;
> + CHECK_FREQ_REG(spi, &xfer_regindex);
> +
> + tbuf[0] = set_tx_byte(START_BYTE(ili922x_id, START_RS_INDEX,
> + START_RW_WRITE));
> + tbuf[1] = set_tx_byte(0);
> + tbuf[2] = set_tx_byte(reg);
> + xfer_regindex.bits_per_word = 8;
> + xfer_regindex.len = 3;
> + spi_message_add_tail(&xfer_regindex, &m);
> +
> + ret = spi_sync(spi, &m);
> +
> + send_dummy(spi);
> +
> + spi_message_init(&m);
> + len = 0;
> + tbuf[0] = set_tx_byte(START_BYTE(ili922x_id, START_RS_REG,
> + START_RW_WRITE));
> + tbuf[1] = set_tx_byte((value & 0xFF00) >> 8);
> + tbuf[2] = set_tx_byte(value & 0x00FF);
> +
> + xfer_regvalue.cs_change = 1;
> + xfer_regvalue.len = 3;
> + xfer_regvalue.tx_buf = tbuf;
> + xfer_regvalue.rx_buf = rbuf;
> + CHECK_FREQ_REG(spi, &xfer_regvalue);
> +
> + spi_message_add_tail(&xfer_regvalue, &m);
> +
> + ret = spi_sync(spi, &m);
> + if (ret < 0) {
> + dev_err(&spi->dev, "Error sending SPI message 0x%x", ret);
> + return ret;
> + }
> + return 0;
> +}
> +
> +#ifdef DEBUG
> +/**
> + * ili922x_reg_dump - dump all registers
> + */
> +static void ili922x_reg_dump(struct spi_device *spi)
> +{
> + u8 reg;
> + u16 rx;
> +
> + pr_info("ILI922x configuration registers:\n");
Please replace pr_info() with dev_info() as below.
dev_err(&spi->dev, "ILI922x configuration registers:\n");
> + for (reg = REG_START_OSCILLATION;
> + reg <= REG_OTP_PROGRAMMING_ID_KEY; reg++) {
> + read_reg(spi, reg, &rx);
> + pr_info("reg @ 0x%02X: 0x%04X\n", reg, rx);
Same as above.
> + }
> +}
> +#else
> +static inline void ili922x_reg_dump(struct spi_device *spi) {}
> +#endif
> +
> +/**
> + * set_write_to_gram_reg - initialize the display to write the GRAM
> + * @spi: spi device
> + */
> +static void set_write_to_gram_reg(struct spi_device *spi)
> +{
> + struct spi_message m;
> + struct spi_transfer xfer;
> + unsigned char tbuf[CMD_BUFSIZE];
> +
> + memset(&xfer, 0, sizeof(struct spi_transfer));
> +
> + spi_message_init(&m);
> + xfer.tx_buf = tbuf;
> + xfer.rx_buf = NULL;
> + xfer.cs_change = 1;
> +
> + tbuf[0] = START_BYTE(ili922x_id, START_RS_INDEX, START_RW_WRITE);
> + tbuf[1] = 0;
> + tbuf[2] = REG_WRITE_DATA_TO_GRAM;
> +
> + xfer.bits_per_word = 8;
> + xfer.len = 3;
> + spi_message_add_tail(&xfer, &m);
> + spi_sync(spi, &m);
> +}
> +
> +/**
> + * ili922x_poweron - turn the display on
> + * @spi: spi device
> + *
> + * The sequence to turn on the display is taken from
> + * the datasheet and/or the example code provided by the
> + * manufacturer.
> + */
> +static int ili922x_poweron(struct spi_device *spi)
> +{
> + int ret = 0;
Initialization is not necessary.
Just declare it as below:
int ret;
> +
> + /* Power on */
> + ret = write_reg(spi, REG_POWER_CONTROL_1, 0x0000);
> + mdelay(10);
> + ret += write_reg(spi, REG_POWER_CONTROL_2, 0x0000);
> + ret += write_reg(spi, REG_POWER_CONTROL_3, 0x0000);
> + mdelay(40);
> + ret += write_reg(spi, REG_POWER_CONTROL_4, 0x0000);
> + mdelay(40);
> + ret += write_reg(spi, 0x56, 0x080F);
Would you replace this hard-coded address with the bit definition?
> + ret += write_reg(spi, REG_POWER_CONTROL_1, 0x4240);
> + mdelay(10);
> + ret += write_reg(spi, REG_POWER_CONTROL_2, 0x0000);
> + ret += write_reg(spi, REG_POWER_CONTROL_3, 0x0014);
> + mdelay(40);
> + ret += write_reg(spi, REG_POWER_CONTROL_4, 0x1319);
> + mdelay(40);
How about replacing mdelay() with msleep()?
> +
> + return ret;
> +}
> +
> +/**
> + * ili922x_poweroff - turn the display off
> + * @spi: spi device
> + */
> +static int ili922x_poweroff(struct spi_device *spi)
> +{
> + int ret = 0;
Initialization is not necessary.
Just declare it as below:
int ret;
> +
> + /* Power off */
> + ret = write_reg(spi, REG_POWER_CONTROL_1, 0x0000);
> + mdelay(10);
> + ret += write_reg(spi, REG_POWER_CONTROL_2, 0x0000);
> + ret += write_reg(spi, REG_POWER_CONTROL_3, 0x0000);
> + mdelay(40);
> + ret += write_reg(spi, REG_POWER_CONTROL_4, 0x0000);
> + mdelay(40);
Same as above.
> +
> + return ret;
> +}
> +
> +/**
> + * ili922x_display_init - initialize the display by setting
> + * the configuration registers
> + * @spi: spi device
> + */
> +static void ili922x_display_init(struct spi_device *spi)
> +{
> + write_reg(spi, REG_START_OSCILLATION, 1);
> + mdelay(10);
Same as above.
> + write_reg(spi, REG_DRIVER_OUTPUT_CONTROL, 0x691B);
> + write_reg(spi, REG_LCD_AC_DRIVEING_CONTROL, 0x0700);
> + write_reg(spi, REG_ENTRY_MODE, 0x1030);
> + write_reg(spi, REG_COMPARE_1, 0x0000);
> + write_reg(spi, REG_COMPARE_2, 0x0000);
> + write_reg(spi, REG_DISPLAY_CONTROL_1, 0x0037);
> + write_reg(spi, REG_DISPLAY_CONTROL_2, 0x0202);
> + write_reg(spi, REG_DISPLAY_CONTROL_3, 0x0000);
> + write_reg(spi, REG_FRAME_CYCLE_CONTROL, 0x0000);
> +
> + /* Set RGB interface */
> + write_reg(spi, REG_EXT_INTF_CONTROL, 0x0110);
> +
> + ili922x_poweron(spi);
> +
> + write_reg(spi, REG_GAMMA_CONTROL_1, 0x0302);
> + write_reg(spi, REG_GAMMA_CONTROL_2, 0x0407);
> + write_reg(spi, REG_GAMMA_CONTROL_3, 0x0304);
> + write_reg(spi, REG_GAMMA_CONTROL_4, 0x0203);
> + write_reg(spi, REG_GAMMA_CONTROL_5, 0x0706);
> + write_reg(spi, REG_GAMMA_CONTROL_6, 0x0407);
> + write_reg(spi, REG_GAMMA_CONTROL_7, 0x0706);
> + write_reg(spi, REG_GAMMA_CONTROL_8, 0x0000);
> + write_reg(spi, REG_GAMMA_CONTROL_9, 0x0C06);
> + write_reg(spi, REG_GAMMA_CONTROL_10, 0x0F00);
> + write_reg(spi, REG_RAM_ADDRESS_SET, 0x0000);
> + write_reg(spi, REG_GATE_SCAN_CONTROL, 0x0000);
> + write_reg(spi, REG_VERT_SCROLL_CONTROL, 0x0000);
> + write_reg(spi, REG_FIRST_SCREEN_DRIVE_POS, 0xDB00);
> + write_reg(spi, REG_SECOND_SCREEN_DRIVE_POS, 0xDB00);
> + write_reg(spi, REG_RAM_ADDR_POS_H, 0xAF00);
> + write_reg(spi, REG_RAM_ADDR_POS_V, 0xDB00);
> + ili922x_reg_dump(spi);
> + set_write_to_gram_reg(spi);
> +}
> +
> +static int ili922x_lcd_power(struct ili922x *lcd, int power)
> +{
> + int ret = 0;
> +
> + if (POWER_IS_ON(power) && !POWER_IS_ON(lcd->power))
> + ret = ili922x_poweron(lcd->spi);
> + else if (!POWER_IS_ON(power) && POWER_IS_ON(lcd->power))
> + ret = ili922x_poweroff(lcd->spi);
> +
> + if (!ret)
> + lcd->power = power;
> +
> + return ret;
> +}
> +
> +static int ili922x_set_power(struct lcd_device *ld, int power)
> +{
> + struct ili922x *ili = lcd_get_data(ld);
> +
> + return ili922x_lcd_power(ili, power);
> +}
> +
> +static int ili922x_get_power(struct lcd_device *ld)
> +{
> + struct ili922x *ili = lcd_get_data(ld);
> +
> + return ili->power;
> +}
> +
> +static struct lcd_ops ili922x_ops = {
> + .get_power = ili922x_get_power,
> + .set_power = ili922x_set_power,
> +};
> +
> +static int ili922x_probe(struct spi_device *spi)
> +{
> + struct ili922x *ili;
> + struct lcd_device *lcd;
> + int ret;
> + u16 reg = 0;
> +
> + ili = devm_kzalloc(&spi->dev, sizeof(*ili), GFP_KERNEL);
> + if (!ili) {
> + dev_err(&spi->dev, "cannot alloc priv data\n");
> + return -ENOMEM;
> + }
> +
> + ili->spi = spi;
> + dev_set_drvdata(&spi->dev, ili);
> +
> + /* check if the device is connected */
> + ret = read_reg(spi, REG_DRIVER_CODE_READ, ®);
> + if (ret || ((reg & ILITEK_DEVICE_ID_MASK) != ILITEK_DEVICE_ID)) {
> + dev_err(&spi->dev,
> + "no LCD found: Chip ID 0x%x, ret %d\n",
> + reg, ret);
> + return -ENODEV;
> + } else {
> + dev_info(&spi->dev, "ILI%x found, SPI freq %d, mode %d\n",
> + reg, spi->max_speed_hz, spi->mode);
> + }
> +
> + dev_dbg(&spi->dev, "status: 0x%x\n", read_status(spi));
> +
> + ili922x_display_init(spi);
> +
> + ili->power = FB_BLANK_POWERDOWN;
> +
> + lcd = lcd_device_register("ili922xlcd", &spi->dev, ili,
> + &ili922x_ops);
> + if (IS_ERR(lcd)) {
> + dev_err(&spi->dev, "cannot register LCD\n");
> + return PTR_ERR(lcd);
> + }
> +
> + ili->ld = lcd;
> + spi_set_drvdata(spi, ili);
> +
> + ili922x_lcd_power(ili, FB_BLANK_UNBLANK);
> +
> + return 0;
> +}
> +
> +static int ili922x_remove(struct spi_device *spi)
> +{
> + struct ili922x *ili = spi_get_drvdata(spi);
> +
> + ili922x_poweroff(spi);
> + lcd_device_unregister(ili->ld);
> + return 0;
> +}
> +
> +static struct spi_driver ili922x_driver = {
> + .driver = {
> + .name = "ili922x",
> + .owner = THIS_MODULE,
> + },
> + .probe = ili922x_probe,
> + .remove = ili922x_remove,
> +};
> +
> +module_spi_driver(ili922x_driver);
> +
> +MODULE_AUTHOR("Stefano Babic <sbabic@denx.de>");
> +MODULE_DESCRIPTION("ILI9221/9222 LCD driver");
> +MODULE_LICENSE("GPL");
> +MODULE_PARM_DESC(ili922x_id, "set controller identifier (default=1)");
> +MODULE_PARM_DESC(tx_invert, "invert bytes before sending");
> --
> 1.7.5.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 v2 0/5] Common Display Framework
From: Marcus Lorentzon @ 2013-02-04 10:05 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Tomasz Figa, Thomas Petazzoni, linux-fbdev@vger.kernel.org,
Philipp Zabel, Tom Gall, Ragesh Radhakrishnan,
dri-devel@lists.freedesktop.org, Rob Clark, Kyungmin Park,
Tomi Valkeinen, sunil joshi, Benjamin Gaignard, Bryan Wu,
Maxime Ripard, Vikas Sajjan, Sumit Semwal, Sebastien Guiriec,
linux-media@vger.kernel.org
In-Reply-To: <3057999.UZLp2j2DkQ@avalon>
On 02/02/2013 12:35 AM, Laurent Pinchart wrote:
> Hi Marcus,
>
> On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote:
>> On 01/08/2013 05:36 PM, Tomasz Figa wrote:
>>> On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:
[...]
>>>> But it is not perfect. After a couple of products we realized that most
>>>> panel drivers want an easy way to send a bunch of init commands in one
>>>> go. So I think it should be an op for sending an array of commands at
>>>> once. Something like
>>>>
>>>> struct dsi_cmd {
>>>> enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
>>>> u8 cmd;
>>>> int dataLen;
>>>> u8 *data;
>>>> }
>>>>
>>>> struct dsi_ops {
>>>> int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
>>>> ...
>>>> }
> Do you have DSI IP(s) that can handle a list of commands ? Or would all DSI
> transmitter drivers need to iterate over the commands manually ? In the later
> case a lower-level API might be easier to implement in DSI transmitter
> drivers. Helper functions could provide the higher-level API you proposed.
The HW has a FIFO, so it can handle a few. Currently we use the low
level type of call with one call per command. But we have found DSI
command mode panels that don't accept any commands during the "update"
(write start+continues). And so we must use a mutex/state machine to
exclude any async calls to send DSI commands during update. But if you
need to send more than one command per frame this will be hard (like
CABC and backlight commands). It will be a ping pong between update and
command calls. One option is to expose the mutex to the caller so it can
make many calls before the next update grabs the mutex again.
So maybe we could create a helper that handle the op for list of
commands and another op for single command that you actually have to
implement.
>>> Yes, this should be flexible enough to cover most of (or even whole) DSI
>>> specification.
>>>
>>> However I'm not sure whether the dsi_write name would be appropriate,
>>> since DSI packet types include also read and special transactions. So,
>>> according to DSI terminology, maybe dsi_transaction would be better?
> Or dsi_transfer or dsi_xfer ? Does the DSI bus have a concept of transactions
> ?
No transactions. And I don't want to mix reads and writes. It should be
similar to I2C and other stream control busses. So one read and one
write should be fine. And then a bunch of helpers on top for callers to
use, like one per major DSI packet type.
>> I think read should still be separate. At least on my HW read and write
>> are quite different. But all "transactions" are very much the same in HW
>> setup. The ... was dsi_write etc ;) Like set_max_packet_size should
>> maybe be an ops. Since only the implementer of the "video source" will
>> know what the max read return packet size for that DSI IP is. The panels
>> don't know that. Maybe another ops to retrieve some info about the caps
>> of the video source would help that. Then a helper could call that and
>> then the dsi_write one.
> If panels (or helper functions) need information about the DSI transmitter
> capabilities, sure, we can add an op.
Yes, a "video source" op for getting caps would be ok too. But so far
the only limits I have found is the read/write sizes. But if anyone else
has other limits, please list them so we could add them to this struct
dsi_host_caps.
>>>> And I think I still prefer the dsi_bus in favor of the abstract video
>>>> source. It just looks like a home made bus with bus-ops ... can't you do
>>>> something similar using the normal driver framework? enable/disable
>>>> looks like suspend/resume, register/unregister_vid_src is like
>>>> bus_(un)register_device, ... the video source anyway seems unattached
>>>> to the panel stuff with the find_video_source call.
> The Linux driver framework is based on control busses. DSI usually handles
> both control and video transfer, but the control and data busses can also be
> separate (think DPI + SPI/I2C for instance). In that case the panel will be a
> child of its control bus master, and will need a separate interface to access
> video data operations. As a separate video interface is thus needed, I think
> we should use it for DSI as well.
>
> My initial proposal included a DBI bus (as I don't have any DSI hardware - DBI
> and DSI can be used interchangeably in this discussions, they both share the
> caracteristic of having a common control + data bus), and panels were children
> of the DBI bus master. The DBI bus API was only used for control, not for data
> transfers. Tomi then removed the DBI bus and moved the control operations to
> the video source, turning the DBI panels into platform devices. I still favor
> my initial approach, but I can agree to drop the DBI bus if there's a
> consensus on that. Video bus operations will be separate in any case.
As discussed at FOSDEM I will give DSI bus with full feature set a try.
BTW. Who got the action to ask Greg about devices with multiple
parents/buses?
>>> Also, as discussed in previous posts, some panels might use DSI only for
>>> video data and another interface (I2C, SPI) for control data. In such case
>>> it would be impossible to represent such device in a reasonable way using
>>> current driver model.
>> I understand that you need to get hold of both the control and data bus
>> device in the driver. (Toshiba DSI-LVDS bridge is a good example and
>> commonly used "encoder" that can use both DSI and I2C control interface.)
>> But the control bus you get from device probe, and I guess you could call
>> bus_find_device_by_name(dsi_bus, "mydev") and return the "datadev" which
>> will have access to dsi bus ops just as you call
>> find_video_source("mysource") to access the "databus" ops directly with
>> a logical device (display entity).
>> I'm not saying I would refuse to use video sources. I just think the two
>> models are so similar so it would be worth exploring how a device model
>> style API would look like and to compare against.
> I don't think we should use the Linux device model for data busses. It hasn't
> been designed for that use case, and definitely doesn't support devices that
> would be children of two separate masters (control and data). For shared bus
> devices such as DSI, having a DSI bus was my preference to start with, as
> mentioned above :-) However, even in that case, I think it would still make
> sense to use video sources to control the video operations. As usual the devil
> is in the details, so there will probably be some tricky problems we'll need
> to solve, but that will require coding the proposed solution.
>
I will give it a try after asking Greg for guidelines.
/BR
/Marcus
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-02-04 12:05 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, linux-kernel, linux-fbdev,
Florian Tobias Schandinat, Bernie Thompson, Steve Glendinning,
stable
In-Reply-To: <20130204011413.GA6413@kroah.com>
Am 04.02.2013 02:14, schrieb Greg KH:
> So you are right in that your driver will wait for forever for a
> disconnect() to happen, as it will never be called. I don't understand
> the problem that this is causing when it happens. What's wrong with
> udlfb that having the cpu suddently reset as the powerdown happened
> without it knowing about it?
There is nothing wrong with that. I've just explained why a problem
doesn't occur on shutdown but on disconnect (of the device).
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-02-04 19:17 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, linux-kernel, linux-fbdev,
Florian Tobias Schandinat, Bernie Thompson, Steve Glendinning,
stable
In-Reply-To: <510FA409.2080201@ahsoftware.de>
Am 04.02.2013 13:05, schrieb Alexander Holler:
> Am 04.02.2013 02:14, schrieb Greg KH:
>
>> So you are right in that your driver will wait for forever for a
>> disconnect() to happen, as it will never be called. I don't understand
>> the problem that this is causing when it happens. What's wrong with
>> udlfb that having the cpu suddently reset as the powerdown happened
>> without it knowing about it?
>
> There is nothing wrong with that. I've just explained why a problem
> doesn't occur on shutdown but on disconnect (of the device).
Maybe my explanation before was just to long and I try to explain it a
bit shorter:
If a device gets disconnected, the disconnect in udlfb might wait
forever in down_interruptible() (because it waits for an urb it never
receives). This even prevents a shutdown afterwards, because that
down_interruptible() never receives a signal (at shutdown, because
kernel threads don't get such).
So the change from down_timeout() to down_interruptible() in
dlfb_free_urb_list() with commit
33077b8d3042e01da61924973e372abe589ba297 only results in that the
following code (thus the break there) will never be reached if an urb
got missed (because of a disconnect).
And the accompanying comment (... at shutdown) is misleading, because on
shutdown, the kernel thread which calls dlfb_free_urb_list() never gets
a signal, so the interruption just never happens.
As I've experienced the "missing urb on disconnect" problem quiet often,
I've changed that down_interruptible() to down_timeout() (in v1 and in
v2 to down_timeout_interruptible, because I wasn't aware that no signal
arrives on shutdown).
Hmm, ok, that explanation isn't much shorter. ;)
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Greg KH @ 2013-02-04 19:25 UTC (permalink / raw)
To: Alexander Holler
Cc: Andrew Morton, linux-kernel, linux-fbdev,
Florian Tobias Schandinat, Bernie Thompson, Steve Glendinning,
stable
In-Reply-To: <51100930.6080405@ahsoftware.de>
On Mon, Feb 04, 2013 at 08:17:04PM +0100, Alexander Holler wrote:
> Am 04.02.2013 13:05, schrieb Alexander Holler:
> >Am 04.02.2013 02:14, schrieb Greg KH:
> >
> >>So you are right in that your driver will wait for forever for a
> >>disconnect() to happen, as it will never be called. I don't understand
> >>the problem that this is causing when it happens. What's wrong with
> >>udlfb that having the cpu suddently reset as the powerdown happened
> >>without it knowing about it?
> >
> >There is nothing wrong with that. I've just explained why a problem
> >doesn't occur on shutdown but on disconnect (of the device).
>
> Maybe my explanation before was just to long and I try to explain it
> a bit shorter:
>
> If a device gets disconnected, the disconnect in udlfb might wait
> forever in down_interruptible() (because it waits for an urb it
> never receives). This even prevents a shutdown afterwards, because
> that down_interruptible() never receives a signal (at shutdown,
> because kernel threads don't get such).
Where was that urb when the disconnect happened? The USB core should
call your urb callback for any outstanding urbs at that point in time,
with the proper error flag being set, are you handling that properly?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-02-05 7:08 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, linux-kernel, linux-fbdev,
Florian Tobias Schandinat, Bernie Thompson, Steve Glendinning,
stable
In-Reply-To: <20130204192514.GA32318@kroah.com>
Am 04.02.2013 20:25, schrieb Greg KH:
> On Mon, Feb 04, 2013 at 08:17:04PM +0100, Alexander Holler wrote:
>> Am 04.02.2013 13:05, schrieb Alexander Holler:
>>> Am 04.02.2013 02:14, schrieb Greg KH:
>>>
>>>> So you are right in that your driver will wait for forever for a
>>>> disconnect() to happen, as it will never be called. I don't understand
>>>> the problem that this is causing when it happens. What's wrong with
>>>> udlfb that having the cpu suddently reset as the powerdown happened
>>>> without it knowing about it?
>>>
>>> There is nothing wrong with that. I've just explained why a problem
>>> doesn't occur on shutdown but on disconnect (of the device).
>>
>> Maybe my explanation before was just to long and I try to explain it
>> a bit shorter:
>>
>> If a device gets disconnected, the disconnect in udlfb might wait
>> forever in down_interruptible() (because it waits for an urb it
>> never receives). This even prevents a shutdown afterwards, because
>> that down_interruptible() never receives a signal (at shutdown,
>> because kernel threads don't get such).
>
> Where was that urb when the disconnect happened? The USB core should
> call your urb callback for any outstanding urbs at that point in time,
> with the proper error flag being set, are you handling that properly?
I don't know where that urb is as I don't handle it. I just know that
_interruptible doesn't make any sense and _timeout is necessary here.
But as nobody else seems to have a problem, nobody else see seems to see
the problem there and I seem to be unable to explain it, just ignore
that patch.
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH] backlight: add an AS3711 PMIC backlight driver
From: Guennadi Liakhovetski @ 2013-02-05 9:08 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Andrew Morton', linux-kernel, linux-fbdev, linux-sh,
'Magnus Damm', 'Richard Purdie'
In-Reply-To: <00fe01cdfcf1$dcfdc970$96f95c50$%han@samsung.com>
Hi Richard
Could you tell us your opinion on this:
On Mon, 28 Jan 2013, Jingoo Han wrote:
> On Friday, January 25, 2013 4:49 PM, Guennadi Liakhovetski wrote
> >
> > Hi Jingoo Han
> >
> > On Fri, 25 Jan 2013, Jingoo Han wrote:
> >
> > > On Wednesday, January 09, 2013 2:55 AM, Guennadi Liakhovetski wrote
> > > >
> > > > This is an initial commit of a backlight driver, using step-up DCDC power
> > > > supplies on AS3711 PMIC. Only one mode has actually been tested, several
> > > > further modes have been implemented "dry," but disabled to avoid accidental
> > > > hardware damage. Anyone wishing to use any of those modes will have to
> > > > modify the driver.
> > > >
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > >
> > > CC'ed Andrew Morton.
> > >
> > > Hi Guennadi Liakhovetski,
> > >
> > > I have reviewed this patch with AS3711 datasheet.
> > > I cannot find any problems. It looks good.
> >
> > Thanks for the review.
> >
> > > However, some hardcoded numbers need to be changed
> > > to the bit definitions.
> >
> > Which specific hardcoded values do you mean? A while ago in a discussion
> > on one of kernel mailing lists a conclusion has been made, that macros do
> > not always improve code readability. E.g. in a statement like this
> >
> > + case AS3711_SU2_CURR_AUTO:
> > + if (pdata->su2_auto_curr1)
> > + ctl = 2;
> > + if (pdata->su2_auto_curr2)
> > + ctl |= 8;
> > + if (pdata->su2_auto_curr3)
> > + ctl |= 0x20;
> >
> > making it
> >
> > + case AS3711_SU2_CURR_AUTO:
> > + if (pdata->su2_auto_curr1)
> > + ctl = SU2_AUTO_CURR1;
> >
> > would hardly make it more readable. IMHO it is already pretty clear, that
> > bit 1 enables the current-1 sink. As long as these fields are only used at
> > one location in the driver (and they are), using a macro and defining it
> > elsewhere only makes it harder to see actual values. Speaking of this, a
> > comment like
>
> I don't think so. Some people feel that it is not clear a bit.
> Of course, I know what you mean.
> Also, your comment is reasonable.
> However, personally, I prefer the latter.
> Because, I think that the meaning of bits is more important than
> actual bits. In the latter case, we can notice the meaning of bits
> more easily.
Do you also find preferable to use symbolic names for every single bit,
occurring in a driver, or you agree, that excessive use of such macros can
only needlessly clutter the code?
Thanks
Guennadi
> Best regards,
> Jingoo Han
>
> >
> > /*
> > * Select, which current sinks shall be used for automatic
> > * feedback selection
> > */
> >
> > would help much more than any macro names. But as it stands, I think the
> > current version is also possible to understand :-) If desired, however,
> > comments can be added in an incremental patch
>
> >
> > Thanks
> > Guennadi
> >
> > > Acked-by: Jingoo Han <jg1.han@samsung.com>
> > >
> > >
> > > Best regards,
> > > Jingoo Han
> > >
> > > > ---
> > > >
> > > > Tested on sh73a0-based kzm9g board. As the commit message says, only one
> > > > mode has been tested and is enabled. That mode copies the sample code from
> > > > the manufacturer. Deviations from that code proved to be fatal for the
> > > > hardware...
> > > >
> > > > drivers/video/backlight/Kconfig | 7 +
> > > > drivers/video/backlight/Makefile | 1 +
> > > > drivers/video/backlight/as3711_bl.c | 379 +++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 387 insertions(+), 0 deletions(-)
> > > > create mode 100644 drivers/video/backlight/as3711_bl.c
> > > >
> > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > > > index 765a945..6ef0ede 100644
> > > > --- a/drivers/video/backlight/Kconfig
> > > > +++ b/drivers/video/backlight/Kconfig
> > > > @@ -390,6 +390,13 @@ config BACKLIGHT_TPS65217
> > > > If you have a Texas Instruments TPS65217 say Y to enable the
> > > > backlight driver.
> > > >
> > > > +config BACKLIGHT_AS3711
> > > > + tristate "AS3711 Backlight"
> > > > + depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
> > > > + help
> > > > + If you have an Austrian Microsystems AS3711 say Y to enable the
> > > > + backlight driver.
> > > > +
> > > > endif # BACKLIGHT_CLASS_DEVICE
> > > >
> > > > endif # BACKLIGHT_LCD_SUPPORT
> > > > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> > > > index e7ce729..d3e188f 100644
> > > > --- a/drivers/video/backlight/Makefile
> > > > +++ b/drivers/video/backlight/Makefile
> > > > @@ -45,3 +45,4 @@ obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
> > > > obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
> > > > obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
> > > > obj-$(CONFIG_BACKLIGHT_TPS65217) += tps65217_bl.o
> > > > +obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
> > > > diff --git a/drivers/video/backlight/as3711_bl.c b/drivers/video/backlight/as3711_bl.c
> > > > new file mode 100644
> > > > index 0000000..c6bc65d
> > > > --- /dev/null
> > > > +++ b/drivers/video/backlight/as3711_bl.c
> > > > @@ -0,0 +1,379 @@
> > > > +/*
> > > > + * AS3711 PMIC backlight driver, using DCDC Step Up Converters
> > > > + *
> > > > + * Copyright (C) 2012 Renesas Electronics Corporation
> > > > + * Author: Guennadi Liakhovetski, <g.liakhovetski@gmx.de>
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the version 2 of the GNU General Public License as
> > > > + * published by the Free Software Foundation
> > > > + */
> > > > +
> > > > +#include <linux/backlight.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/fb.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mfd/as3711.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/slab.h>
> > > > +
> > > > +enum as3711_bl_type {
> > > > + AS3711_BL_SU1,
> > > > + AS3711_BL_SU2,
> > > > +};
> > > > +
> > > > +struct as3711_bl_data {
> > > > + bool powered;
> > > > + const char *fb_name;
> > > > + struct device *fb_dev;
> > > > + enum as3711_bl_type type;
> > > > + int brightness;
> > > > + struct backlight_device *bl;
> > > > +};
> > > > +
> > > > +struct as3711_bl_supply {
> > > > + struct as3711_bl_data su1;
> > > > + struct as3711_bl_data su2;
> > > > + const struct as3711_bl_pdata *pdata;
> > > > + struct as3711 *as3711;
> > > > +};
> > > > +
> > > > +static struct as3711_bl_supply *to_supply(struct as3711_bl_data *su)
> > > > +{
> > > > + switch (su->type) {
> > > > + case AS3711_BL_SU1:
> > > > + return container_of(su, struct as3711_bl_supply, su1);
> > > > + case AS3711_BL_SU2:
> > > > + return container_of(su, struct as3711_bl_supply, su2);
> > > > + }
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +static int as3711_set_brightness_auto_i(struct as3711_bl_data *data,
> > > > + unsigned int brightness)
> > > > +{
> > > > + struct as3711_bl_supply *supply = to_supply(data);
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > > + int ret = 0;
> > > > +
> > > > + /* Only all equal current values are supported */
> > > > + if (pdata->su2_auto_curr1)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > > > + brightness);
> > > > + if (!ret && pdata->su2_auto_curr2)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > > > + brightness);
> > > > + if (!ret && pdata->su2_auto_curr3)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > > > + brightness);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_set_brightness_v(struct as3711 *as3711,
> > > > + unsigned int brightness,
> > > > + unsigned int reg)
> > > > +{
> > > > + if (brightness > 31)
> > > > + return -EINVAL;
> > > > +
> > > > + return regmap_update_bits(as3711->regmap, reg, 0xf0,
> > > > + brightness << 4);
> > > > +}
> > > > +
> > > > +static int as3711_bl_su2_reset(struct as3711_bl_supply *supply)
> > > > +{
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + int ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_5,
> > > > + 3, supply->pdata->su2_fbprot);
> > > > + if (!ret)
> > > > + ret = regmap_update_bits(as3711->regmap,
> > > > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > > > + if (!ret)
> > > > + ret = regmap_update_bits(as3711->regmap,
> > > > + AS3711_STEPUP_CONTROL_2, 1, 1);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Someone with less fragile or less expensive hardware could try to simplify
> > > > + * the brightness adjustment procedure.
> > > > + */
> > > > +static int as3711_bl_update_status(struct backlight_device *bl)
> > > > +{
> > > > + struct as3711_bl_data *data = bl_get_data(bl);
> > > > + struct as3711_bl_supply *supply = to_supply(data);
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + int brightness = bl->props.brightness;
> > > > + int ret = 0;
> > > > +
> > > > + dev_dbg(&bl->dev, "%s(): brightness %u, pwr %x, blank %x, state %x\n",
> > > > + __func__, bl->props.brightness, bl->props.power,
> > > > + bl->props.fb_blank, bl->props.state);
> > > > +
> > > > + if (bl->props.power != FB_BLANK_UNBLANK ||
> > > > + bl->props.fb_blank != FB_BLANK_UNBLANK ||
> > > > + bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> > > > + brightness = 0;
> > > > +
> > > > + if (data->type = AS3711_BL_SU1) {
> > > > + ret = as3711_set_brightness_v(as3711, brightness,
> > > > + AS3711_STEPUP_CONTROL_1);
> > > > + } else {
> > > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > > +
> > > > + switch (pdata->su2_feedback) {
> > > > + case AS3711_SU2_VOLTAGE:
> > > > + ret = as3711_set_brightness_v(as3711, brightness,
> > > > + AS3711_STEPUP_CONTROL_2);
> > > > + break;
> > > > + case AS3711_SU2_CURR_AUTO:
> > > > + ret = as3711_set_brightness_auto_i(data, brightness / 4);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + if (brightness) {
> > > > + ret = as3711_bl_su2_reset(supply);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + udelay(500);
> > > > + ret = as3711_set_brightness_auto_i(data, brightness);
> > > > + } else {
> > > > + ret = regmap_update_bits(as3711->regmap,
> > > > + AS3711_STEPUP_CONTROL_2, 1, 0);
> > > > + }
> > > > + break;
> > > > + /* Manual one current feedback pin below */
> > > > + case AS3711_SU2_CURR1:
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR1_VALUE,
> > > > + brightness);
> > > > + break;
> > > > + case AS3711_SU2_CURR2:
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR2_VALUE,
> > > > + brightness);
> > > > + break;
> > > > + case AS3711_SU2_CURR3:
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR3_VALUE,
> > > > + brightness);
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + }
> > > > + }
> > > > + if (!ret)
> > > > + data->brightness = brightness;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_bl_get_brightness(struct backlight_device *bl)
> > > > +{
> > > > + struct as3711_bl_data *data = bl_get_data(bl);
> > > > +
> > > > + return data->brightness;
> > > > +}
> > > > +
> > > > +static const struct backlight_ops as3711_bl_ops = {
> > > > + .update_status = as3711_bl_update_status,
> > > > + .get_brightness = as3711_bl_get_brightness,
> > > > +};
> > > > +
> > > > +static int as3711_bl_init_su2(struct as3711_bl_supply *supply)
> > > > +{
> > > > + struct as3711 *as3711 = supply->as3711;
> > > > + const struct as3711_bl_pdata *pdata = supply->pdata;
> > > > + u8 ctl = 0;
> > > > + int ret;
> > > > +
> > > > + dev_dbg(as3711->dev, "%s(): use %u\n", __func__, pdata->su2_feedback);
> > > > +
> > > > + /* Turn SU2 off */
> > > > + ret = regmap_write(as3711->regmap, AS3711_STEPUP_CONTROL_2, 0);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + switch (pdata->su2_feedback) {
> > > > + case AS3711_SU2_VOLTAGE:
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 0);
> > > > + break;
> > > > + case AS3711_SU2_CURR1:
> > > > + ctl = 1;
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 1);
> > > > + break;
> > > > + case AS3711_SU2_CURR2:
> > > > + ctl = 4;
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 2);
> > > > + break;
> > > > + case AS3711_SU2_CURR3:
> > > > + ctl = 0x10;
> > > > + ret = regmap_update_bits(as3711->regmap, AS3711_STEPUP_CONTROL_4, 3, 3);
> > > > + break;
> > > > + case AS3711_SU2_CURR_AUTO:
> > > > + if (pdata->su2_auto_curr1)
> > > > + ctl = 2;
> > > > + if (pdata->su2_auto_curr2)
> > > > + ctl |= 8;
> > > > + if (pdata->su2_auto_curr3)
> > > > + ctl |= 0x20;
> > > > + ret = 0;
> > > > + break;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (!ret)
> > > > + ret = regmap_write(as3711->regmap, AS3711_CURR_CONTROL, ctl);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_bl_register(struct platform_device *pdev,
> > > > + unsigned int max_brightness, struct as3711_bl_data *su)
> > > > +{
> > > > + struct backlight_properties props = {.type = BACKLIGHT_RAW,};
> > > > + struct backlight_device *bl;
> > > > +
> > > > + /* max tuning I = 31uA for voltage- and 38250uA for current-feedback */
> > > > + props.max_brightness = max_brightness;
> > > > +
> > > > + bl = backlight_device_register(su->type = AS3711_BL_SU1 ?
> > > > + "as3711-su1" : "as3711-su2",
> > > > + &pdev->dev, su,
> > > > + &as3711_bl_ops, &props);
> > > > + if (IS_ERR(bl)) {
> > > > + dev_err(&pdev->dev, "failed to register backlight\n");
> > > > + return PTR_ERR(bl);
> > > > + }
> > > > +
> > > > + bl->props.brightness = props.max_brightness;
> > > > +
> > > > + backlight_update_status(bl);
> > > > +
> > > > + su->bl = bl;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int as3711_backlight_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct as3711_bl_pdata *pdata = dev_get_platdata(&pdev->dev);
> > > > + struct as3711 *as3711 = dev_get_drvdata(pdev->dev.parent);
> > > > + struct as3711_bl_supply *supply;
> > > > + struct as3711_bl_data *su;
> > > > + unsigned int max_brightness;
> > > > + int ret;
> > > > +
> > > > + if (!pdata || (!pdata->su1_fb && !pdata->su2_fb)) {
> > > > + dev_err(&pdev->dev, "No platform data, exiting...\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Due to possible hardware damage I chose to block all modes,
> > > > + * unsupported on my hardware. Anyone, wishing to use any of those modes
> > > > + * will have to first review the code, then activate and test it.
> > > > + */
> > > > + if (pdata->su1_fb ||
> > > > + pdata->su2_fbprot != AS3711_SU2_GPIO4 ||
> > > > + pdata->su2_feedback != AS3711_SU2_CURR_AUTO) {
> > > > + dev_warn(&pdev->dev,
> > > > + "Attention! An untested mode has been chosen!\n"
> > > > + "Please, review the code, enable, test, and report success:-)\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + supply = devm_kzalloc(&pdev->dev, sizeof(*supply), GFP_KERNEL);
> > > > + if (!supply)
> > > > + return -ENOMEM;
> > > > +
> > > > + supply->as3711 = as3711;
> > > > + supply->pdata = pdata;
> > > > +
> > > > + if (pdata->su1_fb) {
> > > > + su = &supply->su1;
> > > > + su->fb_name = pdata->su1_fb;
> > > > + su->type = AS3711_BL_SU1;
> > > > +
> > > > + max_brightness = min(pdata->su1_max_uA, 31);
> > > > + ret = as3711_bl_register(pdev, max_brightness, su);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (pdata->su2_fb) {
> > > > + su = &supply->su2;
> > > > + su->fb_name = pdata->su2_fb;
> > > > + su->type = AS3711_BL_SU2;
> > > > +
> > > > + switch (pdata->su2_fbprot) {
> > > > + case AS3711_SU2_GPIO2:
> > > > + case AS3711_SU2_GPIO3:
> > > > + case AS3711_SU2_GPIO4:
> > > > + case AS3711_SU2_LX_SD4:
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + goto esu2;
> > > > + }
> > > > +
> > > > + switch (pdata->su2_feedback) {
> > > > + case AS3711_SU2_VOLTAGE:
> > > > + max_brightness = min(pdata->su2_max_uA, 31);
> > > > + break;
> > > > + case AS3711_SU2_CURR1:
> > > > + case AS3711_SU2_CURR2:
> > > > + case AS3711_SU2_CURR3:
> > > > + case AS3711_SU2_CURR_AUTO:
> > > > + max_brightness = min(pdata->su2_max_uA / 150, 255);
> > > > + break;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + goto esu2;
> > > > + }
> > > > +
> > > > + ret = as3711_bl_init_su2(supply);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = as3711_bl_register(pdev, max_brightness, su);
> > > > + if (ret < 0)
> > > > + goto esu2;
> > > > + }
> > > > +
> > > > + platform_set_drvdata(pdev, supply);
> > > > +
> > > > + return 0;
> > > > +
> > > > +esu2:
> > > > + backlight_device_unregister(supply->su1.bl);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int as3711_backlight_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct as3711_bl_supply *supply = platform_get_drvdata(pdev);
> > > > +
> > > > + backlight_device_unregister(supply->su1.bl);
> > > > + backlight_device_unregister(supply->su2.bl);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver as3711_backlight_driver = {
> > > > + .driver = {
> > > > + .name = "as3711-backlight",
> > > > + .owner = THIS_MODULE,
> > > > + },
> > > > + .probe = as3711_backlight_probe,
> > > > + .remove = as3711_backlight_remove,
> > > > +};
> > > > +
> > > > +module_platform_driver(as3711_backlight_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("Backlight Driver for AS3711 PMICs");
> > > > +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de");
> > > > +MODULE_LICENSE("GPL");
> > > > +MODULE_ALIAS("platform:as3711-backlight");
> > > > --
> > > > 1.7.2.5
> > > >
> > > > --
> > > > 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
> > >
> >
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: Revert "console: implement lockdep support for console_lock"
From: Sedat Dilek @ 2013-02-05 10:47 UTC (permalink / raw)
To: Daniel Vetter
Cc: Dave Airlie, Linus Torvalds, linux-fbdev, LKML, linux-next,
Andrew Morton, Stephen Rothwell
In-Reply-To: <CAKMK7uEFCm5JPD9bg4uO9K_q-w0=9GWzRLoB9H-_=ZR7NxA9=A@mail.gmail.com>
On Fri, Feb 1, 2013 at 9:42 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Fri, Feb 1, 2013 at 9:21 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>> people having the fbcon-locking-fixes [1] in their local GIT tree can
>> revert this change?
>
> Yeah, if you have all the fixes reverting this is fine and appreciated
> to increase testing. Dave will probably push the revert himself to
> drm-next soon.
When will that happen: drm-next inclusion?
Just FYI:
Pulling fbcon-locking-fixes into Linux-Next (next-20130205) shows some
ugly UTF-8 mismatch, means "fb: rework locking to fix lock ordering on
takeover" is not the same patch as in -next.
Just wanna let you know.
It would be good to inform Andrew and Stephen when this happened, so
Andrew can drop the two fb-fixes from his mmotm-tree.
Thanks!
- Sedat -
> -Daniel
>
>>
>> commit ff0d05bf73620eb7dc8aee7423e992ef87870bdf
>> Refs: v3.8-rc5-194-gff0d05b
>> Author: Dave Airlie <airlied@gmail.com>
>> AuthorDate: Thu Jan 31 14:27:03 2013 +1100
>> Commit: Dave Airlie <airlied@gmail.com>
>> CommitDate: Thu Jan 31 15:46:56 2013 +1100
>>
>> Revert "console: implement lockdep support for console_lock"
>>
>> This reverts commit daee779718a319ff9f83e1ba3339334ac650bb22.
>>
>> I'll requeue this after the console locking fixes, so lockdep
>> is useful again for people until fbcon is fixed.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>
>> Thanks!
>>
>> Regards,
>> - Sedat
>>
>> [1] http://cgit.freedesktop.org/~airlied/linux/log/?hûcon-locking-fixes
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: Revert "console: implement lockdep support for console_lock"
From: Sedat Dilek @ 2013-02-05 10:50 UTC (permalink / raw)
To: Daniel Vetter
Cc: Dave Airlie, Linus Torvalds, linux-fbdev, LKML, linux-next,
Andrew Morton, Stephen Rothwell
In-Reply-To: <CA+icZUU4JMfws_jFpoCaXnSNJeigONSEik_oGSa_ZQ=uJ_GJOA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]
On Tue, Feb 5, 2013 at 11:47 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> On Fri, Feb 1, 2013 at 9:42 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> On Fri, Feb 1, 2013 at 9:21 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>> people having the fbcon-locking-fixes [1] in their local GIT tree can
>>> revert this change?
>>
>> Yeah, if you have all the fixes reverting this is fine and appreciated
>> to increase testing. Dave will probably push the revert himself to
>> drm-next soon.
>
> When will that happen: drm-next inclusion?
>
> Just FYI:
> Pulling fbcon-locking-fixes into Linux-Next (next-20130205) shows some
> ugly UTF-8 mismatch, means "fb: rework locking to fix lock ordering on
> takeover" is not the same patch as in -next.
> Just wanna let you know.
>
> It would be good to inform Andrew and Stephen when this happened, so
> Andrew can drop the two fb-fixes from his mmotm-tree.
>
Attached the UTF-8 "mismatch" in vt.c.
- Sedat -
> Thanks!
>
> - Sedat -
>
>> -Daniel
>>
>>>
>>> commit ff0d05bf73620eb7dc8aee7423e992ef87870bdf
>>> Refs: v3.8-rc5-194-gff0d05b
>>> Author: Dave Airlie <airlied@gmail.com>
>>> AuthorDate: Thu Jan 31 14:27:03 2013 +1100
>>> Commit: Dave Airlie <airlied@gmail.com>
>>> CommitDate: Thu Jan 31 15:46:56 2013 +1100
>>>
>>> Revert "console: implement lockdep support for console_lock"
>>>
>>> This reverts commit daee779718a319ff9f83e1ba3339334ac650bb22.
>>>
>>> I'll requeue this after the console locking fixes, so lockdep
>>> is useful again for people until fbcon is fixed.
>>>
>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>>
>>> Thanks!
>>>
>>> Regards,
>>> - Sedat
>>>
>>> [1] http://cgit.freedesktop.org/~airlied/linux/log/?h=fbcon-locking-fixes
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[-- Attachment #2: VT_C_UTF-8_CRAP.diff --]
[-- Type: application/octet-stream, Size: 1773 bytes --]
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 56dd69c..86b76f4 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3656,7 +3656,7 @@ EXPORT_SYMBOL_GPL(do_unregister_con_driver);
* when a driver wants to take over some existing consoles
* and become default driver for newly opened ones.
*
- * take_over_console is basically a register followed by unbind
+ * take_over_console is basically a register followed by unbind
*/
int do_take_over_console(const struct consw *csw, int first, int last, int deflt)
{
@@ -3666,7 +3666,7 @@ int do_take_over_console(const struct consw *csw, int first, int last, int deflt
/*
* If we get an busy error we still want to bind the console driver
* and return success, as we may have unbound the console driver
- * but not unregistered it.
+ * but not unregistered it.
*/
if (err == -EBUSY)
err = 0;
@@ -3682,7 +3682,7 @@ EXPORT_SYMBOL_GPL(do_take_over_console);
* when a driver wants to take over some existing consoles
* and become default driver for newly opened ones.
*
- * take_over_console is basically a register followed by unbind
+ * take_over_console is basically a register followed by unbind
*/
int take_over_console(const struct consw *csw, int first, int last, int deflt)
{
@@ -3692,7 +3692,7 @@ int take_over_console(const struct consw *csw, int first, int last, int deflt)
/*
* If we get an busy error we still want to bind the console driver
* and return success, as we may have unbound the console driver
- * but not unregistered it.
+ * but not unregistered it.
*/
if (err == -EBUSY)
err = 0;
^ permalink raw reply related
* [PATCH 0/5] atmel_lcdfb: regression fixes and cpu_is removal
From: Johan Hovold @ 2013-02-05 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130129135435.GN7360@game.jcrosoft.org>
The first three patches are resends of two regression fixes and one clean up
posted in December (with previous acked-bys included). The regression fixes are
kept minimal and are tagged for stable.
The last two patches replace all uses of cpu_is macros in atmel_lcdfb with a
platform-device-id table and static configurations.
Thanks,
Johan
Johan Hovold (5):
atmel_lcdfb: fix 16-bpp modes on older SOCs
ARM: at91/neocore926: fix LCD-wiring mode
atmel_lcdfb: remove unsupported 15-bpp mode
atmel_lcdfb: move lcdcon2 register access to compute_hozval
ARM: at91/avr32/atmel_lcdfb: replace cpu_is macros with device-id
table
arch/arm/mach-at91/at91sam9261_devices.c | 6 +-
arch/arm/mach-at91/at91sam9263_devices.c | 2 +-
arch/arm/mach-at91/at91sam9g45_devices.c | 6 +-
arch/arm/mach-at91/at91sam9rl_devices.c | 2 +-
arch/arm/mach-at91/board-neocore926.c | 2 +-
arch/avr32/mach-at32ap/at32ap700x.c | 2 +
drivers/video/atmel_lcdfb.c | 115 ++++++++++++++++++++++++++-----
include/video/atmel_lcdc.h | 4 +-
8 files changed, 116 insertions(+), 23 deletions(-)
--
1.8.1.1
^ permalink raw reply
* [PATCH 1/5] atmel_lcdfb: fix 16-bpp modes on older SOCs
From: Johan Hovold @ 2013-02-05 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1360071315-4032-1-git-send-email-jhovold@gmail.com>
Fix regression introduced by commit 787f9fd23283 ("atmel_lcdfb: support
16bit BGR:565 mode, remove unsupported 15bit modes") which broke 16-bpp
modes for older SOCs which use IBGR:555 (msb is intensity) rather
than BGR:565.
Use SOC-type to determine the pixel layout.
Tested on at91sam9263 and at91sam9g45.
Cc: <stable@vger.kernel.org>
Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/atmel_lcdfb.c | 22 +++++++++++++++-------
include/video/atmel_lcdc.h | 1 +
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 12cf5f3..025428e 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -422,17 +422,22 @@ static int atmel_lcdfb_check_var(struct fb_var_screeninfo *var,
= var->bits_per_pixel;
break;
case 16:
+ /* Older SOCs use IBGR:555 rather than BGR:565. */
+ if (sinfo->have_intensity_bit)
+ var->green.length = 5;
+ else
+ var->green.length = 6;
+
if (sinfo->lcd_wiring_mode = ATMEL_LCDC_WIRING_RGB) {
- /* RGB:565 mode */
- var->red.offset = 11;
+ /* RGB:5X5 mode */
+ var->red.offset = var->green.length + 5;
var->blue.offset = 0;
} else {
- /* BGR:565 mode */
+ /* BGR:5X5 mode */
var->red.offset = 0;
- var->blue.offset = 11;
+ var->blue.offset = var->green.length + 5;
}
var->green.offset = 5;
- var->green.length = 6;
var->red.length = var->blue.length = 5;
break;
case 32:
@@ -679,8 +684,7 @@ static int atmel_lcdfb_setcolreg(unsigned int regno, unsigned int red,
case FB_VISUAL_PSEUDOCOLOR:
if (regno < 256) {
- if (cpu_is_at91sam9261() || cpu_is_at91sam9263()
- || cpu_is_at91sam9rl()) {
+ if (sinfo->have_intensity_bit) {
/* old style I+BGR:555 */
val = ((red >> 11) & 0x001f);
val |= ((green >> 6) & 0x03e0);
@@ -870,6 +874,10 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev)
}
sinfo->info = info;
sinfo->pdev = pdev;
+ if (cpu_is_at91sam9261() || cpu_is_at91sam9263() ||
+ cpu_is_at91sam9rl()) {
+ sinfo->have_intensity_bit = true;
+ }
strcpy(info->fix.id, sinfo->pdev->name);
info->flags = ATMEL_LCDFB_FBINFO_DEFAULT;
diff --git a/include/video/atmel_lcdc.h b/include/video/atmel_lcdc.h
index 28447f1..5f0e234 100644
--- a/include/video/atmel_lcdc.h
+++ b/include/video/atmel_lcdc.h
@@ -62,6 +62,7 @@ struct atmel_lcdfb_info {
void (*atmel_lcdfb_power_control)(int on);
struct fb_monspecs *default_monspecs;
u32 pseudo_palette[16];
+ bool have_intensity_bit;
};
#define ATMEL_LCDC_DMABADDR1 0x00
--
1.8.1.1
^ permalink raw reply related
* [PATCH 2/5] ARM: at91/neocore926: fix LCD-wiring mode
From: Johan Hovold @ 2013-02-05 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1360071315-4032-1-git-send-email-jhovold@gmail.com>
Fix regression introduced by commit 787f9fd23283 ("atmel_lcdfb: support
16bit BGR:565 mode, remove unsupported 15bit modes") which broke 16-bpp
modes for older SOCs which use IBGR:555 (msb is intensity) rather than
BGR:565.
The above commit also removed the RGB:555-wiring hack without fixing the
neocore926 board which used it. Fix by specifying RGB-wiring and let the
driver handle the final SOC-dependant layout.
Remove the no longer used ATMEL_LCDC_WIRING_RGB555 define.
Compile-only tested.
Cc: <stable@vger.kernel.org>
Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
arch/arm/mach-at91/board-neocore926.c | 2 +-
include/video/atmel_lcdc.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm/mach-at91/board-neocore926.c b/arch/arm/mach-at91/board-neocore926.c
index bc7a1c4..4726297 100644
--- a/arch/arm/mach-at91/board-neocore926.c
+++ b/arch/arm/mach-at91/board-neocore926.c
@@ -266,7 +266,7 @@ static struct atmel_lcdfb_info __initdata neocore926_lcdc_data = {
.default_monspecs = &at91fb_default_monspecs,
.atmel_lcdfb_power_control = at91_lcdc_power_control,
.guard_time = 1,
- .lcd_wiring_mode = ATMEL_LCDC_WIRING_RGB555,
+ .lcd_wiring_mode = ATMEL_LCDC_WIRING_RGB,
};
#else
diff --git a/include/video/atmel_lcdc.h b/include/video/atmel_lcdc.h
index 5f0e234..8deb226 100644
--- a/include/video/atmel_lcdc.h
+++ b/include/video/atmel_lcdc.h
@@ -30,7 +30,6 @@
*/
#define ATMEL_LCDC_WIRING_BGR 0
#define ATMEL_LCDC_WIRING_RGB 1
-#define ATMEL_LCDC_WIRING_RGB555 2
/* LCD Controller info data structure, stored in device platform_data */
--
1.8.1.1
^ permalink raw reply related
* [PATCH 3/5] atmel_lcdfb: remove unsupported 15-bpp mode
From: Johan Hovold @ 2013-02-05 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1360071315-4032-1-git-send-email-jhovold@gmail.com>
Since commit 787f9fd23283 ("atmel_lcdfb: support 16bit BGR:565 mode,
remove unsupported 15bit modes") atmel_lcdfb_check_var does not accept
15-bpp mode so remove it from atmel_lcdfb_set_par as well.
Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/atmel_lcdfb.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 025428e..93910e3 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -567,7 +567,6 @@ static int atmel_lcdfb_set_par(struct fb_info *info)
case 2: value |= ATMEL_LCDC_PIXELSIZE_2; break;
case 4: value |= ATMEL_LCDC_PIXELSIZE_4; break;
case 8: value |= ATMEL_LCDC_PIXELSIZE_8; break;
- case 15: /* fall through */
case 16: value |= ATMEL_LCDC_PIXELSIZE_16; break;
case 24: value |= ATMEL_LCDC_PIXELSIZE_24; break;
case 32: value |= ATMEL_LCDC_PIXELSIZE_32; break;
--
1.8.1.1
^ permalink raw reply related
* [PATCH 4/5] atmel_lcdfb: move lcdcon2 register access to compute_hozval
From: Johan Hovold @ 2013-02-05 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1360071315-4032-1-git-send-email-jhovold@gmail.com>
Pass atmel_lcd_info structure to compute_hozval and only do the register
access on SOCs that actually use it.
This will also simplify the removal of the cpu_is macros.
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
drivers/video/atmel_lcdfb.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 93910e3..347bab2 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -193,14 +193,17 @@ static struct fb_fix_screeninfo atmel_lcdfb_fix __initdata = {
.accel = FB_ACCEL_NONE,
};
-static unsigned long compute_hozval(unsigned long xres, unsigned long lcdcon2)
+static unsigned long compute_hozval(struct atmel_lcdfb_info *sinfo,
+ unsigned long xres)
{
+ unsigned long lcdcon2;
unsigned long value;
if (!(cpu_is_at91sam9261() || cpu_is_at91sam9g10()
|| cpu_is_at32ap7000()))
return xres;
+ lcdcon2 = lcdc_readl(sinfo, ATMEL_LCDC_LCDCON2);
value = xres;
if ((lcdcon2 & ATMEL_LCDC_DISTYPE) != ATMEL_LCDC_DISTYPE_TFT) {
/* STN display */
@@ -590,8 +593,7 @@ static int atmel_lcdfb_set_par(struct fb_info *info)
lcdc_writel(sinfo, ATMEL_LCDC_TIM2, value);
/* Horizontal value (aka line size) */
- hozval_linesz = compute_hozval(info->var.xres,
- lcdc_readl(sinfo, ATMEL_LCDC_LCDCON2));
+ hozval_linesz = compute_hozval(sinfo, info->var.xres);
/* Display size */
value = (hozval_linesz - 1) << ATMEL_LCDC_HOZVAL_OFFSET;
--
1.8.1.1
^ permalink raw reply related
* [PATCH 5/5] ARM: at91/avr32/atmel_lcdfb: replace cpu_is macros with device-id table
From: Johan Hovold @ 2013-02-05 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1360071315-4032-1-git-send-email-jhovold@gmail.com>
Remove cpu_is macros from atmel lcdfb driver and use platform-device-id
table to determine platform configuration parameters.
The currently used configuration parameters are:
have_alt_pixclock
- SOC uses an alternate pixel-clock calculation formula (at91sam9g45
non-ES)
have_bus_clk
- SOC has bus clock hck1 (at91sam9261, at921sam9g10 and at32ap)
have_hozval
- SOC has a HOZVAL field in LCDFRMCFG which is used to determine the
linesize for STN displays (at91sam9261, at921sam9g10 and at32ap)
have_intensity_bit
- SOC uses IBGR:555 rather than BGR:565 16-bit pixel layout
(at91sam9261, at91sam9263 and at91sam9rl)
Tested on at91sam9g45, compile-tested for other AT91 SOCs, and untested
for AVR32.
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
arch/arm/mach-at91/at91sam9261_devices.c | 6 +-
arch/arm/mach-at91/at91sam9263_devices.c | 2 +-
arch/arm/mach-at91/at91sam9g45_devices.c | 6 +-
arch/arm/mach-at91/at91sam9rl_devices.c | 2 +-
arch/avr32/mach-at32ap/at32ap700x.c | 2 +
drivers/video/atmel_lcdfb.c | 96 ++++++++++++++++++++++++++++----
include/video/atmel_lcdc.h | 4 +-
7 files changed, 101 insertions(+), 17 deletions(-)
diff --git a/arch/arm/mach-at91/at91sam9261_devices.c b/arch/arm/mach-at91/at91sam9261_devices.c
index 92e0f86..01647cb 100644
--- a/arch/arm/mach-at91/at91sam9261_devices.c
+++ b/arch/arm/mach-at91/at91sam9261_devices.c
@@ -488,7 +488,6 @@ static struct resource lcdc_resources[] = {
};
static struct platform_device at91_lcdc_device = {
- .name = "atmel_lcdfb",
.id = 0,
.dev = {
.dma_mask = &lcdc_dmamask,
@@ -505,6 +504,11 @@ void __init at91_add_device_lcdc(struct atmel_lcdfb_info *data)
return;
}
+ if (cpu_is_at91sam9g10())
+ at91_lcdc_device.name = "fb-at91sam9g10";
+ else
+ at91_lcdc_device.name = "fb-at91sam9261";
+
#if defined(CONFIG_FB_ATMEL_STN)
at91_set_A_periph(AT91_PIN_PB0, 0); /* LCDVSYNC */
at91_set_A_periph(AT91_PIN_PB1, 0); /* LCDHSYNC */
diff --git a/arch/arm/mach-at91/at91sam9263_devices.c b/arch/arm/mach-at91/at91sam9263_devices.c
index ed666f5..a34f39a 100644
--- a/arch/arm/mach-at91/at91sam9263_devices.c
+++ b/arch/arm/mach-at91/at91sam9263_devices.c
@@ -848,7 +848,7 @@ static struct resource lcdc_resources[] = {
};
static struct platform_device at91_lcdc_device = {
- .name = "atmel_lcdfb",
+ .name = "fb-at91sam9263",
.id = 0,
.dev = {
.dma_mask = &lcdc_dmamask,
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 827c9f2..1d5cc51 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -981,7 +981,6 @@ static struct resource lcdc_resources[] = {
};
static struct platform_device at91_lcdc_device = {
- .name = "atmel_lcdfb",
.id = 0,
.dev = {
.dma_mask = &lcdc_dmamask,
@@ -997,6 +996,11 @@ void __init at91_add_device_lcdc(struct atmel_lcdfb_info *data)
if (!data)
return;
+ if (cpu_is_at91sam9g45es())
+ at91_lcdc_device.name = "fb-at91sam9g45es";
+ else
+ at91_lcdc_device.name = "fb-at91sam9g45";
+
at91_set_A_periph(AT91_PIN_PE0, 0); /* LCDDPWR */
at91_set_A_periph(AT91_PIN_PE2, 0); /* LCDCC */
diff --git a/arch/arm/mach-at91/at91sam9rl_devices.c b/arch/arm/mach-at91/at91sam9rl_devices.c
index ddf223f..13cac0a 100644
--- a/arch/arm/mach-at91/at91sam9rl_devices.c
+++ b/arch/arm/mach-at91/at91sam9rl_devices.c
@@ -514,7 +514,7 @@ static struct resource lcdc_resources[] = {
};
static struct platform_device at91_lcdc_device = {
- .name = "atmel_lcdfb",
+ .name = "fb-at91sam9rl",
.id = 0,
.dev = {
.dma_mask = &lcdc_dmamask,
diff --git a/arch/avr32/mach-at32ap/at32ap700x.c b/arch/avr32/mach-at32ap/at32ap700x.c
index b323d8d..5cdaa07 100644
--- a/arch/avr32/mach-at32ap/at32ap700x.c
+++ b/arch/avr32/mach-at32ap/at32ap700x.c
@@ -1530,6 +1530,8 @@ at32_add_device_lcdc(unsigned int id, struct atmel_lcdfb_info *data,
memcpy(info, data, sizeof(struct atmel_lcdfb_info));
info->default_monspecs = monspecs;
+ pdev->name = "fb-at32ap";
+
platform_device_register(pdev);
return pdev;
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 347bab2..5ad49ed 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -34,6 +34,81 @@
#define ATMEL_LCDC_DMA_BURST_LEN 8 /* words */
#define ATMEL_LCDC_FIFO_SIZE 512 /* words */
+struct atmel_lcdfb_config {
+ bool have_alt_pixclock;
+ bool have_bus_clk;
+ bool have_hozval;
+ bool have_intensity_bit;
+};
+
+static struct atmel_lcdfb_config at91sam9261_config = {
+ .have_bus_clk = true,
+ .have_intensity_bit = true,
+ .have_hozval = true,
+};
+
+static struct atmel_lcdfb_config at91sam9263_config = {
+ .have_intensity_bit = true,
+};
+
+static struct atmel_lcdfb_config at91sam9g10_config = {
+ .have_bus_clk = true,
+ .have_hozval = true,
+};
+
+static struct atmel_lcdfb_config at91sam9g45_config = {
+ .have_alt_pixclock = true,
+};
+
+static struct atmel_lcdfb_config at91sam9g45es_config = {
+};
+
+static struct atmel_lcdfb_config at91sam9rl_config = {
+ .have_intensity_bit = true,
+};
+
+static struct atmel_lcdfb_config at32ap_config = {
+ .have_bus_clk = true,
+ .have_hozval = true,
+};
+
+static const struct platform_device_id atmel_lcdfb_devtypes[] = {
+ {
+ .name = "fb-at91sam9261",
+ .driver_data = (unsigned long)&at91sam9261_config,
+ }, {
+ .name = "fb-at91sam9263",
+ .driver_data = (unsigned long)&at91sam9263_config,
+ }, {
+ .name = "fb-at91sam9g10",
+ .driver_data = (unsigned long)&at91sam9g10_config,
+ }, {
+ .name = "fb-at91sam9g45",
+ .driver_data = (unsigned long)&at91sam9g45_config,
+ }, {
+ .name = "fb-at91sam9g45es",
+ .driver_data = (unsigned long)&at91sam9g45es_config,
+ }, {
+ .name = "fb-at91sam9rl",
+ .driver_data = (unsigned long)&at91sam9rl_config,
+ }, {
+ .name = "fb-at32ap",
+ .driver_data = (unsigned long)&at32ap_config,
+ }, {
+ /* terminator */
+ }
+};
+
+static struct atmel_lcdfb_config *
+atmel_lcdfb_get_config(struct platform_device *pdev)
+{
+ unsigned long data;
+
+ data = platform_get_device_id(pdev)->driver_data;
+
+ return (struct atmel_lcdfb_config *)data;
+}
+
#if defined(CONFIG_ARCH_AT91)
#define ATMEL_LCDFB_FBINFO_DEFAULT (FBINFO_DEFAULT \
| FBINFO_PARTIAL_PAN_OK \
@@ -199,8 +274,7 @@ static unsigned long compute_hozval(struct atmel_lcdfb_info *sinfo,
unsigned long lcdcon2;
unsigned long value;
- if (!(cpu_is_at91sam9261() || cpu_is_at91sam9g10()
- || cpu_is_at32ap7000()))
+ if (!sinfo->config->have_hozval)
return xres;
lcdcon2 = lcdc_readl(sinfo, ATMEL_LCDC_LCDCON2);
@@ -426,7 +500,7 @@ static int atmel_lcdfb_check_var(struct fb_var_screeninfo *var,
break;
case 16:
/* Older SOCs use IBGR:555 rather than BGR:565. */
- if (sinfo->have_intensity_bit)
+ if (sinfo->config->have_intensity_bit)
var->green.length = 5;
else
var->green.length = 6;
@@ -534,7 +608,7 @@ static int atmel_lcdfb_set_par(struct fb_info *info)
/* Now, the LCDC core... */
/* Set pixel clock */
- if (cpu_is_at91sam9g45() && !cpu_is_at91sam9g45es())
+ if (sinfo->config->have_alt_pixclock)
pix_factor = 1;
clk_value_khz = clk_get_rate(sinfo->lcdc_clk) / 1000;
@@ -685,7 +759,7 @@ static int atmel_lcdfb_setcolreg(unsigned int regno, unsigned int red,
case FB_VISUAL_PSEUDOCOLOR:
if (regno < 256) {
- if (sinfo->have_intensity_bit) {
+ if (sinfo->config->have_intensity_bit) {
/* old style I+BGR:555 */
val = ((red >> 11) & 0x001f);
val |= ((green >> 6) & 0x03e0);
@@ -875,10 +949,9 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev)
}
sinfo->info = info;
sinfo->pdev = pdev;
- if (cpu_is_at91sam9261() || cpu_is_at91sam9263() ||
- cpu_is_at91sam9rl()) {
- sinfo->have_intensity_bit = true;
- }
+ sinfo->config = atmel_lcdfb_get_config(pdev);
+ if (!sinfo->config)
+ goto free_info;
strcpy(info->fix.id, sinfo->pdev->name);
info->flags = ATMEL_LCDFB_FBINFO_DEFAULT;
@@ -889,8 +962,7 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev)
info->fix = atmel_lcdfb_fix;
/* Enable LCDC Clocks */
- if (cpu_is_at91sam9261() || cpu_is_at91sam9g10()
- || cpu_is_at32ap7000()) {
+ if (sinfo->config->have_bus_clk) {
sinfo->bus_clk = clk_get(dev, "hck1");
if (IS_ERR(sinfo->bus_clk)) {
ret = PTR_ERR(sinfo->bus_clk);
@@ -1152,7 +1224,7 @@ static struct platform_driver atmel_lcdfb_driver = {
.remove = __exit_p(atmel_lcdfb_remove),
.suspend = atmel_lcdfb_suspend,
.resume = atmel_lcdfb_resume,
-
+ .id_table = atmel_lcdfb_devtypes,
.driver = {
.name = "atmel_lcdfb",
.owner = THIS_MODULE,
diff --git a/include/video/atmel_lcdc.h b/include/video/atmel_lcdc.h
index 8deb226..0f5a2fc 100644
--- a/include/video/atmel_lcdc.h
+++ b/include/video/atmel_lcdc.h
@@ -31,6 +31,7 @@
#define ATMEL_LCDC_WIRING_BGR 0
#define ATMEL_LCDC_WIRING_RGB 1
+struct atmel_lcdfb_config;
/* LCD Controller info data structure, stored in device platform_data */
struct atmel_lcdfb_info {
@@ -61,7 +62,8 @@ struct atmel_lcdfb_info {
void (*atmel_lcdfb_power_control)(int on);
struct fb_monspecs *default_monspecs;
u32 pseudo_palette[16];
- bool have_intensity_bit;
+
+ struct atmel_lcdfb_config *config;
};
#define ATMEL_LCDC_DMABADDR1 0x00
--
1.8.1.1
^ permalink raw reply related
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Greg KH @ 2013-02-05 17:22 UTC (permalink / raw)
To: Alexander Holler
Cc: Andrew Morton, linux-kernel, linux-fbdev,
Florian Tobias Schandinat, Bernie Thompson, Steve Glendinning,
stable
In-Reply-To: <5110AFEC.8020406@ahsoftware.de>
On Tue, Feb 05, 2013 at 08:08:28AM +0100, Alexander Holler wrote:
> Am 04.02.2013 20:25, schrieb Greg KH:
> > On Mon, Feb 04, 2013 at 08:17:04PM +0100, Alexander Holler wrote:
> >> Am 04.02.2013 13:05, schrieb Alexander Holler:
> >>> Am 04.02.2013 02:14, schrieb Greg KH:
> >>>
> >>>> So you are right in that your driver will wait for forever for a
> >>>> disconnect() to happen, as it will never be called. I don't understand
> >>>> the problem that this is causing when it happens. What's wrong with
> >>>> udlfb that having the cpu suddently reset as the powerdown happened
> >>>> without it knowing about it?
> >>>
> >>> There is nothing wrong with that. I've just explained why a problem
> >>> doesn't occur on shutdown but on disconnect (of the device).
> >>
> >> Maybe my explanation before was just to long and I try to explain it
> >> a bit shorter:
> >>
> >> If a device gets disconnected, the disconnect in udlfb might wait
> >> forever in down_interruptible() (because it waits for an urb it
> >> never receives). This even prevents a shutdown afterwards, because
> >> that down_interruptible() never receives a signal (at shutdown,
> >> because kernel threads don't get such).
> >
> > Where was that urb when the disconnect happened? The USB core should
> > call your urb callback for any outstanding urbs at that point in time,
> > with the proper error flag being set, are you handling that properly?
>
> I don't know where that urb is as I don't handle it.
What do you mean by that? The urb is being sent back to your driver,
right? If not, that's a bug, but please be sure that your urb callback
isn't really being called.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-02-05 17:36 UTC (permalink / raw)
To: Greg KH
Cc: Andrew Morton, linux-kernel, linux-fbdev,
Florian Tobias Schandinat, Bernie Thompson, Steve Glendinning,
stable
In-Reply-To: <20130205172245.GA1426@kroah.com>
Am 05.02.2013 18:22, schrieb Greg KH:
> On Tue, Feb 05, 2013 at 08:08:28AM +0100, Alexander Holler wrote:
>> Am 04.02.2013 20:25, schrieb Greg KH:
>>> Where was that urb when the disconnect happened? The USB core should
>>> call your urb callback for any outstanding urbs at that point in time,
>>> with the proper error flag being set, are you handling that properly?
>>
>> I don't know where that urb is as I don't handle it.
>
> What do you mean by that? The urb is being sent back to your driver,
> right? If not, that's a bug, but please be sure that your urb callback
> isn't really being called.
I meant it isn't _my_ driver. ;)
I'm just trying to add some würgarounds without having the need to
rewrite the whole driver.
In regard to that "urb missing problem", I think I've just named it
wrong and the actual problem is a race-condition between the semaphore
handling (which is used to keep track of the urbs) and the urb handling
inside the driver.
But I've just switched to udl (instead of udlfb) and will see if I can
fix the bugs there to make it usable as a console. udl is a rewrite of
udlfb with some additional features (e.g. drm), so hopefully fixing the
remaining problems there will require less work.
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH v17 4/7] fbmon: add videomode helpers
From: Steffen Trumtrar @ 2013-02-05 18:29 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Mohammed, Afzal', 'Florian Tobias Schandinat',
'Dave Airlie', devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
'Tomi Valkeinen', 'Laurent Pinchart',
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, 'Guennady Liakhovetski',
linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <003401ce005e$af665c50$0e3314f0$%han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Hi!
On Fri, Feb 01, 2013 at 06:29:50PM +0900, Jingoo Han wrote:
> On Friday, January 25, 2013 6:02 PM, Steffen Trumtrar wrote
> >
> > + fbmode->sync = 0;
> > + fbmode->vmode = 0;
> > + if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH)
> > + fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > + if (vm->dmt_flags & VESA_DMT_HSYNC_HIGH)
>
> Um, it seems to be a type. 'H'SYNC -> 'V'SYNC
> Thus, it would be changed as below:
>
> VESA_DMT_HSYNC_HIGH -> VESA_DMT_VSYNC_HIGH
Damn. You are right, that is a typo. But I guess some maintainer (Dave) really,
really wants to take the series now and this can wait for an -rc. No?! ;-)
Thanks,
Steffen
>
> > + fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > + if (vm->data_flags & DISPLAY_FLAGS_INTERLACED)
> > + fbmode->vmode |= FB_VMODE_INTERLACED;
> > + if (vm->data_flags & DISPLAY_FLAGS_DOUBLESCAN)
> > + fbmode->vmode |= FB_VMODE_DOUBLE;
> > + fbmode->flag = 0;
> > +
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH 5/5] ARM: at91/avr32/atmel_lcdfb: replace cpu_is macros with device-id table
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-02-05 20:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1360071315-4032-6-git-send-email-jhovold@gmail.com>
On 14:35 Tue 05 Feb , Johan Hovold wrote:
> Remove cpu_is macros from atmel lcdfb driver and use platform-device-id
> table to determine platform configuration parameters.
>
> The currently used configuration parameters are:
>
> have_alt_pixclock
> - SOC uses an alternate pixel-clock calculation formula (at91sam9g45
> non-ES)
>
> have_bus_clk
> - SOC has bus clock hck1 (at91sam9261, at921sam9g10 and at32ap)
no provide a clkdev a we do for macb
>
> have_hozval
> - SOC has a HOZVAL field in LCDFRMCFG which is used to determine the
> linesize for STN displays (at91sam9261, at921sam9g10 and at32ap)
>
> have_intensity_bit
> - SOC uses IBGR:555 rather than BGR:565 16-bit pixel layout
> (at91sam9261, at91sam9263 and at91sam9rl)
>
> Tested on at91sam9g45, compile-tested for other AT91 SOCs, and untested
> for AVR32.
>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> arch/arm/mach-at91/at91sam9261_devices.c | 6 +-
> arch/arm/mach-at91/at91sam9263_devices.c | 2 +-
> arch/arm/mach-at91/at91sam9g45_devices.c | 6 +-
> arch/arm/mach-at91/at91sam9rl_devices.c | 2 +-
> arch/avr32/mach-at32ap/at32ap700x.c | 2 +
> drivers/video/atmel_lcdfb.c | 96 ++++++++++++++++++++++++++++----
> include/video/atmel_lcdc.h | 4 +-
> 7 files changed, 101 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/mach-at91/at91sam9261_devices.c b/arch/arm/mach-at91/at91sam9261_devices.c
> index 92e0f86..01647cb 100644
> --- a/arch/arm/mach-at91/at91sam9261_devices.c
> +++ b/arch/arm/mach-at91/at91sam9261_devices.c
> @@ -488,7 +488,6 @@ static struct resource lcdc_resources[] = {
> };
>
> static struct platform_device at91_lcdc_device = {
> - .name = "atmel_lcdfb",
> .id = 0,
> .dev = {
> .dma_mask = &lcdc_dmamask,
> @@ -505,6 +504,11 @@ void __init at91_add_device_lcdc(struct atmel_lcdfb_info *data)
> return;
> }
>
> + if (cpu_is_at91sam9g10())
> + at91_lcdc_device.name = "fb-at91sam9g10";
use this
at91sam9g10-lcdfb
as we will use for dt
> + else
> + at91_lcdc_device.name = "fb-at91sam9261";
> +
> #if defined(CONFIG_FB_ATMEL_STN)
> at91_set_A_periph(AT91_PIN_PB0, 0); /* LCDVSYNC */
> at91_set_A_periph(AT91_PIN_PB1, 0); /* LCDHSYNC */
> diff --git a/arch/arm/mach-at91/at91sam9263_devices.c b/arch/arm/mach-at91/at91sam9263_devices.c
> index ed666f5..a34f39a 100644
> --- a/arch/arm/mach-at91/at91sam9263_devices.c
> +++ b/arch/arm/mach-at91/at91sam9263_devices.c
> @@ -848,7 +848,7 @@ static struct resource lcdc_resources[] = {
> };
>
> static struct platform_device at91_lcdc_device = {
> - .name = "atmel_lcdfb",
> + .name = "fb-at91sam9263",
> .id = 0,
> .dev = {
> .dma_mask = &lcdc_dmamask,
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index 827c9f2..1d5cc51 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -981,7 +981,6 @@ static struct resource lcdc_resources[] = {
> };
>
> static struct platform_device at91_lcdc_device = {
> - .name = "atmel_lcdfb",
> .id = 0,
> .dev = {
> .dma_mask = &lcdc_dmamask,
> @@ -997,6 +996,11 @@ void __init at91_add_device_lcdc(struct atmel_lcdfb_info *data)
> if (!data)
> return;
>
> + if (cpu_is_at91sam9g45es())
> + at91_lcdc_device.name = "fb-at91sam9g45es";
> + else
> + at91_lcdc_device.name = "fb-at91sam9g45";
> +
> at91_set_A_periph(AT91_PIN_PE0, 0); /* LCDDPWR */
>
> at91_set_A_periph(AT91_PIN_PE2, 0); /* LCDCC */
> diff --git a/arch/arm/mach-at91/at91sam9rl_devices.c b/arch/arm/mach-at91/at91sam9rl_devices.c
> index ddf223f..13cac0a 100644
> --- a/arch/arm/mach-at91/at91sam9rl_devices.c
> +++ b/arch/arm/mach-at91/at91sam9rl_devices.c
> @@ -514,7 +514,7 @@ static struct resource lcdc_resources[] = {
> };
>
> static struct platform_device at91_lcdc_device = {
> - .name = "atmel_lcdfb",
> + .name = "fb-at91sam9rl",
> .id = 0,
> .dev = {
> .dma_mask = &lcdc_dmamask,
> diff --git a/arch/avr32/mach-at32ap/at32ap700x.c b/arch/avr32/mach-at32ap/at32ap700x.c
> index b323d8d..5cdaa07 100644
> --- a/arch/avr32/mach-at32ap/at32ap700x.c
> +++ b/arch/avr32/mach-at32ap/at32ap700x.c
> @@ -1530,6 +1530,8 @@ at32_add_device_lcdc(unsigned int id, struct atmel_lcdfb_info *data,
> memcpy(info, data, sizeof(struct atmel_lcdfb_info));
> info->default_monspecs = monspecs;
>
> + pdev->name = "fb-at32ap";
> +
> platform_device_register(pdev);
> return pdev;
>
> diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
> index 347bab2..5ad49ed 100644
> --- a/drivers/video/atmel_lcdfb.c
> +++ b/drivers/video/atmel_lcdfb.c
> @@ -34,6 +34,81 @@
> #define ATMEL_LCDC_DMA_BURST_LEN 8 /* words */
> #define ATMEL_LCDC_FIFO_SIZE 512 /* words */
>
> +struct atmel_lcdfb_config {
> + bool have_alt_pixclock;
> + bool have_bus_clk;
> + bool have_hozval;
> + bool have_intensity_bit;
> +};
> +
> +static struct atmel_lcdfb_config at91sam9261_config = {
> + .have_bus_clk = true,
> + .have_intensity_bit = true,
> + .have_hozval = true,
> +};
> +
> +static struct atmel_lcdfb_config at91sam9263_config = {
> + .have_intensity_bit = true,
> +};
> +
> +static struct atmel_lcdfb_config at91sam9g10_config = {
> + .have_bus_clk = true,
> + .have_hozval = true,
> +};
> +
> +static struct atmel_lcdfb_config at91sam9g45_config = {
> + .have_alt_pixclock = true,
> +};
> +
> +static struct atmel_lcdfb_config at91sam9g45es_config = {
> +};
> +
> +static struct atmel_lcdfb_config at91sam9rl_config = {
> + .have_intensity_bit = true,
> +};
> +
> +static struct atmel_lcdfb_config at32ap_config = {
> + .have_bus_clk = true,
> + .have_hozval = true,
> +};
> +
> +static const struct platform_device_id atmel_lcdfb_devtypes[] = {
> + {
> + .name = "fb-at91sam9261",
> + .driver_data = (unsigned long)&at91sam9261_config,
> + }, {
> + .name = "fb-at91sam9263",
> + .driver_data = (unsigned long)&at91sam9263_config,
> + }, {
> + .name = "fb-at91sam9g10",
> + .driver_data = (unsigned long)&at91sam9g10_config,
> + }, {
> + .name = "fb-at91sam9g45",
> + .driver_data = (unsigned long)&at91sam9g45_config,
> + }, {
> + .name = "fb-at91sam9g45es",
> + .driver_data = (unsigned long)&at91sam9g45es_config,
> + }, {
> + .name = "fb-at91sam9rl",
> + .driver_data = (unsigned long)&at91sam9rl_config,
> + }, {
> + .name = "fb-at32ap",
> + .driver_data = (unsigned long)&at32ap_config,
> + }, {
> + /* terminator */
> + }
> +};
> +
> +static struct atmel_lcdfb_config *
> +atmel_lcdfb_get_config(struct platform_device *pdev)
> +{
> + unsigned long data;
> +
> + data = platform_get_device_id(pdev)->driver_data;
> +
> + return (struct atmel_lcdfb_config *)data;
> +}
> +
> #if defined(CONFIG_ARCH_AT91)
> #define ATMEL_LCDFB_FBINFO_DEFAULT (FBINFO_DEFAULT \
> | FBINFO_PARTIAL_PAN_OK \
> @@ -199,8 +274,7 @@ static unsigned long compute_hozval(struct atmel_lcdfb_info *sinfo,
> unsigned long lcdcon2;
> unsigned long value;
>
> - if (!(cpu_is_at91sam9261() || cpu_is_at91sam9g10()
> - || cpu_is_at32ap7000()))
> + if (!sinfo->config->have_hozval)
> return xres;
>
> lcdcon2 = lcdc_readl(sinfo, ATMEL_LCDC_LCDCON2);
> @@ -426,7 +500,7 @@ static int atmel_lcdfb_check_var(struct fb_var_screeninfo *var,
> break;
> case 16:
> /* Older SOCs use IBGR:555 rather than BGR:565. */
> - if (sinfo->have_intensity_bit)
> + if (sinfo->config->have_intensity_bit)
> var->green.length = 5;
> else
> var->green.length = 6;
> @@ -534,7 +608,7 @@ static int atmel_lcdfb_set_par(struct fb_info *info)
> /* Now, the LCDC core... */
>
> /* Set pixel clock */
> - if (cpu_is_at91sam9g45() && !cpu_is_at91sam9g45es())
> + if (sinfo->config->have_alt_pixclock)
> pix_factor = 1;
>
> clk_value_khz = clk_get_rate(sinfo->lcdc_clk) / 1000;
> @@ -685,7 +759,7 @@ static int atmel_lcdfb_setcolreg(unsigned int regno, unsigned int red,
>
> case FB_VISUAL_PSEUDOCOLOR:
> if (regno < 256) {
> - if (sinfo->have_intensity_bit) {
> + if (sinfo->config->have_intensity_bit) {
> /* old style I+BGR:555 */
> val = ((red >> 11) & 0x001f);
> val |= ((green >> 6) & 0x03e0);
> @@ -875,10 +949,9 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev)
> }
> sinfo->info = info;
> sinfo->pdev = pdev;
> - if (cpu_is_at91sam9261() || cpu_is_at91sam9263() ||
> - cpu_is_at91sam9rl()) {
> - sinfo->have_intensity_bit = true;
> - }
> + sinfo->config = atmel_lcdfb_get_config(pdev);
> + if (!sinfo->config)
> + goto free_info;
>
> strcpy(info->fix.id, sinfo->pdev->name);
> info->flags = ATMEL_LCDFB_FBINFO_DEFAULT;
> @@ -889,8 +962,7 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev)
> info->fix = atmel_lcdfb_fix;
>
> /* Enable LCDC Clocks */
> - if (cpu_is_at91sam9261() || cpu_is_at91sam9g10()
> - || cpu_is_at32ap7000()) {
> + if (sinfo->config->have_bus_clk) {
> sinfo->bus_clk = clk_get(dev, "hck1");
> if (IS_ERR(sinfo->bus_clk)) {
> ret = PTR_ERR(sinfo->bus_clk);
> @@ -1152,7 +1224,7 @@ static struct platform_driver atmel_lcdfb_driver = {
> .remove = __exit_p(atmel_lcdfb_remove),
> .suspend = atmel_lcdfb_suspend,
> .resume = atmel_lcdfb_resume,
> -
> + .id_table = atmel_lcdfb_devtypes,
> .driver = {
> .name = "atmel_lcdfb",
> .owner = THIS_MODULE,
> diff --git a/include/video/atmel_lcdc.h b/include/video/atmel_lcdc.h
> index 8deb226..0f5a2fc 100644
> --- a/include/video/atmel_lcdc.h
> +++ b/include/video/atmel_lcdc.h
> @@ -31,6 +31,7 @@
> #define ATMEL_LCDC_WIRING_BGR 0
> #define ATMEL_LCDC_WIRING_RGB 1
>
> +struct atmel_lcdfb_config;
>
> /* LCD Controller info data structure, stored in device platform_data */
> struct atmel_lcdfb_info {
> @@ -61,7 +62,8 @@ struct atmel_lcdfb_info {
> void (*atmel_lcdfb_power_control)(int on);
> struct fb_monspecs *default_monspecs;
> u32 pseudo_palette[16];
> - bool have_intensity_bit;
> +
> + struct atmel_lcdfb_config *config;
> };
>
> #define ATMEL_LCDC_DMABADDR1 0x00
> --
> 1.8.1.1
>
^ permalink raw reply
* Re: 答复: [PATCH]Siliconmotion initial patch
From: Daniel Vetter @ 2013-02-05 21:04 UTC (permalink / raw)
To: Aaron.Chen 陈俊杰
Cc: Greg KH, Linux Fbdev development list,
caesar.qiu 裘赛海, dri-devel
In-Reply-To: <4CE6A5494DEECD498EBCD4C5488B1AFCEAC6F7@CNDC08.cn.smi.ad>
On Tue, Feb 05, 2013 at 03:51:55PM +0800, Aaron.Chen 陈俊杰 wrote:
> This is an initial patch for Siliconmotion Graphics chips. It add SM750
> chip support with 2d accelerate and hw curser support. It is a complete
> new driver. So the patch is a little bit big. Is it OK for review?
Adding a new fbdev driver while established consensus pretty much boils
down to fbdev being a dead subsystem which should only be used for legacy
applications and drivers: Not good. You should implement this as a drm
modesetting driver, and if required base your 2d acceleration on top of
the drm fb helpers. Or just implement a drm/gem driver to expose this.
Additionally you should include the appropriate mailing lists, which is
linux-fbdev for fbdev drivers.
On a very quick read the patch has serious issues:
- checkpatch.pl:
total: 2 errors, 779 warnings, 7961 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
- Remants of hungarian notation (dunno whether checkpatch checks for
those, too much other stuff flying by, but it really stuck out).
- Not using the linux i2c layer in the ddk750_hwi2c.c file.
- Reimplementing i2c bit-banging code in the ddk750_swi2c.c
- Adding a new implementation of a sil 164 dvi driver, we already have at
least two in drivers/gpu/drm/i2c/sil164_drv.c and
drivers/gpu/drm/i915/dvo_sil164.c. This should be unified and probably
use the drm encoder slave framework.
At that point I've given up on further review, since there's clearly a lot
of work left to do. Please review SubmittingPatches from the kernel
documentation carefully. Also cc'ing our dear staging maintainer, he
should be able to point you at further useful resources for getting this
going.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: 答复: [PATCH]Siliconmotion initial patch
From: Greg KH @ 2013-02-05 21:19 UTC (permalink / raw)
To: Daniel Vetter
Cc: Linux Fbdev development list, caesar.qiu 裘赛海,
dri-devel, Aaron.Chen 陈俊杰
In-Reply-To: <20130205210427.GF5813@phenom.ffwll.local>
On Tue, Feb 05, 2013 at 10:04:27PM +0100, Daniel Vetter wrote:
> On Tue, Feb 05, 2013 at 03:51:55PM +0800, Aaron.Chen 陈俊杰 wrote:
> > This is an initial patch for Siliconmotion Graphics chips. It add SM750
> > chip support with 2d accelerate and hw curser support. It is a complete
> > new driver. So the patch is a little bit big. Is it OK for review?
>
> Adding a new fbdev driver while established consensus pretty much boils
> down to fbdev being a dead subsystem which should only be used for legacy
> applications and drivers: Not good. You should implement this as a drm
> modesetting driver, and if required base your 2d acceleration on top of
> the drm fb helpers. Or just implement a drm/gem driver to expose this.
>
> Additionally you should include the appropriate mailing lists, which is
> linux-fbdev for fbdev drivers.
>
> On a very quick read the patch has serious issues:
>
> - checkpatch.pl:
>
> total: 2 errors, 779 warnings, 7961 lines checked
>
> NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
> scripts/cleanfile
>
> - Remants of hungarian notation (dunno whether checkpatch checks for
> those, too much other stuff flying by, but it really stuck out).
>
> - Not using the linux i2c layer in the ddk750_hwi2c.c file.
>
> - Reimplementing i2c bit-banging code in the ddk750_swi2c.c
>
> - Adding a new implementation of a sil 164 dvi driver, we already have at
> least two in drivers/gpu/drm/i2c/sil164_drv.c and
> drivers/gpu/drm/i915/dvo_sil164.c. This should be unified and probably
> use the drm encoder slave framework.
>
> At that point I've given up on further review, since there's clearly a lot
> of work left to do. Please review SubmittingPatches from the kernel
> documentation carefully. Also cc'ing our dear staging maintainer, he
> should be able to point you at further useful resources for getting this
> going.
If you wish to put this in the drivers/staging/ directory and do the
cleanup there before it moves to the "real" part of the kernel, I have
no objection to that.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC v2 0/5] Common Display Framework
From: Archit Taneja @ 2013-02-06 9:52 UTC (permalink / raw)
To: Marcus Lorentzon
Cc: Laurent Pinchart, Thomas Petazzoni, linux-fbdev@vger.kernel.org,
Vikas Sajjan, Benjamin Gaignard, Tom Gall, Kyungmin Park,
dri-devel@lists.freedesktop.org, Rob Clark, Ragesh Radhakrishnan,
Tomi Valkeinen, Bryan Wu, Maxime Ripard, sunil joshi,
Sumit Semwal, Sebastien Guiriec, linux-media@vger.kernel.org
In-Reply-To: <510F8807.2020406@stericsson.com>
On Monday 04 February 2013 03:35 PM, Marcus Lorentzon wrote:
> On 02/02/2013 12:35 AM, Laurent Pinchart wrote:
>> Hi Marcus,
>>
>> On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote:
>>> On 01/08/2013 05:36 PM, Tomasz Figa wrote:
>>>> On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:
> [...]
>>>>> But it is not perfect. After a couple of products we realized that
>>>>> most
>>>>> panel drivers want an easy way to send a bunch of init commands in one
>>>>> go. So I think it should be an op for sending an array of commands at
>>>>> once. Something like
>>>>>
>>>>> struct dsi_cmd {
>>>>> enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
>>>>> u8 cmd;
>>>>> int dataLen;
>>>>> u8 *data;
>>>>> }
>>>>>
>>>>> struct dsi_ops {
>>>>> int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
>>>>> ...
>>>>> }
>> Do you have DSI IP(s) that can handle a list of commands ? Or would
>> all DSI
>> transmitter drivers need to iterate over the commands manually ? In
>> the later
>> case a lower-level API might be easier to implement in DSI transmitter
>> drivers. Helper functions could provide the higher-level API you
>> proposed.
>
> The HW has a FIFO, so it can handle a few. Currently we use the low
> level type of call with one call per command. But we have found DSI
> command mode panels that don't accept any commands during the "update"
> (write start+continues). And so we must use a mutex/state machine to
> exclude any async calls to send DSI commands during update. But if you
> need to send more than one command per frame this will be hard (like
> CABC and backlight commands). It will be a ping pong between update and
> command calls. One option is to expose the mutex to the caller so it can
> make many calls before the next update grabs the mutex again.
> So maybe we could create a helper that handle the op for list of
> commands and another op for single command that you actually have to
> implement.
fyi, the DSI IP on OMAP3+ SoCs also has a FIFO. It can provide
interrupts after each command is pushed out, and also when the FIFO gets
empty(all commands are pushed). The only thing to take care is to not
overflow FIFO.
DSI video mode panels generally have a few dozen internal registers
which need to be configured via DSI commands. It's more fast(and
convenient) to configure a handful of internal registers in one shot,
and then perform a single BTA to know from the panel whether the
commands were received correctly.
Regards,
Archit
^ permalink raw reply
* Re: CDF meeting @FOSDEM report
From: Alex Deucher @ 2013-02-06 14:44 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thomas Petazzoni, linux-fbdev, Tom Gall, Stephen Warren,
Thierry Reding, Mark Zhang, dri-devel, Sunil Joshi, linaro-mm-sig,
Stéphane Marchesin, Kyungmin Park, Jesse Barnes,
Laurent Pinchart, Sebastien Guiriec, Alexandre Courbot,
Maxime Ripard, Vikas Sajjan, Sumit Semwal, Ragesh Radhakrishnan,
linux-media
In-Reply-To: <51123A5F.9050604@ti.com>
On Wed, Feb 6, 2013 at 6:11 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
> On 2013-02-06 00:27, Laurent Pinchart wrote:
>> Hello,
>>
>> We've hosted a CDF meeting at the FOSDEM on Sunday morning. Here's a summary
>> of the discussions.
>
> Thanks for the summary. I've been on a longish leave, and just got back,
> so I haven't read the recent CDF discussions on lists yet. I thought
> I'll start by replying to this summary first =).
>
>> 0. Abbreviations
>> ----------------
>>
>> DBI - Display Bus Interface, a parallel video control and data bus that
>> transmits data using parallel data, read/write, chip select and address
>> signals, similarly to 8051-style microcontroller parallel busses. This is a
>> mixed video control and data bus.
>>
>> DPI - Display Pixel Interface, a parallel video data bus that transmits data
>> using parallel data, h/v sync and clock signals. This is a video data bus
>> only.
>>
>> DSI - Display Serial Interface, a serial video control and data bus that
>> transmits data using one or more differential serial lines. This is a mixed
>> video control and data bus.
>
> In case you'll re-use these abbrevs in later posts, I think it would be
> good to mention that DPI is a one-way bus, whereas DBI and DSI are
> two-way (perhaps that's implicit with control bus, though).
>
>> 1. Goals
>> --------
>>
>> The meeting started with a brief discussion about the CDF goals.
>>
>> Tomi Valkeinin and Tomasz Figa have sent RFC patches to show their views of
>> what CDF could/should be. Many others have provided very valuable feedback.
>> Given the early development stage propositions were sometimes contradictory,
>> and focused on different areas of interest. We have thus started the meeting
>> with a discussion about what CDF should try to achieve, and what it shouldn't.
>>
>> CDF has two main purposes. The original goal was to support display panels in
>> a platform- and subsystem-independent way. While mostly useful for embedded
>> systems, the emergence of platforms such as Intel Medfield and ARM-based PCs
>> that blends the embedded and PC worlds makes panel support useful for the PC
>> world as well.
>>
>> The second purpose is to provide a cross-subsystem interface to support video
>> encoders. The idea originally came from a generalisation of the original RFC
>> that supported panels only. While encoder support is considered as lower
>> priority than display panel support by developers focussed on display
>> controller driver (Intel, Renesas, ST Ericsson, TI), companies that produce
>> video encoders (Analog Devices, and likely others) don't share that point of
>> view and would like to provide a single encoder driver that can be used in
>> both KMS and V4L2 drivers.
>
> What is an encoder? Something that takes a video signal in, and lets the
> CPU store the received data to memory? Isn't that a decoder?
>
> Or do you mean something that takes a video signal in, and outputs a
> video signal in another format? (transcoder?)
In KMS parlance, we have two objects a crtc and an encoder. A crtc
reads data from memory and produces a data stream with display timing.
The encoder then takes that datastream and timing from the crtc and
converts it some sort of physical signal (LVDS, TMDS, DP, etc.). It's
not always a perfect match to the hardware. For example a lot of GPUs
have a DVO encoder which feeds a secondary encoder like an sil164 DVO
to TMDS encoder.
Alex
^ permalink raw reply
* Re: CDF meeting @FOSDEM report
From: Tomi Valkeinen @ 2013-02-06 15:00 UTC (permalink / raw)
To: Alex Deucher
Cc: Laurent Pinchart, linux-fbdev, Sebastien Guiriec, dri-devel,
Jesse Barnes, Benjamin Gaignard, Sumit Semwal, Tom Gall,
Kyungmin Park, linux-media, Stephen Warren, Thierry Reding,
Mark Zhang, linaro-mm-sig, Stéphane Marchesin,
Alexandre Courbot, Ragesh Radhakrishnan, Thomas Petazzoni,
Sunil Joshi, Maxime Ripard, Vikas Sajjan
In-Reply-To: <CADnq5_P1GFbAwoe9kTeARq8ZLP1tOBc9Rn1h2KrRYxkoLxLXfw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]
On 2013-02-06 16:44, Alex Deucher wrote:
> On Wed, Feb 6, 2013 at 6:11 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> What is an encoder? Something that takes a video signal in, and lets the
>> CPU store the received data to memory? Isn't that a decoder?
>>
>> Or do you mean something that takes a video signal in, and outputs a
>> video signal in another format? (transcoder?)
>
> In KMS parlance, we have two objects a crtc and an encoder. A crtc
> reads data from memory and produces a data stream with display timing.
> The encoder then takes that datastream and timing from the crtc and
> converts it some sort of physical signal (LVDS, TMDS, DP, etc.). It's
Isn't the video stream between CRTC and encoder just as physical, it
just happens to be inside the GPU?
This is the case for OMAP, at least, where DISPC could be considered
CRTC, and DSI/HDMI/etc could be considered encoder. The stream between
DISPC and DSI/HDMI is plain parallel RGB signal. The video stream could
as well be outside OMAP.
> not always a perfect match to the hardware. For example a lot of GPUs
> have a DVO encoder which feeds a secondary encoder like an sil164 DVO
> to TMDS encoder.
Right. I think mapping the DRM entities to CDF ones is one of the bigger
question marks we have with CDF. While I'm no expert on DRM, I think we
have the following options:
1. Force DRM's model to CDF, meaning one encoder.
2. Extend DRM to support multiple encoders in a chain.
3. Support multiple encoders in a chain in CDF, but somehow map them to
a single encoder in DRM side.
I really dislike the first option, as it would severely limit where CDF
can be used, or would force you to write some kind of combined drivers,
so that you can have one encoder driver running multiple encoder devices.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ 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