[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