[OpenBIOS] [PATCH 1/5] Remove struct ata_sector and all references to it

Laurent Vivier Laurent at vivier.eu
Sun May 17 14:06:52 CEST 2009


This patch looks good, except one comment, see below.

Regards,
Laurent

Le samedi 16 mai 2009 à 20:16 -0400, Pavel Roskin a écrit :
> struct ata_sector was meant to facilitate taking upper and lower byte of
> the sector.  However, the implementation is incorrect, as "struct" and
> "union" are swapped in the definition.  What's worse, it's an overkill
> for that simple task.  The code should be transparent without knowing
> how struct ata_sector is defined.
> ---
> 
>  drivers/ide.c |   10 +++-------
>  drivers/ide.h |   14 --------------
>  2 files changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/ide.c b/drivers/ide.c
> index 1166f57..4d1e58c 100644
> --- a/drivers/ide.c
> +++ b/drivers/ide.c
> @@ -764,7 +764,6 @@ ob_ide_read_ata_chs(struct ide_drive *drive, unsigned long long block,
>  	unsigned int sect = (block % drive->sect) + 1;
>  	unsigned int head = (track % drive->head);
>  	unsigned int cyl = (track / drive->head);
> -	struct ata_sector ata_sector;
>  
>  	/*
>  	 * fill in chs command to read from disk at given location
> @@ -772,8 +771,7 @@ ob_ide_read_ata_chs(struct ide_drive *drive, unsigned long long block,
>  	cmd->buffer = buf;
>  	cmd->buflen = sectors * 512;
>  
> -	ata_sector.all = sectors;
> -	cmd->nsector = ata_sector.low;
> +	cmd->nsector = sectors & 0xff;
>  	cmd->sector = sect;
>  	cmd->lcyl = cyl;
>  	cmd->hcyl = cyl >> 8;
> @@ -815,19 +813,17 @@ ob_ide_read_ata_lba48(struct ide_drive *drive, unsigned long long block,
>                        unsigned char *buf, unsigned int sectors)
>  {
>  	struct ata_command *cmd = &drive->channel->ata_cmd;
> -	struct ata_sector ata_sector;
>  
>  	memset(cmd, 0, sizeof(*cmd));
>  
>  	cmd->buffer = buf;
>  	cmd->buflen = sectors * 512;
> -	ata_sector.all = sectors;
>  
>  	/*
>  	 * we are using tasklet addressing here
>  	 */
> -	cmd->task[2] = ata_sector.low;
> -	cmd->task[3] = ata_sector.high;
> +	cmd->task[2] = sectors & 0xff;
> +	cmd->task[3] = (sectors >> 8) & 0xff;

This "& 0xff" is useless as task is "unsigned char", see below for
block:

>  	cmd->task[4] = block;
>  	cmd->task[5] = block >>  8;
>  	cmd->task[6] = block >> 16;
> diff --git a/drivers/ide.h b/drivers/ide.h
> index fec0e0e..da79a71 100644
> --- a/drivers/ide.h
> +++ b/drivers/ide.h
> @@ -208,20 +208,6 @@ enum {
>  	atapi_ddir_write,
>  };
>  
> -struct ata_sector {
> -	u16 all;
> -	union {
> -#ifdef CONFIG_BIG_ENDIAN
> -		u8 high;
> -		u8 low;
> -#endif
> -#ifdef CONFIG_LITTLE_ENDIAN
> -		u8 low;
> -		u8 high;
> -#endif
> -	};
> -};
> -
>  static int ob_ide_atapi_request_sense(struct ide_drive *drive);
>  
>  #endif
> 




More information about the OpenBIOS mailing list