Menu
▾
▴
Re: [linuxsh-dev] Re: [PATCH] EC3104/Aero 8000 support, RTC generalization
|
From: Philipp R. <pr...@pa...> - 2000-12-26 16:36:19
|
On Tue, Dec 26, 2000 at 06:34:58PM +1100, Mitch Davis wrote:
> Hi Philipp, Merry Christmas and congratulations on your
> driver. You're using the SED1355 too eh? We've also been
> using it.
Cool. I wasn't aware it was a popular / still current chip, which
explains some of the driver "features" you're objecting to :)
> 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.
Can you release it now ? If it's significantly cleaner/has more features
than mine, I'm not opposed to replacing it with yours at all.
> 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.
Sounds good to me :)
> 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
There are SuperH machines with ISA busses, and the current procedure seems
to be to set CONFIG_ISA when the architecture supports ISA drivers; I
think this is true for most SH machines, so it seemed like a good idea
to remove the comment since it implied real ISA busses don't occur on
SH machines
> > +/* 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?
Which names are you talking about ? I'm opposed to
E1355_LOOK_UP_TABLE_DATA_REGISTER and the like, but maybe I missed the nice
names in the documentation.
> > +
> > +#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?
Agreed.
> > +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.
Agreed. (It's enabled by Windows CE for me).
> 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.
True, though we can't handle that situation nicely anyway (we already did
a write).
> 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.
Not sure it's a real issue, since it should never cause real problems even
if we're trying to use more than 512KB of the RAM.
> > + 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.
Agreed, but I don't think the name the documentation I have uses ("memory
address offset") is suitable. I'll wait for your fix for this ;)
> 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.
Correct.
> > +
> > +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).
The LUT table has 4 bits for each of red, green, and blue only. The
lower four bits are documented as "n/a", which I interpreted as
"don't write ones to this/read as zeroes".
> 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.
Can you ever read 0xff ? I think it would be 0xf0, so your trick becomes
*red = (v>>4) * 0x1111;
Anyway, that sounds okay as well, I'd just like to know whether the LUT has
12 or 24 bit entries.
> 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
I haven't figured out how to do that on the Aero yet. In fact, I have no
idea how LCDs usually implement this.
> 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);
Why are those separate ? disabling the backlight and setting the brightness
to 0 sound like the same thing to me. Also, what is the unit for brightness ?
percent ?
> 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.
I think you should try submitting this as drivers/video/backlight.c (or
similar), since it seems to be pretty generic; I'm not opposed to using
it, though I'd like the interface to be as clean as possible.
> 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.
Okay.
> Anyway, thank you for releasing it, and we look forward to enhancing
> it with you in the future.
Thanks for your comments. I think this patch should fix some of the more
problematic issues you pointed out; does this make my driver work on your
hardware ?
diff -ur linuxsh/kernel/drivers/video/Config.in linux-aero/drivers/video/Config.in
--- linuxsh/kernel/drivers/video/Config.in Mon Dec 25 16:37:29 2000
+++ linux-aero/drivers/video/Config.in Tue Dec 26 08:09:32 2000
@@ -99,6 +99,10 @@
fi
fi
bool 'Epson 1355 framebuffer support' CONFIG_FB_E1355
+ if [ "$CONFIG_FB_E1355" != "n" ]; then
+ hex 'Register Base Address' CONFIG_E1355_REG_BASE a8000000
+ hex 'Framebuffer Base Address' CONFIG_E1355_FB_BASE a8200000
+ fi
if [ "$CONFIG_EXPERIMENTAL" = "y" ]; then
if [ "$CONFIG_PCI" != "n" ]; then
tristate ' Matrox acceleration (EXPERIMENTAL)' CONFIG_FB_MATROX
diff -ur linuxsh/kernel/drivers/video/epson1355fb.c linux-aero/drivers/video/epson1355fb.c
--- linuxsh/kernel/drivers/video/epson1355fb.c Mon Dec 25 19:02:08 2000
+++ linux-aero/drivers/video/epson1355fb.c Tue Dec 26 08:22:47 2000
@@ -15,6 +15,7 @@
* 16 bpp support
* crt support
* hw cursor support
+ * SwivelView
*/
#include <asm/io.h>
@@ -38,14 +39,14 @@
#define E1355_PANEL 0x02
#define E1355_DISPLAY 0x0D
+#define E1355_MISC 0x1B
#define E1355_GPIO 0x20
#define E1355_LUT_INDEX 0x24
#define E1355_LUT_DATA 0x26
#ifdef CONFIG_SUPERH
-/* tell me if your machine has a different base address .. */
-#define E1355_REG_BASE 0xB0000000
-#define E1355_FB_BASE 0xB0200000
+#define E1355_REG_BASE CONFIG_E1355_REG_BASE
+#define E1355_FB_BASE CONFIG_E1355_FB_BASE
static inline u8 e1355_read_reg(int index)
{
@@ -87,16 +88,30 @@
/* ------------------- chipset specific functions -------------------------- */
+static void disable_hw_cursor(void)
+{
+ u8 curs;
+
+ curs = e1355_read_reg(0x27);
+ curs &= ~0xc0;
+ e1355_write_reg(curs, 0x27);
+}
+
static void e1355_detect(void)
{
u8 rev;
+ e1355_write_reg(0x00, E1355_MISC);
+
rev = e1355_read_reg(0x00);
if ((rev & 0xfc) != 0x0c) {
printk(KERN_WARNING "Epson 1355 not detected\n");
}
+ /* XXX */
+ disable_hw_cursor();
+
e1355_encode_var(&default_var, NULL, NULL);
}
@@ -373,7 +388,7 @@
* 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
+#undef AERO_HACKS
static int e1355_blank(int blank_mode, struct fb_info_gen *info)
{
@@ -478,19 +493,8 @@
return 0;
}
-static void disable_hw_cursor(void)
-{
- u8 curs;
-
- curs = e1355_read_reg(0x27);
- curs &= ~0xc0;
- e1355_write_reg(curs, 0x27);
-}
-
int __init e1355fb_init(void)
{
- disable_hw_cursor();
-
fb_info.gen.fbhw = &e1355_switch;
fb_info.gen.fbhw->detect();
strcpy(fb_info.gen.info.modename, "SED1355");
|
