Re: [PATCH 10/28] ata: separate PATA timings code from libata-core.c

[Christoph Hellwig]

> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  libata-pata-timings.c - helper library for PATA timings

Please remove the file name from the top of the file header.

> +static void ata_timing_quantize(const struct ata_timing *t, struct ata_timing *q, int T, int UT)
> +{

> +void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,
> +		      struct ata_timing *m, unsigned int what)
> +{

Please fix the overly long lines while you're at it.

> +	if (what & ATA_TIMING_SETUP  ) m->setup   = max(a->setup,   b->setup);
> +	if (what & ATA_TIMING_ACT8B  ) m->act8b   = max(a->act8b,   b->act8b);
> +	if (what & ATA_TIMING_REC8B  ) m->rec8b   = max(a->rec8b,   b->rec8b);
> +	if (what & ATA_TIMING_CYC8B  ) m->cyc8b   = max(a->cyc8b,   b->cyc8b);
> +	if (what & ATA_TIMING_ACTIVE ) m->active  = max(a->active,  b->active);
> +	if (what & ATA_TIMING_RECOVER) m->recover = max(a->recover, b->recover);
> +	if (what & ATA_TIMING_DMACK_HOLD) m->dmack_hold = max(a->dmack_hold, b->dmack_hold);
> +	if (what & ATA_TIMING_CYCLE  ) m->cycle   = max(a->cycle,   b->cycle);
> +	if (what & ATA_TIMING_UDMA   ) m->udma    = max(a->udma,    b->udma);

and this very strange coding style.

> +	if (!(s = ata_timing_find_mode(speed)))
> +		return -EINVAL;

This should be:

	s = ata_timing_find_mode(speed);
	if (!s)
		return -EINVAL;

> +	/* In a few cases quantisation may produce enough errors to
> +	   leave t->cycle too low for the sum of active and recovery
> +	   if so we must correct this */

.. non-standard comment style here.

> +#ifdef CONFIG_ATA_ACPI
>  extern u8 ata_timing_cycle2mode(unsigned int xfer_shift, int cycle);
> +#endif

I don't think we need this ifdef - unused prototypes are completely
harmless.