[PATCH v6 1/2] usb: gadget: pxa27x_udc: prepare device-tree support
For this preparation, a preliminary cleanup is done :
- convert the probing of pxa27x_udc to gpio_desc.
The conversion is partial because :
- the platform data still provides a gpio number, not a gpio desc
- the invert attribute is lost, hence a loss in the translation
- convert the mach info, and store the udc_command in the pxa_udc
control structure
- loose the udc_is_connected() in mach info
This was not really used, as mioa701 machine doesn't need it,
balloon3 doesn't really use it, and most importantly the current
driver never uses it.
The drawback with the gpio_desc conversion is that the inverted gpio
attribute is lost, as no gpiod_*() function exists to set the
active_low state of a gpio, and that attribute was at driver's
creation forecast to be set up by the driver and not the machine
specific code.
Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
drivers/usb/gadget/udc/pxa27x_udc.c | 51 -
drivers/usb/gadget/udc/pxa27x_udc.h | 6 +++--
2 files changed, 26 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 597d39f..e11f9c5 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -22,6 +22,7 @@
#include linux/clk.h
#include linux/irq.h
#include linux/gpio.h
+#include linux/gpio/consumer.h
#include linux/slab.h
#include linux/prefetch.h
#include linux/byteorder/generic.h
@@ -1507,18 +1508,13 @@ static struct usb_ep_ops pxa_ep_ops = {
*/
static void dplus_pullup(struct pxa_udc *udc, int on)
{
- if (on) {
- if (gpio_is_valid(udc-mach-gpio_pullup))
- gpio_set_value(udc-mach-gpio_pullup,
- !udc-mach-gpio_pullup_inverted);
- if (udc-mach-udc_command)
- udc-mach-udc_command(PXA2XX_UDC_CMD_CONNECT);
- } else {
- if (gpio_is_valid(udc-mach-gpio_pullup))
- gpio_set_value(udc-mach-gpio_pullup,
- udc-mach-gpio_pullup_inverted);
- if (udc-mach-udc_command)
- udc-mach-udc_command(PXA2XX_UDC_CMD_DISCONNECT);
+ if (udc-gpiod) {
+ gpiod_set_value(udc-gpiod, on);
+ } else if (udc-udc_command) {
+ if (on)
+ udc-udc_command(PXA2XX_UDC_CMD_CONNECT);
+ else
+ udc-udc_command(PXA2XX_UDC_CMD_DISCONNECT);
}
udc-pullup_on = on;
}
@@ -1609,7 +1605,7 @@ static int pxa_udc_pullup(struct usb_gadget *_gadget, int
is_active)
{
struct pxa_udc *udc = to_gadget_udc(_gadget);
- if (!gpio_is_valid(udc-mach-gpio_pullup) !udc-mach-udc_command)
+ if (!udc-gpiod !udc-udc_command)
return -EOPNOTSUPP;
dplus_pullup(udc, is_active);
@@ -2415,7 +2411,13 @@ static int pxa_udc_probe(struct platform_device *pdev)
{
struct resource *regs;
struct pxa_udc *udc = memory;
- int retval = 0, gpio;
+ struct pxa2xx_udc_mach_info *mach = dev_get_platdata(pdev-dev);
+ int retval = 0;
+
+ if (mach) {
+ udc-gpiod = gpio_to_desc(mach-gpio_pullup);
+ udc-udc_command = mach-udc_command;
+ }
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!regs)
@@ -2425,21 +2427,15 @@ static int pxa_udc_probe(struct platform_device *pdev)
return udc-irq;
udc-dev = pdev-dev;
- udc-mach = dev_get_platdata(pdev-dev);
udc-transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
- gpio = udc-mach-gpio_pullup;
- if (gpio_is_valid(gpio)) {
- retval = gpio_request(gpio, USB D+ pullup);
- if (retval == 0)
- gpio_direction_output(gpio,
- udc-mach-gpio_pullup_inverted);
- }
- if (retval) {
- dev_err(pdev-dev, Couldn't request gpio %d : %d\n,
- gpio, retval);
- return retval;
+ if (IS_ERR(udc-gpiod)) {
+ dev_err(pdev-dev, Couldn't find or request D+ gpio : %ld\n,
+ PTR_ERR(udc-gpiod));
+ return PTR_ERR(udc-gpiod);
}
+ if (udc-gpiod)
+ gpiod_direction_output(udc-gpiod, 0);
udc-clk = clk_get(pdev-dev, NULL);
if (IS_ERR(udc-clk)) {
@@ -2501,14 +2497,11 @@ err_clk:
static int pxa_udc_remove(struct platform_device *_dev)
{
struct pxa_udc *udc = platform_get_drvdata(_dev);
- int gpio = udc-mach-gpio_pullup;
usb_del_gadget_udc(udc-gadget);
usb_gadget_unregister_driver(udc-driver);
free_irq(udc-irq, udc);
pxa_cleanup_debugfs(udc);
- if (gpio_is_valid(gpio))
- gpio_free(gpio);
usb_put_phy(udc-transceiver);
diff --git
Re: [PATCH v6 1/2] usb: gadget: pxa27x_udc: prepare device-tree support
On Wed, Sep 24, 2014 at 09:41:12PM +0200, Robert Jarzmik wrote: For this preparation, a preliminary cleanup is done : - convert the probing of pxa27x_udc to gpio_desc. The conversion is partial because : - the platform data still provides a gpio number, not a gpio desc - the invert attribute is lost, hence a loss in the translation I asked for gpio to gpiod conversion to be a separate patch. It'll make things a lot easier to review. - convert the mach info, and store the udc_command in the pxa_udc control structure - loose the udc_is_connected() in mach info This was not really used, as mioa701 machine doesn't need it, balloon3 doesn't really use it, and most importantly the current driver never uses it. The drawback with the gpio_desc conversion is that the inverted gpio attribute is lost, as no gpiod_*() function exists to set the it's not lost, it's handled for you by the gpio library. Look at how GPIO_ACTIVE_LOW is used. -- balbi signature.asc Description: Digital signature
Re: [PATCH v6 1/2] usb: gadget: pxa27x_udc: prepare device-tree support
Felipe Balbi ba...@ti.com writes: On Wed, Sep 24, 2014 at 09:41:12PM +0200, Robert Jarzmik wrote: For this preparation, a preliminary cleanup is done : - convert the probing of pxa27x_udc to gpio_desc. The conversion is partial because : - the platform data still provides a gpio number, not a gpio desc - the invert attribute is lost, hence a loss in the translation I asked for gpio to gpiod conversion to be a separate patch. It'll make things a lot easier to review. What does patch 1/2 do then ? There are 2 lines for udc_command (1 in .h and 1.c), and all the remaining is the gpiod conversion. Is it difficult for you to review a 51 lines changed patch you asked for ? Do you want me to ask other people to help you ? - convert the mach info, and store the udc_command in the pxa_udc control structure - loose the udc_is_connected() in mach info This was not really used, as mioa701 machine doesn't need it, balloon3 doesn't really use it, and most importantly the current driver never uses it. The drawback with the gpio_desc conversion is that the inverted gpio attribute is lost, as no gpiod_*() function exists to set the it's not lost, it's handled for you by the gpio library. Look at how GPIO_ACTIVE_LOW is used. It is so, the above assertion no gpiod_*() function exists is false. Therefore, which is the right function in the gpio library accessible to a driver ? Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] usb: gadget: pxa27x_udc: prepare device-tree support
Hi,
On Wed, Sep 24, 2014 at 10:14:53PM +0200, Robert Jarzmik wrote:
Felipe Balbi ba...@ti.com writes:
On Wed, Sep 24, 2014 at 09:41:12PM +0200, Robert Jarzmik wrote:
For this preparation, a preliminary cleanup is done :
- convert the probing of pxa27x_udc to gpio_desc.
The conversion is partial because :
- the platform data still provides a gpio number, not a gpio desc
- the invert attribute is lost, hence a loss in the translation
I asked for gpio to gpiod conversion to be a separate patch. It'll make
things a lot easier to review.
What does patch 1/2 do then ? There are 2 lines for udc_command (1 in .h and
1.c), and all the remaining is the gpiod conversion.
Is it difficult for you to review a 51 lines changed patch you asked for ?
Do
you want me to ask other people to help you ?
if you reply like this again I'll start ignoring your patches. Settle
down, I'm trying to help you get your patches merged.
With that said:
patch 1 should *ONLY* convert gpio_* to gpiod_*. That turns the patch
into a clear cleanup where people don't need to spend too much time
reviewing details.
- convert the mach info, and store the udc_command in the pxa_udc
control structure
this is patch 2.
- loose the udc_is_connected() in mach info
This was not really used, as mioa701 machine doesn't need it,
balloon3 doesn't really use it, and most importantly the current
driver never uses it.
and this is patch 3.
The drawback with the gpio_desc conversion is that the inverted gpio
attribute is lost, as no gpiod_*() function exists to set the
it's not lost, it's handled for you by the gpio library. Look at how
GPIO_ACTIVE_LOW is used.
It is so, the above assertion no gpiod_*() function exists is
false. Therefore, which is the right function in the gpio library accessible
to
a driver ?
look at the implementation of gpiod_set_value:
1271 void gpiod_set_value(struct gpio_desc *desc, int value)
1272 {
1273 if (!desc)
1274 return;
1275 /* Should be using gpio_set_value_cansleep() */
1276 WARN_ON(desc-chip-can_sleep);
1277 if (test_bit(FLAG_ACTIVE_LOW, desc-flags))
1278 value = !value;
1279 _gpiod_set_raw_value(desc, value);
1280 }
1281 EXPORT_SYMBOL_GPL(gpiod_set_value);
do you see how it handles our inverted flag internally. It seems like
what you're missing is a helper to set FLAG_ACTIVE_LOW when !OF
!ACPI. If that's really not part of the framework yet, propose a patch
as that would help many others convert to gpio descs and, later, DT.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v6 1/2] usb: gadget: pxa27x_udc: prepare device-tree support
Felipe Balbi ba...@ti.com writes:
Hi,
On Wed, Sep 24, 2014 at 10:14:53PM +0200, Robert Jarzmik wrote:
if you reply like this again I'll start ignoring your patches. Settle
down, I'm trying to help you get your patches merged.
With that said:
patch 1 should *ONLY* convert gpio_* to gpiod_*. That turns the patch
into a clear cleanup where people don't need to spend too much time
reviewing details.
- convert the mach info, and store the udc_command in the pxa_udc
control structure
this is patch 2.
So be it, I'll send it right after this mail.
- loose the udc_is_connected() in mach info
This was not really used, as mioa701 machine doesn't need it,
balloon3 doesn't really use it, and most importantly the current
driver never uses it.
and this is patch 3.
This is no patch, there is not a single line in the patch for that. This is just
a statement, I can remove it if you want.
The drawback with the gpio_desc conversion is that the inverted gpio
attribute is lost, as no gpiod_*() function exists to set the
it's not lost, it's handled for you by the gpio library. Look at how
GPIO_ACTIVE_LOW is used.
It is so, the above assertion no gpiod_*() function exists is
false. Therefore, which is the right function in the gpio library accessible
to
a driver ?
look at the implementation of gpiod_set_value:
1271 void gpiod_set_value(struct gpio_desc *desc, int value)
1272 {
1273 if (!desc)
1274 return;
1275 /* Should be using gpio_set_value_cansleep() */
1276 WARN_ON(desc-chip-can_sleep);
1277 if (test_bit(FLAG_ACTIVE_LOW, desc-flags))
1278 value = !value;
1279 _gpiod_set_raw_value(desc, value);
1280 }
1281 EXPORT_SYMBOL_GPL(gpiod_set_value);
do you see how it handles our inverted flag internally. It seems like
what you're missing is a helper to set FLAG_ACTIVE_LOW when !OF
!ACPI. If that's really not part of the framework yet, propose a patch
as that would help many others convert to gpio descs and, later, DT.
You still don't get my point I'm afraid, sic... It's not about using the flag,
it's about setting the flag from the driver. Or said differently the assertion
is there is not gpiod_*() function which toggles the FLAG_ACTIVE_LOW bit id
gpio_desc-flags.
I'll wait for Linus on that topic.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] usb: gadget: pxa27x_udc: prepare device-tree support
Hi,
On Wed, Sep 24, 2014 at 10:44:23PM +0200, Robert Jarzmik wrote:
On Wed, Sep 24, 2014 at 10:14:53PM +0200, Robert Jarzmik wrote:
if you reply like this again I'll start ignoring your patches. Settle
down, I'm trying to help you get your patches merged.
With that said:
patch 1 should *ONLY* convert gpio_* to gpiod_*. That turns the patch
into a clear cleanup where people don't need to spend too much time
reviewing details.
- convert the mach info, and store the udc_command in the pxa_udc
control structure
this is patch 2.
So be it, I'll send it right after this mail.
- loose the udc_is_connected() in mach info
This was not really used, as mioa701 machine doesn't need it,
balloon3 doesn't really use it, and most importantly the current
driver never uses it.
and this is patch 3.
This is no patch, there is not a single line in the patch for that. This is
just
a statement, I can remove it if you want.
nah, that's fine then.
The drawback with the gpio_desc conversion is that the inverted gpio
attribute is lost, as no gpiod_*() function exists to set the
it's not lost, it's handled for you by the gpio library. Look at how
GPIO_ACTIVE_LOW is used.
It is so, the above assertion no gpiod_*() function exists is
false. Therefore, which is the right function in the gpio library
accessible to
a driver ?
look at the implementation of gpiod_set_value:
1271 void gpiod_set_value(struct gpio_desc *desc, int value)
1272 {
1273 if (!desc)
1274 return;
1275 /* Should be using gpio_set_value_cansleep() */
1276 WARN_ON(desc-chip-can_sleep);
1277 if (test_bit(FLAG_ACTIVE_LOW, desc-flags))
1278 value = !value;
1279 _gpiod_set_raw_value(desc, value);
1280 }
1281 EXPORT_SYMBOL_GPL(gpiod_set_value);
do you see how it handles our inverted flag internally. It seems like
what you're missing is a helper to set FLAG_ACTIVE_LOW when !OF
!ACPI. If that's really not part of the framework yet, propose a patch
as that would help many others convert to gpio descs and, later, DT.
You still don't get my point I'm afraid, sic... It's not about using the
flag,
it's about setting the flag from the driver. Or said differently the
assertion
is there is not gpiod_*() function which toggles the FLAG_ACTIVE_LOW bit id
gpio_desc-flags.
I'll wait for Linus on that topic.
might be better, if I'm wrong, I'm wrong; but I'd expect gpiod_* to also
support legacy board-file-based booting so people can convert to DT in
smaller steps.
--
balbi
signature.asc
Description: Digital signature
