* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 1:37 ` [PATCH 1/3] usb: misc: generic_onboard_hub: " Peter Chen
@ 2015-12-08 2:59 ` Felipe Balbi
2015-12-08 9:18 ` Peter Chen
2015-12-08 3:16 ` kbuild test robot
` (3 subsequent siblings)
4 siblings, 1 reply; 35+ messages in thread
From: Felipe Balbi @ 2015-12-08 2:59 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Peter Chen <peter.chen@freescale.com> writes:
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o
>
> obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
> obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB) += generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@freescale.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG
nope
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>
<linux/platform_data/generic_onboard_hub.h> ?
> +struct usb_hub_generic_data {
> + struct clk *clk;
seriously ? Is this really all ? What about that reset line below ?
In fact, that reset line should be using a generic reset-gpio.c instead
of a raw gpio.
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> + struct usb_hub_generic_data *hub_data;
> + int reset_pol = 0, duration_us = 50, ret = 0;
> + struct gpio_desc *gpiod_reset = NULL;
> +
> + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> + if (!hub_data)
> + return -ENOMEM;
> +
> + if (dev->of_node) {
> + struct device_node *node = dev->of_node;
> +
> + hub_data->clk = devm_clk_get(dev, "external_clk");
> + if (IS_ERR(hub_data->clk)) {
> + dev_dbg(dev, "Can't get external clock: %ld\n",
> + PTR_ERR(hub_data->clk));
how about setting clock to NULL to here ? then you don't need IS_ERR()
checks anywhere else.
> + }
braces shouldn't be used here, if you add NULL trick above,
however... then you can keep them.
> + /*
> + * Try to get the information for HUB reset, the
> + * default setting like below:
> + *
> + * - Reset state is low
> + * - The duration is 50us
> + */
> + if (of_find_property(node, "hub-reset-active-high", NULL))
> + reset_pol = 1;
you see, this is definitely *not* generic. You should write a generic
reset-gpio.c reset controller and describe the polarity on the gpio
binding. This driver *always* uses reset_assert(); reset_deassert(); and
reset-gpio implements though by gpiod_set_value() correctly.
Polarity _must_ be described elsewhere in DT.
> + of_property_read_u32(node, "hub-reset-duration-us",
> + &duration_us);
likewise, this should be described as a debounce time for the GPIO.
> + gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> + reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> + ret = PTR_ERR_OR_ZERO(gpiod_reset);
> + if (ret) {
> + dev_err(dev, "Failed to get reset gpio, err = %d\n",
> + ret);
> + return ret;
> + }
> + } else if (pdata) {
> + hub_data->clk = pdata->ext_clk;
> + duration_us = pdata->gpio_reset_duration_us;
> + reset_pol = pdata->gpio_reset_polarity;
> +
> + if (gpio_is_valid(pdata->gpio_reset)) {
> + ret = devm_gpio_request_one(
> + dev, pdata->gpio_reset, reset_pol
> + ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> + dev_name(dev));
> + if (!ret)
> + gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> + }
> + }
> +
> + if (!IS_ERR(hub_data->clk)) {
> + ret = clk_prepare_enable(hub_data->clk);
> + if (ret) {
> + dev_err(dev,
> + "Can't enable external clock: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (gpiod_reset) {
> + /* Sanity check */
> + if (duration_us > 1000000)
> + duration_us = 50;
this check makes no sense whatsoever. What I *really* need over 1s of
debounce ?
> + usleep_range(duration_us, duration_us + 100);
> + gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
wrong. You should _not_ have polarity checks here. You should have
already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
will handle the polarity for you.
> + }
> +
> + dev_set_drvdata(dev, hub_data);
> + return ret;
> +}
> +
> +static int usb_hub_generic_remove(struct platform_device *pdev)
> +{
> + struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> +
> + if (!IS_ERR(hub_data->clk))
> + clk_disable_unprepare(hub_data->clk);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> + {.compatible = "generic-onboard-hub"},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> + .probe = usb_hub_generic_probe,
> + .remove = usb_hub_generic_remove,
> + .driver = {
> + .name = "usb_hub_generic_onboard",
> + .of_match_table = usb_hub_generic_dt_ids,
> + },
> +};
> +
> +static int __init usb_hub_generic_init(void)
> +{
> + return platform_driver_register(&usb_hub_generic_driver);
> +}
> +subsys_initcall(usb_hub_generic_init);
> +
> +static void __exit usb_hub_generic_exit(void)
> +{
> + platform_driver_unregister(&usb_hub_generic_driver);
> +}
> +module_exit(usb_hub_generic_exit);
module_platform_driver();
> +MODULE_AUTHOR("Peter Chen <peter.chen@freescale.com>");
> +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> +MODULE_LICENSE("GPL");
comment header says GPL v2 only, this is GPL v2+. Which is correct ?
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151207/f2e29aad/attachment.sig>
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 2:59 ` Felipe Balbi
@ 2015-12-08 9:18 ` Peter Chen
2015-12-08 13:55 ` Felipe Balbi
0 siblings, 1 reply; 35+ messages in thread
From: Peter Chen @ 2015-12-08 9:18 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 07, 2015 at 08:59:19PM -0600, Felipe Balbi wrote:
>
> Hi,
>
> Peter Chen <peter.chen@freescale.com> writes:
> > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> > index 45fd4ac..da52e9a 100644
> > --- a/drivers/usb/misc/Makefile
> > +++ b/drivers/usb/misc/Makefile
> > @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o
> >
> > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
> > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o
> > +obj-$(CONFIG_USB_ONBOARD_HUB) += generic_onboard_hub.o
> > diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> > new file mode 100644
> > index 0000000..df722a0
> > --- /dev/null
> > +++ b/drivers/usb/misc/generic_onboard_hub.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * usb_hub_generic.c The generic onboard USB HUB driver
> > + *
> > + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> > + * Author: Peter Chen <peter.chen@freescale.com>
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 of
> > + * the License as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +/*
> > + * This driver is only for the USB HUB devices which need to control
> > + * their external pins(clock, reset, etc), and these USB HUB devices
> > + * are soldered at the board.
> > + */
> > +
> > +#define DEBUG
>
> nope
>
Will change.
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb/generic_onboard_hub.h>
>
> <linux/platform_data/generic_onboard_hub.h> ?
>
Yes, you are right, I did not know it
> > +struct usb_hub_generic_data {
> > + struct clk *clk;
>
> seriously ? Is this really all ? What about that reset line below ?
The clock is PHY input clock on the HUB, this clock may from SoC's PLL.
>
> In fact, that reset line should be using a generic reset-gpio.c instead
> of a raw gpio.
>
This reset gpio is not at mainline, I don't know what's reason
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186693.html
Philipp, do you know the reason?
> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> > + struct usb_hub_generic_data *hub_data;
> > + int reset_pol = 0, duration_us = 50, ret = 0;
> > + struct gpio_desc *gpiod_reset = NULL;
> > +
> > + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > + if (!hub_data)
> > + return -ENOMEM;
> > +
> > + if (dev->of_node) {
> > + struct device_node *node = dev->of_node;
> > +
> > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > + if (IS_ERR(hub_data->clk)) {
> > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > + PTR_ERR(hub_data->clk));
>
> how about setting clock to NULL to here ? then you don't need IS_ERR()
> checks anywhere else.
>
> > + }
>
> braces shouldn't be used here, if you add NULL trick above,
> however... then you can keep them.
>
Braces aren't needed, it may not too much useful to using NULL
as a indicator for error pointer.
> > + /*
> > + * Try to get the information for HUB reset, the
> > + * default setting like below:
> > + *
> > + * - Reset state is low
> > + * - The duration is 50us
> > + */
> > + if (of_find_property(node, "hub-reset-active-high", NULL))
> > + reset_pol = 1;
>
> you see, this is definitely *not* generic. You should write a generic
> reset-gpio.c reset controller and describe the polarity on the gpio
> binding. This driver *always* uses reset_assert(); reset_deassert(); and
> reset-gpio implements though by gpiod_set_value() correctly.
>
> Polarity _must_ be described elsewhere in DT.
>
> > + of_property_read_u32(node, "hub-reset-duration-us",
> > + &duration_us);
>
> likewise, this should be described as a debounce time for the GPIO.
>
Yes, if we are a reset gpio driver.
> > + gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > + reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > + ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > + if (ret) {
> > + dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > + ret);
> > + return ret;
> > + }
> > + } else if (pdata) {
> > + hub_data->clk = pdata->ext_clk;
> > + duration_us = pdata->gpio_reset_duration_us;
> > + reset_pol = pdata->gpio_reset_polarity;
> > +
> > + if (gpio_is_valid(pdata->gpio_reset)) {
> > + ret = devm_gpio_request_one(
> > + dev, pdata->gpio_reset, reset_pol
> > + ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > + dev_name(dev));
> > + if (!ret)
> > + gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > + }
> > + }
> > +
> > + if (!IS_ERR(hub_data->clk)) {
> > + ret = clk_prepare_enable(hub_data->clk);
> > + if (ret) {
> > + dev_err(dev,
> > + "Can't enable external clock: %d\n",
> > + ret);
> > + return ret;
> > + }
> > + }
> > +
> > + if (gpiod_reset) {
> > + /* Sanity check */
> > + if (duration_us > 1000000)
> > + duration_us = 50;
>
> this check makes no sense whatsoever. What I *really* need over 1s of
> debounce ?
Ok, I will delete this check.
>
> > + usleep_range(duration_us, duration_us + 100);
> > + gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>
> wrong. You should _not_ have polarity checks here. You should have
> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
> will handle the polarity for you.
Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
before.
>
> > + }
> > +
> > + dev_set_drvdata(dev, hub_data);
> > + return ret;
> > +}
> > +
> > +static int usb_hub_generic_remove(struct platform_device *pdev)
> > +{
> > + struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> > +
> > + if (!IS_ERR(hub_data->clk))
> > + clk_disable_unprepare(hub_data->clk);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> > + {.compatible = "generic-onboard-hub"},
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> > +#endif
> > +
> > +static struct platform_driver usb_hub_generic_driver = {
> > + .probe = usb_hub_generic_probe,
> > + .remove = usb_hub_generic_remove,
> > + .driver = {
> > + .name = "usb_hub_generic_onboard",
> > + .of_match_table = usb_hub_generic_dt_ids,
> > + },
> > +};
> > +
> > +static int __init usb_hub_generic_init(void)
> > +{
> > + return platform_driver_register(&usb_hub_generic_driver);
> > +}
> > +subsys_initcall(usb_hub_generic_init);
> > +
> > +static void __exit usb_hub_generic_exit(void)
> > +{
> > + platform_driver_unregister(&usb_hub_generic_driver);
> > +}
> > +module_exit(usb_hub_generic_exit);
>
> module_platform_driver();
I want this driver to be called before module init's.
>
> > +MODULE_AUTHOR("Peter Chen <peter.chen@freescale.com>");
> > +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> > +MODULE_LICENSE("GPL");
>
> comment header says GPL v2 only, this is GPL v2+. Which is correct ?
>
I will change it to GPL v2, thanks.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 9:18 ` Peter Chen
@ 2015-12-08 13:55 ` Felipe Balbi
2015-12-09 8:45 ` Peter Chen
0 siblings, 1 reply; 35+ messages in thread
From: Felipe Balbi @ 2015-12-08 13:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Peter Chen <peter.chen@freescale.com> writes:
>> seriously ? Is this really all ? What about that reset line below ?
>
> The clock is PHY input clock on the HUB, this clock may from SoC's
> PLL.
oh, you might have misunderstood my comment. I'm saying that there is
more than one thing you could cache in your private structure. That's
all.
>> > +static int usb_hub_generic_probe(struct platform_device *pdev)
>> > +{
>> > + struct device *dev = &pdev->dev;
>> > + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
>> > + struct usb_hub_generic_data *hub_data;
>> > + int reset_pol = 0, duration_us = 50, ret = 0;
>> > + struct gpio_desc *gpiod_reset = NULL;
>> > +
>> > + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
>> > + if (!hub_data)
>> > + return -ENOMEM;
>> > +
>> > + if (dev->of_node) {
>> > + struct device_node *node = dev->of_node;
>> > +
>> > + hub_data->clk = devm_clk_get(dev, "external_clk");
>> > + if (IS_ERR(hub_data->clk)) {
>> > + dev_dbg(dev, "Can't get external clock: %ld\n",
>> > + PTR_ERR(hub_data->clk));
>>
>> how about setting clock to NULL to here ? then you don't need IS_ERR()
>> checks anywhere else.
>>
>> > + }
>>
>> braces shouldn't be used here, if you add NULL trick above,
>> however... then you can keep them.
>>
>
> Braces aren't needed, it may not too much useful to using NULL
> as a indicator for error pointer.
heh, it's not about using it as an error pointer. I'm merely trying to
make clk optional. NULL is a valid clk, meaning you won't get NULL
pointer dereferences when passing it along clk_*() calls (if you find
any, it's likely a bug in CCF), so NULL can be used to cope with
optional clocks:
clk = clk_get(dev, "foo");
if (IS_ERR(clk)) {
if (PTR_ERR(clk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
else
clk = NULL;
}
>> > + /*
>> > + * Try to get the information for HUB reset, the
>> > + * default setting like below:
>> > + *
>> > + * - Reset state is low
>> > + * - The duration is 50us
>> > + */
>> > + if (of_find_property(node, "hub-reset-active-high", NULL))
>> > + reset_pol = 1;
>>
>> you see, this is definitely *not* generic. You should write a generic
>> reset-gpio.c reset controller and describe the polarity on the gpio
>> binding. This driver *always* uses reset_assert(); reset_deassert(); and
>> reset-gpio implements though by gpiod_set_value() correctly.
>>
>> Polarity _must_ be described elsewhere in DT.
>>
>> > + of_property_read_u32(node, "hub-reset-duration-us",
>> > + &duration_us);
>>
>> likewise, this should be described as a debounce time for the GPIO.
>>
>
> Yes, if we are a reset gpio driver.
even if you use a raw GPIO, polarity and duration must come through DT.
>> > + usleep_range(duration_us, duration_us + 100);
>> > + gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>>
>> wrong. You should _not_ have polarity checks here. You should have
>> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
>> will handle the polarity for you.
>
> Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
> before.
with open source code, that's a rather poor excuse, Peter.
>> > +static int __init usb_hub_generic_init(void)
>> > +{
>> > + return platform_driver_register(&usb_hub_generic_driver);
>> > +}
>> > +subsys_initcall(usb_hub_generic_init);
>> > +
>> > +static void __exit usb_hub_generic_exit(void)
>> > +{
>> > + platform_driver_unregister(&usb_hub_generic_driver);
>> > +}
>> > +module_exit(usb_hub_generic_exit);
>>
>> module_platform_driver();
>
> I want this driver to be called before module init's.
why ?
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151208/46d4f17b/attachment.sig>
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 13:55 ` Felipe Balbi
@ 2015-12-09 8:45 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2015-12-09 8:45 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 07:55:05AM -0600, Felipe Balbi wrote:
>
> Hi,
>
> Peter Chen <peter.chen@freescale.com> writes:
> >> seriously ? Is this really all ? What about that reset line below ?
> >
> > The clock is PHY input clock on the HUB, this clock may from SoC's
> > PLL.
>
> oh, you might have misunderstood my comment. I'm saying that there is
> more than one thing you could cache in your private structure. That's
> all.
>
How? I need to handle clock at both ->probe and ->remove.
> >> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> >> > +{
> >> > + struct device *dev = &pdev->dev;
> >> > + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> >> > + struct usb_hub_generic_data *hub_data;
> >> > + int reset_pol = 0, duration_us = 50, ret = 0;
> >> > + struct gpio_desc *gpiod_reset = NULL;
> >> > +
> >> > + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> >> > + if (!hub_data)
> >> > + return -ENOMEM;
> >> > +
> >> > + if (dev->of_node) {
> >> > + struct device_node *node = dev->of_node;
> >> > +
> >> > + hub_data->clk = devm_clk_get(dev, "external_clk");
> >> > + if (IS_ERR(hub_data->clk)) {
> >> > + dev_dbg(dev, "Can't get external clock: %ld\n",
> >> > + PTR_ERR(hub_data->clk));
> >>
> >> how about setting clock to NULL to here ? then you don't need IS_ERR()
> >> checks anywhere else.
> >>
> >> > + }
> >>
> >> braces shouldn't be used here, if you add NULL trick above,
> >> however... then you can keep them.
> >>
> >
> > Braces aren't needed, it may not too much useful to using NULL
> > as a indicator for error pointer.
>
> heh, it's not about using it as an error pointer. I'm merely trying to
> make clk optional. NULL is a valid clk, meaning you won't get NULL
> pointer dereferences when passing it along clk_*() calls (if you find
> any, it's likely a bug in CCF), so NULL can be used to cope with
> optional clocks:
>
> clk = clk_get(dev, "foo");
> if (IS_ERR(clk)) {
> if (PTR_ERR(clk) == -EPROBE_DEFER)
> return -EPROBE_DEFER;
> else
> clk = NULL;
> }
>
Get your point, so at coming code, we don't need to add condition
to enable optional clock.
> >> > + /*
> >> > + * Try to get the information for HUB reset, the
> >> > + * default setting like below:
> >> > + *
> >> > + * - Reset state is low
> >> > + * - The duration is 50us
> >> > + */
> >> > + if (of_find_property(node, "hub-reset-active-high", NULL))
> >> > + reset_pol = 1;
> >>
> >> you see, this is definitely *not* generic. You should write a generic
> >> reset-gpio.c reset controller and describe the polarity on the gpio
> >> binding. This driver *always* uses reset_assert(); reset_deassert(); and
> >> reset-gpio implements though by gpiod_set_value() correctly.
> >>
> >> Polarity _must_ be described elsewhere in DT.
> >>
> >> > + of_property_read_u32(node, "hub-reset-duration-us",
> >> > + &duration_us);
> >>
> >> likewise, this should be described as a debounce time for the GPIO.
> >>
> >
> > Yes, if we are a reset gpio driver.
>
> even if you use a raw GPIO, polarity and duration must come through DT.
>
> >> > + usleep_range(duration_us, duration_us + 100);
> >> > + gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> >>
> >> wrong. You should _not_ have polarity checks here. You should have
> >> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
> >> will handle the polarity for you.
> >
> > Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
> > before.
>
> with open source code, that's a rather poor excuse, Peter.
I will pay attention to it, thanks.
At my dts example, it is like below, I just treat it at raw gpio
handling.
usb_hub1 {
compatible = "generic-onboard-hub";
clocks = <&clks IMX6QDL_CLK_CKO>;
clock-names = "external_clk";
hub-reset-active-high;
hub-reset-gpios = <&gpio7 12 0>;
hub-reset-duration-us = <2>;
};
I will change it like below:
usb_hub1 {
compatible = "generic-onboard-hub";
clocks = <&clks IMX6QDL_CLK_CKO>;
clock-names = "clk";
reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
reset-duration-us = <2>;
};
>
> >> > +static int __init usb_hub_generic_init(void)
> >> > +{
> >> > + return platform_driver_register(&usb_hub_generic_driver);
> >> > +}
> >> > +subsys_initcall(usb_hub_generic_init);
> >> > +
> >> > +static void __exit usb_hub_generic_exit(void)
> >> > +{
> >> > + platform_driver_unregister(&usb_hub_generic_driver);
> >> > +}
> >> > +module_exit(usb_hub_generic_exit);
> >>
> >> module_platform_driver();
> >
> > I want this driver to be called before module init's.
>
> why ?
The USB HUB should be in ready state before controller
tries to talk with it, otherwise, it may has noise at
the bus during the HUB reset/power on process.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 1:37 ` [PATCH 1/3] usb: misc: generic_onboard_hub: " Peter Chen
2015-12-08 2:59 ` Felipe Balbi
@ 2015-12-08 3:16 ` kbuild test robot
2015-12-08 9:36 ` Peter Chen
2015-12-08 6:19 ` Sascha Hauer
` (2 subsequent siblings)
4 siblings, 1 reply; 35+ messages in thread
From: kbuild test robot @ 2015-12-08 3:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi Peter,
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.4-rc4 next-20151207]
url: https://github.com/0day-ci/linux/commits/Peter-Chen/USB-add-generic-onboard-USB-HUB-driver/20151208-094428
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: tile-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile
All error/warnings (new ones prefixed by >>):
drivers/usb/misc/generic_onboard_hub.c: In function 'usb_hub_generic_probe':
>> drivers/usb/misc/generic_onboard_hub.c:76:3: error: implicit declaration of function 'devm_gpiod_get_optional'
>> drivers/usb/misc/generic_onboard_hub.c:77:16: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
drivers/usb/misc/generic_onboard_hub.c:77:16: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/usb/misc/generic_onboard_hub.c:77:33: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
>> drivers/usb/misc/generic_onboard_hub.c:95:5: error: implicit declaration of function 'gpio_to_desc'
>> drivers/usb/misc/generic_onboard_hub.c:95:17: warning: assignment makes pointer from integer without a cast [enabled by default]
>> drivers/usb/misc/generic_onboard_hub.c:114:3: error: implicit declaration of function 'gpiod_set_value'
cc1: some warnings being treated as errors
vim +/devm_gpiod_get_optional +76 drivers/usb/misc/generic_onboard_hub.c
70 if (of_find_property(node, "hub-reset-active-high", NULL))
71 reset_pol = 1;
72
73 of_property_read_u32(node, "hub-reset-duration-us",
74 &duration_us);
75
> 76 gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> 77 reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
78 ret = PTR_ERR_OR_ZERO(gpiod_reset);
79 if (ret) {
80 dev_err(dev, "Failed to get reset gpio, err = %d\n",
81 ret);
82 return ret;
83 }
84 } else if (pdata) {
85 hub_data->clk = pdata->ext_clk;
86 duration_us = pdata->gpio_reset_duration_us;
87 reset_pol = pdata->gpio_reset_polarity;
88
89 if (gpio_is_valid(pdata->gpio_reset)) {
90 ret = devm_gpio_request_one(
91 dev, pdata->gpio_reset, reset_pol
92 ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
93 dev_name(dev));
94 if (!ret)
> 95 gpiod_reset = gpio_to_desc(pdata->gpio_reset);
96 }
97 }
98
99 if (!IS_ERR(hub_data->clk)) {
100 ret = clk_prepare_enable(hub_data->clk);
101 if (ret) {
102 dev_err(dev,
103 "Can't enable external clock: %d\n",
104 ret);
105 return ret;
106 }
107 }
108
109 if (gpiod_reset) {
110 /* Sanity check */
111 if (duration_us > 1000000)
112 duration_us = 50;
113 usleep_range(duration_us, duration_us + 100);
> 114 gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
115 }
116
117 dev_set_drvdata(dev, hub_data);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 42151 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151208/1811f234/attachment-0001.obj>
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 3:16 ` kbuild test robot
@ 2015-12-08 9:36 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2015-12-08 9:36 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 11:16:31AM +0800, kbuild test robot wrote:
> Hi Peter,
>
> [auto build test ERROR on usb/usb-testing]
> [also build test ERROR on v4.4-rc4 next-20151207]
>
> url: https://github.com/0day-ci/linux/commits/Peter-Chen/USB-add-generic-onboard-USB-HUB-driver/20151208-094428
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> config: tile-allmodconfig (attached as .config)
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=tile
>
> All error/warnings (new ones prefixed by >>):
>
> drivers/usb/misc/generic_onboard_hub.c: In function 'usb_hub_generic_probe':
> >> drivers/usb/misc/generic_onboard_hub.c:76:3: error: implicit declaration of function 'devm_gpiod_get_optional'
> >> drivers/usb/misc/generic_onboard_hub.c:77:16: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
> drivers/usb/misc/generic_onboard_hub.c:77:16: note: each undeclared identifier is reported only once for each function it appears in
> >> drivers/usb/misc/generic_onboard_hub.c:77:33: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
> >> drivers/usb/misc/generic_onboard_hub.c:95:5: error: implicit declaration of function 'gpio_to_desc'
> >> drivers/usb/misc/generic_onboard_hub.c:95:17: warning: assignment makes pointer from integer without a cast [enabled by default]
> >> drivers/usb/misc/generic_onboard_hub.c:114:3: error: implicit declaration of function 'gpiod_set_value'
> cc1: some warnings being treated as errors
>
> vim +/devm_gpiod_get_optional +76 drivers/usb/misc/generic_onboard_hub.c
>
> 70 if (of_find_property(node, "hub-reset-active-high", NULL))
> 71 reset_pol = 1;
> 72
> 73 of_property_read_u32(node, "hub-reset-duration-us",
> 74 &duration_us);
> 75
> > 76 gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > 77 reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> 78 ret = PTR_ERR_OR_ZERO(gpiod_reset);
> 79 if (ret) {
> 80 dev_err(dev, "Failed to get reset gpio, err = %d\n",
> 81 ret);
> 82 return ret;
> 83 }
> 84 } else if (pdata) {
> 85 hub_data->clk = pdata->ext_clk;
> 86 duration_us = pdata->gpio_reset_duration_us;
> 87 reset_pol = pdata->gpio_reset_polarity;
> 88
> 89 if (gpio_is_valid(pdata->gpio_reset)) {
> 90 ret = devm_gpio_request_one(
> 91 dev, pdata->gpio_reset, reset_pol
> 92 ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> 93 dev_name(dev));
> 94 if (!ret)
> > 95 gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> 96 }
> 97 }
> 98
> 99 if (!IS_ERR(hub_data->clk)) {
> 100 ret = clk_prepare_enable(hub_data->clk);
> 101 if (ret) {
> 102 dev_err(dev,
> 103 "Can't enable external clock: %d\n",
> 104 ret);
> 105 return ret;
> 106 }
> 107 }
> 108
> 109 if (gpiod_reset) {
> 110 /* Sanity check */
> 111 if (duration_us > 1000000)
> 112 duration_us = 50;
> 113 usleep_range(duration_us, duration_us + 100);
> > 114 gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> 115 }
> 116
> 117 dev_set_drvdata(dev, hub_data);
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
I will add <linux/gpio/consumer.h> at header file list.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 1:37 ` [PATCH 1/3] usb: misc: generic_onboard_hub: " Peter Chen
2015-12-08 2:59 ` Felipe Balbi
2015-12-08 3:16 ` kbuild test robot
@ 2015-12-08 6:19 ` Sascha Hauer
2015-12-08 9:26 ` Peter Chen
2015-12-08 9:48 ` Arnd Bergmann
2015-12-08 15:36 ` Mathieu Poirier
4 siblings, 1 reply; 35+ messages in thread
From: Sascha Hauer @ 2015-12-08 6:19 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 09:37:48AM +0800, Peter Chen wrote:
> Current USB HUB driver lacks of platform interfaces to configure
> external signal on HUB chip, eg, the PHY input clock and gpio reset
> pin for HUB, these kinds of HUBs are usually soldered at the board,
> and they are not hot-plug USB devices.
>
> With this patch, the user can configure the HUB's pins at platform
> part (through dts or platform), and these configuration will be
> effective before the USB bus can be used, we use subsys_initcall
> for this driver.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
> MAINTAINERS | 7 ++
> drivers/usb/misc/Kconfig | 9 ++
> drivers/usb/misc/Makefile | 1 +
> drivers/usb/misc/generic_onboard_hub.c | 162 ++++++++++++++++++++++++++++++++
> include/linux/usb/generic_onboard_hub.h | 11 +++
> 5 files changed, 190 insertions(+)
> create mode 100644 drivers/usb/misc/generic_onboard_hub.c
> create mode 100644 include/linux/usb/generic_onboard_hub.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S: Maintained
> F: Documentation/usb/ohci.txt
> F: drivers/usb/host/ohci*
>
> +USB Generic Onboard HUB Driver
> +M: Peter Chen <Peter.Chen@freescale.com>
> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L: linux-usb at vger.kernel.org
> +S: Maintained
> +F: drivers/usb/misc/generic_onboard_hub.c
> +
> USB OTG FSM (Finite State Machine)
> M: Peter Chen <Peter.Chen@freescale.com>
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -268,3 +268,12 @@ config USB_CHAOSKEY
>
> To compile this driver as a module, choose M here: the
> module will be called chaoskey.
> +
> +config USB_ONBOARD_HUB
> + tristate "Generic USB Onboard HUB"
> + help
> + Say Y here if your board has an onboard HUB, and this hub needs
> + to control its PHY clock and reset pin through external signals.
> + If you are not sure, say N.
> +
> + To compile this driver as a module, choose M here.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o
>
> obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
> obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB) += generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@freescale.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>
> +
> +struct usb_hub_generic_data {
> + struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> + struct usb_hub_generic_data *hub_data;
> + int reset_pol = 0, duration_us = 50, ret = 0;
> + struct gpio_desc *gpiod_reset = NULL;
> +
> + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> + if (!hub_data)
> + return -ENOMEM;
> +
> + if (dev->of_node) {
> + struct device_node *node = dev->of_node;
> +
> + hub_data->clk = devm_clk_get(dev, "external_clk");
Please drop such _clk suffixes. The context already makes it clear that
it's a clock.
> + if (IS_ERR(hub_data->clk)) {
> + dev_dbg(dev, "Can't get external clock: %ld\n",
> + PTR_ERR(hub_data->clk));
> + }
> +
> + /*
> + * Try to get the information for HUB reset, the
> + * default setting like below:
> + *
> + * - Reset state is low
> + * - The duration is 50us
> + */
> + if (of_find_property(node, "hub-reset-active-high", NULL))
> + reset_pol = 1;
> +
> + of_property_read_u32(node, "hub-reset-duration-us",
> + &duration_us);
> +
> + gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> + reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> + ret = PTR_ERR_OR_ZERO(gpiod_reset);
> + if (ret) {
> + dev_err(dev, "Failed to get reset gpio, err = %d\n",
> + ret);
> + return ret;
> + }
> + } else if (pdata) {
> + hub_data->clk = pdata->ext_clk;
Passing clocks in platform_data is a no go. clocks must always be
retrieved with clk_get.
> + duration_us = pdata->gpio_reset_duration_us;
> + reset_pol = pdata->gpio_reset_polarity;
> +
> + if (gpio_is_valid(pdata->gpio_reset)) {
> + ret = devm_gpio_request_one(
> + dev, pdata->gpio_reset, reset_pol
> + ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> + dev_name(dev));
> + if (!ret)
> + gpiod_reset = gpio_to_desc(pdata->gpio_reset);
If the gpio is valid then being unable to get it is an error.
Do you need this platform_data stuff at all?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 6:19 ` Sascha Hauer
@ 2015-12-08 9:26 ` Peter Chen
2015-12-08 9:44 ` Sascha Hauer
0 siblings, 1 reply; 35+ messages in thread
From: Peter Chen @ 2015-12-08 9:26 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > + if (!hub_data)
> > + return -ENOMEM;
> > +
> > + if (dev->of_node) {
> > + struct device_node *node = dev->of_node;
> > +
> > + hub_data->clk = devm_clk_get(dev, "external_clk");
>
> Please drop such _clk suffixes. The context already makes it clear that
> it's a clock.
>
Will change.
> > + if (IS_ERR(hub_data->clk)) {
> > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > + PTR_ERR(hub_data->clk));
> > + }
> > +
> > + /*
> > + * Try to get the information for HUB reset, the
> > + * default setting like below:
> > + *
> > + * - Reset state is low
> > + * - The duration is 50us
> > + */
> > + if (of_find_property(node, "hub-reset-active-high", NULL))
> > + reset_pol = 1;
> > +
> > + of_property_read_u32(node, "hub-reset-duration-us",
> > + &duration_us);
> > +
> > + gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > + reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > + ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > + if (ret) {
> > + dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > + ret);
> > + return ret;
> > + }
> > + } else if (pdata) {
> > + hub_data->clk = pdata->ext_clk;
>
> Passing clocks in platform_data is a no go. clocks must always be
> retrieved with clk_get.
Yes, you are right.
>
> > + duration_us = pdata->gpio_reset_duration_us;
> > + reset_pol = pdata->gpio_reset_polarity;
> > +
> > + if (gpio_is_valid(pdata->gpio_reset)) {
> > + ret = devm_gpio_request_one(
> > + dev, pdata->gpio_reset, reset_pol
> > + ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > + dev_name(dev));
> > + if (!ret)
> > + gpiod_reset = gpio_to_desc(pdata->gpio_reset);
>
> If the gpio is valid then being unable to get it is an error.
I can't catch you, if the gpio is invalid, its descriptor will be NULL,
then, the code will not do any gpio operation.
>
> Do you need this platform_data stuff at all?
>
If not, how can I cover the platform which does not use dts, but still
uses these kinds of HUBs?
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 9:26 ` Peter Chen
@ 2015-12-08 9:44 ` Sascha Hauer
2015-12-09 8:23 ` Peter Chen
0 siblings, 1 reply; 35+ messages in thread
From: Sascha Hauer @ 2015-12-08 9:44 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > > + if (!hub_data)
> > > + return -ENOMEM;
> > > +
> > > + if (dev->of_node) {
> > > + struct device_node *node = dev->of_node;
> > > +
> > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> >
> > Please drop such _clk suffixes. The context already makes it clear that
> > it's a clock.
> >
>
> Will change.
>
> > > + if (IS_ERR(hub_data->clk)) {
> > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > + PTR_ERR(hub_data->clk));
> > > + }
> > > +
> > > + /*
> > > + * Try to get the information for HUB reset, the
> > > + * default setting like below:
> > > + *
> > > + * - Reset state is low
> > > + * - The duration is 50us
> > > + */
> > > + if (of_find_property(node, "hub-reset-active-high", NULL))
> > > + reset_pol = 1;
> > > +
> > > + of_property_read_u32(node, "hub-reset-duration-us",
> > > + &duration_us);
> > > +
> > > + gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > > + reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > > + ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > > + ret);
> > > + return ret;
> > > + }
> > > + } else if (pdata) {
> > > + hub_data->clk = pdata->ext_clk;
> >
> > Passing clocks in platform_data is a no go. clocks must always be
> > retrieved with clk_get.
>
> Yes, you are right.
>
> >
> > > + duration_us = pdata->gpio_reset_duration_us;
> > > + reset_pol = pdata->gpio_reset_polarity;
> > > +
> > > + if (gpio_is_valid(pdata->gpio_reset)) {
> > > + ret = devm_gpio_request_one(
> > > + dev, pdata->gpio_reset, reset_pol
> > > + ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > + dev_name(dev));
> > > + if (!ret)
> > > + gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> >
> > If the gpio is valid then being unable to get it is an error.
>
> I can't catch you, if the gpio is invalid, its descriptor will be NULL,
> then, the code will not do any gpio operation.
When the platform_data provides a gpio (gpio_is_valid() is true) then
you must use the gpio. If then devm_gpio_request_one() fails you must
bail out.
>
> >
> > Do you need this platform_data stuff at all?
> >
>
> If not, how can I cover the platform which does not use dts, but still
> uses these kinds of HUBs?
You can't, but are there any non device tree platforms which want to use
this driver?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 9:44 ` Sascha Hauer
@ 2015-12-09 8:23 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2015-12-09 8:23 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 10:44:02AM +0100, Sascha Hauer wrote:
> On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> > On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > > + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > > > + if (!hub_data)
> > > > + return -ENOMEM;
> > > > +
> > > > + if (dev->of_node) {
> > > > + struct device_node *node = dev->of_node;
> > > > +
> > > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > >
> > > Please drop such _clk suffixes. The context already makes it clear that
> > > it's a clock.
> > >
> >
> > Will change.
> >
> > > > + if (IS_ERR(hub_data->clk)) {
> > > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > + PTR_ERR(hub_data->clk));
> > > > + }
> > > > +
> > > > + /*
> > > > + * Try to get the information for HUB reset, the
> > > > + * default setting like below:
> > > > + *
> > > > + * - Reset state is low
> > > > + * - The duration is 50us
> > > > + */
> > > > + if (of_find_property(node, "hub-reset-active-high", NULL))
> > > > + reset_pol = 1;
> > > > +
> > > > + of_property_read_u32(node, "hub-reset-duration-us",
> > > > + &duration_us);
> > > > +
> > > > + gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > > > + reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > > > + ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > > > + ret);
> > > > + return ret;
> > > > + }
> > > > + } else if (pdata) {
> > > > + hub_data->clk = pdata->ext_clk;
> > >
> > > Passing clocks in platform_data is a no go. clocks must always be
> > > retrieved with clk_get.
> >
> > Yes, you are right.
> >
> > >
> > > > + duration_us = pdata->gpio_reset_duration_us;
> > > > + reset_pol = pdata->gpio_reset_polarity;
> > > > +
> > > > + if (gpio_is_valid(pdata->gpio_reset)) {
> > > > + ret = devm_gpio_request_one(
> > > > + dev, pdata->gpio_reset, reset_pol
> > > > + ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > > + dev_name(dev));
> > > > + if (!ret)
> > > > + gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > >
> > > If the gpio is valid then being unable to get it is an error.
> >
> > I can't catch you, if the gpio is invalid, its descriptor will be NULL,
> > then, the code will not do any gpio operation.
>
> When the platform_data provides a gpio (gpio_is_valid() is true) then
> you must use the gpio. If then devm_gpio_request_one() fails you must
> bail out.
I see.
>
> >
> > >
> > > Do you need this platform_data stuff at all?
> > >
> >
> > If not, how can I cover the platform which does not use dts, but still
> > uses these kinds of HUBs?
>
> You can't, but are there any non device tree platforms which want to use
> this driver?
>
I thought there are i386 embedded devices, it may use the HUB, like this
driver tries to handle. I agree that we only handle dt in this driver
first until the non-dt user appears.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 1:37 ` [PATCH 1/3] usb: misc: generic_onboard_hub: " Peter Chen
` (2 preceding siblings ...)
2015-12-08 6:19 ` Sascha Hauer
@ 2015-12-08 9:48 ` Arnd Bergmann
2015-12-09 8:14 ` Peter Chen
2015-12-08 15:36 ` Mathieu Poirier
4 siblings, 1 reply; 35+ messages in thread
From: Arnd Bergmann @ 2015-12-08 9:48 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:
> +struct usb_hub_generic_data {
> + struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> + struct usb_hub_generic_data *hub_data;
> + int reset_pol = 0, duration_us = 50, ret = 0;
> + struct gpio_desc *gpiod_reset = NULL;
> +
> + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> + if (!hub_data)
> + return -ENOMEM;
> +
> + if (dev->of_node) {
Let's not worry about the !DT case until someone adds a board file
that needs it. Just remove the if() here along and the whole else
block.
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> + {.compatible = "generic-onboard-hub"},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> + .probe = usb_hub_generic_probe,
> + .remove = usb_hub_generic_remove,
> + .driver = {
> + .name = "usb_hub_generic_onboard",
> + .of_match_table = usb_hub_generic_dt_ids,
> + },
> +};
Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device
table.
> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -0,0 +1,11 @@
> +#ifndef __LINUX_USB_GENERIC_HUB_H
> +#define __LINUX_USB_GENERIC_HUB_H
> +
> +struct usb_hub_generic_platform_data {
> + int gpio_reset;
> + int gpio_reset_polarity;
> + int gpio_reset_duration_us;
> + struct clk *ext_clk;
> +};
Merge this structure into struct usb_hub_generic_data and remove the header.
ARnd
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 9:48 ` Arnd Bergmann
@ 2015-12-09 8:14 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2015-12-09 8:14 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 10:48:28AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:
>
> > +struct usb_hub_generic_data {
> > + struct clk *clk;
> > +};
> > +
> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> > + struct usb_hub_generic_data *hub_data;
> > + int reset_pol = 0, duration_us = 50, ret = 0;
> > + struct gpio_desc *gpiod_reset = NULL;
> > +
> > + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > + if (!hub_data)
> > + return -ENOMEM;
> > +
> > + if (dev->of_node) {
>
> Let's not worry about the !DT case until someone adds a board file
> that needs it. Just remove the if() here along and the whole else
> block.
>
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> > + {.compatible = "generic-onboard-hub"},
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> > +#endif
> > +
> > +static struct platform_driver usb_hub_generic_driver = {
> > + .probe = usb_hub_generic_probe,
> > + .remove = usb_hub_generic_remove,
> > + .driver = {
> > + .name = "usb_hub_generic_onboard",
> > + .of_match_table = usb_hub_generic_dt_ids,
> > + },
> > +};
>
> Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device
> table.
>
> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > +#define __LINUX_USB_GENERIC_HUB_H
> > +
> > +struct usb_hub_generic_platform_data {
> > + int gpio_reset;
> > + int gpio_reset_polarity;
> > + int gpio_reset_duration_us;
> > + struct clk *ext_clk;
> > +};
>
> Merge this structure into struct usb_hub_generic_data and remove the header.
>
> ARnd
Agree.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 1:37 ` [PATCH 1/3] usb: misc: generic_onboard_hub: " Peter Chen
` (3 preceding siblings ...)
2015-12-08 9:48 ` Arnd Bergmann
@ 2015-12-08 15:36 ` Mathieu Poirier
2015-12-09 8:50 ` Peter Chen
4 siblings, 1 reply; 35+ messages in thread
From: Mathieu Poirier @ 2015-12-08 15:36 UTC (permalink / raw)
To: linux-arm-kernel
On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> Current USB HUB driver lacks of platform interfaces to configure
> external signal on HUB chip, eg, the PHY input clock and gpio reset
> pin for HUB, these kinds of HUBs are usually soldered at the board,
> and they are not hot-plug USB devices.
>
> With this patch, the user can configure the HUB's pins at platform
> part (through dts or platform), and these configuration will be
> effective before the USB bus can be used, we use subsys_initcall
> for this driver.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
> MAINTAINERS | 7 ++
> drivers/usb/misc/Kconfig | 9 ++
> drivers/usb/misc/Makefile | 1 +
> drivers/usb/misc/generic_onboard_hub.c | 162 ++++++++++++++++++++++++++++++++
> include/linux/usb/generic_onboard_hub.h | 11 +++
> 5 files changed, 190 insertions(+)
> create mode 100644 drivers/usb/misc/generic_onboard_hub.c
> create mode 100644 include/linux/usb/generic_onboard_hub.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S: Maintained
> F: Documentation/usb/ohci.txt
> F: drivers/usb/host/ohci*
>
> +USB Generic Onboard HUB Driver
> +M: Peter Chen <Peter.Chen@freescale.com>
> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L: linux-usb at vger.kernel.org
> +S: Maintained
> +F: drivers/usb/misc/generic_onboard_hub.c
> +
> USB OTG FSM (Finite State Machine)
> M: Peter Chen <Peter.Chen@freescale.com>
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -268,3 +268,12 @@ config USB_CHAOSKEY
>
> To compile this driver as a module, choose M here: the
> module will be called chaoskey.
> +
> +config USB_ONBOARD_HUB
> + tristate "Generic USB Onboard HUB"
> + help
> + Say Y here if your board has an onboard HUB, and this hub needs
> + to control its PHY clock and reset pin through external signals.
> + If you are not sure, say N.
> +
> + To compile this driver as a module, choose M here.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o
>
> obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
> obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB) += generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@freescale.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>
> +
> +struct usb_hub_generic_data {
> + struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> + struct usb_hub_generic_data *hub_data;
> + int reset_pol = 0, duration_us = 50, ret = 0;
> + struct gpio_desc *gpiod_reset = NULL;
> +
> + hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> + if (!hub_data)
> + return -ENOMEM;
> +
> + if (dev->of_node) {
> + struct device_node *node = dev->of_node;
> +
> + hub_data->clk = devm_clk_get(dev, "external_clk");
> + if (IS_ERR(hub_data->clk)) {
> + dev_dbg(dev, "Can't get external clock: %ld\n",
> + PTR_ERR(hub_data->clk));
> + }
Is the intended behaviour to keep going here event when there is an
error? Can the "hub_data" really work without a clock?
> +
> + /*
> + * Try to get the information for HUB reset, the
> + * default setting like below:
> + *
> + * - Reset state is low
> + * - The duration is 50us
> + */
> + if (of_find_property(node, "hub-reset-active-high", NULL))
> + reset_pol = 1;
> +
> + of_property_read_u32(node, "hub-reset-duration-us",
> + &duration_us);
> +
> + gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> + reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> + ret = PTR_ERR_OR_ZERO(gpiod_reset);
> + if (ret) {
> + dev_err(dev, "Failed to get reset gpio, err = %d\n",
> + ret);
> + return ret;
> + }
> + } else if (pdata) {
> + hub_data->clk = pdata->ext_clk;
> + duration_us = pdata->gpio_reset_duration_us;
> + reset_pol = pdata->gpio_reset_polarity;
> +
> + if (gpio_is_valid(pdata->gpio_reset)) {
> + ret = devm_gpio_request_one(
> + dev, pdata->gpio_reset, reset_pol
> + ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> + dev_name(dev));
> + if (!ret)
> + gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> + }
> + }
> +
> + if (!IS_ERR(hub_data->clk)) {
> + ret = clk_prepare_enable(hub_data->clk);
> + if (ret) {
> + dev_err(dev,
> + "Can't enable external clock: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (gpiod_reset) {
> + /* Sanity check */
> + if (duration_us > 1000000)
> + duration_us = 50;
> + usleep_range(duration_us, duration_us + 100);
> + gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
What are these hard-coded values? Shouldn't this be taken from the
DT? If not then some comments explaining the values would be
appreciated.
> + }
> +
> + dev_set_drvdata(dev, hub_data);
> + return ret;
> +}
> +
> +static int usb_hub_generic_remove(struct platform_device *pdev)
> +{
> + struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> +
> + if (!IS_ERR(hub_data->clk))
> + clk_disable_unprepare(hub_data->clk);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> + {.compatible = "generic-onboard-hub"},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> + .probe = usb_hub_generic_probe,
> + .remove = usb_hub_generic_remove,
> + .driver = {
> + .name = "usb_hub_generic_onboard",
> + .of_match_table = usb_hub_generic_dt_ids,
> + },
> +};
> +
> +static int __init usb_hub_generic_init(void)
> +{
> + return platform_driver_register(&usb_hub_generic_driver);
> +}
> +subsys_initcall(usb_hub_generic_init);
> +
> +static void __exit usb_hub_generic_exit(void)
> +{
> + platform_driver_unregister(&usb_hub_generic_driver);
> +}
> +module_exit(usb_hub_generic_exit);
> +
> +MODULE_AUTHOR("Peter Chen <peter.chen@freescale.com>");
> +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -0,0 +1,11 @@
> +#ifndef __LINUX_USB_GENERIC_HUB_H
> +#define __LINUX_USB_GENERIC_HUB_H
> +
> +struct usb_hub_generic_platform_data {
> + int gpio_reset;
> + int gpio_reset_polarity;
> + int gpio_reset_duration_us;
> + struct clk *ext_clk;
> +};
Please document this structure in accordance with kernel documentation
standards.
> +
> +#endif /* __LINUX_USB_GENERIC_HUB_H */
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-08 15:36 ` Mathieu Poirier
@ 2015-12-09 8:50 ` Peter Chen
2015-12-09 8:57 ` Lucas Stach
0 siblings, 1 reply; 35+ messages in thread
From: Peter Chen @ 2015-12-09 8:50 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > +
> > + if (dev->of_node) {
> > + struct device_node *node = dev->of_node;
> > +
> > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > + if (IS_ERR(hub_data->clk)) {
> > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > + PTR_ERR(hub_data->clk));
> > + }
>
> Is the intended behaviour to keep going here event when there is an
> error? Can the "hub_data" really work without a clock?
Yes, some HUB may work with fixed 24M OSC at the board, but they need to
reset through external IO, so the clock is not need at this case, but
reset pin is mandatory.
> > + }
> > +
> > + if (gpiod_reset) {
> > + /* Sanity check */
> > + if (duration_us > 1000000)
> > + duration_us = 50;
> > + usleep_range(duration_us, duration_us + 100);
> > + gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>
> What are these hard-coded values? Shouldn't this be taken from the
> DT? If not then some comments explaining the values would be
> appreciated.
Yes, I did it wrongly, thanks.
> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > +#define __LINUX_USB_GENERIC_HUB_H
> > +
> > +struct usb_hub_generic_platform_data {
> > + int gpio_reset;
> > + int gpio_reset_polarity;
> > + int gpio_reset_duration_us;
> > + struct clk *ext_clk;
> > +};
>
> Please document this structure in accordance with kernel documentation
> standards.
>
Thanks, it should be.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-09 8:50 ` Peter Chen
@ 2015-12-09 8:57 ` Lucas Stach
2015-12-09 9:00 ` Peter Chen
2015-12-09 9:10 ` Arnd Bergmann
0 siblings, 2 replies; 35+ messages in thread
From: Lucas Stach @ 2015-12-09 8:57 UTC (permalink / raw)
To: linux-arm-kernel
Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > +
> > > + if (dev->of_node) {
> > > + struct device_node *node = dev->of_node;
> > > +
> > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > > + if (IS_ERR(hub_data->clk)) {
> > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > + PTR_ERR(hub_data->clk));
> > > + }
> >
> > Is the intended behaviour to keep going here event when there is an
> > error? Can the "hub_data" really work without a clock?
>
> Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> reset through external IO, so the clock is not need at this case, but
> reset pin is mandatory.
>
If the hub always requires a clock it must not be optional. If you have
a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
it as an input to the hub.
> > > + }
> > > +
> > > + if (gpiod_reset) {
> > > + /* Sanity check */
> > > + if (duration_us > 1000000)
> > > + duration_us = 50;
> > > + usleep_range(duration_us, duration_us + 100);
> > > + gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> >
> > What are these hard-coded values? Shouldn't this be taken from the
> > DT? If not then some comments explaining the values would be
> > appreciated.
>
> Yes, I did it wrongly, thanks.
>
> > > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > > new file mode 100644
> > > index 0000000..1b70ccc
> > > --- /dev/null
> > > +++ b/include/linux/usb/generic_onboard_hub.h
> > > @@ -0,0 +1,11 @@
> > > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > > +#define __LINUX_USB_GENERIC_HUB_H
> > > +
> > > +struct usb_hub_generic_platform_data {
> > > + int gpio_reset;
> > > + int gpio_reset_polarity;
> > > + int gpio_reset_duration_us;
> > > + struct clk *ext_clk;
> > > +};
> >
> > Please document this structure in accordance with kernel documentation
> > standards.
> >
>
> Thanks, it should be.
>
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-09 8:57 ` Lucas Stach
@ 2015-12-09 9:00 ` Peter Chen
2015-12-09 9:13 ` Lucas Stach
2015-12-09 9:10 ` Arnd Bergmann
1 sibling, 1 reply; 35+ messages in thread
From: Peter Chen @ 2015-12-09 9:00 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > +
> > > > + if (dev->of_node) {
> > > > + struct device_node *node = dev->of_node;
> > > > +
> > > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > + if (IS_ERR(hub_data->clk)) {
> > > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > + PTR_ERR(hub_data->clk));
> > > > + }
> > >
> > > Is the intended behaviour to keep going here event when there is an
> > > error? Can the "hub_data" really work without a clock?
> >
> > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > reset through external IO, so the clock is not need at this case, but
> > reset pin is mandatory.
> >
> If the hub always requires a clock it must not be optional. If you have
> a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> it as an input to the hub.
>
This fixed 24MHz clock may from a fixed crystal on board, it is always
on, no software need to control it, imx51-bbg board is an example.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-09 9:00 ` Peter Chen
@ 2015-12-09 9:13 ` Lucas Stach
2015-12-09 9:29 ` Peter Chen
0 siblings, 1 reply; 35+ messages in thread
From: Lucas Stach @ 2015-12-09 9:13 UTC (permalink / raw)
To: linux-arm-kernel
Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:
> On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > > +
> > > > > + if (dev->of_node) {
> > > > > + struct device_node *node = dev->of_node;
> > > > > +
> > > > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > + if (IS_ERR(hub_data->clk)) {
> > > > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > + PTR_ERR(hub_data->clk));
> > > > > + }
> > > >
> > > > Is the intended behaviour to keep going here event when there is an
> > > > error? Can the "hub_data" really work without a clock?
> > >
> > > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > > reset through external IO, so the clock is not need at this case, but
> > > reset pin is mandatory.
> > >
> > If the hub always requires a clock it must not be optional. If you have
> > a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> > it as an input to the hub.
> >
>
> This fixed 24MHz clock may from a fixed crystal on board, it is always
> on, no software need to control it, imx51-bbg board is an example.
>
And that's wh it is a fixed-clock. Please look it up in the DT binding
documentation.
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-09 9:13 ` Lucas Stach
@ 2015-12-09 9:29 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2015-12-09 9:29 UTC (permalink / raw)
To: linux-arm-kernel
Best regards,
Peter Chen
> -----Original Message-----
> From: Lucas Stach [mailto:l.stach at pengutronix.de]
> Sent: Wednesday, December 09, 2015 5:13 PM
> To: Chen Peter-B29397 <Peter.Chen@freescale.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>; Mark Rutland
> <mark.rutland@arm.com>; devicetree at vger.kernel.org; festevam at gmail.com;
> Philipp Zabel <p.zabel@pengutronix.de>; Pawe? Moll <pawel.moll@arm.com>;
> Greg KH <gregkh@linuxfoundation.org>; linux-usb at vger.kernel.org;
> patryk at kowalczyk.ws; Rob Herring <robh+dt@kernel.org>;
> stern at rowland.harvard.edu; kernel at pengutronix.de; Shawn Guo
> <shawnguo@kernel.org>; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic
> onboard USB HUB driver
>
> Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:
> > On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> > > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com>
> wrote:
> > > > > > +
> > > > > > + if (dev->of_node) {
> > > > > > + struct device_node *node = dev->of_node;
> > > > > > +
> > > > > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > > + if (IS_ERR(hub_data->clk)) {
> > > > > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > > + PTR_ERR(hub_data->clk));
> > > > > > + }
> > > > >
> > > > > Is the intended behaviour to keep going here event when there is
> > > > > an error? Can the "hub_data" really work without a clock?
> > > >
> > > > Yes, some HUB may work with fixed 24M OSC at the board, but they
> > > > need to reset through external IO, so the clock is not need at
> > > > this case, but reset pin is mandatory.
> > > >
> > > If the hub always requires a clock it must not be optional. If you
> > > have a fixed 24MHz clock on board add this to the DT as a
> > > fixed-clock and use it as an input to the hub.
> > >
> >
> > This fixed 24MHz clock may from a fixed crystal on board, it is always
> > on, no software need to control it, imx51-bbg board is an example.
> >
> And that's wh it is a fixed-clock. Please look it up in the DT binding
> documentation.
Yes, I see similar things are described at [1], but why we need this
additional entry at dts for this case, it is always on, no software
is needed.
[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
Peter
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-09 8:57 ` Lucas Stach
2015-12-09 9:00 ` Peter Chen
@ 2015-12-09 9:10 ` Arnd Bergmann
2015-12-09 9:08 ` Peter Chen
1 sibling, 1 reply; 35+ messages in thread
From: Arnd Bergmann @ 2015-12-09 9:10 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > +
> > > > + if (dev->of_node) {
> > > > + struct device_node *node = dev->of_node;
> > > > +
> > > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > + if (IS_ERR(hub_data->clk)) {
> > > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > + PTR_ERR(hub_data->clk));
> > > > + }
> > >
> > > Is the intended behaviour to keep going here event when there is an
> > > error? Can the "hub_data" really work without a clock?
> >
> > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > reset through external IO, so the clock is not need at this case, but
> > reset pin is mandatory.
> >
> If the hub always requires a clock it must not be optional. If you have
> a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> it as an input to the hub.
I think it's fine to make the clock optional in the sense that you only
need to list non-fixed clocks that have to be enabled and or controlled.
Which reminds me, should the driver call clk_set_rate()? It currently
doesn't, but other machines might need that.
Arnd
^ permalink raw reply [flat|nested] 35+ messages in thread* [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
2015-12-09 9:10 ` Arnd Bergmann
@ 2015-12-09 9:08 ` Peter Chen
0 siblings, 0 replies; 35+ messages in thread
From: Peter Chen @ 2015-12-09 9:08 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Dec 09, 2015 at 10:10:51AM +0100, Arnd Bergmann wrote:
> On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > > +
> > > > > + if (dev->of_node) {
> > > > > + struct device_node *node = dev->of_node;
> > > > > +
> > > > > + hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > + if (IS_ERR(hub_data->clk)) {
> > > > > + dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > + PTR_ERR(hub_data->clk));
> > > > > + }
> > > >
> > > > Is the intended behaviour to keep going here event when there is an
> > > > error? Can the "hub_data" really work without a clock?
> > >
> > > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > > reset through external IO, so the clock is not need at this case, but
> > > reset pin is mandatory.
> > >
> > If the hub always requires a clock it must not be optional. If you have
> > a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> > it as an input to the hub.
>
> I think it's fine to make the clock optional in the sense that you only
> need to list non-fixed clocks that have to be enabled and or controlled.
>
> Which reminds me, should the driver call clk_set_rate()? It currently
> doesn't, but other machines might need that.
>
Yes, you are right, I will add it at v2.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 35+ messages in thread