[PATCH 00/33] devm improvement series, part 1, take 2

[PATCH 00/33] devm improvement series, part 1, take 2

[Wolfram Sang]

Lately, I have been experimenting how to improve the devm interface to make
writing device drivers easier and less error prone while also getting rid of
its subtle issues. I think it has more potential but still needs work and
definately conistency, especiall in its usage.

The first thing I come up with is a low hanging fruit regarding
devm_ioremap_resouce(). This function already checks if the passed resource is
valid and gives an error message if not. So, we can remove similar checks from
the drivers and get rid of a bit of code and a number of inconsistent error
strings.

Unlike the RFC version, this series only removes the unneeded check iff
devm_ioremap_resource() follows platform_get_resource directly. The previous
version tried to shuffle code if needed, too, what lead to an embarrasing bug.
It turned out to me that shuffling code for all cases found will make the
automated script too complex, so I am unsure if an automated cleanup is the
proper tool for this case. Removing the easy stuff seems worthwhile to me,
though, so I post this series in a simplified form.

Despite various architectures and platform dependencies, I managed to compile
test 45 out of 57 modified files locally using heuristics and defconfigs.
If somebody knows how to create a minimal .config with a certain kconfig symbol
(and its dependencies) set, I'd love to hear about it.

Since this series looks quite different from the RFC (less files touched
mainly) I did not copy over the ACKs from the RFC, although a few people agreed
with the aproach basically (except the major flaw the old series had).

The repo is here [1]. I'd think it would be nice to have in 3.10. and sending a
pull request to Linus would be easiest, probably. Some non-buggy RFC patches
already slipped into subtrees, yet this won't cause any conflicts.

Looking forward to comments.

Thanks,

   Wolfram

[1] git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git devm_no_resource_check


