Re: Re: [PATCH] tdfxfb: move I2C functionality into the tdfxfb

[krzysztof.h1-IjDXvh/HVVUAvxtiuMwx3w@public.gmane.org]

Jean Delvare napisał(a):
> Hi Krzysztof,
> 
> On Sat, 21 Mar 2009 20:29:54 +0100, Krzysztof Helt wrote:
> > From: Krzysztof Helt >krzysztof.h1-5tc4TXWwyLM@public.gmane.org>
> > 
> > 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 >krzysztof.h1-5tc4TXWwyLM@public.gmane.org>
> > ---
> > 
> > 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 >phil-KXOFo5pg7o1l57MIdRCFDg@public.gmane.org>) 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 = "640x480@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