RE: [PATCH] unicore32: add i2c bus driver for PKUnity SoC

["Guan Xuetao" <guanxuetao-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>]

> -----Original Message-----
> From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
> Sent: Monday, January 24, 2011 5:19 PM
> To: Guan Xuetao
> Cc: ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] unicore32: add i2c bus driver for PKUnity SoC
> 
> Hi Guan,
> 
> On Sat, 22 Jan 2011 18:55:09 +0800, Guan Xuetao wrote:
> > From: Guan Xuetao <gxt-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>
> >
> > This patch adds framebuffer driver for unigfx engine in PKUnity SoC.
> 
> While I don't have the time for a full review, I would like to at least
> sort out the integration issues. Framebuffer driver, really? I doubt
> it. If this is a copy-and-paste mistake, and this is actually an I2C
> system bus driver, you will have to change the driver name (see below.)
> If what you actually meant is that this is a driver for the I2C/DDC
> channels on a graphics chip, then you have to move this driver to the
> same directory the framebuffer driver lives in, as all other
> framebuffer drivers do.
Yes, it's an I2C system bus driver. Sorry for my mistake.

> 
> > Signed-off-by: Guan Xuetao <gxt-TG0Ac1+ktVePQbnJrJN+5g@public.gmane.org>
> > ---
> >  drivers/i2c/busses/Kconfig    |    5 +
> >  drivers/i2c/busses/Makefile   |    1 +
> >  drivers/i2c/busses/puv3_i2c.c |  300 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 306 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 3a6321c..20d328c 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -236,6 +236,11 @@ config I2C_VIAPRO
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called i2c-viapro.
> >
> > +config I2C_PUV3
> > +	tristate "PKUnity v3 I2C bus support"
> > +	depends on I2C && UNICORE32 && ARCH_PUV3
> 
> The whole menu depends on I2C, so you don't have to repeat it.
Ok.

> 
> > +	select I2C_ALGOBIT
> 
> Where is the description of what the driver is for? All other entries
> have it, yours is no exception.
A description block is added.

> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 84cb16a..cb4a097 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
> >  obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
> >  obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
> >  obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
> > +obj-$(CONFIG_I2C_PUV3)		+= puv3_i2c.o
> 
> Assuming that this is the right place for your driver, then the name is
> NOT correct. As you can see, all drivers are names i2c-*, so your
> driver should be too. And the it should be inserted in alphabetical
> order.
Ok.

> 
> See also my comments below related to the probe() and remove()
> functions.
> 
> >
> >  # External I2C/SMBus adapter drivers
> >  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
> > diff --git a/drivers/i2c/busses/puv3_i2c.c b/drivers/i2c/busses/puv3_i2c.c
> > +
> > +static int i2c_reg = -1;
> 
> Please allocate and use per-device data structure for device state
> tracking. Global variables are evil (for this use case at least.)
Yes, i2c_reg is replaced by local variables.

> > +
> > +/*
> > + * Main initialization routine.
> > + */
> > +static int __devinit puv3_i2c_probe(struct platform_device *pdev)
> > +{
> > +	struct i2c_adapter *adapter;
> > +	struct resource *res;
> > +	int rc;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENXIO;
> 
> -ENODEV would seem more appropriate.
Ok.

> > +	adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> > +	if (adapter == NULL) {
> > +		dev_err(&pdev->dev, "can't allocate inteface!\n");
> > +		rc = -ENOMEM;
> > +		goto fail2;
> > +	}
> > +	snprintf(adapter->name, sizeof(adapter->name), "PUV3");
> 
> Please include some uniqueness in the name. The address of the mem
> region would be a good candidate:
> 
> 	snprintf(adapter->name, sizeof(adapter->name), "PUV3 at 0x%08x",
> 		 res->start);
> 
> This lets user-space differentiate between the adapters if there are
> more than one.
Ok.

> > +
> > +	platform_set_drvdata(pdev, adapter);
> > +
> > +	rc = i2c_add_numbered_adapter(adapter);
> 
> You can't call i2c_add_numbered_adapter() if you haven't set
> adapter->nr first.
Corrected.

> 
> > +	if (rc) {
> > +		dev_err(&pdev->dev, "Adapter %s registration failed\n",
> > +				adapter->name);
> 
> Quotes around %s would be welcome.
Ok.

> 
> > +		goto fail3;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "PKUnity v3 i2c bus driver.\n");
> 
> Wrong message. You have just registered a device, not a driver.
Corrected.

> 
> > +	return 0;
> > +
> > +fail3:
> 
> Please use descriptive labels, otherwise it's easy to mess up and jump
> to the wrong one.
Ok.

> 
> > +	platform_set_drvdata(pdev, NULL);
> > +	kfree(adapter);
> > +fail2:
> > +	release_mem_region(res->start, resource_size(res));
> > +
> > +	return rc;
> > +}
> > +
> > +static int puv3_i2c_remove(struct platform_device *pdev)
> 
> Missing __devexit.
Added.

> 
> > +{
> > +	struct i2c_adapter *adapter = platform_get_drvdata(pdev);
> > +	struct resource *res;
> > +	int rc;
> > +
> > +	rc = i2c_del_adapter(adapter);
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	release_mem_region(res->start, resource_size(res));
> > +
> > +	return rc;
> 
> The logic in this function doesn't make sense. If i2c_del_adapter()
> fails, you must exit immediately. Freeing the resources while the
> adapter is still registered can only lead to a crash.
> 
> Additionally, you are leaking memory in the success case, as adapter is
> never freed.
Above two are corrected.

> 
> --
> Jean Delvare

Thanks Jean.

Guan Xuetao