This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: =?UTF-8?q?=5BPATCH=20v3=5D=20sim =3A=20cfi=3A=20new=20flash=20device=20simulati on?=
- From: fche at redhat dot com (Frank Ch. Eigler)
- To: Mike Frysinger <vapier at gentoo dot org>
- Cc: gdb-patches at sourceware dot org, toolchain-devel at blackfin dot uclinux dot org
- Date: Fri, 25 Mar 2011 11:15:09 -0400
- Subject: Re: =?UTF-8?q?=5BPATCH=20v3=5D=20sim =3A=20cfi=3A=20new=20flash=20device=20simulati on?=
- References: <1293750414-14626-1-git-send-email-vapier@gentoo.org> <1301012212-2372-1-git-send-email-vapier__17299.0819243298$1301012210$gmane$org@gentoo.org>
vapier wrote:
> This simulates a CFI flash. [...]
Nice work, good looking code. Only some tiny nits:
> +static unsigned
> +cfi_unshift_addr (struct cfi *cfi, unsigned addr)
> +{
> + switch (cfi->width)
> + {
> + case 4: addr >>= 1;
> + case 2: addr >>= 1;
> + }
A note about the deliberate case fallthrough might be nice.
> +/* Clean up any state when this device is removed (e.g. when shutting down,
> + or when reloading via gdb). */
> +static void
> +cfi_delete_callback (struct hw *me)
> +{
> +#ifdef HAVE_MMAP
> + struct cfi *cfi = hw_data (me);
> +
> + if (cfi->mmap)
> + munmap (cfi->mmap, cfi->dev_size);
> +#endif
> +}
Woudln't you want to write(2) out the contents of the flash backing
store file, if !HAVE_MMAP?
> +static void
> +attach_cfi_regs (struct hw *me, struct cfi *cfi)
> +[...]
> + cfi->data = HW_NALLOC (me, unsigned char, cfi->dev_size);
> + if (fd != -1)
> + read_len = read (fd, cfi->data, cfi->dev_size);
It's more typical to loop in read(2), in case of an EINTR or somesuch
temporary-failure return code. Or use fread(3)?
- FChE