linux-kernel - RE: [PATCH 03/11] gpio: davinci: Modify to platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 11 Jun 2013 12:55:22 +0000
From:	"Philip, Avinash" <avinashphilip@...com>
To:	"Nori, Sekhar" <nsekhar@...com>
CC:	"khilman@...prootsystems.com" <khilman@...prootsystems.com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"davinci-linux-open-source@...ux.davincidsp.com" 
	<davinci-linux-open-source@...ux.davincidsp.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 03/11] gpio: davinci: Modify to platform driver

On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
> 
> 
> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> > From: KV Sujith <sujithkv@...com>
> > 
> > Modify GPIO Davinci driver to be compliant to standard platform drivers.
> > The driver did not have platform driver structure or a probe. Instead,
> > had a davinci_gpio_setup() function which is called in the pure_init
> > sequence. The function also had dependency on davinci_soc_info structure
> > of the corresponding platform. For Device Tree(DT) implementation, we
> > need to get rid of the dependency on the davinci_soc_info structure.
> > Hence as a first stage of DT conversion, we implement a probe. Future
> > commits shall modify the probe to read platform related data from DT.
> > 
> > - Add platform_driver structure and driver register function for davinci
> >   GPIO driver. The driver registration is made to happen in
> >   postcore_initcall. This is required since machine init functions like
> >   da850_lcd_hw_init() make use of GPIO.
> > - Convert the davinci_gpio_setup() to davinci_gpio_probe().
> > - Remove access of members in soc_info structure. Instead, relevant data
> >   are taken from davinci_gpio_platform_data structure pointed by
> >   pdev->dev.platform_data.
> > - Change clk_get() to devm_clk_get() as devm_clk_get() is a device
> >   managed function and makes error handling simpler.
> > - Change pr_err to dev_err for ngpio error reporting.
> > - Arrange include files and variables in alphabetical order
> > 
> > Signed-off-by: KV Sujith <sujithkv@...com>
> > [avinashphilip@...com: Move global definition for "struct
> > davinci_gpio_controller" variable to local in probe and set it as driver
> > data.]
> > Signed-off-by: Philip Avinash <avinashphilip@...com>
> > ---
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/gpio-davinci.h>
> 
> > +#include <mach/gpio-davinci.h>
> 
> This include seems unnecessary.

This include is not required.

> 
> >  
> >  #include <asm/mach/irq.h>
> 
> While at it, you can get rid of this include and use <linux/irq.h> instead?

Ok

> 
> >  
> > +	pdata = dev->platform_data;
> > +	if (!pdata) {
> > +		dev_err(dev, "GPIO: No Platform Data Supplied\n");
> 
> dev_err should already tell that the error is coming from davinci-gpio
> so no need to prefix GPIO: again.

Ok

> 
> > +		return -EINVAL;
> > +	}
> > -	if (WARN_ON(!gpio_base))
> > +	ctlrs = devm_kzalloc(dev,
> > +		ngpio * sizeof(struct davinci_gpio_controller), GFP_KERNEL);
> 
> Line break alignment needs fixing.

Ok

> 
> > +	if (!ctlrs) {
> > +		dev_err(dev, "Memory alloc failed\n");
> >  		return -ENOMEM;
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (unlikely(!res)) {
> > +		dev_err(dev, "Invalid mem resource\n");
> > +		return -ENODEV;
> 
> -EBUSY is better if you cannot get the resource.

Ok

> 
> > +	}
> > +
> > +	gpio_base = devm_ioremap_resource(dev, res);
> > +	if (!gpio_base)
> > +		return -EADDRNOTAVAIL;
> 
> devm_ioremap_resource gives an error encoder pointer if it fails so
> please use that instead of masking it.

Ok

> 
> >  
> >  	for (i = 0, base = 0; base < ngpio; i++, base += 32) {
> >  		ctlrs[i].chip.label = "DaVinci";
> > @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
> >  		gpiochip_add(&ctlrs[i].chip);
> >  	}
> >  
> > -	soc_info->gpio_ctlrs = ctlrs;
> 
> > -	soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
> 
> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
> else in the patchset so in effect you render the inline gpio get/set API
> useless. Looks like this initialization should be moved to platform code?

With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
Has no more dependency on soc_info->gpio_ctlrs_num.

I can merge [PATCH 08/11] ARM: davinci: start using gpiolib support to
[PATCH 03/11] gpio: davinci: Modify to platform driver

> 
> > -

> > +	pdata = dev->platform_data;
> > +	ngpio = pdata->ngpio;
> > +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +	if (unlikely(!res)) {
> > +		dev_err(dev, "Invalid IRQ resource\n");
> > +		return -ENODEV;
> 
> -EBUSY again?

Ok

Thanks
Avinash

> 
> Thanks,
> Sekhar
>