[PATCH 0/3] pinctrl: tegra-xusb: Clean-up and fixes

[PATCH 0/3] pinctrl: tegra-xusb: Clean-up and fixes

[Jon Hunter]

Some clean-up and fixes for tegra-xusb driver.

This had been boot tested on tegra20-trimslice, tegra30-beaver,
tegra114-dalmore-a04 and tegra124-jetson-tk1 and no issues seen.
Complete test information can be found below.

http://nvt.pwsan.com/pub/jonathanh-tester/testlogs/test_20150519061107_8d5ae2aeafa78eca9fd0d92d4069960782deb761/20150519061107/

Jon Hunter (3):
  pinctrl: tegra-xusb: Remove unused structure
  pinctrl: tegra-xusb: Fix allocation of pins
  pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer

 drivers/pinctrl/pinctrl-tegra-xusb.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH 1/3] pinctrl: tegra-xusb: Remove unused structure

[Jon Hunter]

The structure tegra_xusb_padctl_group is defined but never used and so
remove this.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pinctrl/pinctrl-tegra-xusb.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c
index 753d747d4261..3e8e4a914fb4 100644
--- a/drivers/pinctrl/pinctrl-tegra-xusb.c
+++ b/drivers/pinctrl/pinctrl-tegra-xusb.c
@@ -59,11 +59,6 @@ struct tegra_xusb_padctl_function {
 	unsigned int num_groups;
 };
 
-struct tegra_xusb_padctl_group {
-	const unsigned int *funcs;
-	unsigned int num_funcs;
-};
-
 struct tegra_xusb_padctl_soc {
 	const struct pinctrl_pin_desc *pins;
 	unsigned int num_pins;
-- 
1.9.1

[PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins

[Jon Hunter]

Commit e5b3b2d9ed20 ("pinctrl: allows not to define the get_group_pins
operation") allows pin controllers not to register the get_group_pins()
function. However, a side-effect of not registering this function is
that pins are not allocated and potentially multiple devices could
attempt to configure the same pins [1]. Although this problem exists in
the pinctrl core, because only a few devices are impacted by this, fix
this for tegra-xusb by adding the get_group_pins() function.

Please note that in addition to adding the get_group_pins() functions
the pins/lanes for the tegra-xusb also need to be registered when
calling pinctrl_register(). This also allows the current pinmux state
to be viewed by the debugfs node "pinmux-pins" for the tegra-xusb pad
controller.

[1] http://www.spinics.net/lists/linux-gpio/msg05810.html

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/pinctrl/pinctrl-tegra-xusb.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c
index 3e8e4a914fb4..c61594066e26 100644
--- a/drivers/pinctrl/pinctrl-tegra-xusb.c
+++ b/drivers/pinctrl/pinctrl-tegra-xusb.c
@@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl,
 	return padctl->soc->pins[group].name;
 }
 
+static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl,
+					    unsigned group,
+					    const unsigned **pins,
+					    unsigned *num_pins)
+{
+	struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl);
+
+	/*
+	 * For the tegra-xusb pad controller groups are synonomous
+	 * with lanes/pins and there is always one lane/pin per group.
+	 */
+	*pins = &padctl->soc->pins[group].number;
+	*num_pins = 1;
+
+	return 0;
+}
+
 enum tegra_xusb_padctl_param {
 	TEGRA_XUSB_PADCTL_IDDQ,
 };
@@ -248,6 +265,7 @@ static int tegra_xusb_padctl_dt_node_to_map(struct pinctrl_dev *pinctrl,
 static const struct pinctrl_ops tegra_xusb_padctl_pinctrl_ops = {
 	.get_groups_count = tegra_xusb_padctl_get_groups_count,
 	.get_group_name = tegra_xusb_padctl_get_group_name,
+	.get_group_pins = tegra_xusb_padctl_get_group_pins,
 	.dt_node_to_map = tegra_xusb_padctl_dt_node_to_map,
 	.dt_free_map = pinctrl_utils_dt_free_map,
 };
@@ -898,6 +916,8 @@ static int tegra_xusb_padctl_probe(struct platform_device *pdev)
 
 	memset(&padctl->desc, 0, sizeof(padctl->desc));
 	padctl->desc.name = dev_name(padctl->dev);
+	padctl->desc.pins = tegra124_pins;
+	padctl->desc.npins = ARRAY_SIZE(tegra124_pins);
 	padctl->desc.pctlops = &tegra_xusb_padctl_pinctrl_ops;
 	padctl->desc.pmxops = &tegra_xusb_padctl_pinmux_ops;
 	padctl->desc.confops = &tegra_xusb_padctl_pinconf_ops;
-- 
1.9.1

[PATCH 3/3] pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer

[Jon Hunter]

of_match_node() will return NULL if no match is found and so check for
this before dereferencing the pointer.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/pinctrl/pinctrl-tegra-xusb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c
index c61594066e26..0030cfb9c0a8 100644
--- a/drivers/pinctrl/pinctrl-tegra-xusb.c
+++ b/drivers/pinctrl/pinctrl-tegra-xusb.c
@@ -899,6 +899,9 @@ static int tegra_xusb_padctl_probe(struct platform_device *pdev)
 	padctl->dev = &pdev->dev;
 
 	match = of_match_node(tegra_xusb_padctl_of_match, pdev->dev.of_node);
+	if (!match)
+		return -EINVAL;
+
 	padctl->soc = match->data;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.9.1

Re: [PATCH 3/3] pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Tue, May 19, 2015 at 03:14:23PM +0100, Jon Hunter wrote:
> of_match_node() will return NULL if no match is found and so check for
> this before dereferencing the pointer.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/pinctrl/pinctrl-tegra-xusb.c | 3 +++
>  1 file changed, 3 insertions(+)

This isn't necessary. The driver is probed from DT only and therefore
.probe() will only be called if there's a matching entry in the OF
device ID table. match can never be NULL.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] pinctrl: tegra-xusb: Remove unused structure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

On Tue, May 19, 2015 at 03:14:21PM +0100, Jon Hunter wrote:
> The structure tegra_xusb_padctl_group is defined but never used and so
> remove this.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/pinctrl/pinctrl-tegra-xusb.c | 5 -----
>  1 file changed, 5 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] pinctrl: tegra-xusb: Check that of_match_node returns a valid pointer

[Jon Hunter]

On 19/05/15 15:18, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, May 19, 2015 at 03:14:23PM +0100, Jon Hunter wrote:
>> of_match_node() will return NULL if no match is found and so check for
>> this before dereferencing the pointer.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/pinctrl/pinctrl-tegra-xusb.c | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> This isn't necessary. The driver is probed from DT only and therefore
> .probe() will only be called if there's a matching entry in the OF
> device ID table. match can never be NULL.

Ah, ok that would make sense.

Cheers Jon

Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
> Commit e5b3b2d9ed20 ("pinctrl: allows not to define the get_group_pins
> operation") allows pin controllers not to register the get_group_pins()
> function. However, a side-effect of not registering this function is
> that pins are not allocated and potentially multiple devices could
> attempt to configure the same pins [1]. Although this problem exists in
> the pinctrl core, because only a few devices are impacted by this, fix
> this for tegra-xusb by adding the get_group_pins() function.

If I understand correctly this effectively makes get_group_pins()
mandatory, doesn't it? In that case, shouldn't commit e5b3b2d9ed20 be
reverted?

> Please note that in addition to adding the get_group_pins() functions
> the pins/lanes for the tegra-xusb also need to be registered when
> calling pinctrl_register(). This also allows the current pinmux state
> to be viewed by the debugfs node "pinmux-pins" for the tegra-xusb pad
> controller.

This sounds like a logically separate change and hence could warrant a
separate patch. I'll defer to Linus on this, though, so:

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
[...]

One more thing:

[...]
> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c
> index 3e8e4a914fb4..c61594066e26 100644
> --- a/drivers/pinctrl/pinctrl-tegra-xusb.c
> +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c
> @@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl,
>  	return padctl->soc->pins[group].name;
>  }
>  
> +static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl,
> +					    unsigned group,
> +					    const unsigned **pins,
> +					    unsigned *num_pins)
> +{
> +	struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl);
> +
> +	/*
> +	 * For the tegra-xusb pad controller groups are synonomous
> +	 * with lanes/pins and there is always one lane/pin per group.
> +	 */
> +	*pins = &padctl->soc->pins[group].number;

Shouldn't this be the same as pinctrl->desc->pins? In that case, maybe a
better solution would be to make .get_group_pins() mandatory again and
turn this into a pinctrl helper function that can be used by all group-
only pinctrl drivers?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins

[Jon Hunter]

