linux-kernel - Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

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: <2398099b-16e6-f155-5852-45ba3dbc21ef@ti.com>
Date:   Wed, 29 May 2019 06:51:32 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Mark Brown <broonie@...nel.org>
CC:     <jacek.anaszewski@...il.com>, <pavel@....cz>,
        <lgirdwood@...il.com>, <lee.jones@...aro.org>,
        <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register
 enable flexible

Mark

On 5/26/19 7:48 AM, Mark Brown wrote:
> On Thu, May 23, 2019 at 08:50:20AM -0500, Dan Murphy wrote:
>> On 5/23/19 8:03 AM, Mark Brown wrote:
>>> On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:
>>> Is it guaranteed that the bitmask for enabling the use of the GPIO is
>>> going to be the same for all regulators?  The bitmasks for the regulator
>>> enable look to be different, and it also looks like this setting might
>>> affect multiple regulators since it seems there are multiple enable bits
>>> in the same register.  If this affects multiple regulators then how's
>>> that working at the minute?
>> Yes for the 3632 and 36274 bit0 is the EXT_EN for LCM on these chips.
>> LM3631 does not have LCM GPIO control so there is no setting and this should not be called.
>> If it is then the developer implemented the DT wrong.
> This feels fragile - it works for the current users but it's just
> assuming that the placement of this bit will always be in the same
> position in the same register as the enable and will silently fail if a
> new chip variant does things differently.  Either storing the data
> separately somewhere driver specific or just having explicit switch
> statements would be more robust.


Although I don't disagree with you I don't see how the interface is 
fragile with only these 3 regulators defined.

Would it not be prudent to amend this driver if/when a new regulator is 
needed that has a different enable bit/register combination?   And if 
that was the case I would almost expect a different driver completely if 
the regmap did not line up correctly.  I only reused this driver because 
the registers and bits lined up and did not think it was necessary to 
create a whole new driver.

Dan