[02/03] pinctrl: sh-pfc: r8a7790: Break out USB0 OVC/VBUS

Commit Message

Magnus Damm Jan. 29, 2014, 11:10 p.m. UTC

From: Magnus Damm <[e-mail]>

Create a new group for the USB0 OVC/VBUS pin by itself. This
allows us to monitor PWEN as GPIO on the Lager board.

Signed-off-by: Magnus Damm <[e-mail]>
---

 drivers/pinctrl/sh-pfc/pfc-r8a7790.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Laurent Pinchart Jan. 31, 2014, 1:17 a.m. UTC | #1

Hi Magnus,

Thank you for the patch.

On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
> From: Magnus Damm <[e-mail]>
> 
> Create a new group for the USB0 OVC/VBUS pin by itself. This
> allows us to monitor PWEN as GPIO on the Lager board.
>
> Signed-off-by: Magnus Damm <[e-mail]>
> ---
> 
>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> --- 0001/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> +++ work/drivers/pinctrl/sh-pfc/pfc-r8a7790.c	2014-01-24 
10:23:32.000000000
> +0900 @@ -3231,6 +3231,13 @@ static const unsigned int usb0_pins[] =
>  static const unsigned int usb0_mux[] = {
>  	USB0_PWEN_MARK, USB0_OVC_VBUS_MARK,
>  };
> +static const unsigned int usb0_ovc_vbus_pins[] = {
> +	/* OVC/VBUS */
> +	RCAR_GP_PIN(5, 19),
> +};
> +static const unsigned int usb0_ovc_vbus_mux[] = {
> +	USB0_OVC_VBUS_MARK,
> +};

Another option would have been to split the existing usb0 group in usb0_pwen 
and usb0_ovc. I'm not sure which is better though, I'd just like to know if 
you had given it a thought.

Regardless, what about naming the new group usb0_ovc instead of usb0_ovc_bus 
to keep names short ?

>  /* - USB1
> ------------------------------------------------------------------- */
> static const unsigned int usb1_pins[] = {
>  	/* PWEN, OVC */
> @@ -3789,6 +3796,7 @@ static const struct sh_pfc_pin_group pin
>  	SH_PFC_PIN_GROUP(tpu0_to2),
>  	SH_PFC_PIN_GROUP(tpu0_to3),
>  	SH_PFC_PIN_GROUP(usb0),
> +	SH_PFC_PIN_GROUP(usb0_ovc_vbus),
>  	SH_PFC_PIN_GROUP(usb1),
>  	SH_PFC_PIN_GROUP(usb2),
>  	VIN_DATA_PIN_GROUP(vin0_data, 24),
> @@ -4134,6 +4142,7 @@ static const char * const tpu0_groups[]
> 
>  static const char * const usb0_groups[] = {
>  	"usb0",
> +	"usb0_ovc_vbus",
>  };
> 
>  static const char * const usb1_groups[] = {

Magnus Damm Jan. 31, 2014, 3:10 a.m. UTC | #2

Hi Laurent,

On Fri, Jan 31, 2014 at 10:17 AM, Laurent Pinchart
<[e-mail]> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
>> From: Magnus Damm <[e-mail]>
>>
>> Create a new group for the USB0 OVC/VBUS pin by itself. This
>> allows us to monitor PWEN as GPIO on the Lager board.
>>
>> Signed-off-by: Magnus Damm <[e-mail]>
>> ---
>>
>>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> --- 0001/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
>> +++ work/drivers/pinctrl/sh-pfc/pfc-r8a7790.c 2014-01-24
> 10:23:32.000000000
>> +0900 @@ -3231,6 +3231,13 @@ static const unsigned int usb0_pins[] =
>>  static const unsigned int usb0_mux[] = {
>>       USB0_PWEN_MARK, USB0_OVC_VBUS_MARK,
>>  };
>> +static const unsigned int usb0_ovc_vbus_pins[] = {
>> +     /* OVC/VBUS */
>> +     RCAR_GP_PIN(5, 19),
>> +};
>> +static const unsigned int usb0_ovc_vbus_mux[] = {
>> +     USB0_OVC_VBUS_MARK,
>> +};
>
> Another option would have been to split the existing usb0 group in usb0_pwen
> and usb0_ovc. I'm not sure which is better though, I'd just like to know if
> you had given it a thought.

I actually did just that in my first local attempt, but I decided not
to since it will only cause potential breakage.

> Regardless, what about naming the new group usb0_ovc instead of usb0_ovc_bus
> to keep names short ?

Is there any particular reason why you want shorter names?

From my side, I prefer to keep the names in sync with the data sheet.
In this particular case it is a shared pin so OVC is used for Host
while VBUS is used for gadget, so if you're proposing to ditch VBUS
then this feels somewhat inconsistent with the current gadget use
case. =)

Thanks,

/ magnus

Simon Horman Feb. 6, 2014, 6:24 a.m. UTC | #3

Hi Laurent,

On Fri, Jan 31, 2014 at 12:10:05PM +0900, Magnus Damm wrote:
> Hi Laurent,
> 
> On Fri, Jan 31, 2014 at 10:17 AM, Laurent Pinchart
> <[e-mail]> wrote:
> > Hi Magnus,
> >
> > Thank you for the patch.
> >
> > On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
> >> From: Magnus Damm <[e-mail]>
> >>
> >> Create a new group for the USB0 OVC/VBUS pin by itself. This
> >> allows us to monitor PWEN as GPIO on the Lager board.
> >>
> >> Signed-off-by: Magnus Damm <[e-mail]>
> >> ---
> >>
> >>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c |    9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> --- 0001/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> >> +++ work/drivers/pinctrl/sh-pfc/pfc-r8a7790.c 2014-01-24
> > 10:23:32.000000000
> >> +0900 @@ -3231,6 +3231,13 @@ static const unsigned int usb0_pins[] =
> >>  static const unsigned int usb0_mux[] = {
> >>       USB0_PWEN_MARK, USB0_OVC_VBUS_MARK,
> >>  };
> >> +static const unsigned int usb0_ovc_vbus_pins[] = {
> >> +     /* OVC/VBUS */
> >> +     RCAR_GP_PIN(5, 19),
> >> +};
> >> +static const unsigned int usb0_ovc_vbus_mux[] = {
> >> +     USB0_OVC_VBUS_MARK,
> >> +};
> >
> > Another option would have been to split the existing usb0 group in usb0_pwen
> > and usb0_ovc. I'm not sure which is better though, I'd just like to know if
> > you had given it a thought.
> 
> I actually did just that in my first local attempt, but I decided not
> to since it will only cause potential breakage.
> 
> > Regardless, what about naming the new group usb0_ovc instead of usb0_ovc_bus
> > to keep names short ?
> 
> Is there any particular reason why you want shorter names?
> 
> >From my side, I prefer to keep the names in sync with the data sheet.
> In this particular case it is a shared pin so OVC is used for Host
> while VBUS is used for gadget, so if you're proposing to ditch VBUS
> then this feels somewhat inconsistent with the current gadget use
> case. =)

