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

[Laurent Vivier]

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