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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <57289FB5.5040705@nvidia.com>
Date:	Tue, 3 May 2016 13:55:17 +0100
From:	Jon Hunter <jonathanh@...dia.com>
To:	Laxman Dewangan <ldewangan@...dia.com>, <swarren@...dotorg.org>,
	<linus.walleij@...aro.org>, <gnurou@...il.com>,
	<robh+dt@...nel.org>, <mark.rutland@....com>,
	<thierry.reding@...il.com>
CC:	<linux-tegra@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH 3/6] soc/tegra: pmc: Add support for IO pads power state
 and voltage


On 03/05/16 13:31, Laxman Dewangan wrote:
> 
> On Tuesday 03 May 2016 06:04 PM, Jon Hunter wrote:
>> On 02/05/16 13:17, Laxman Dewangan wrote:
>>> +
>>> +    return tegra_io_rail_power_on(dpd_bit);
>>  From a readability standpoint the above seems weird because
>> tegra_io_pads_power_enable() takes an ID as the argument, translates it
>> to a bit value and passes it to tegra_io_rail_power_on() which also
>> takes an ID for the argument.
> 
> Yaah, I did not want to duplicate the implementation and hence it is there.
> We will get rid of the older APIs once this is IN and new mechanism there.
> 
> 
> 
>>> +
>>> +    bval = (io_volt_uv == 3300000) ? BIT(pwr_bit) : 0;
>>> +
>>> +    mutex_lock(&pmc->powergates_lock);
>>> +    tegra_pmc_read_modify_write(PMC_PWR_DET, BIT(pwr_bit),
>>> BIT(pwr_bit));
>>> +    tegra_pmc_read_modify_write(PMC_PWR_DET_VAL, BIT(pwr_bit), bval);
>>> +    mutex_unlock(&pmc->powergates_lock);
>> There are 4 instances of BIT(pwr_bit). May be we should do this once or
>> have tegra_io_pads_to_power_val() return the bit?
> 
> OK, will re-write this part.
> 
> 
>>
>>> +int tegra_io_pads_power_disable(int io_pad_id);
>>> +int tegra_io_pads_power_is_enabled(int io_pad_id);
>> What I don't like here, is now we have two public APIs to do the same
>> job because tegra_io_pads_power_enable/disable() calls
>> tegra_io_rail_power_off/on() internally. Furthermore, the two APIs use
>> different ID definitions to accomplish the same job. This shouldn't be
>> necessary.
> 
> Currently SOR driver is using the tegra_io_rail_power_off/on() APIs.
> Once the proper interface available then I will move sor driver to use
> new method and then we can full get rid of older APIs and macros.
> 
> Till that, we need to have this.

I prefer it is done before this series. In other words, if we need a
proper enum for the rail/pad IDs then add one and convert any existing
drivers over to use any new APIs first. The other option is to live with
the existing API names.

Jon

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