Hi Laurent,

I would like to move this patch forwards somehow.
If you are happy with it as-is could you consider merging it?
Otherwise, could you let me know what changes you would like made
so I can see about making it so?

Thanks

Laurent Pinchart Feb. 6, 2014, 11:01 a.m. UTC | #4

Hi Magnus,

On Friday 31 January 2014 12:10:05 Magnus Damm wrote:
> On Fri, Jan 31, 2014 at 10:17 AM, Laurent Pinchart wrote:
> > On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
> >> From: Magnus Damm <[e-mail]>
> >> 
> >> Create a new group for the USB0 OVC/VBUS pin by itself. This
> >> allows us to monitor PWEN as GPIO on the Lager board.
> >> 
> >> Signed-off-by: Magnus Damm <[e-mail]>
> >> ---
> >> 
> >>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c |    9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >> 
> >> --- 0001/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> >> +++ work/drivers/pinctrl/sh-pfc/pfc-r8a7790.c 2014-01-24
> > 10:23:32.000000000
> >> +0900 @@ -3231,6 +3231,13 @@ static const unsigned int usb0_pins[] =
> >>  static const unsigned int usb0_mux[] = {
> >>       USB0_PWEN_MARK, USB0_OVC_VBUS_MARK,
> >>  };
> >> +static const unsigned int usb0_ovc_vbus_pins[] = {
> >> +     /* OVC/VBUS */
> >> +     RCAR_GP_PIN(5, 19),
> >> +};
> >> +static const unsigned int usb0_ovc_vbus_mux[] = {
> >> +     USB0_OVC_VBUS_MARK,
> >> +};
> > 
> > Another option would have been to split the existing usb0 group in
> > usb0_pwen and usb0_ovc. I'm not sure which is better though, I'd just
> > like to know if you had given it a thought.
> 
> I actually did just that in my first local attempt, but I decided not
> to since it will only cause potential breakage.

OK. I assume that using PWEN without OVC/VBUS doesn't make sense, right ?

> > Regardless, what about naming the new group usb0_ovc instead of
> > usb0_ovc_bus to keep names short ?
> 
> Is there any particular reason why you want shorter names?

When it doesn't reduce clarity I prefer to keep names short, as that makes the 
code easier to read and write, and (slightly) lowers the memory footprint.

> From my side, I prefer to keep the names in sync with the data sheet. In
> this particular case it is a shared pin so OVC is used for Host while VBUS
> is used for gadget, so if you're proposing to ditch VBUS then this feels
> somewhat inconsistent with the current gadget use case. =)

I thought the pin was used for over current detection only, but that doesn't 
make sense for function mode, you're right. Let's keep the name as-is then.

Provided PWEN without OVC/VBUS doesn't make sense and won't be needed,

Acked-by: Laurent Pinchart <[e-mail]>

Magnus Damm Feb. 6, 2014, 1:34 p.m. UTC | #5

Hi Laurent,

On Thu, Feb 6, 2014 at 8:01 PM, Laurent Pinchart
<[e-mail]> wrote:
> Hi Magnus,
>
> On Friday 31 January 2014 12:10:05 Magnus Damm wrote:
>> On Fri, Jan 31, 2014 at 10:17 AM, Laurent Pinchart wrote:
>> > On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
>> >> From: Magnus Damm <[e-mail]>
>> >>
>> >> Create a new group for the USB0 OVC/VBUS pin by itself. This
>> >> allows us to monitor PWEN as GPIO on the Lager board.
>> >>
>> >> Signed-off-by: Magnus Damm <[e-mail]>
>> >> ---
>> >>
>> >>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c |    9 +++++++++
>> >>  1 file changed, 9 insertions(+)
>> >>
>> >> --- 0001/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
>> >> +++ work/drivers/pinctrl/sh-pfc/pfc-r8a7790.c 2014-01-24
>> > 10:23:32.000000000
>> >> +0900 @@ -3231,6 +3231,13 @@ static const unsigned int usb0_pins[] =
>> >>  static const unsigned int usb0_mux[] = {
>> >>       USB0_PWEN_MARK, USB0_OVC_VBUS_MARK,
>> >>  };
>> >> +static const unsigned int usb0_ovc_vbus_pins[] = {
>> >> +     /* OVC/VBUS */
>> >> +     RCAR_GP_PIN(5, 19),
>> >> +};
>> >> +static const unsigned int usb0_ovc_vbus_mux[] = {
>> >> +     USB0_OVC_VBUS_MARK,
>> >> +};
>> >
>> > Another option would have been to split the existing usb0 group in
>> > usb0_pwen and usb0_ovc. I'm not sure which is better though, I'd just
>> > like to know if you had given it a thought.
>>
>> I actually did just that in my first local attempt, but I decided not
>> to since it will only cause potential breakage.
>
> OK. I assume that using PWEN without OVC/VBUS doesn't make sense, right ?

Correct!

>> > Regardless, what about naming the new group usb0_ovc instead of
>> > usb0_ovc_bus to keep names short ?
>>
>> Is there any particular reason why you want shorter names?
>
> When it doesn't reduce clarity I prefer to keep names short, as that makes the
> code easier to read and write, and (slightly) lowers the memory footprint.

That sounds sane. =)

>> From my side, I prefer to keep the names in sync with the data sheet. In
>> this particular case it is a shared pin so OVC is used for Host while VBUS
>> is used for gadget, so if you're proposing to ditch VBUS then this feels
>> somewhat inconsistent with the current gadget use case. =)
>
> I thought the pin was used for over current detection only, but that doesn't
> make sense for function mode, you're right. Let's keep the name as-is then.
>
> Provided PWEN without OVC/VBUS doesn't make sense and won't be needed,
>
> Acked-by: Laurent Pinchart <[e-mail]>

Thanks,

/ magnus

Simon Horman Feb. 7, 2014, 12:15 a.m. UTC | #6

