On 1/29/20 6:23 PM, Christoph Hellwig wrote:
>> +// 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.
I've added additional patch in v2 version of the patchset fixing
above CodingStyle issues in PATA timings code in libata-core.c
(just before the code gets separated to libata-pata-timings.c).
>> +#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.
Fixed in v2 version of the patch.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