Wolfram Sang (33):
  drivers/ata: don't check resource with devm_ioremap_resource
  drivers/char/hw_random: don't check resource with
    devm_ioremap_resource
  drivers/cpufreq: don't check resource with devm_ioremap_resource
  drivers/dma: don't check resource with devm_ioremap_resource
  drivers/gpio: don't check resource with devm_ioremap_resource
  drivers/gpu/drm/exynos: don't check resource with
    devm_ioremap_resource
  drivers/gpu/host1x/drm: don't check resource with
    devm_ioremap_resource
  drivers/i2c/busses: don't check resource with devm_ioremap_resource
  drivers/memory: don't check resource with devm_ioremap_resource
  drivers/mfd: don't check resource with devm_ioremap_resource
  drivers/misc: don't check resource with devm_ioremap_resource
  drivers/mtd/nand: don't check resource with devm_ioremap_resource
  drivers/net/ethernet/renesas: don't check resource with
    devm_ioremap_resource
  drivers/pinctrl: don't check resource with devm_ioremap_resource
  drivers/pwm: don't check resource with devm_ioremap_resource
  drivers/rtc: don't check resource with devm_ioremap_resource
  drivers/spi: don't check resource with devm_ioremap_resource
  drivers/staging/dwc2: don't check resource with devm_ioremap_resource
  drivers/staging/nvec: don't check resource with devm_ioremap_resource
  drivers/thermal: don't check resource with devm_ioremap_resource
  drivers/usb/chipidea: don't check resource with devm_ioremap_resource
  drivers/usb/gadget: don't check resource with devm_ioremap_resource
  drivers/usb/host: don't check resource with devm_ioremap_resource
  drivers/usb/phy: don't check resource with devm_ioremap_resource
  drivers/video/omap2: don't check resource with devm_ioremap_resource
  drivers/video/omap2/dss: don't check resource with
    devm_ioremap_resource
  drivers/w1/masters: don't check resource with devm_ioremap_resource
  drivers/watchdog: don't check resource with devm_ioremap_resource
  arch/arm/mach-tegra: don't check resource with devm_ioremap_resource
  arch/arm/plat-samsung: don't check resource with
    devm_ioremap_resource
  arch/mips/lantiq/xway: don't check resource with
    devm_ioremap_resource
  sound/soc/fsl: don't check resource with devm_ioremap_resource
  sound/soc/kirkwood: don't check resource with devm_ioremap_resource

 arch/arm/mach-tegra/tegra2_emc.c      |    5 -----
 arch/arm/plat-samsung/adc.c           |    5 -----
 arch/mips/lantiq/xway/gptu.c          |    4 ----
 drivers/ata/pata_ep93xx.c             |    5 -----
 drivers/char/hw_random/mxc-rnga.c     |    6 ------
 drivers/char/hw_random/omap-rng.c     |    5 -----
 drivers/cpufreq/kirkwood-cpufreq.c    |    4 ----
 drivers/dma/tegra20-apb-dma.c         |    5 -----
 drivers/gpio/gpio-mvebu.c             |    5 -----
 drivers/gpio/gpio-tegra.c             |    5 -----
 drivers/gpu/drm/exynos/exynos_hdmi.c  |    5 -----
 drivers/gpu/host1x/drm/dc.c           |    5 -----
 drivers/i2c/busses/i2c-s3c2410.c      |    5 -----
 drivers/i2c/busses/i2c-sirf.c         |    6 ------
 drivers/i2c/busses/i2c-tegra.c        |    5 -----
 drivers/memory/emif.c                 |    6 ------
 drivers/mfd/intel_msic.c              |    5 -----
 drivers/misc/atmel-ssc.c              |    5 -----
 drivers/mtd/nand/lpc32xx_mlc.c        |    5 -----
 drivers/net/ethernet/renesas/sh_eth.c |    5 -----
 drivers/pinctrl/pinctrl-coh901.c      |    5 -----
 drivers/pinctrl/pinctrl-exynos5440.c  |    5 -----
 drivers/pinctrl/pinctrl-samsung.c     |    5 -----
 drivers/pinctrl/pinctrl-xway.c        |    4 ----
 drivers/pwm/pwm-imx.c                 |    5 -----
 drivers/pwm/pwm-puv3.c                |    5 -----
 drivers/pwm/pwm-pxa.c                 |    5 -----
 drivers/pwm/pwm-tegra.c               |    5 -----
 drivers/pwm/pwm-tiecap.c              |    5 -----
 drivers/pwm/pwm-tiehrpwm.c            |    5 -----
 drivers/pwm/pwm-tipwmss.c             |    5 -----
 drivers/pwm/pwm-vt8500.c              |    5 -----
 drivers/rtc/rtc-nuc900.c              |    5 -----
 drivers/rtc/rtc-omap.c                |    5 -----
 drivers/rtc/rtc-s3c.c                 |    5 -----
 drivers/rtc/rtc-tegra.c               |    6 ------
 drivers/spi/spi-tegra20-sflash.c      |    5 -----
 drivers/staging/dwc2/platform.c       |    5 -----
 drivers/staging/nvec/nvec.c           |    5 -----
 drivers/thermal/armada_thermal.c      |   10 ----------
 drivers/thermal/dove_thermal.c        |    4 ----
 drivers/thermal/exynos_thermal.c      |    5 -----
 drivers/usb/chipidea/core.c           |    5 -----
 drivers/usb/gadget/bcm63xx_udc.c      |   10 ----------
 drivers/usb/host/ohci-nxp.c           |    6 ------
 drivers/usb/phy/phy-mv-u3d-usb.c      |    5 -----
 drivers/usb/phy/phy-mxs-usb.c         |    5 -----
 drivers/usb/phy/phy-samsung-usb2.c    |    5 -----
 drivers/usb/phy/phy-samsung-usb3.c    |    5 -----
 drivers/video/omap2/dss/hdmi.c        |    4 ----
 drivers/video/omap2/vrfb.c            |    5 -----
 drivers/w1/masters/omap_hdq.c         |    5 -----
 drivers/watchdog/ath79_wdt.c          |    5 -----
 drivers/watchdog/davinci_wdt.c        |    5 -----
 drivers/watchdog/imx2_wdt.c           |    5 -----
 sound/soc/fsl/imx-ssi.c               |    6 ------
 sound/soc/kirkwood/kirkwood-i2s.c     |    5 -----
 57 files changed, 296 deletions(-)

-- 
1.7.10.4

[PATCH 08/33] drivers/i2c/busses: don't check resource with devm_ioremap_resource

[Wolfram Sang]

devm_ioremap_resource does sanity checks on the given resource. No need to
duplicate this in the driver.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-s3c2410.c |    5 -----
 drivers/i2c/busses/i2c-sirf.c    |    6 ------
 drivers/i2c/busses/i2c-tegra.c   |    5 -----
 3 files changed, 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 6e8ee92..cab1c91 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -1082,11 +1082,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	/* map the registers */
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res == NULL) {
-		dev_err(&pdev->dev, "cannot find IO resource\n");
-		return -ENOENT;
-	}
-
 	i2c->regs = devm_ioremap_resource(&pdev->dev, res);
 
 	if (IS_ERR(i2c->regs))
diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index 5a7ad24..a63c7d5 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -303,12 +303,6 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
 	adap->class = I2C_CLASS_HWMON;
 
 	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (mem_res == NULL) {
-		dev_err(&pdev->dev, "Unable to get MEM resource\n");
-		err = -EINVAL;
-		goto out;
-	}
-
 	siic->base = devm_ioremap_resource(&pdev->dev, mem_res);
 	if (IS_ERR(siic->base)) {
 		err = PTR_ERR(siic->base);
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index b60ff90..9aa1b60 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -714,11 +714,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	int ret = 0;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "no mem resource\n");
-		return -EINVAL;
-	}
-
 	base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
-- 
1.7.10.4

Re: [PATCH 08/33] drivers/i2c/busses: don't check resource with devm_ioremap_resource

[Barry Song]

2013/5/16 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
> devm_ioremap_resource does sanity checks on the given resource. No need to
> duplicate this in the driver.
>
> Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>

Acked-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>

-barry

Re: [PATCH 08/33] drivers/i2c/busses: don't check resource with devm_ioremap_resource

[Kukjin Kim]

On 05/16/13 20:15, Wolfram Sang wrote:
> devm_ioremap_resource does sanity checks on the given resource. No need to
> duplicate this in the driver.
>
> Signed-off-by: Wolfram Sang<wsa@the-dreams.de>

For i2c-s3c2410:
Acked-by: Kukjin Kim <kgene.kim@samsung.com>

Thanks.

- Kukjin

> ---
>   drivers/i2c/busses/i2c-s3c2410.c |    5 -----
>   drivers/i2c/busses/i2c-sirf.c    |    6 ------
>   drivers/i2c/busses/i2c-tegra.c   |    5 -----
>   3 files changed, 16 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 6e8ee92..cab1c91 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -1082,11 +1082,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>   	/* map the registers */
>
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (res == NULL) {
> -		dev_err(&pdev->dev, "cannot find IO resource\n");
> -		return -ENOENT;
> -	}
> -
>   	i2c->regs = devm_ioremap_resource(&pdev->dev, res);
>
>   	if (IS_ERR(i2c->regs))
> diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
> index 5a7ad24..a63c7d5 100644
> --- a/drivers/i2c/busses/i2c-sirf.c
> +++ b/drivers/i2c/busses/i2c-sirf.c
> @@ -303,12 +303,6 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
>   	adap->class = I2C_CLASS_HWMON;
>
>   	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (mem_res == NULL) {
> -		dev_err(&pdev->dev, "Unable to get MEM resource\n");
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
>   	siic->base = devm_ioremap_resource(&pdev->dev, mem_res);
>   	if (IS_ERR(siic->base)) {
>   		err = PTR_ERR(siic->base);
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index b60ff90..9aa1b60 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -714,11 +714,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>   	int ret = 0;
>
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "no mem resource\n");
> -		return -EINVAL;
> -	}
> -
>   	base = devm_ioremap_resource(&pdev->dev, res);
>   	if (IS_ERR(base))
>   		return PTR_ERR(base);

Re: [PATCH 00/33] devm improvement series, part 1, take 2

[Artem Bityutskiy]

On Thu, 2013-05-16 at 13:15 +0200, Wolfram Sang wrote:
> Despite various architectures and platform dependencies, I managed to
> compile
> test 45 out of 57 modified files locally using heuristics and
> defconfigs.
> If somebody knows how to create a minimal .config with a certain
> kconfig symbol
> (and its dependencies) set, I'd love to hear about it.

If you find this out, please, share!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 00/33] devm improvement series, part 1, take 2

[Viresh Kumar]

On 16 May 2013 17:27, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Thu, 2013-05-16 at 13:15 +0200, Wolfram Sang wrote:
>> Despite various architectures and platform dependencies, I managed to
>> compile
>> test 45 out of 57 modified files locally using heuristics and
>> defconfigs.
>> If somebody knows how to create a minimal .config with a certain
>> kconfig symbol
>> (and its dependencies) set, I'd love to hear about it.
>
> If you find this out, please, share!

Are you guys looking for "make savedefconfig" ??

Re: [PATCH 00/33] devm improvement series, part 1, take 2

[Artem Bityutskiy]

