Messages in this thread | | | Date | Fri, 11 Apr 2008 08:21:06 +0200 | From | Uwe Kleine-König <> | Subject | Re: [PATCH 4/4] [RFC] UIO: generic platform driver |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |