Jean Delvare napisał(a): > Hi Krzysztof, > > On Sat, 21 Mar 2009 20:29:54 +0100, Krzysztof Helt wrote: > > From: Krzysztof Helt >[email protected]> > > > > The I2C functionality provided by the > > i2c-voodoo3 driver is moved into the > > tdfxfb (frame buffer driver for Voodoo3 > > cards). This way there is no conflict > > between i2c driver and the fb driver. > > > > The tdfxfb does not make use from the > > DDC functionality yet. It just provides > > two I2C buses like the i2c-voodoo3 driver > > (DDC and I2C). > > This change has my full support. In fact it was on my to-do list for > this year, so I am very happy to see someone else beat me at it :) > > (BTW: can you please trim lines at ~70 characters rather than 42? Too > frequent new lines make the text harder to read IMHO.) >
I will make lines longer > > > > Signed-off-by: Krzysztof Helt >[email protected]> > > --- > > > > This open way to remove the i2c-voodoo3 driver. > > > > > > drivers/video/Kconfig | 9 ++ > > drivers/video/tdfxfb.c | 191 > +++++++++++++++++++++++++++++++++++++++++++++++- > > include/video/tdfx.h | 24 ++++++ > > 3 files changed, 223 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig > > index 6ff3364..837479b 100644 > > --- a/drivers/video/Kconfig > > +++ b/drivers/video/Kconfig > > @@ -1550,6 +1550,7 @@ config FB_3DFX > > select FB_CFB_IMAGEBLIT > > select FB_CFB_FILLRECT > > select FB_CFB_COPYAREA > > + select FB_MODE_HELPERS > > help > > This driver supports graphics boards with the 3Dfx Banshee, > > Voodoo3 or VSA-100 (aka Voodoo4/5) chips. Say Y if you have > > @@ -1565,6 +1566,14 @@ config FB_3DFX_ACCEL > > This will compile the 3Dfx Banshee/Voodoo3/VSA-100 frame buffer > > device driver with acceleration functions. > > > > +config FB_3DFX_I2C > > + bool "DDC/I2C for 3dfx Voodoo3 support" > > I see that some other drivers have a similarly unfortunate wording, but > I'd prefer something clearer and less redundant such as "Enable DDC/I2C > Support" (as the nvidiafb, rivafb and i810fb drivers do.) > Ok. The word "Enable" is a good hint. I'll change this. > > + depends on FB_3DFX >> EXPERIMENTAL > > + select FB_DDC > > + default y > > + help > > + Say Y here if you want DDC/I2C support for your 3dfx Voodoo3. > > It might be worth adding a note that the driver itself doesn't yet take > benefit of the DDC channel. > IHMO, it does not matter. A separate patch which make use of it is already ready to post. If this patch is accepted, the second one will be posted. It is not worth to add a comment then remove it in the next patch. > > + > > config FB_VOODOO1 > > tristate "3Dfx Voodoo Graphics (sst1) support" > > depends on FB >> PCI > > diff --git a/drivers/video/tdfxfb.c b/drivers/video/tdfxfb.c > > index 14bd3f3..c4152a5 100644 > > --- a/drivers/video/tdfxfb.c > > +++ b/drivers/video/tdfxfb.c > > @@ -1167,6 +1167,190 @@ static struct fb_ops tdfxfb_ops = { > > #endif > > }; > > > > +#ifdef CONFIG_FB_3DFX_I2C > > +/* The voo GPIO registers don't have individual masks for each bit > > + so we always have to read before writing. */ > > As far as I can see, some of the code and comments is taken directly > from the i2c-voodoo3 driver, so it would seem fair to acknowledge the > work of its author (Philip Edelbrock >[email protected]>) in the > header comment of the file. > Ok (...) > > +static int __devinit tdfxfb_setup_ddc_bus(struct tdfxfb_i2c_chan > *chan, > > + const char *name, struct device *dev) > > +{ > > + int rc; > > + > > + strcpy(chan->adapter.name, name); > > Please use strlcpy() instead. > > > + chan->adapter.owner = THIS_MODULE; > > + chan->adapter.class = I2C_CLASS_DDC; > > You must include >linux/i2c.h> to use this. I know it is indirectly > included but relying on indirect includes is a bad practice. > It is included by the tdfx.h which is vital to the tdfxfb.c. > > +static void __devinit tdfxfb_create_i2c_busses(struct fb_info *info) > > +{ > > + struct tdfx_par *par = info->par; > > + > > + tdfx_outl(par, VIDINFORMAT, 0x8160); > > + tdfx_outl(par, VIDSERPARPORT, 0xcffc0020); > > + > > + par->chan[0].par = par; > > + par->chan[1].par = par; > > + > > + tdfxfb_setup_ddc_bus(>par->chan[0], "VOODOO3-DDC", info->dev); > > + tdfxfb_setup_i2c_bus(>par->chan[1], "VOODOO3-I2C", info->dev); > > Why all-caps names? What about "Voodoo3 DDC" and "Voodoo3 I2C" instead? > Just a choice. One other driver does it this way. I'll change to the proposed names. > > @@ -1284,7 +1468,9 @@ static int __devinit tdfxfb_probe(struct pci_dev > *pdev, > > if (hwcursor) > > info->fix.smem_len = (info->fix.smem_len - 1024) > > > (PAGE_MASK >> 1); > > - > > +#ifdef CONFIG_FB_3DFX_I2C > > + tdfxfb_create_i2c_busses(info); > > +#endif > > if (!mode_option) > > mode_option = "640x...@60"; > > > > Later in this function, errors can occur which cause an error path to > be taken. There you should call tdfxfb_delete_i2c_busses(par) otherwise > you're leaking resources. > ok. > > diff --git a/include/video/tdfx.h b/include/video/tdfx.h > > index 7431d96..bc37022 100644 > > --- a/include/video/tdfx.h > > +++ b/include/video/tdfx.h > > @@ -168,12 +183,21 @@ struct banshee_reg { > > unsigned long miscinit0; > > }; > > > > +struct tdfx_par; > > + > > +struct tdfxfb_i2c_chan { > > + struct tdfx_par *par; > > + struct i2c_adapter adapter; > > + struct i2c_algo_bit_data algo; > > +}; > > + > > struct tdfx_par { > > u32 max_pixclock; > > u32 palette[16]; > > void __iomem *regbase_virt; > > unsigned long iobase; > > int mtrr_handle; > > + struct tdfxfb_i2c_chan chan[2]; > > ... and this struct member if CONFIG_FB_3DFX_I2C is defined (same as > intelfb and radeonfb are doing for example.) > ok. > > Other than these details, it looks OK to me. Please send an updated > version addressing my comments and I'll be happy to ack it. I've > also > tested the new code on my Voodoo3 and it worked just fine. > > Was your code tested on a Voodoo5? The i2c-voodoo3 driver didn't claim > to support it, so I wonder if it is compatible DDC/I2C-wise. > It works on Voodoo5 at least. > Next step is to deprecate i2c-voodoo3. This requires changes to > drivers/i2c/busses/Kconfig and > Documentation/feature-removal-schedule.txt. Are you going to do it or > should I? > I don't care. First, this patch must be accepted. Thank you, Krzysztof ---------------------------------------------------------------------- Szukasz pieniedzy? Wez podwojny limit zadluzenia w koncie direct. >> http://link.interia.pl/f20a3 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