On Thu, 2013-05-16 at 18:25 +0530, Viresh Kumar wrote:
> On 16 May 2013 17:27, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Thu, 2013-05-16 at 13:15 +0200, Wolfram Sang wrote:
> >> Despite various architectures and platform dependencies, I managed to
> >> compile
> >> test 45 out of 57 modified files locally using heuristics and
> >> defconfigs.
> >> If somebody knows how to create a minimal .config with a certain
> >> kconfig symbol
> >> (and its dependencies) set, I'd love to hear about it.
> >
> > If you find this out, please, share!
> 
> Are you guys looking for "make savedefconfig" ??

No. It is more like:

I have changed this strange driver.

I want to compile-test my changes.

I need a defconfig which would have this driver enabled. I also want to
know <arch> for my "make ARCH=<arch>" command.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 00/33] devm improvement series, part 1, take 2

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1568 bytes --]

On 16/05/13 16:11, Artem Bityutskiy wrote:
> On Thu, 2013-05-16 at 18:25 +0530, Viresh Kumar wrote:
>> On 16 May 2013 17:27, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>>> On Thu, 2013-05-16 at 13:15 +0200, Wolfram Sang wrote:
>>>> Despite various architectures and platform dependencies, I managed to
>>>> compile
>>>> test 45 out of 57 modified files locally using heuristics and
>>>> defconfigs.
>>>> If somebody knows how to create a minimal .config with a certain
>>>> kconfig symbol
>>>> (and its dependencies) set, I'd love to hear about it.
>>>
>>> If you find this out, please, share!
>>
>> Are you guys looking for "make savedefconfig" ??
> 
> No. It is more like:
> 
> I have changed this strange driver.
> 
> I want to compile-test my changes.
> 
> I need a defconfig which would have this driver enabled. I also want to
> know <arch> for my "make ARCH=<arch>" command.

Not quite the same thing, but I sent this a while ago:

http://lkml.indiana.edu/hypermail/linux/kernel/1304.3/02847.html

My "softdepends" feature is probably not needed, as pointed out in the
thread, as the behavior can be implemented with the current Kconfig
language just fine.

I had a quick look at fbdev drivers, and some of them compile fine on
all (well, arm and x86) archs. But many do have real arch dependencies.

I think it'd be a good long term goal to make drivers arch-independent,
and add CONFIG_SHOW_ALL_DRIVERS or such, which would allow compiling
drivers that are not used on your arch, but still compile fine.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 00/33] devm improvement series, part 1, take 2

[Wolfram Sang]

> I need a defconfig which would have this driver enabled. 

My wish would be a minimal config. Right now, I try to build the driver
with the current config and when that fails I grep through the
(uncompressed) defconfigs for the symbol needed. Gives me 45/57 success
rate on this series. Not perfect, but the best I could come up with
without writing a .config-generator myself.

Re: [PATCH 00/33] devm improvement series, part 1, take 2

[Stephen Warren]

On 05/16/2013 05:15 AM, Wolfram Sang wrote:
> Lately, I have been experimenting how to improve the devm interface to make
> writing device drivers easier and less error prone while also getting rid of
> its subtle issues. I think it has more potential but still needs work and
> definately conistency, especiall in its usage.
...

The Tegra parts in patches 4, 5, 8, 15, 16, 17, 29 all,
Acked-by: Stephen Warren <swarren@nvidia.com>

Re: [PATCH 00/33] devm improvement series, part 1, take 2

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1291 bytes --]

On Thu, May 16, 2013 at 01:15:28PM +0200, Wolfram Sang wrote:
> Lately, I have been experimenting how to improve the devm interface to make
> writing device drivers easier and less error prone while also getting rid of
> its subtle issues. I think it has more potential but still needs work and
> definately conistency, especiall in its usage.
> 
> The first thing I come up with is a low hanging fruit regarding
> devm_ioremap_resouce(). This function already checks if the passed resource is
> valid and gives an error message if not. So, we can remove similar checks from
> the drivers and get rid of a bit of code and a number of inconsistent error
> strings.

Sorry for jumping in so late. I generally like the idea. One small
inconvenience is that devm_ioremap_resource() returns -EINVAL if
res == NULL, which means that drivers will now also return -EINVAL
in cases where no resource was returned. Typically drivers handle
this by returning something like -ENODEV, -ENXIO, -ENOENT. Some do
return -EINVAL but perhaps having a separate error code (and maybe
error message as well) for a missing resource would be helpful.

Doing this would be rather easy now that you've paved the way by
making devm_ioremap_resource() usage consistent across drivers.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel