Re: [PATCH 4/4] [RFC] UIO: generic platform driver

[Uwe Kleine-König]

Hello Hans,

Hans J. Koch wrote:
> > +/* XXX: I thought there already exists something like STRINGIFY, but I could not
> > + * find it ...
> > + */
> 
> Why do you want that macro? You only use it once. The macro definition and the
> comment above are more than you can ever expect to save with it.
See below.
BTW, I found it, it's in linux/stringify.h.  And there are several
possible users:

	ukleinek@zentaur:~/gsrc/linux-2.6$ git ls-files -z | xargs -0 perl -n -e 'print "$ARGV\n" if (/define\s+\w+\((\w+)\)\s*#\s*$1/)'
	arch/arm/vfp/vfpinstr.h
	arch/cris/arch-v10/boot/tools/build.c
	arch/m68k/lib/checksum.c
	arch/mips/kernel/unaligned.c
	arch/powerpc/boot/reg.h
	arch/um/drivers/mconsole_user.c
	arch/um/include/sysdep-i386/kernel-offsets.h
	arch/um/include/sysdep-x86_64/kernel-offsets.h
	arch/x86/kernel/machine_kexec_32.c
	drivers/atm/ambassador.c
	drivers/media/video/cpia2/cpia2_v4l.c
	drivers/mtd/maps/amd76xrom.c
	drivers/mtd/maps/ichxrom.c
	drivers/s390/cio/qdio.h
	drivers/scsi/aacraid/aacraid.h
	drivers/scsi/aacraid/linit.c
	drivers/scsi/arm/acornscsi.c
	drivers/scsi/g_NCR5380.h
	drivers/uio/uio_pdrv.c
	drivers/usb/serial/garmin_gps.c
	include/asm-cris/arch-v10/irq.h
	include/asm-cris/arch-v32/hwregs/supp_reg.h
	include/asm-cris/arch-v32/irq.h
	include/asm-m32r/assembler.h
	include/asm-m68k/entry.h
	include/asm-mips/mipsregs.h
	include/asm-mips/sim.h
	include/asm-sh/cpu-sh5/registers.h
	include/asm-v850/macrology.h
	include/linux/stringify.h
	include/sound/core.h
	sound/core/memalloc.c
	usr/gen_init_cpio.c

> > +static int uio_pdrv_open(struct uio_info *info, struct inode *inode)
> > +{
> > +	struct uio_platdata *pdata = info->priv;
> > +	int ret;
> > +
> > +	BUG_ON(pdata->uioinfo != info);
> 
> How can this BUG ever be triggered?
I hope it cannot, that's why it is a bug if it happens. :-)  And one
should expect that no BUG_ON should ever be triggered.  I usually use
BUG_ON for "declaring" preconditions.
 
> > +	for (i = 0; i < pdev->num_resources; ++i) {
> > +		struct resource *r = &pdev->resource[i];
> 
> Please don't define new variables in the middle of a function.
This is a matter of taste.  In my eyes it's better to declare it here
because then it's easier to see where it's used.
 
> > +
> > +		if (r->flags != IORESOURCE_MEM)
> > +			continue;
> > +
> > +		if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
> 
> I'd prefer this:
> 		if (i >= MAX_UIO_MAPS) {
> 		...
OK.

BTW would you be open to redefine uio_info as:

	struct uio_info {
		struct uio_device       *uio_dev;
		...
		size_t			num_memmaps;
		struct uio_mem		mem[];
	}

This allows to allocate exactly the number of members in the mem array
that are needed (for the cost of a size_t).  (You just do:

	uio_info uioinfo = kzalloc(sizeof(*uioinfo) + num_memmaps * sizeof(uioinfo->mem[0]), GFP_KERNEL);

it's still one chunk of memory and usage is similar---just with
MAX_UIO_MAPS substituted by uioinfo->num_memmaps.)
	
> > +			dev_warn(&pdev->dev, "device has more than "
> > +					STRINGIFY(MAX_UIO_MAPS)
> > +					" I/O memory resources.\n");
> 
> What about this:
> 
> 			dev_warn(&pdev->dev, "device has more than %d"
> 				" I/O memory resources.\n", MAX_UIO_MAPS);
> 
> would save the macro.
The macro is for free, using "%d" is not:

	ukleinek@zentaur:~/kbuild-ns921x$ size drivers/uio/uio_pdrv_with*
	   text    data     bss     dec     hex filename
	    808      72       0     880     370 drivers/uio/uio_pdrv_withoutstringify.o
	    800      72       0     872     368 drivers/uio/uio_pdrv_withstringify.o

You might consider that a bit over-engineered :-)

> > +	ret = uio_register_device(&pdev->dev, pdata->uioinfo);
> > +
> > +	if (ret) {
> > +		clk_put(pdata->clk);
> > +err_clk_get:
> > +
> > +		kfree(pdata);
> > +err_alloc_pdata:
> > +err_uioinfo:
> > +		return ret;
> > +	}
> 
> How I hate labels within blocks... OK, I admit, it looks good here...
> Well...
That's a matter of taste, too, I like it that way.  Probably it doesn't
matter for the compiler (I havn't tried), but this way it's one goto
less.  And if it doesn't matter for the compiler it's at least nice for
the reader.  Though obviously YMMV.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/