Re: [RFC PATCH 02/13] drm/tegra: Add helper functions for setting up DPAUX pads

From: Jon Hunter <jonathanh@nvidia.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Alexandre Courbot <gnurou@gmail.com>,
	Wolfram Sang <wsa@the-dreams.de>,
	Stephen Warren <swarren@wwwdotorg.org>,
	dri-devel@lists.freedesktop.org, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: Re: [RFC PATCH 02/13] drm/tegra: Add helper functions for setting up DPAUX pads
Date: Mon, 20 Jun 2016 08:59:19 +0100	[thread overview]
Message-ID: <5767A257.6080106@nvidia.com> (raw)
In-Reply-To: <20160617161138.GC27475@ulmo.ba.sec>

On 17/06/16 17:11, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Jun 17, 2016 at 01:03:36PM +0100, Jon Hunter wrote:
>> In preparation for adding pinctrl support for the DPAUX pads, add
>> helpers functions for configuring the pads and controlling the power
>> for the pads.
>>
>> Please note that although a simple if-statement could be used instead
>> of a case statement for configuring the pads as there are only two
>> possible modes, a case statement is used because when integrating with
>> the pinctrl framework, we need to be able to handle invalid modes that
>> could be passed.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/gpu/drm/tegra/dpaux.c | 75 ++++++++++++++++++++++++++-----------------
>>  1 file changed, 45 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
>> index 0874a7e5b37b..aa3a037fcd3b 100644
>> --- a/drivers/gpu/drm/tegra/dpaux.c
>> +++ b/drivers/gpu/drm/tegra/dpaux.c
>> @@ -267,6 +267,45 @@ static irqreturn_t tegra_dpaux_irq(int irq, void *data)
>>  	return ret;
>>  }
>>  
>> +static void tegra_dpaux_powerdown(struct tegra_dpaux *dpaux, bool enable)
>> +{
>> +	u32 value = tegra_dpaux_readl(dpaux, DPAUX_HYBRID_SPARE);
>> +
>> +	if (enable)
>> +		value |= DPAUX_HYBRID_SPARE_PAD_POWER_DOWN;
>> +	else
>> +		value &= ~DPAUX_HYBRID_SPARE_PAD_POWER_DOWN;
>> +
>> +	tegra_dpaux_writel(dpaux, value, DPAUX_HYBRID_SPARE);
>> +}
> 
> I'd like for this to be two functions without the boolean parameter. The
> reason is that without looking at the implementation there's no way to
> understand what the meaning of true and false is. If instead you call
> this:
> 
> 	static void tegra_dpaux_pad_power_down(struct tegra_dpaux *dpaux)
> 	{
> 		...
> 	}
> 
> and
> 
> 	static void tegra_dpaux_pad_power_up(struct tegra_dpaux *dpaux)
> 	{
> 		...
> 	}
> 
> you can easily deduce from the name what's going on.

OK, fine with me. I was not sure if it was obvious or not.

>> +static int tegra_dpaux_config(struct tegra_dpaux *dpaux, int function)
> 
> Can function not be unsigned?

Yes it can and in fact should be. I had thought that it needed to be
signed to match with the pinctrl-ops but on closer inspection that is
not the case.

Jon

-- 
nvpublic
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel