On 14/01/16 14:11, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Fri, Dec 04, 2015 at 02:57:09PM +0000, Jon Hunter wrote:
>> The tegra power partitions are referenced by a numerical ID which are
>> the same values programmed into the PMC registers for controlling the
>> partition. For a given device, the valid partition IDs may not be
>> contiguous and so simply checking that an ID is not greater than the
>> maximum ID supported may not mean it is valid. Fix this by computing
>> a bit-mask of the valid partition IDs for a device and add a macro that
>> will test if the partition is valid based upon this mask.
>>
>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>> ---
>> drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 28d3106d3add..0967bba13947 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -103,6 +103,7 @@
>> #define GPU_RG_CNTRL 0x2d4
>>
>> #define PMC_PWRGATE_STATE(status, id) ((status & BIT(id)) != 0)
>> +#define PMC_PWRGATE_IS_VALID(id) (pmc->powergates_mask & BIT(id))
>
> I'd prefer a static inline function here, too. If you do that you will
> also need to move the static inline below the struct tegra_pmc to avoid
> build errors. Also don't forget to check for id < 0, because it
> technically could be. It may also be worth checking for an upper bound
> just in case anybody was going to pass in a value other that the ones
> defined in include/soc/tegra/pmc.h.
Ok.
>> struct tegra_pmc_soc {
>> unsigned int num_powergates;
>> @@ -134,6 +135,7 @@ struct tegra_pmc_soc {
>> * @cpu_pwr_good_en: CPU power good signal is enabled
>> * @lp0_vec_phys: physical base address of the LP0 warm boot code
>> * @lp0_vec_size: size of the LP0 warm boot code
>> + * @powergates_mask: Bit mask of valid power gates
>> * @powergates_lock: mutex for power gate register access
>> */
>> struct tegra_pmc {
>> @@ -158,6 +160,7 @@ struct tegra_pmc {
>> bool cpu_pwr_good_en;
>> u32 lp0_vec_phys;
>> u32 lp0_vec_size;
>> + u32 powergates_mask;
>
> This seems rather risky. The highest partition ID that we currently have
> is 29, so there is potential that 32 bits will be exceeded in the near
> future. It'd be more future-proof to turn this into a bitmap from the
> start so that we never have to worry about it.
Yes, I had thought about that, but figured we could handle later. Ok, I
will look at this now.
>>
>> struct mutex powergates_lock;
>> };
>> @@ -213,7 +216,7 @@ static int tegra_powergate_set(int id, bool new_state)
>> */
>> int tegra_powergate_power_on(int id)
>> {
>> - if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> + if (!PMC_PWRGATE_IS_VALID(id))
>> return -EINVAL;
>
> In a similar vein I've been meaning, for a long time, to make the ID an
> unsigned integer. It might be worth doing that, even with this patch
> applied, but it can be a separate patch.
>
>> return tegra_powergate_set(id, true);
>> @@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
>> */
>> int tegra_powergate_power_off(int id)
>> {
>> - if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> + if (!PMC_PWRGATE_IS_VALID(id))
>> return -EINVAL;
>>
>> return tegra_powergate_set(id, false);
>> @@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
>> {
>> u32 status;
>>
>> - if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> + if (!PMC_PWRGATE_IS_VALID(id))
>> return -EINVAL;
>>
>> status = tegra_pmc_readl(PWRGATE_STATUS);
>> @@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
>> {
>> u32 mask;
>>
>> - if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> + if (!PMC_PWRGATE_IS_VALID(id))
>> return -EINVAL;
>>
>> /*
>> @@ -1084,7 +1087,7 @@ static int __init tegra_pmc_early_init(void)
>> struct device_node *np;
>> struct resource regs;
>> bool invert;
>> - u32 value;
>> + u32 value, i;
>
> I'd prefer an unsized type (unsigned int) for i because there is no
> reason why it should be sized.
Ok.
Jon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
