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

[Bartlomiej Zolnierkiewicz]

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