Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
Hi, On 2019/3/4 14:55, Andy Shevchenko wrote: > On Mon, Mar 4, 2019 at 4:35 AM Chen Yu wrote: >> On 2019/3/3 0:01, Andy Shevchenko wrote: >>> On Sat, Mar 2, 2019 at 11:05 AM Yu Chen wrote: > +config HISI_HIKEY_USB + tristate "USB functionality of HiSilicon Hikey Platform" + depends on OF && GPIOLIB + help + If you say yes here you get support for usb functionality of HiSilicon Hikey Platform. >>> +#include >>> >>> It's hard to see why this have >>> depends on OF followed by above header inclusion. >>> >> This driver depends on devicetree, so I add "depends on OF". >> But is seems that "#include " can be removed after "of_" API >> have been removed. Thanks for your reminder! > > So, it means that technically there is no such dependency, rather > administratively. > OK. I will remove the dependent next version. + hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus", + GPIOD_OUT_LOW); >>> + if (!hisi_hikey_usb->typec_vbus) + return -ENOENT; >>> >>> Hmm... Is it possible to get NULL pointer from gpiod_get() at all? >>> + if (!hisi_hikey_usb->otg_switch) + return -ENOENT; >>> >>> Ditto. >>> >> I check the comments of devm_gpio_get API, it will not return NULL pointer. >> But is it more safe to keep the NULL checking? What is your advice? > > Why do we need dead code? > OK.I will remove it. Thanks Yu Chen
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
hi, On Mon, 2019-03-04 at 08:50 +0200, Andy Shevchenko wrote: > On Mon, Mar 4, 2019 at 3:47 AM Chunfeng Yun wrote: > > On Sat, 2019-03-02 at 17:05 +0800, Yu Chen wrote: > > > > + hisi_hikey_usb->role_sw = usb_role_switch_get(dev); > > > + if (!hisi_hikey_usb->role_sw) > > > + return -EPROBE_DEFER; > > Here return EPROBE_DEFFER means the related device_connection is > > registered after this probe is called, right? > > if not, use IS_ERR_OR_NULL then return PTR_ERR is enough > > How enough? If return value is NULL it would be transformered to 0, > which is success return code from the ->probe() which means we will > have ->probed() and not functional device. > You are right:) > Am I missing something? >
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
On Mon, Mar 4, 2019 at 4:35 AM Chen Yu wrote: > On 2019/3/3 0:01, Andy Shevchenko wrote: > > On Sat, Mar 2, 2019 at 11:05 AM Yu Chen wrote: > >> +config HISI_HIKEY_USB > >> + tristate "USB functionality of HiSilicon Hikey Platform" > >> + depends on OF && GPIOLIB > >> + help > >> + If you say yes here you get support for usb functionality of > >> HiSilicon Hikey Platform. > > > >> +#include > > > > It's hard to see why this have > > depends on OF followed by above header inclusion. > > > This driver depends on devicetree, so I add "depends on OF". > But is seems that "#include " can be removed after "of_" API > have been removed. Thanks for your reminder! So, it means that technically there is no such dependency, rather administratively. > >> + hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus", > >> + GPIOD_OUT_LOW); > > > >> + if (!hisi_hikey_usb->typec_vbus) > >> + return -ENOENT; > > > > Hmm... Is it possible to get NULL pointer from gpiod_get() at all? > > > >> + if (!hisi_hikey_usb->otg_switch) > >> + return -ENOENT; > > > > Ditto. > > > I check the comments of devm_gpio_get API, it will not return NULL pointer. > But is it more safe to keep the NULL checking? What is your advice? Why do we need dead code? -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
On Mon, Mar 4, 2019 at 3:47 AM Chunfeng Yun wrote: > On Sat, 2019-03-02 at 17:05 +0800, Yu Chen wrote: > > + hisi_hikey_usb->role_sw = usb_role_switch_get(dev); > > + if (!hisi_hikey_usb->role_sw) > > + return -EPROBE_DEFER; > Here return EPROBE_DEFFER means the related device_connection is > registered after this probe is called, right? > if not, use IS_ERR_OR_NULL then return PTR_ERR is enough How enough? If return value is NULL it would be transformered to 0, which is success return code from the ->probe() which means we will have ->probed() and not functional device. Am I missing something? -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
Hi Andy,
On 2019/3/3 0:01, Andy Shevchenko wrote:
> On Sat, Mar 2, 2019 at 11:05 AM Yu Chen wrote:
>>
>> This driver handles usb hub power on and typeC port event of HiKey960 board:
>> 1)DP switching between usb hub and typeC port base on typeC port
>> state
>> 2)Control power of usb hub on Hikey960
>> 3)Control vbus of typeC port
>
>> +config HISI_HIKEY_USB
>> + tristate "USB functionality of HiSilicon Hikey Platform"
>> + depends on OF && GPIOLIB
>> + help
>> + If you say yes here you get support for usb functionality of
>> HiSilicon Hikey Platform.
>
>> +#include
>
> It's hard to see why this have
> depends on OF followed by above header inclusion.
>
This driver depends on devicetree, so I add "depends on OF".
But is seems that "#include " can be removed after "of_" API
have been removed. Thanks for your reminder!
>> + hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus",
>> + GPIOD_OUT_LOW);
>
>> + if (!hisi_hikey_usb->typec_vbus)
>> + return -ENOENT;
>
> Hmm... Is it possible to get NULL pointer from gpiod_get() at all?
>
>> + if (!hisi_hikey_usb->otg_switch)
>> + return -ENOENT;
>
> Ditto.
>
I check the comments of devm_gpio_get API, it will not return NULL pointer.
But is it more safe to keep the NULL checking? What is your advice?
>> + /* hub-vdd33-en is optional */
>> + hisi_hikey_usb->hub_vbus = devm_gpiod_get(dev, "hub-vdd33-en",
>> + GPIOD_OUT_HIGH);
>
> devm_gpio_get_optional() if it's indeed optional.
>
OK.Thanks!
>> + hisi_hikey_usb->role_sw = usb_role_switch_get(dev);
>> + if (!hisi_hikey_usb->role_sw)
>> + return -EPROBE_DEFER;
>
>> + else if (IS_ERR(hisi_hikey_usb->role_sw))
>
> Redundant 'else'
>
OK.
>> + return PTR_ERR(hisi_hikey_usb->role_sw);
>
>> +static const struct of_device_id id_table_hisi_hikey_usb[] = {
>> + {.compatible = "hisilicon,gpio_hubv1"},
>> + {.compatible = "hisilicon,hikey960_usb"},
>> + {}
>> +};
>
> MODULE_DEVICE_TABLE()?
>
OK.
Thanks
Yu Chen
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
Hi Chunfeng Yun,
On 2019/3/4 9:47, Chunfeng Yun wrote:
>> +
>> +hisi_hikey_usb->role_sw = usb_role_switch_get(dev);
>> +if (!hisi_hikey_usb->role_sw)
>> +return -EPROBE_DEFER;
> Here return EPROBE_DEFFER means the related device_connection is
> registered after this probe is called, right?
> if not, use IS_ERR_OR_NULL then return PTR_ERR is enough
Yes, the driver which register the usb_role_switch may finish probe after
this driver is probed for the first time.
>> +else if (IS_ERR(hisi_hikey_usb->role_sw))
>> +return PTR_ERR(hisi_hikey_usb->role_sw);
>> +
>> +ret = usb_role_switch_register_notifier(hisi_hikey_usb->role_sw,
>> +_hikey_usb->nb);
>> +if (ret) {
>> +usb_role_switch_put(hisi_hikey_usb->role_sw);
>> +return ret;
>> +}
>> +
>> +platform_set_drvdata(pdev, hisi_hikey_usb);
>> +
>> +return 0;
>> +}
>> +
>
>
> .
>
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
hi,
On Sat, 2019-03-02 at 17:05 +0800, Yu Chen wrote:
> This driver handles usb hub power on and typeC port event of HiKey960 board:
> 1)DP switching between usb hub and typeC port base on typeC port
> state
> 2)Control power of usb hub on Hikey960
> 3)Control vbus of typeC port
>
> Cc: Chunfeng Yun
> Cc: Andy Shevchenko
> Cc: Arnd Bergmann
> Cc: Greg Kroah-Hartman
> Cc: John Stultz
> Cc: Binghui Wang
> Cc: Heikki Krogerus
> Signed-off-by: Yu Chen
> ---
> v1:
> * Using gpiod API with the gpios.
> * Removing registering usb role switch.
> * Registering usb role switch notifier.
> v2:
> * Fix license declaration.
> * Add configuration of gpio direction.
> * Remove some log print.
> v3:
> * Remove property of "typec_vbus_enable_val".
> * Remove gpiod_direction_output and set initial value of gpio by
> devm_gpiod_get.
> ---
> ---
> drivers/misc/Kconfig | 6 ++
> drivers/misc/Makefile | 1 +
> drivers/misc/hisi_hikey_usb.c | 167
> ++
> 3 files changed, 174 insertions(+)
> create mode 100644 drivers/misc/hisi_hikey_usb.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f417b06e11c5..8d8b717759e2 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -521,6 +521,12 @@ config PVPANIC
> a paravirtualized device provided by QEMU; it lets a virtual machine
> (guest) communicate panic events to the host.
>
> +config HISI_HIKEY_USB
> + tristate "USB functionality of HiSilicon Hikey Platform"
> + depends on OF && GPIOLIB
> + help
> + If you say yes here you get support for usb functionality of
> HiSilicon Hikey Platform.
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e39ccbbc1b3a..dc8892b13a1a 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
> obj-$(CONFIG_OCXL) += ocxl/
> obj-y+= cardreader/
> obj-$(CONFIG_PVPANIC)+= pvpanic.o
> +obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
> diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> new file mode 100644
> index ..85d6fee468c0
> --- /dev/null
> +++ b/drivers/misc/hisi_hikey_usb.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for usb functionality of Hikey series boards
> + * based on Hisilicon Kirin Soc.
> + *
> + * Copyright (C) 2017-2018 Hilisicon Electronics Co., Ltd.
> + * http://www.huawei.com
> + *
> + * Authors: Yu Chen
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DEVICE_DRIVER_NAME "hisi_hikey_usb"
> +
> +#define HUB_VBUS_POWER_ON 1
> +#define HUB_VBUS_POWER_OFF 0
> +#define USB_SWITCH_TO_HUB 1
> +#define USB_SWITCH_TO_TYPEC 0
> +#define TYPEC_VBUS_POWER_ON 1
> +#define TYPEC_VBUS_POWER_OFF 0
> +
> +struct hisi_hikey_usb {
> + struct gpio_desc *otg_switch;
> + struct gpio_desc *typec_vbus;
> + struct gpio_desc *hub_vbus;
> +
> + struct usb_role_switch *role_sw;
> + struct notifier_block nb;
> +};
> +
> +static void hub_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb, int value)
> +{
> + gpiod_set_value_cansleep(hisi_hikey_usb->hub_vbus, value);
> +}
> +
> +static void usb_switch_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
> + int switch_to)
> +{
> + gpiod_set_value_cansleep(hisi_hikey_usb->otg_switch, switch_to);
> +}
> +
> +static void usb_typec_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
> + int value)
> +{
> + gpiod_set_value_cansleep(hisi_hikey_usb->typec_vbus, value);
> +}
> +
> +static int hisi_hikey_role_switch(struct notifier_block *nb,
> + unsigned long state, void *data)
> +{
> + struct hisi_hikey_usb *hisi_hikey_usb;
> +
> + hisi_hikey_usb = container_of(nb, struct hisi_hikey_usb, nb);
> +
> + switch (state) {
> + case USB_ROLE_NONE:
> + usb_typec_power_ctrl(hisi_hikey_usb, TYPEC_VBUS_POWER_OFF);
> + usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_HUB);
> + hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_ON);
> + break;
> + case USB_ROLE_HOST:
> + usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
> + usb_typec_power_ctrl(hisi_hikey_usb, TYPEC_VBUS_POWER_ON);
> + break;
> + case USB_ROLE_DEVICE:
> + hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF);
> + usb_typec_power_ctrl(hisi_hikey_usb, TYPEC_VBUS_POWER_OFF);
> + usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int hisi_hikey_usb_probe(struct platform_device
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
On 2019/3/4 4:17, John Stultz wrote:
> On Sat, Mar 2, 2019 at 4:05 AM Yu Chen wrote:
>> +static int hisi_hikey_usb_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = >dev;
>> + struct device_node *root = dev->of_node;
>
> Minor nit: root is unused and generates build warnings.
>
OK. Thanks!
> thanks
> -john
>
> .
>
Thanks
Yu Chen
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
On Sat, Mar 2, 2019 at 4:05 AM Yu Chen wrote:
> +static int hisi_hikey_usb_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct device_node *root = dev->of_node;
Minor nit: root is unused and generates build warnings.
thanks
-john
Re: [PATCH v3 10/12] hikey960: Support usb functionality of Hikey960
On Sat, Mar 2, 2019 at 11:05 AM Yu Chen wrote:
>
> This driver handles usb hub power on and typeC port event of HiKey960 board:
> 1)DP switching between usb hub and typeC port base on typeC port
> state
> 2)Control power of usb hub on Hikey960
> 3)Control vbus of typeC port
> +config HISI_HIKEY_USB
> + tristate "USB functionality of HiSilicon Hikey Platform"
> + depends on OF && GPIOLIB
> + help
> + If you say yes here you get support for usb functionality of
> HiSilicon Hikey Platform.
> +#include
It's hard to see why this have
depends on OF followed by above header inclusion.
> + hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus",
> + GPIOD_OUT_LOW);
> + if (!hisi_hikey_usb->typec_vbus)
> + return -ENOENT;
Hmm... Is it possible to get NULL pointer from gpiod_get() at all?
> + if (!hisi_hikey_usb->otg_switch)
> + return -ENOENT;
Ditto.
> + /* hub-vdd33-en is optional */
> + hisi_hikey_usb->hub_vbus = devm_gpiod_get(dev, "hub-vdd33-en",
> + GPIOD_OUT_HIGH);
devm_gpio_get_optional() if it's indeed optional.
> + hisi_hikey_usb->role_sw = usb_role_switch_get(dev);
> + if (!hisi_hikey_usb->role_sw)
> + return -EPROBE_DEFER;
> + else if (IS_ERR(hisi_hikey_usb->role_sw))
Redundant 'else'
> + return PTR_ERR(hisi_hikey_usb->role_sw);
> +static const struct of_device_id id_table_hisi_hikey_usb[] = {
> + {.compatible = "hisilicon,gpio_hubv1"},
> + {.compatible = "hisilicon,hikey960_usb"},
> + {}
> +};
MODULE_DEVICE_TABLE()?
--
With Best Regards,
Andy Shevchenko
