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] [day] [month] [year] [list]
Date: Thu, 18 Jan 2024 17:37:40 +0100
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Mark Brown <broonie@...nel.org>
Cc: Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>,
 Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v5 5/5] hwmon: Add support for Amphenol ChipCap 2


On 18.01.24 17:04, Mark Brown wrote:
> On Thu, Jan 18, 2024 at 04:30:37PM +0100, Javier Carrasco wrote:
>> On 18.01.24 14:49, Mark Brown wrote:
>>> On Mon, Jan 15, 2024 at 09:02:25PM +0100, Javier Carrasco wrote:
> 
>>>> +static int cc2_enable(struct cc2_data *data)
>>>> +{
>>>> +	int ret;
> 
>>>> +	if (regulator_is_enabled(data->regulator))
>>>> +		return 0;
> 
>>> This is generally a sign that the regulator API usage is not good, the
>>> driver should not rely on references to the regulator held by anything
>>> else since whatever else is holding the regulator on could turn it off
>>> at any time.  If the driver did the enable itself then it should know
>>> that it did so and not need to query.
> 
>> The driver handles a dedicated regulator, but I wanted to account for
>> the cases where the attempts to enable and disable the regulator fail
>> and keep parity. If the disabling attempt fails, will the regulator not
>> stay enabled? In that case, an additional call to regulator_enable would
>> not be required, right?
>> That is the only reason I am using regulator_is_enabled(), but maybe
>> things don't work like that.
> 
> With exclusive use you can get away with this, you should have a comment
> for that case though.
> 
I will add a comment to clarify it.
>>>> +	ret = regulator_enable(data->regulator);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	/*
>>>> +	 * TODO: the startup-delay-us property of the regulator might be
>>>> +	 * added to the delay (if provided).
>>>> +	 * Currently there is no interface to read its value apart from
>>>> +	 * a direct access to regulator->rdev->constraints->enable_time,
>>>> +	 * which is discouraged like any direct access to the regulator_dev
>>>> +	 * structure. This would be relevant in cases where the startup delay
>>>> +	 * is in the range of milliseconds.
>>>> +	 */
>>>> +	usleep_range(CC2_STARTUP_TIME_US, CC2_STARTUP_TIME_US + 125);
> 
>>> Note that the regulator startup delay is the time taken for the
>>> regulator to power up so if the device needs additional delay then that
>>> will always need to be in addition to whatever the regulator is doing.
> 
>> What I mean by that is that the device cannot be ready until the
>> regulator powers it up (obvious) plus the start up time of the device
>> itself once it gets powered up. So if a regulator takes for example 1 ms
>> to power up, the sleep function could (and should) wait for 1 ms longer.
> 
> No, the sleep function should do nothing of the sort - if any delay is
> neeeded for the regulator it will be handled as part of enabling the
> regulator.  This is not exposed to client drivers because it is
> transparent to them.
That sounds great. Then there is no need for the comment altogether and
the TODO will go away.

Thank you again and best regards,
Javier Carrrasco

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