Re: [Qemu-devel] [PATCH 08/10] fdc: add CCR (Configuration Control Register) write register

[Andreas Färber]

Am 08.01.2012 21:27, schrieb Hervé Poussineau:
> 
> Signed-off-by: Hervé Poussineau <address@hidden>
> ---
>  hw/fdc.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/fdc.c b/hw/fdc.c
> index ddfa91f..67cd14f 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -227,6 +227,7 @@ static void fdctrl_write_rate(FDCtrl *fdctrl, uint32_t 
> value);
>  static uint32_t fdctrl_read_data(FDCtrl *fdctrl);
>  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value);
>  static uint32_t fdctrl_read_dir(FDCtrl *fdctrl);
> +static void fdctrl_write_ccr(FDCtrl *fdctrl, uint32_t value);
>  
>  enum {
>      FD_DIR_WRITE   = 0,
> @@ -251,6 +252,7 @@ enum {
>      FD_REG_DSR = 0x04,
>      FD_REG_FIFO = 0x05,
>      FD_REG_DIR = 0x07,
> +    FD_REG_CCR = 0x07,

I'm not familiar with FDCs, so this reads weird: Do DIR and CCR share
the same address, with DIR being read-only and CCR write-only? If so, an
explanatory sentence in the commit message would be nice. (dito for most
other patches in the series)

>  };
>  
>  enum {
> @@ -495,6 +497,8 @@ static void fdctrl_write (void *opaque, uint32_t reg, 
> uint32_t value)
>      case FD_REG_FIFO:
>          fdctrl_write_data(fdctrl, value);
>          break;
> +    case FD_REG_CCR:
> +        fdctrl_write_ccr(fdctrl, value);

break; please, to avoid future errors or complaints by static analysis
tools.

Andreas

>      default:
>          break;
>      }

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg