[<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