>>>>> Thomas Schwinge <tho...@codesourcery.com> writes:
[…]
> Does anyone spot any problems, or does this need more testing, or is
> it OK to commit right away?
I see none (but then, I'm not really a Mach or ATA expert),
apart from a few style and formatting issues.
[…]
> + if (id->cyls == 16383 && id->sectors == 63 &&
> + (id->heads == 15 || id->heads == 16) &&
> + id->lba_capacity >= 16383*63*id->heads)
> return 1; /* lba_capacity is our only option */
> - }
> +
Here, a diff line could've been saved by adding an opening { to
the if above. (IOW, no need to drop the C grouping construct
from here.)
[…]
> @@ -2568,8 +2581,7 @@ static inline void do_identify (ide_drive_t *drive,
> byte cmd)
> }
> /* Handle logical geometry translation by the drive */
> if ((id->field_valid & 1) && id->cur_cyls && id->cur_heads
> - && (id->cur_heads <= 16) && id->cur_sectors)
> - {
> + && (id->cur_heads <= 16) && id->cur_sectors) {
> /*
> * Extract the physical drive geometry for our use.
> * Note that we purposely do *not* update the bios info.
> @@ -2594,7 +2606,8 @@ static inline void do_identify (ide_drive_t *drive,
> byte cmd)
> }
> }
> /* Use physical geometry if what we have still makes no sense */
> - if ((!drive->head || drive->head > 16) && id->heads && id->heads <= 16)
> {
> + if ((!drive->head || drive->head > 16) &&
> + id->heads && id->heads <= 16) {
> drive->cyl = id->cyls;
> drive->head = id->heads;
> drive->sect = id->sectors;
These two are only stylistic changes, aren't they? For clarity,
it may be reasonable to put them into a separate patch.
Also, for consistency with the style of the code above, my
suggestion would be to put && to the second line instead:
+ if ((!drive->head || drive->head > 16)
+ && id->heads && id->heads <= 16) {
[…]
> @@ -3346,7 +3360,7 @@ done:
> * This routine is called from the partition-table code in genhd.c
> * to "convert" a drive to a logical geometry with fewer than 1024 cyls.
> *
> - * The second parameter, "xparm", determines exactly how the translation
> + * The second parameter, "xparm", determines exactly how the translation
> * will be handled:
> * 0 = convert to CHS with fewer than 1024 cyls
> * using the same method as Ontrack DiskManager.
> @@ -3354,10 +3368,11 @@ done:
> * -1 = similar to "0", plus redirect sector 0 to sector 1.
> * >1 = convert to a CHS geometry with "xparm" heads.
> *
> - * Returns 0 if the translation was not possible, if the device was not
> + * Returns 0 if the translation was not possible, if the device was not
In the two instances above, a (presumably unwanted) trailing
blank was added.
> * an IDE disk drive, or if a geometry was "forced" on the commandline.
> * Returns 1 if the geometry translation was successful.
> */
> +
It seems that no blank lines separate the function's description
from its definition (at least as well as the rest of the diff is
considered.)
> int ide_xlate_1024 (kdev_t i_rdev, int xparm, const char *msg)
> {
> ide_drive_t *drive;
> @@ -3365,7 +3380,11 @@ int ide_xlate_1024 (kdev_t i_rdev, int xparm, const
> char *msg)
> const byte *heads = head_vals;
> unsigned long tracks;
>
> - if ((drive = get_info_ptr(i_rdev)) == NULL || drive->forced_geom)
> + drive = get_info_ptr(i_rdev);
> + if (!drive)
> + return 0;
> +
> + if (drive->forced_geom)
> return 0;
>
> if (xparm > 1 && xparm <= drive->bios_head && drive->bios_sect == 63)
This looks like a purely stylistic change as well?
[…]
--
FSF associate member #7257