On 19/05/15 15:30, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
>> Commit e5b3b2d9ed20 ("pinctrl: allows not to define the get_group_pins
>> operation") allows pin controllers not to register the get_group_pins()
>> function. However, a side-effect of not registering this function is
>> that pins are not allocated and potentially multiple devices could
>> attempt to configure the same pins [1]. Although this problem exists in
>> the pinctrl core, because only a few devices are impacted by this, fix
>> this for tegra-xusb by adding the get_group_pins() function.
> 
> If I understand correctly this effectively makes get_group_pins()
> mandatory, doesn't it? In that case, shouldn't commit e5b3b2d9ed20 be
> reverted?

Yes may be it should. I will defer to Linus here.

>> Please note that in addition to adding the get_group_pins() functions
>> the pins/lanes for the tegra-xusb also need to be registered when
>> calling pinctrl_register(). This also allows the current pinmux state
>> to be viewed by the debugfs node "pinmux-pins" for the tegra-xusb pad
>> controller.
> 
> This sounds like a logically separate change and hence could warrant a
> separate patch. I'll defer to Linus on this, though, so:

It needs to be registered as part of this patch otherwise the pins will
not be registered and pin_request() will fail. Today pin_request() does
not even get called for these pins. So I believe that this is needed as
part of this patch.

May be I was not clear above, but the debugfs "pinmux-pins" entry shows
nothing today for these pins. A by-product of this fix is that now we
can view the pinmux state of the pins via the debugfs.

> Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins

[Jon Hunter]

On 19/05/15 15:33, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
> [...]
> 
> One more thing:
> 
> [...]
>> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c
>> index 3e8e4a914fb4..c61594066e26 100644
>> --- a/drivers/pinctrl/pinctrl-tegra-xusb.c
>> +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c
>> @@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl,
>>  	return padctl->soc->pins[group].name;
>>  }
>>  
>> +static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl,
>> +					    unsigned group,
>> +					    const unsigned **pins,
>> +					    unsigned *num_pins)
>> +{
>> +	struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl);
>> +
>> +	/*
>> +	 * For the tegra-xusb pad controller groups are synonomous
>> +	 * with lanes/pins and there is always one lane/pin per group.
>> +	 */
>> +	*pins = &padctl->soc->pins[group].number;
> 
> Shouldn't this be the same as pinctrl->desc->pins? In that case, maybe a
> better solution would be to make .get_group_pins() mandatory again and
> turn this into a pinctrl helper function that can be used by all group-
> only pinctrl drivers?

Yes I believe that you are right. However, I don't think a helper
function would work here, because this function returns a pointer to an
array of pin numbers. In the case of xusb this is simplified a bit
because we are only returning a single number here for the group.
However, if you look at other pinctrl devices (such as pinctrl-tegra.c)
there could be more than one number in the array.

Cheers
Jon

Re: [PATCH 2/3] pinctrl: tegra-xusb: Fix allocation of pins

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]

On Tue, May 19, 2015 at 04:23:15PM +0100, Jon Hunter wrote:
> 
> On 19/05/15 15:33, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, May 19, 2015 at 03:14:22PM +0100, Jon Hunter wrote:
> > [...]
> > 
> > One more thing:
> > 
> > [...]
> >> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c b/drivers/pinctrl/pinctrl-tegra-xusb.c
> >> index 3e8e4a914fb4..c61594066e26 100644
> >> --- a/drivers/pinctrl/pinctrl-tegra-xusb.c
> >> +++ b/drivers/pinctrl/pinctrl-tegra-xusb.c
> >> @@ -125,6 +125,23 @@ static const char *tegra_xusb_padctl_get_group_name(struct pinctrl_dev *pinctrl,
> >>  	return padctl->soc->pins[group].name;
> >>  }
> >>  
> >> +static int tegra_xusb_padctl_get_group_pins(struct pinctrl_dev *pinctrl,
> >> +					    unsigned group,
> >> +					    const unsigned **pins,
> >> +					    unsigned *num_pins)
> >> +{
> >> +	struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl);
> >> +
> >> +	/*
> >> +	 * For the tegra-xusb pad controller groups are synonomous
> >> +	 * with lanes/pins and there is always one lane/pin per group.
> >> +	 */
> >> +	*pins = &padctl->soc->pins[group].number;
> > 
> > Shouldn't this be the same as pinctrl->desc->pins? In that case, maybe a
> > better solution would be to make .get_group_pins() mandatory again and
> > turn this into a pinctrl helper function that can be used by all group-
> > only pinctrl drivers?
> 
> Yes I believe that you are right. However, I don't think a helper
> function would work here, because this function returns a pointer to an
> array of pin numbers. In the case of xusb this is simplified a bit
> because we are only returning a single number here for the group.
> However, if you look at other pinctrl devices (such as pinctrl-tegra.c)
> there could be more than one number in the array.

I see. Oh well, guess this is as good as it gets then.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]