Re: [PATCH V6 3/3] soc/tegra: pmc: Add support for IO pads power state and voltage

[Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On Friday 20 May 2016 07:02 PM, Jon Hunter wrote:
> On 20/05/16 12:59, Laxman Dewangan wrote:
>> +/* tegra_io_pads_config_info: Tegra IO pads bit config info.
>> + * @dpd_config_bit: DPD configuration bit position. -1 if not supported.
>> + * @voltage_config_bit: Voltage configuration bit position. -1 if not supported.
>> + * @soc_mask: Bitwise OR of SoC masks if IO pads supported on that SoC.
>> + */
> Comment coding style :-(

I saw this style multiple places and so intentionally left here.
If comment is inside the code then
/*
  * first-line comment
  * second line
  */

but for function, it can have in single line.

Anyhow, I will correct in next cycle.


>
>> +static inline int tegra_io_pads_to_dpd_bit(const struct tegra_pmc_soc *soc,
>> +					   enum tegra_io_pads id)
>>   {
>> -	unsigned long rate, value;
>> +	if (tegra_io_pads_configs[id].soc_mask & soc->io_pads_soc_mask)
>> +		return tegra_io_pads_configs[id].dpd_config_bit;
> I realise now that we are not checking if 'id' is greater than
> TEGRA_IO_PADS_MAX anywhere. This should probably be handled here.

Do we need to check? Our parameter type is enum type and hence it is not 
expected to have outside of the MAX.
I think it will be unnecessarily check here.


>>   
>>
>> +	int ret;
>> +
>> +	ret = tegra_io_pads_to_dpd_bit(pmc->soc, id);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*bit = ret % 32;
>> +
>> +	if (*bit < 32) {
> Isn't bit always less than 32 here now?
>
Yaah this is bug. should be if (ret < 32)

My testcase has the pad name whose dpd bit is < 32 and hence did not 
catch it..

BTW, do you have the T124 based platform for SOR testing? I have T210 
platforms where I am testing.