On Thu, Feb 06, 2014 at 10:34:27PM +0900, Magnus Damm wrote:
> Hi Laurent,
> 
> On Thu, Feb 6, 2014 at 8:01 PM, Laurent Pinchart
> <[e-mail]> wrote:
> > Hi Magnus,
> >
> > On Friday 31 January 2014 12:10:05 Magnus Damm wrote:
> >> On Fri, Jan 31, 2014 at 10:17 AM, Laurent Pinchart wrote:
> >> > On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
> >> >> From: Magnus Damm <[e-mail]>
> >> >>
> >> >> Create a new group for the USB0 OVC/VBUS pin by itself. This
> >> >> allows us to monitor PWEN as GPIO on the Lager board.
> >> >>
> >> >> Signed-off-by: Magnus Damm <[e-mail]>
> >> >> ---
> >> >>
> >> >>  drivers/pinctrl/sh-pfc/pfc-r8a7790.c |    9 +++++++++
> >> >>  1 file changed, 9 insertions(+)
> >> >>
> >> >> --- 0001/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> >> >> +++ work/drivers/pinctrl/sh-pfc/pfc-r8a7790.c 2014-01-24
> >> > 10:23:32.000000000
> >> >> +0900 @@ -3231,6 +3231,13 @@ static const unsigned int usb0_pins[] =
> >> >>  static const unsigned int usb0_mux[] = {
> >> >>       USB0_PWEN_MARK, USB0_OVC_VBUS_MARK,
> >> >>  };
> >> >> +static const unsigned int usb0_ovc_vbus_pins[] = {
> >> >> +     /* OVC/VBUS */
> >> >> +     RCAR_GP_PIN(5, 19),
> >> >> +};
> >> >> +static const unsigned int usb0_ovc_vbus_mux[] = {
> >> >> +     USB0_OVC_VBUS_MARK,
> >> >> +};
> >> >
> >> > Another option would have been to split the existing usb0 group in
> >> > usb0_pwen and usb0_ovc. I'm not sure which is better though, I'd just
> >> > like to know if you had given it a thought.
> >>
> >> I actually did just that in my first local attempt, but I decided not
> >> to since it will only cause potential breakage.
> >
> > OK. I assume that using PWEN without OVC/VBUS doesn't make sense, right ?
> 
> Correct!
> 
> >> > Regardless, what about naming the new group usb0_ovc instead of
> >> > usb0_ovc_bus to keep names short ?
> >>
> >> Is there any particular reason why you want shorter names?
> >
> > When it doesn't reduce clarity I prefer to keep names short, as that makes the
> > code easier to read and write, and (slightly) lowers the memory footprint.
> 
> That sounds sane. =)
> 
> >> From my side, I prefer to keep the names in sync with the data sheet. In
> >> this particular case it is a shared pin so OVC is used for Host while VBUS
> >> is used for gadget, so if you're proposing to ditch VBUS then this feels
> >> somewhat inconsistent with the current gadget use case. =)
> >
> > I thought the pin was used for over current detection only, but that doesn't
> > make sense for function mode, you're right. Let's keep the name as-is then.
> >
> > Provided PWEN without OVC/VBUS doesn't make sense and won't be needed,
> >
> > Acked-by: Laurent Pinchart <[e-mail]>
> 
> Thanks,

Thanks Magnus.

Laurent, with that in mind could you pick up this patch?

Laurent Pinchart Feb. 7, 2014, 12:36 a.m. UTC | #7

Hi Simon,

On Friday 07 February 2014 09:15:44 Simon Horman wrote:
> On Thu, Feb 06, 2014 at 10:34:27PM +0900, Magnus Damm wrote:
> > On Thu, Feb 6, 2014 at 8:01 PM, Laurent Pinchart wrote:
> > > On Friday 31 January 2014 12:10:05 Magnus Damm wrote:
> > >> On Fri, Jan 31, 2014 at 10:17 AM, Laurent Pinchart wrote:
> > >> > On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
> > >> >> From: Magnus Damm <[e-mail]>
> > >> >> 
> > >> >> Create a new group for the USB0 OVC/VBUS pin by itself. This
> > >> >> allows us to monitor PWEN as GPIO on the Lager board.
> > >> >> 
> > >> >> Signed-off-by: Magnus Damm <[e-mail]>

[snip]

> > > 
> > > Acked-by: Laurent Pinchart <[e-mail]>
> > 
> > Thanks,
> 
> Thanks Magnus.
> 
> Laurent, with that in mind could you pick up this patch?

I was thinking about letting Linus pick up the PFC patches again now that the 
flood is over. Of course, if it can help, I can still pick the patches up and 
submit pull requests to Linus.

Linus, what's your opinion on this ? Would you rather pick the patches 
directly after I've acked them, or process pull requests ?

Simon Horman Feb. 7, 2014, 1:39 a.m. UTC | #8

