Menu
▾
▴
Re: [linuxsh-dev] Re: [PATCH] EC3104/Aero 8000 support, RTC generalization
|
From: Mitch D. <md...@po...> - 2000-12-26 07:39:56
|
Philipp Rumpf wrote:
>
>
Hi Philipp, Merry Christmas and congratulations on your
driver. You're using the SED1355 too eh? We've also been
using it. We wrote a framebuffer driver a couple of months ago,
but haven't had the time to release it. I guess this
means it's too late to put our driver (as it stands) into
the kernel.
Instead, maybe we can both work on your driver. We can offer
our experience with this chip to make your driver more generic
so it also works with our hardware, and to make other
improvements.
Please find attached our review to your code. I will follow
this up in a few days with some changes adapted from our code.
> diff -urNx CVS linuxsh/kernel/arch/sh/config.in linux-aero/arch/sh/config.in
> --- linuxsh/kernel/arch/sh/config.in Fri Dec 8 13:30:10 2000
> +++ linux-aero/arch/sh/config.in Mon Dec 25 03:54:17 2000
> [...]
> @@ -73,9 +76,6 @@
> mainmenu_option next_comment
> comment 'General setup'
>
> -# Obviously SuperH doesn't *really* have an ISA bus,
> -# but this variable helps the PCMCIA modules handle
> -# IRQ requesting properly -- Greg Banks.
> define_bool CONFIG_ISA y
We found we needed to define this so that IDE over PCMCIA
would work properly. But not everyone will want or need to
enable it. Whether or not we set this value, we should
explain to the reader why it should be set. (Perhaps we
should wrap CONFIG_ISA in some machine-specific checks?)
In any case, I think it's a good idea to have a comment.
> diff -urNx CVS linuxsh/kernel/drivers/video/epson1355fb.c linux-aero/drivers/video/epson1355fb.c
> --- linuxsh/kernel/drivers/video/epson1355fb.c Wed Dec 31 16:00:00 1969
> +++ linux-aero/drivers/video/epson1355fb.c Mon Dec 25 02:59:52 2000
> @@ -0,0 +1,554 @@
> +/*
> + * linux/drivers/video/epson1355fb.c
> + * -- Support for the Epson SED1355 LCD/CRT controller
> + *
> + * Copyright (C) 2000 Philipp Rumpf <pr...@tu...>
> + *
> + * based on linux/drivers/video/skeletonfb.c, which was
> + * Created 28 Dec 1997 by Geert Uytterhoeven
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file COPYING in the main directory of this archive
> + * for more details.
> + */
> +/* TODO (roughly in order of priority):
> + * 16 bpp support
> + * crt support
> + * hw cursor support
> + */
> +
> +#include <linux/init.h>
> +#include <linux/sched.h>
> +#include <linux/mm.h>
> +#include <asm/io.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/mm.h>
> +#include <linux/tty.h>
> +#include <linux/malloc.h>
> +#include <linux/delay.h>
> +#include <linux/fb.h>
> +#include <linux/init.h>
> +
> +#include <video/fbcon.h>
> +#include <video/fbcon-mfb.h>
> +#include <video/fbcon-cfb8.h>
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/mm.h>
> +#include <linux/tty.h>
> +#include <linux/malloc.h>
> +#include <linux/delay.h>
> +#include <linux/fb.h>
> +#include <linux/init.h>
> +
> +#include <video/fbcon.h>
> +
> +/* Register defines. The docs don't seem to provide nice mnemonic names
> + * so I made them up myself ... */
> +
> +#define E1355_PANEL 0x02
> +#define E1355_DISPLAY 0x0D
> +#define E1355_GPIO 0x20
> +#define E1355_LUT_INDEX 0x24
> +#define E1355_LUT_DATA 0x26
In our driver, we have used names as close as possible to the
names in the SED documentation. I believe this is in line with
the rest of our project, which uses the official names for the
SuperH and companion chip registers. If we gave you a patch
which changed the names to the official ones, would you accept it?
> +
> +#ifdef CONFIG_SUPERH
> +/* tell me if your machine has a different base address .. */
> +#define E1355_REG_BASE 0xB0000000
> +#define E1355_FB_BASE 0xB0200000
We thought others might want to use this chip at other locations,
so our driver has these two values as config variables. (The values
for our hardware are 0xA8000000 and 0xA8200000 respectively. In
our hardware, the 0xB0000000 region is occupied by the HD64465
companion chip). Would you like a patch to make these config
variables?
> +static void e1355_detect(void)
> +{
> + u8 rev;
> +
> + rev = e1355_read_reg(0x00);
> +
> + if ((rev & 0xfc) != 0x0c) {
> + printk(KERN_WARNING "Epson 1355 not detected\n");
> + }
> +
> + e1355_encode_var(&default_var, NULL, NULL);
> +}
The Epson docs say you're supposed to enable the SED's bus interface
first by writing 0x00 to register 0x1b. We found this to be true: it's
necessary to do it. Is that perhaps being done by some kind of BIOS
or other code in your setup? In any case, I think we should add the
enabling code. It won't hurt your application, and we need it for ours.
Also, it appears that this function is called *after*
disable_hw_cursor() which reads and writes a SED register. The
detection routine should be the first thing that's called, because
if that region is occupied by some other hardware, random reads and
writes may confuse the other device.
Also, your code doesn't seem to check for the amount of video
RAM available. The SED chip uses an external video ram and may be
wired to either 512K or 2M of video ram. If the machine has the
smaller ram, some video modes are not possible. It would be a good
idea to add something which checks for how much memory is available.
> +static int e1355_encode_var(struct fb_var_screeninfo *var, const void *raw_par,
> + struct fb_info_gen *info)
> +{
> + u8 panel, display;
> + u32 xres, xres_virtual, yres;
> + static int width[2][4] = { { 4, 8, 16, -1 }, { 9, 12, 16, -1 } };
> + static int bpp_tab[8] = { 1, 2, 4, 8, 15, 16 };
> + int bpp, hw_bpp;
> + int is_color, is_dual, is_tft;
> + int lcd_enabled, crt_enabled;
> +
> + panel = e1355_read_reg(E1355_PANEL);
> + display = e1355_read_reg(E1355_DISPLAY);
> +
> + is_color = (panel & 0x04) != 0;
> + is_dual = (panel & 0x02) != 0;
> + is_tft = (panel & 0x01) != 0;
> +
> + bpp = bpp_tab[(display>>2)&7];
> + e1355_bpp_to_var(bpp, var);
> +
> + crt_enabled = (display & 0x02) != 0;
> + lcd_enabled = (display & 0x02) != 0;
> +
> + hw_bpp = width[is_tft][(panel>>4)&3];
> +
> + xres = e1355_read_reg(0x04) + 1;
> + yres = e1355_read_reg16(0x08) + 1;
> +
> + xres *= 8;
> + /* talk about weird hardware .. */
> + yres *= (is_dual && !crt_enabled) ? 2 : 1;
Yep ;-)
> +
> + xres_virtual = e1355_read_reg16(0x16);
This would be more readable with a suitable register
name. We will provide you with a fix if you like.
> + /* it's in 2-byte words initially */
> + xres_virtual *= 16;
> + xres_virtual /= var->bits_per_pixel;
> +
> + var->xres = xres;
> + var->yres = yres;
> + var->xres_virtual = xres_virtual;
> + var->yres_virtual = yres;
> +
> + var->xoffset = var->yoffset = 0;
> +
> + var->grayscale = !is_color;
> +
> + return 0;
I'm assuming here that you're not dealing with the
SwivelView feature yet. (If you were, I'd expect
to see (1024 - xres) somewhere). We don't deal with
SwivelView yet either, but we will in the near
future.
> +
> +static int e1355_setcolreg(unsigned regno, unsigned red, unsigned green,
> + unsigned blue, unsigned transp,
> + struct fb_info *info)
> +{
> + u8 r = (red >> 8) & 0xf0;
> + u8 g = (green>>8) & 0xf0;
> + u8 b = (blue>> 8) & 0xf0;
Do you mean 0xff? As an example problem case, with this code
it's not possible to get white (#ffffff).
Also, when reading the palette and converting the 8-bit
values to 16-bit values, you can use this trick:
/* This code linearly scales 8 bits into 16. */
v = sed_inb(LUTDR);
*red = (v << 8) + v;
v = sed_inb(LUTDR);
*green = (v << 8) + v;
v = sed_inb(LUTDR);
*blue = (v << 8) + v;
This will convert 0xff to 0xffff, etc.
> +
> +/*
> + * The AERO_HACKS parts disable/enable the backlight on the Compaq Aero 8000.
> + * I'm not sure they aren't dangerous to the hardware, so be warned.
> + */
> +#define AERO_HACKS
> +
> +static int e1355_blank(int blank_mode, struct fb_info_gen *info)
> +{
> + u8 disp;
> +
> + switch (blank_mode) {
> + case VESA_NO_BLANKING:
> + disp = e1355_read_reg(E1355_DISPLAY);
> + disp |= 1;
> + e1355_write_reg(disp, E1355_DISPLAY);
> +
> +#ifdef AERO_HACKS
> + e1355_write_reg(0x6, 0x20);
> +#endif
> + break;
> +
> + case VESA_VSYNC_SUSPEND:
> + case VESA_HSYNC_SUSPEND:
> + case VESA_POWERDOWN:
> + disp = e1355_read_reg(E1355_DISPLAY);
> + disp &= ~1;
> + e1355_write_reg(disp, E1355_DISPLAY);
> +
> +#ifdef AERO_HACKS
> + e1355_write_reg(0x0, 0x20);
> +#endif
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Our driver also needed to futz about to do hardware-specific stuff for
setting LCD contrast and brightness, so I split out that functionality into
a separate .c file with a device-like abstraction struct:
struct lcd_info
{
const char *name;
struct lcd_ops *ops;
/* TODO: type, size, resolution etc */
int contrast_min, contrast_max;
int brightness_min, brightness_max;
};
struct lcd_ops
{
void (*set_backlight)(struct lcd_info *, int on);
void (*set_brightness)(struct lcd_info *, int brightness);
void (*set_contrast)(struct lcd_info *, int contrast);
};
extern void lcd_set_backlight(struct lcd_info *, int on);
extern void lcd_set_contrast(struct lcd_info *, int percent);
extern void lcd_set_brightness(struct lcd_info *, int percent);
Our SED driver then sets these parameters by calling these last
3 functions. Does your hardware have this functionality? We can
give your driver this code.
> +
> +static struct e1355fb_info fb_info;
> +
> +int __init e1355fb_setup(char *str)
> +{
> + return 0;
> +}
> +
> +}
> +
> +
> + /*
> + * Cleanup
> + */
> +
> +void e1355fb_cleanup(struct fb_info *info)
> +{
> + /*
> + * If your driver supports multiple boards, you should unregister and
> + * clean up all instances.
> + */
> +
> + unregister_framebuffer(info);
> + /* ... */
> +}
Does this driver support being used as a module? We found that
making it a module made it much easier to test, all we had to do was
shut down the X server, rmmod and modprobe the module, and start X again.
Much quicker than restarting Linux. Our patch will include the
modularization changes.
Anyway, thank you for releasing it, and we look forward to enhancing
it with you in the future.
Greg.
--
These are my opinions not PPIs.
|
