Re: =?UTF-8?q?=5BPATCH=20v3=5D=20sim=3A=20cfi=3A=20new=20flash=20device=20simulation?=

[fche@redhat.com (Frank Ch. Eigler)]

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