On Fri, Feb 07, 2014 at 01:36:35AM +0100, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Friday 07 February 2014 09:15:44 Simon Horman wrote:
> > On Thu, Feb 06, 2014 at 10:34:27PM +0900, Magnus Damm wrote:
> > > On Thu, Feb 6, 2014 at 8:01 PM, Laurent Pinchart wrote:
> > > > On Friday 31 January 2014 12:10:05 Magnus Damm wrote:
> > > >> On Fri, Jan 31, 2014 at 10:17 AM, Laurent Pinchart wrote:
> > > >> > On Thursday 30 January 2014 08:10:19 Magnus Damm wrote:
> > > >> >> From: Magnus Damm <[e-mail]>
> > > >> >> 
> > > >> >> Create a new group for the USB0 OVC/VBUS pin by itself. This
> > > >> >> allows us to monitor PWEN as GPIO on the Lager board.
> > > >> >> 
> > > >> >> Signed-off-by: Magnus Damm <[e-mail]>
> 
> [snip]
> 
> > > > 
> > > > Acked-by: Laurent Pinchart <[e-mail]>

Acked-by: Simon Horman <[e-mail]>

> > > 
> > > Thanks,
> > 
> > Thanks Magnus.
> > 
> > Laurent, with that in mind could you pick up this patch?
> 
> I was thinking about letting Linus pick up the PFC patches again now that the 
> flood is over. Of course, if it can help, I can still pick the patches up and 
> submit pull requests to Linus.
> 
> Linus, what's your opinion on this ? Would you rather pick the patches 
> directly after I've acked them, or process pull requests ?

Either way is fine for me :)

Linus Walleij Feb. 10, 2014, 9:17 a.m. UTC | #9

On Fri, Feb 7, 2014 at 1:36 AM, Laurent Pinchart
<[e-mail]> wrote:

> I was thinking about letting Linus pick up the PFC patches again now that the
> flood is over. Of course, if it can help, I can still pick the patches up and
> submit pull requests to Linus.
>
> Linus, what's your opinion on this ? Would you rather pick the patches
> directly after I've acked them, or process pull requests ?

As long as it's reasonable traffic I'll take it. :-)

I'll queue this one then, I guess only patch 2/3?

Yours,
Linus Walleij

Linus Walleij Feb. 10, 2014, 9:19 a.m. UTC | #10

On Thu, Jan 30, 2014 at 12:10 AM, Magnus Damm <[e-mail]> wrote:

> From: Magnus Damm <[e-mail]>
>
> Create a new group for the USB0 OVC/VBUS pin by itself. This
> allows us to monitor PWEN as GPIO on the Lager board.
>
> Signed-off-by: Magnus Damm <[e-mail]>

Patch applied with Laurent's and Simon's ACKs.

Yours,
Linus Walleij

Laurent Pinchart Feb. 10, 2014, 12:08 p.m. UTC | #11

Hi Linus,

On Monday 10 February 2014 10:17:00 Linus Walleij wrote:
> On Fri, Feb 7, 2014 at 1:36 AM, Laurent Pinchart wrote:
> > I was thinking about letting Linus pick up the PFC patches again now that
> > the flood is over. Of course, if it can help, I can still pick the
> > patches up and submit pull requests to Linus.
> > 
> > Linus, what's your opinion on this ? Would you rather pick the patches
> > directly after I've acked them, or process pull requests ?
> 
> As long as it's reasonable traffic I'll take it. :-)
> 
> I'll queue this one then, I guess only patch 2/3?

That's correct. Thank you.

Patch

--- 0001/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ work/drivers/pinctrl/sh-pfc/pfc-r8a7790.c	2014-01-24 10:23:32.000000000 +0900
@@ -3231,6 +3231,13 @@  static const unsigned int usb0_pins[] =
 static const unsigned int usb0_mux[] = {
 	USB0_PWEN_MARK, USB0_OVC_VBUS_MARK,
 };
+static const unsigned int usb0_ovc_vbus_pins[] = {
+	/* OVC/VBUS */
+	RCAR_GP_PIN(5, 19),
+};
+static const unsigned int usb0_ovc_vbus_mux[] = {
+	USB0_OVC_VBUS_MARK,
+};
 /* - USB1 ------------------------------------------------------------------- */
 static const unsigned int usb1_pins[] = {
 	/* PWEN, OVC */
@@ -3789,6 +3796,7 @@  static const struct sh_pfc_pin_group pin
 	SH_PFC_PIN_GROUP(tpu0_to2),
 	SH_PFC_PIN_GROUP(tpu0_to3),
 	SH_PFC_PIN_GROUP(usb0),
+	SH_PFC_PIN_GROUP(usb0_ovc_vbus),
 	SH_PFC_PIN_GROUP(usb1),
 	SH_PFC_PIN_GROUP(usb2),
 	VIN_DATA_PIN_GROUP(vin0_data, 24),
@@ -4134,6 +4142,7 @@  static const char * const tpu0_groups[]
 
 static const char * const usb0_groups[] = {
 	"usb0",
+	"usb0_ovc_vbus",
 };
 
 static const char * const usb1_groups[] = {