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&DM 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, >> +&hisi_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&DM 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 = &pdev->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 = &pdev->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&DM 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