netdev - Re: [PATCH v1 net-next] net: dsa: microchip: add KSZ9477 I2C driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f0fce96-797d-ac9a-ae3c-591dfd6c5a41@denx.de>
Date:   Thu, 20 Dec 2018 00:12:42 +0100
From:   Marek Vasut <marex@...x.de>
To:     Tristram.Ha@...rochip.com, pavel@...x.de, jiri@...nulli.us,
        andrew@...n.ch
Cc:     sergio.paracuellos@...il.com, f.fainelli@...il.com,
        dan.carpenter@...cle.com, vivien.didelot@...oirfairelinux.com,
        UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org
Subject: Re: [PATCH v1 net-next] net: dsa: microchip: add KSZ9477 I2C driver

On 12/19/2018 11:50 PM, Tristram.Ha@...rochip.com wrote:
>>>>>> This header file makes no sense. Please move the functions into .c
>>>>>
>>>>> No, that would make code bigger & slower.
>>>>>
>>>>> It makes sense to me. But I'd add "inline" keyword to make the goal
>>>>> explicit.
>>>>
>>>> 1) It makes no sense to have header files for things like this. The
>>>>    functions are only used within the single .c file.
>>>>
>>>> 2) You cannot inline them, as they are used as ops.
>>>
>>> Ok, sorry for the noise.
>>
>> If you were to use regmap, this whole boilerplate would go away ...
> 
> Sometimes I am confused about this review process.
> 
> The new code is same as previous code submitted.  The old code was accepted,
> but then there are objections to the new code.

Yes, continuous quality improvement is important.

> So if I change the new code, should I go back to correct the old code?

Maintaining old code and testing old code is indeed most welcome.

> The ksz9477_i2c.c should be the same as ksz9477_i2c.c while ksz_i2c.h matches
> ksz_spi.h.  The idea is ksz_i2c.h can be used by another switch driver like ksz####_i2c.c.

The regmap hides all the complexity and avoids re-implementations of
these kinds of accessors over and over again.

-- 
Best regards,
Marek Vasut