Re: [PATCH 2/3] usb: typec: qcom-pmic-typec: Only select DRM_AUX_HPD_BRIDGE with OF
On Tue, Dec 05, 2023 at 01:13:35PM -0700, Nathan Chancellor wrote:
> CONFIG_DRM_AUX_HPD_BRIDGE depends on CONFIG_OF but that dependency is
> not included when CONFIG_TYPEC_QCOM_PMIC selects it, resulting in a
> Kconfig warning when CONFIG_OF is disabled:
>
> WARNING: unmet direct dependencies detected for DRM_AUX_HPD_BRIDGE
> Depends on [n]: HAS_IOMEM [=y] && DRM_BRIDGE [=y] && OF [=n]
> Selected by [m]:
> - TYPEC_QCOM_PMIC [=m] && USB_SUPPORT [=y] && TYPEC [=m] && TYPEC_TCPM
> [=m] && (ARCH_QCOM || COMPILE_TEST [=y]) && (DRM [=m] || DRM [=m]=n) &&
> DRM_BRIDGE [=y]
>
> Only select CONFIG_DRM_AUX_HPD_BRIDGE with both CONFIG_DRM_BRIDGE and
> CONFIG_OF to clear up the warning.
>
> Fixes: 7d9f1b72b296 ("usb: typec: qcom-pmic-typec: switch to
> DRM_AUX_HPD_BRIDGE")
> Signed-off-by: Nathan Chancellor
Shouldn't DRM_BRIDGE depend on/select OF instead?
Reviewed-by: Heikki Krogerus
> ---
> drivers/usb/typec/tcpm/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> index 64d5421c69e6..8cdd84ca5d6f 100644
> --- a/drivers/usb/typec/tcpm/Kconfig
> +++ b/drivers/usb/typec/tcpm/Kconfig
> @@ -80,7 +80,7 @@ config TYPEC_QCOM_PMIC
> tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver"
> depends on ARCH_QCOM || COMPILE_TEST
> depends on DRM || DRM=n
> - select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE
> + select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE && OF
> help
> A Type-C port and Power Delivery driver which aggregates two
> discrete pieces of silicon in the PM8150b PMIC block: the
>
> --
> 2.43.0
--
heikki
Re: [PATCH 1/3] usb: typec: nb7vpq904m: Only select DRM_AUX_BRIDGE with OF
On Tue, Dec 05, 2023 at 01:13:34PM -0700, Nathan Chancellor wrote:
> CONFIG_DRM_AUX_BRIDGE depends on CONFIG_OF but that dependency is not
> included when CONFIG_TYPEC_MUX_NB7VPQ904M selects it, resulting in a
> Kconfig warning when CONFIG_OF is disabled:
>
> WARNING: unmet direct dependencies detected for DRM_AUX_BRIDGE
> Depends on [n]: HAS_IOMEM [=y] && DRM_BRIDGE [=y] && OF [=n]
> Selected by [y]:
> - TYPEC_MUX_NB7VPQ904M [=y] && USB_SUPPORT [=y] && TYPEC [=y] && I2C [=y]
> && (DRM [=y] || DRM [=y]=n) && DRM_BRIDGE [=y]
>
> Only select CONFIG_DRM_AUX_BRIDGE with both CONFIG_DRM_BRIDGE and
> CONFIG_OF to clear up the warning.
>
> Fixes: c5d296bad640 ("usb: typec: nb7vpq904m: switch to DRM_AUX_BRIDGE")
> Signed-off-by: Nathan Chancellor
Reviewed-by: Heikki Krogerus
> ---
> drivers/usb/typec/mux/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> index 5120942f309d..38416fb0cc3c 100644
> --- a/drivers/usb/typec/mux/Kconfig
> +++ b/drivers/usb/typec/mux/Kconfig
> @@ -40,7 +40,7 @@ config TYPEC_MUX_NB7VPQ904M
> tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
> depends on I2C
> depends on DRM || DRM=n
> - select DRM_AUX_BRIDGE if DRM_BRIDGE
> + select DRM_AUX_BRIDGE if DRM_BRIDGE && OF
> select REGMAP_I2C
> help
> Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
>
> --
> 2.43.0
--
heikki
Re: [PATCH RESEND 6/6] usb: typec: qcom-pmic-typec: switch to DRM_AUX_HPD_BRIDGE
Hi Dmitry,
On Sun, Dec 03, 2023 at 02:43:33PM +0300, Dmitry Baryshkov wrote:
> Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the
> same functionality for the DRM bridge chain termination.
>
> Acked-by: Bryan O'Donoghue
> Signed-off-by: Dmitry Baryshkov
I'm sorry, I've completely missed this second typec patch.
Reviewed-by: Heikki Krogerus
> ---
> drivers/usb/typec/tcpm/Kconfig| 1 +
> drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 41 +++
> 2 files changed, 7 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> index 0b2993fef564..64d5421c69e6 100644
> --- a/drivers/usb/typec/tcpm/Kconfig
> +++ b/drivers/usb/typec/tcpm/Kconfig
> @@ -80,6 +80,7 @@ config TYPEC_QCOM_PMIC
> tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver"
> depends on ARCH_QCOM || COMPILE_TEST
> depends on DRM || DRM=n
> + select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE
> help
> A Type-C port and Power Delivery driver which aggregates two
> discrete pieces of silicon in the PM8150b PMIC block: the
> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> index 581199d37b49..1a2b4bddaa97 100644
> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
> @@ -18,7 +18,7 @@
> #include
> #include
>
> -#include
> +#include
>
> #include "qcom_pmic_typec_pdphy.h"
> #include "qcom_pmic_typec_port.h"
> @@ -36,7 +36,6 @@ struct pmic_typec {
> struct pmic_typec_port *pmic_typec_port;
> boolvbus_enabled;
> struct mutexlock; /* VBUS state serialization */
> - struct drm_bridge bridge;
> };
>
> #define tcpc_to_tcpm(_tcpc_) container_of(_tcpc_, struct pmic_typec, tcpc)
> @@ -150,35 +149,6 @@ static int qcom_pmic_typec_init(struct tcpc_dev *tcpc)
> return 0;
> }
>
> -#if IS_ENABLED(CONFIG_DRM)
> -static int qcom_pmic_typec_attach(struct drm_bridge *bridge,
> - enum drm_bridge_attach_flags flags)
> -{
> - return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
> -}
> -
> -static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
> - .attach = qcom_pmic_typec_attach,
> -};
> -
> -static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
> -{
> - tcpm->bridge.funcs = _pmic_typec_bridge_funcs;
> -#ifdef CONFIG_OF
> - tcpm->bridge.of_node = of_get_child_by_name(tcpm->dev->of_node,
> "connector");
> -#endif
> - tcpm->bridge.ops = DRM_BRIDGE_OP_HPD;
> - tcpm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> -
> - return devm_drm_bridge_add(tcpm->dev, >bridge);
> -}
> -#else
> -static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
> -{
> - return 0;
> -}
> -#endif
> -
> static int qcom_pmic_typec_probe(struct platform_device *pdev)
> {
> struct pmic_typec *tcpm;
> @@ -186,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device
> *pdev)
> struct device_node *np = dev->of_node;
> const struct pmic_typec_resources *res;
> struct regmap *regmap;
> + struct device *bridge_dev;
> u32 base[2];
> int ret;
>
> @@ -241,14 +212,14 @@ static int qcom_pmic_typec_probe(struct platform_device
> *pdev)
> mutex_init(>lock);
> platform_set_drvdata(pdev, tcpm);
>
> - ret = qcom_pmic_typec_init_drm(tcpm);
> - if (ret)
> - return ret;
> -
> tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
> if (!tcpm->tcpc.fwnode)
> return -EINVAL;
>
> + bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev,
> to_of_node(tcpm->tcpc.fwnode));
> + if (IS_ERR(bridge_dev))
> + return PTR_ERR(bridge_dev);
> +
> tcpm->tcpm_port = tcpm_register_port(tcpm->dev, >tcpc);
> if (IS_ERR(tcpm->tcpm_port)) {
> ret = PTR_ERR(tcpm->tcpm_port);
> --
> 2.39.2
--
heikki
Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path
On Mon, Oct 23, 2023 at 09:24:33PM +0300, Dmitry Baryshkov wrote: > On 15 September 2023 15:14:35 EEST, Heikki Krogerus > wrote: > >Hi Dmitry, > > > >On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote: > >> In order to notify the userspace about the DRM connector's USB-C port, > >> export the corresponding port's name as the bridge's path field. > >> > >> Signed-off-by: Dmitry Baryshkov > >> --- > >> drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++ > >> drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++- > >> drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 -- > >> 3 files changed, 14 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > >> b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > >> index b9d4856101c7..452dc6437861 100644 > >> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > >> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > >> @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct > >> platform_device *pdev) > >>struct device_node *np = dev->of_node; > >>const struct pmic_typec_resources *res; > >>struct regmap *regmap; > >> + char *tcpm_name; > >>u32 base[2]; > >>int ret; > >> > >> @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct > >> platform_device *pdev) > >>mutex_init(>lock); > >>platform_set_drvdata(pdev, tcpm); > >> > >> - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev); > >> - if (IS_ERR(tcpm->pmic_typec_drm)) > >> - return PTR_ERR(tcpm->pmic_typec_drm); > >> - > >>tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector"); > >>if (!tcpm->tcpc.fwnode) > >>return -EINVAL; > >> @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct > >> platform_device *pdev) > >>goto fwnode_remove; > >>} > >> > >> + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port); > >> + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name); > > > >So I got some questions and concerns off-list. This was one of the > >concerns. That tcpm_name is now the actual port device name, so I'm > >afraid this is not acceptable. > > > >You can't use device name as a reference, ever. There is no way to > >guarantee that a device with a specific name is what you meant it to > >be by the time it's accessed. > > Hmm, could you please be more specific, why? I mean, class devices are not > that easy to be renamed in sysfs, are they? Or are you concerned about the > device being destroyed behind userspace's back? At least for MSM this will be > a huge problem already, with the bridge driver suddenly being removed. The race exists even in your case, but please do not look at this as a solution for only your platform. This is about showing the user space a link between two device instances (struct device), and the way you do that is by creating a symlink. That way the kernel can take care of reference counting and guarantee that the link always points to the correct device. That way the link will also be always visible in user space without requirement for any specific ABI like it should. thanks, -- heikki
Re: [RFC PATCH v1 12/12] usb: typec: qcom: define the bridge's path
Hi Dmitry, On Mon, Sep 04, 2023 at 12:41:50AM +0300, Dmitry Baryshkov wrote: > In order to notify the userspace about the DRM connector's USB-C port, > export the corresponding port's name as the bridge's path field. > > Signed-off-by: Dmitry Baryshkov > --- > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 11 +++ > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.c | 4 +++- > drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_drm.h | 6 -- > 3 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > index b9d4856101c7..452dc6437861 100644 > --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > @@ -156,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device > *pdev) > struct device_node *np = dev->of_node; > const struct pmic_typec_resources *res; > struct regmap *regmap; > + char *tcpm_name; > u32 base[2]; > int ret; > > @@ -211,10 +212,6 @@ static int qcom_pmic_typec_probe(struct platform_device > *pdev) > mutex_init(>lock); > platform_set_drvdata(pdev, tcpm); > > - tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev); > - if (IS_ERR(tcpm->pmic_typec_drm)) > - return PTR_ERR(tcpm->pmic_typec_drm); > - > tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector"); > if (!tcpm->tcpc.fwnode) > return -EINVAL; > @@ -225,6 +222,12 @@ static int qcom_pmic_typec_probe(struct platform_device > *pdev) > goto fwnode_remove; > } > > + tcpm_name = tcpm_port_get_name(tcpm->tcpm_port); > + tcpm->pmic_typec_drm = qcom_pmic_typec_init_drm(dev, tcpm_name); So I got some questions and concerns off-list. This was one of the concerns. That tcpm_name is now the actual port device name, so I'm afraid this is not acceptable. You can't use device name as a reference, ever. There is no way to guarantee that a device with a specific name is what you meant it to be by the time it's accessed. If you need to deal with a device, then you have to get an actual reference to it (class_find_device_by_fwnode() should work in this case). Ideally you would get the reference in the place where you actually use it (so drm_connector.c or more likely drm_sysfs.c) but that would mean a dependency on typec in there, if the component framework or something like that (device links?) is not an option. You could of course try to confine the dependency somehow. drm_class does not have implementation for dev_uevent, so you could take over that as a temporary solution. The only way to avoid the dependency completely would be to pass that device reference from here through your drm bridge chain somehow. But that's also really fragile. But it could be acceptable as a temporary solution perhaps, if it's even possible. Br, -- heikki
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
Hi guys, On Thu, Sep 14, 2023 at 01:40:49PM +0300, Dmitry Baryshkov wrote: > On Thu, 14 Sept 2023 at 12:26, Heikki Krogerus > wrote: > > > > Hi Dmitry, > > > > On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus > > > wrote: > > > > > > > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > > > > > Hi Heikki, > > > > > > > > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > > > > > wrote: > > > > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > > > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov > > > > > > > > wrote: > > > > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov > > > > > > > > > > wrote: > > > > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry > > > > > > > > > > > > Baryshkov wrote: > > > > > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry > > > > > > > > > > > > > > Baryshkov wrote: > > > > > > > > > > > > > > > The kdev->fwnode pointer is never set in > > > > > > > > > > > > > > > drm_sysfs_connector_add(), so > > > > > > > > > > > > > > > dev_fwnode() checks never succeed, making the > > > > > > > > > > > > > > > respective commit NOP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when > > > > > > > > > > > > > > the device is > > > > > > > > > > > > > > created on ACPI platforms automatically. If the > > > > > > > > > > > > > > drm_connector fwnode > > > > > > > > > > > > > > member is assigned before the device is registered, > > > > > > > > > > > > > > then that fwnode > > > > > > > > > > > > > > is assigned also to the device - see > > > > > > > > > > > > > > drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not > > > > > > > > > > > > > > have anything in > > > > > > > > > > > > > > its fwnode member, the device may still be assigned > > > > > > > > > > > > > > fwnode, just based > > > > > > > > > > > > > > on some other logic (maybe in > > > > > > > > > > > > > > drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to > > > > > > > > > > > > > > > set kdev->fwnode, it > > > > > > > > > > > > > > > breaks drivers already using components (as it > > > > > > > > > > > > > > > was pointed at [1]), > > > > > > >
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
Hi Dmitry, On Wed, Sep 13, 2023 at 04:47:12PM +0300, Dmitry Baryshkov wrote: > On Wed, 13 Sept 2023 at 16:15, Heikki Krogerus > wrote: > > > > On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > > > Hi Heikki, > > > > > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > > > wrote: > > > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov > > > > > > > > wrote: > > > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov > > > > > > > > > > wrote: > > > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry > > > > > > > > > > > > Baryshkov wrote: > > > > > > > > > > > > > The kdev->fwnode pointer is never set in > > > > > > > > > > > > > drm_sysfs_connector_add(), so > > > > > > > > > > > > > dev_fwnode() checks never succeed, making the > > > > > > > > > > > > > respective commit NOP. > > > > > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the > > > > > > > > > > > > device is > > > > > > > > > > > > created on ACPI platforms automatically. If the > > > > > > > > > > > > drm_connector fwnode > > > > > > > > > > > > member is assigned before the device is registered, > > > > > > > > > > > > then that fwnode > > > > > > > > > > > > is assigned also to the device - see > > > > > > > > > > > > drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not > > > > > > > > > > > > have anything in > > > > > > > > > > > > its fwnode member, the device may still be assigned > > > > > > > > > > > > fwnode, just based > > > > > > > > > > > > on some other logic (maybe in > > > > > > > > > > > > drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set > > > > > > > > > > > > > kdev->fwnode, it > > > > > > > > > > > > > breaks drivers already using components (as it was > > > > > > > > > > > > > pointed at [1]), > > > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided > > > > > > > > > > > > > below. > > > > > > > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix > > > > > > > > > > > > > this commit in any > > > > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies > > > > > > > > > > > > on these links, > > &g
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Wed, Sep 13, 2023 at 01:26:14PM +0300, Dmitry Baryshkov wrote: > Hi Heikki, > > On Wed, 13 Sept 2023 at 12:27, Heikki Krogerus > wrote: > > On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > > > On 12/09/2023 14:05, Heikki Krogerus wrote: > > > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > > > wrote: > > > > > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov > > > > > > > > wrote: > > > > > > > > > Hi Heikki, > > > > > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov > > > > > > > > > > wrote: > > > > > > > > > > > The kdev->fwnode pointer is never set in > > > > > > > > > > > drm_sysfs_connector_add(), so > > > > > > > > > > > dev_fwnode() checks never succeed, making the respective > > > > > > > > > > > commit NOP. > > > > > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the > > > > > > > > > > device is > > > > > > > > > > created on ACPI platforms automatically. If the > > > > > > > > > > drm_connector fwnode > > > > > > > > > > member is assigned before the device is registered, then > > > > > > > > > > that fwnode > > > > > > > > > > is assigned also to the device - see > > > > > > > > > > drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have > > > > > > > > > > anything in > > > > > > > > > > its fwnode member, the device may still be assigned fwnode, > > > > > > > > > > just based > > > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set > > > > > > > > > > > kdev->fwnode, it > > > > > > > > > > > breaks drivers already using components (as it was > > > > > > > > > > > pointed at [1]), > > > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix > > > > > > > > > > > this commit in any > > > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on > > > > > > > > > > these links, > > > > > > > > > > so I'm not sure you can just remove them like that. If the > > > > > > > > > > component > > > > > > > > > > framework is not the correct tool here, then I think you > > > > > > > > > > need to > > > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having > > > > > > > > > a > > > > > > > > > component code in the framework code can lead to lockups. > > > > > > > > > With the > > > > > > > > > patch #2 in place (which is the only logical way to set > >
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
Hi Neil, On Wed, Sep 13, 2023 at 11:38:19AM +0200, Neil Armstrong wrote: > On new platforms (starting from SM8450) UCSI is mandatory to have > pmic_glink_altmode events triggering. You can also populate the typec devices conditionally, only if UCSI is not supported. However, I took a peek at drivers/soc/qcom/pmic_glink_altmode.c, and it seems to be mostly is dealing with the muxes and retimer, and sending the HPD notifications to the DRM side. All that is already done in typec drivers, so there is actually a potential race here when UCSI is used. On top of that, it is sending two commands to the PMIC (ALTMODE_PAN_EN and ALTMODE_PAN_ACK). I'm pretty sure both could be handled in the UCSI glue driver (drivers/usb/typec/ucsi/ucsi_glink.c) if they are even needed when UCSI is supported. So why do you need that pmic_glibk_altmode driver at all when UCSI is supported? I don't know the hardware, so I may be missing something. thanks, -- heikki
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
Hi Dmitry, On Tue, Sep 12, 2023 at 08:39:45PM +0300, Dmitry Baryshkov wrote: > On 12/09/2023 14:05, Heikki Krogerus wrote: > > On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > > > On 06/09/2023 16:38, Heikki Krogerus wrote: > > > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > > > wrote: > > > > > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > > > Hi Heikki, > > > > > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov > > > > > > > > wrote: > > > > > > > > > The kdev->fwnode pointer is never set in > > > > > > > > > drm_sysfs_connector_add(), so > > > > > > > > > dev_fwnode() checks never succeed, making the respective > > > > > > > > > commit NOP. > > > > > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > > > created on ACPI platforms automatically. If the drm_connector > > > > > > > > fwnode > > > > > > > > member is assigned before the device is registered, then that > > > > > > > > fwnode > > > > > > > > is assigned also to the device - see > > > > > > > > drm_connector_acpi_find_companion(). > > > > > > > > > > > > > > > > But please note that even if drm_connector does not have > > > > > > > > anything in > > > > > > > > its fwnode member, the device may still be assigned fwnode, > > > > > > > > just based > > > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set > > > > > > > > > kdev->fwnode, it > > > > > > > > > breaks drivers already using components (as it was pointed at > > > > > > > > > [1]), > > > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this > > > > > > > > > commit in any > > > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > > > > > I think there is already user space stuff that relies on these > > > > > > > > links, > > > > > > > > so I'm not sure you can just remove them like that. If the > > > > > > > > component > > > > > > > > framework is not the correct tool here, then I think you need to > > > > > > > > suggest some other way of creating them. > > > > > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > > > component code in the framework code can lead to lockups. With the > > > > > > > patch #2 in place (which is the only logical way to set > > > > > > > kdev->fwnode > > > > > > > for non-ACPI systems) probing of drivers which use components and > > > > > > > set > > > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > > > kdev's fwnode pointer. > > > > > > > > > > > > > > Another option might be to make this drm_sysfs component > > > > > > > registration optional. > > > > > > > > > > > > You don't need to use the component framework at all if there is > > > > > > a better way of determining the connection b
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Tue, Sep 12, 2023 at 12:15:10AM +0300, Dmitry Baryshkov wrote: > On 06/09/2023 16:38, Heikki Krogerus wrote: > > On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > > > wrote: > > > > > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > > > Hi Heikki, > > > > > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > > > wrote: > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > > > The kdev->fwnode pointer is never set in > > > > > > > drm_sysfs_connector_add(), so > > > > > > > dev_fwnode() checks never succeed, making the respective commit > > > > > > > NOP. > > > > > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > > > member is assigned before the device is registered, then that fwnode > > > > > > is assigned also to the device - see > > > > > > drm_connector_acpi_find_companion(). > > > > > > > > > > > > But please note that even if drm_connector does not have anything in > > > > > > its fwnode member, the device may still be assigned fwnode, just > > > > > > based > > > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, > > > > > > > it > > > > > > > breaks drivers already using components (as it was pointed at > > > > > > > [1]), > > > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit > > > > > > > in any > > > > > > > sane way. Revert it instead. > > > > > > > > > > > > I think there is already user space stuff that relies on these > > > > > > links, > > > > > > so I'm not sure you can just remove them like that. If the component > > > > > > framework is not the correct tool here, then I think you need to > > > > > > suggest some other way of creating them. > > > > > > > > > > The issue (that was pointed out during review) is that having a > > > > > component code in the framework code can lead to lockups. With the > > > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > > > for non-ACPI systems) probing of drivers which use components and set > > > > > drm_connector::fwnode breaks immediately. > > > > > > > > > > Can we move the component part to the respective drivers? With the > > > > > patch 2 in place, connector->fwnode will be copied to the created > > > > > kdev's fwnode pointer. > > > > > > > > > > Another option might be to make this drm_sysfs component registration > > > > > optional. > > > > > > > > You don't need to use the component framework at all if there is > > > > a better way of determining the connection between the DP and its > > > > Type-C connector (I'm assuming that that's what this series is about). > > > > You just need the symlinks, not the component. > > > > > > The problem is that right now this component registration has become > > > mandatory. And if I set the kdev->fwnode manually (like in the patch > > > 2), the kernel hangs inside the component code. > > > That's why I proposed to move the components to the place where they > > > are really necessary, e.g. i915 and amd drivers. > > > > So why can't we replace the component with the method you are > > proposing in this series of finding out the Type-C port also with > > i915, AMD, or whatever driver and platform (that's the only thing that > > component is used for)? > > The drm/msm driver uses drm_bridge for the pipeline (including the last DP > entry) and the drm_bridge_connector to create the connector. I think that >
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Wed, Sep 06, 2023 at 03:48:35PM +0300, Dmitry Baryshkov wrote: > On Wed, 6 Sept 2023 at 15:44, Heikki Krogerus > wrote: > > > > On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > > > Hi Heikki, > > > > > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > > > wrote: > > > > > > > > Hi Dmitry, > > > > > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > > > > > That's not true. The dev->fwnode is assigned when the device is > > > > created on ACPI platforms automatically. If the drm_connector fwnode > > > > member is assigned before the device is registered, then that fwnode > > > > is assigned also to the device - see > > > > drm_connector_acpi_find_companion(). > > > > > > > > But please note that even if drm_connector does not have anything in > > > > its fwnode member, the device may still be assigned fwnode, just based > > > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > > > breaks drivers already using components (as it was pointed at [1]), > > > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > > > > > Granted these two issues, it seems impractical to fix this commit in > > > > > any > > > > > sane way. Revert it instead. > > > > > > > > I think there is already user space stuff that relies on these links, > > > > so I'm not sure you can just remove them like that. If the component > > > > framework is not the correct tool here, then I think you need to > > > > suggest some other way of creating them. > > > > > > The issue (that was pointed out during review) is that having a > > > component code in the framework code can lead to lockups. With the > > > patch #2 in place (which is the only logical way to set kdev->fwnode > > > for non-ACPI systems) probing of drivers which use components and set > > > drm_connector::fwnode breaks immediately. > > > > > > Can we move the component part to the respective drivers? With the > > > patch 2 in place, connector->fwnode will be copied to the created > > > kdev's fwnode pointer. > > > > > > Another option might be to make this drm_sysfs component registration > > > optional. > > > > You don't need to use the component framework at all if there is > > a better way of determining the connection between the DP and its > > Type-C connector (I'm assuming that that's what this series is about). > > You just need the symlinks, not the component. > > The problem is that right now this component registration has become > mandatory. And if I set the kdev->fwnode manually (like in the patch > 2), the kernel hangs inside the component code. > That's why I proposed to move the components to the place where they > are really necessary, e.g. i915 and amd drivers. So why can't we replace the component with the method you are proposing in this series of finding out the Type-C port also with i915, AMD, or whatever driver and platform (that's the only thing that component is used for)? Determining the connection between a DP and its Type-C connector is starting to get really important, so ideally we have a common solution for that. thanks, -- heikki
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
On Tue, Sep 05, 2023 at 01:56:59PM +0300, Dmitry Baryshkov wrote: > Hi Heikki, > > On Tue, 5 Sept 2023 at 11:50, Heikki Krogerus > wrote: > > > > Hi Dmitry, > > > > On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > > > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > > > dev_fwnode() checks never succeed, making the respective commit NOP. > > > > That's not true. The dev->fwnode is assigned when the device is > > created on ACPI platforms automatically. If the drm_connector fwnode > > member is assigned before the device is registered, then that fwnode > > is assigned also to the device - see drm_connector_acpi_find_companion(). > > > > But please note that even if drm_connector does not have anything in > > its fwnode member, the device may still be assigned fwnode, just based > > on some other logic (maybe in drivers/acpi/acpi_video.c?). > > > > > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > > > breaks drivers already using components (as it was pointed at [1]), > > > resulting in a deadlock. Lockdep trace is provided below. > > > > > > Granted these two issues, it seems impractical to fix this commit in any > > > sane way. Revert it instead. > > > > I think there is already user space stuff that relies on these links, > > so I'm not sure you can just remove them like that. If the component > > framework is not the correct tool here, then I think you need to > > suggest some other way of creating them. > > The issue (that was pointed out during review) is that having a > component code in the framework code can lead to lockups. With the > patch #2 in place (which is the only logical way to set kdev->fwnode > for non-ACPI systems) probing of drivers which use components and set > drm_connector::fwnode breaks immediately. > > Can we move the component part to the respective drivers? With the > patch 2 in place, connector->fwnode will be copied to the created > kdev's fwnode pointer. > > Another option might be to make this drm_sysfs component registration > optional. You don't need to use the component framework at all if there is a better way of determining the connection between the DP and its Type-C connector (I'm assuming that that's what this series is about). You just need the symlinks, not the component. thanks, -- heikki
Re: [RFC PATCH v1 01/12] Revert "drm/sysfs: Link DRM connectors to corresponding Type-C connectors"
Hi Dmitry, On Mon, Sep 04, 2023 at 12:41:39AM +0300, Dmitry Baryshkov wrote: > The kdev->fwnode pointer is never set in drm_sysfs_connector_add(), so > dev_fwnode() checks never succeed, making the respective commit NOP. That's not true. The dev->fwnode is assigned when the device is created on ACPI platforms automatically. If the drm_connector fwnode member is assigned before the device is registered, then that fwnode is assigned also to the device - see drm_connector_acpi_find_companion(). But please note that even if drm_connector does not have anything in its fwnode member, the device may still be assigned fwnode, just based on some other logic (maybe in drivers/acpi/acpi_video.c?). > And if drm_sysfs_connector_add() is modified to set kdev->fwnode, it > breaks drivers already using components (as it was pointed at [1]), > resulting in a deadlock. Lockdep trace is provided below. > > Granted these two issues, it seems impractical to fix this commit in any > sane way. Revert it instead. I think there is already user space stuff that relies on these links, so I'm not sure you can just remove them like that. If the component framework is not the correct tool here, then I think you need to suggest some other way of creating them. Side note. The problem you are describing here is a limitation in the component framework - right now it's made with the idea that a device can represent a single component, but it really should allow a device to represent multiple components. I'm not saying that you should try to fix the component framework, but I just wanted to make a note about this (and this is not the only problem with the component framework). I like the component framework as a concept, but I think it needs a lot of improvements - possibly rewrite. thanks, -- heikki
Re: [PATCH v3 3/3] usb: typec: nb7vpq904m: switch to DRM_SIMPLE_BRIDGE
On Wed, Aug 02, 2023 at 04:18:45AM +0300, Dmitry Baryshkov wrote:
> Switch to using the new DRM_SIMPLE_BRIDGE helper to create the
> transparent DRM bridge device instead of handcoding corresponding
> functionality.
>
> Signed-off-by: Dmitry Baryshkov
Reviewed-by: Heikki Krogerus
> ---
> drivers/usb/typec/mux/Kconfig | 2 +-
> drivers/usb/typec/mux/nb7vpq904m.c | 44 ++
> 2 files changed, 3 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> index 784b9d8107e9..350a7ffce67e 100644
> --- a/drivers/usb/typec/mux/Kconfig
> +++ b/drivers/usb/typec/mux/Kconfig
> @@ -39,7 +39,7 @@ config TYPEC_MUX_NB7VPQ904M
> tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
> depends on I2C
> depends on DRM || DRM=n
> - select DRM_PANEL_BRIDGE if DRM
> + select DRM_SIMPLE_BRIDGE if DRM
> select REGMAP_I2C
> help
> Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
> diff --git a/drivers/usb/typec/mux/nb7vpq904m.c
> b/drivers/usb/typec/mux/nb7vpq904m.c
> index 9360b65e8b06..c89a956412ea 100644
> --- a/drivers/usb/typec/mux/nb7vpq904m.c
> +++ b/drivers/usb/typec/mux/nb7vpq904m.c
> @@ -11,7 +11,7 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -70,8 +70,6 @@ struct nb7vpq904m {
> bool swap_data_lanes;
> struct typec_switch *typec_switch;
>
> - struct drm_bridge bridge;
> -
> struct mutex lock; /* protect non-concurrent retimer & switch */
>
> enum typec_orientation orientation;
> @@ -297,44 +295,6 @@ static int nb7vpq904m_retimer_set(struct typec_retimer
> *retimer, struct typec_re
> return ret;
> }
>
> -#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_DRM_PANEL_BRIDGE)
> -static int nb7vpq904m_bridge_attach(struct drm_bridge *bridge,
> - enum drm_bridge_attach_flags flags)
> -{
> - struct nb7vpq904m *nb7 = container_of(bridge, struct nb7vpq904m,
> bridge);
> - struct drm_bridge *next_bridge;
> -
> - if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> - return -EINVAL;
> -
> - next_bridge = devm_drm_of_get_bridge(>client->dev,
> nb7->client->dev.of_node, 0, 0);
> - if (IS_ERR(next_bridge)) {
> - dev_err(>client->dev, "failed to acquire drm_bridge:
> %pe\n", next_bridge);
> - return PTR_ERR(next_bridge);
> - }
> -
> - return drm_bridge_attach(bridge->encoder, next_bridge, bridge,
> - DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> -}
> -
> -static const struct drm_bridge_funcs nb7vpq904m_bridge_funcs = {
> - .attach = nb7vpq904m_bridge_attach,
> -};
> -
> -static int nb7vpq904m_register_bridge(struct nb7vpq904m *nb7)
> -{
> - nb7->bridge.funcs = _bridge_funcs;
> - nb7->bridge.of_node = nb7->client->dev.of_node;
> -
> - return devm_drm_bridge_add(>client->dev, >bridge);
> -}
> -#else
> -static int nb7vpq904m_register_bridge(struct nb7vpq904m *nb7)
> -{
> - return 0;
> -}
> -#endif
> -
> static const struct regmap_config nb7_regmap = {
> .max_register = 0x1f,
> .reg_bits = 8,
> @@ -461,7 +421,7 @@ static int nb7vpq904m_probe(struct i2c_client *client)
>
> gpiod_set_value(nb7->enable_gpio, 1);
>
> - ret = nb7vpq904m_register_bridge(nb7);
> + ret = drm_simple_bridge_register(dev);
> if (ret)
> return ret;
>
> --
> 2.39.2
--
heikki
Re: [RFC] drm/msm/dp: Allow attaching a drm_panel
On Mon, May 22, 2023 at 02:53:48PM -0700, Bjorn Andersson wrote:
> On Mon, May 22, 2023 at 03:51:01PM -0500, Bjorn Andersson wrote:
> > On Fri, Oct 08, 2021 at 03:38:21PM +0300, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Thu, Oct 07, 2021 at 09:15:12AM -0700, Bjorn Andersson wrote:
> > > > The one thing that I still don't understand though is, if the typec_mux
> > > > is used by the typec controller to inform _the_ mux about the function
> > > > to be used, what's up with the complexity in typec_mux_match()? This is
> > > > what lead me to believe that typec_mux was enabling/disabling individual
> > > > altmodes, rather just flipping the physical switch at the bottom.
> > >
> > > Ah, typec_mux_match() is a mess. I'm sorry about that. I think most of
> > > the code in that function is not used by anybody. If I remember
> > > correctly, all that complexity is attempting to solve some
> > > hypothetical corner case(s). Probable a case where we have multiple
> > > muxes per port to deal with.
> > >
> > > I think it would probable be best to clean the function to the bare
> > > minimum by keeping only the parts that are actually used today
> > > (attached).
> > >
> >
> > Sorry for not replying to this in a timely manner Heikki. I've been
> > ignoring this issue for a long time now, just adding "svid" to our dts
> > files. But, this obviously shows up in DT validation - and I'd prefer
> > not defining these properties as valid.
> >
> > The attached patch works as expected.
> >
>
> Sorry, I must have failed at applying the patch - it doesn't work...
>
> > Could you please spin this as a proper patch, so we can get it merged?
> >
> > Regards,
> > Bjorn
> >
> > > thanks,
> > >
> > > --
> > > heikki
> >
> > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> > > index c8340de0ed495..44f168c9bd9bf 100644
> > > --- a/drivers/usb/typec/mux.c
> > > +++ b/drivers/usb/typec/mux.c
> > > @@ -193,56 +193,15 @@ static int mux_fwnode_match(struct device *dev,
> > > const void *fwnode)
> > > static void *typec_mux_match(struct fwnode_handle *fwnode, const char
> > > *id,
> > >void *data)
> > > {
> > > - const struct typec_altmode_desc *desc = data;
> > > struct device *dev;
> > > - bool match;
> > > - int nval;
> > > - u16 *val;
> > > - int ret;
> > > - int i;
> > >
> > > /*
> > > - * Check has the identifier already been "consumed". If it
> > > - * has, no need to do any extra connection identification.
> > > + * The connection identifier will be needed with device graph (OF
> > > graph).
> > > + * Device graph is not supported by this code yet, so bailing out.
> > >*/
> > > - match = !id;
> > > - if (match)
> > > - goto find_mux;
> > > -
> > > - /* Accessory Mode muxes */
> > > - if (!desc) {
> > > - match = fwnode_property_present(fwnode, "accessory");
> > > - if (match)
> > > - goto find_mux;
> > > - return NULL;
> > > - }
> > > -
> > > - /* Alternate Mode muxes */
> > > - nval = fwnode_property_count_u16(fwnode, "svid");
> > > - if (nval <= 0)
> > > - return NULL;
> > > -
> > > - val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
> > > - if (!val)
> > > - return ERR_PTR(-ENOMEM);
> > > -
> > > - ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval);
> > > - if (ret < 0) {
> > > - kfree(val);
> > > - return ERR_PTR(ret);
> > > - }
> > > -
> > > - for (i = 0; i < nval; i++) {
> > > - match = val[i] == desc->svid;
> > > - if (match) {
> > > - kfree(val);
> > > - goto find_mux;
> > > - }
> > > - }
> > > - kfree(val);
> > > - return NULL;
> > > + if (id)
>
> We pass id as "mode-switch", so this will never be NULL. But we also only
> want to consider endpoints with "mode-switch", otherwise we'll fail if
> any of the referred endpoints is not implementing a typec_mux...
>
> So this needs the same snippet we find in typec_switch_match():
>
> /*
>* Device graph (OF graph) does not give any means to identify the
>* device type or the device class of the remote port parent that
> @fwnode
>* represents, so in order to identify the type or the class of @fwnode
>* an additional device property is needed. With typec switches the
>* property is named "orientation-switch" (@id). The value of the device
>* property is ignored.
>*/
> if (id && !fwnode_property_present(fwnode, id))
> return NULL;
>
> With that, this works as expected!
Okay. I'll change that and send the patch out.
thanks,
--
heikki
Re: [PATCH v13 01/10] device property: Add remote endpoint to devcon matcher
On Fri, Mar 03, 2023 at 10:33:41PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani
>
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
>
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
>
> Signed-off-by: Prashant Malani
> Signed-off-by: Pin-yen Lin
Reviewed-by: Heikki Krogerus
> ---
>
> Changes in v13:
> - Update the kernel doc of fwnode_connection_find_match
>
> Changes in v12:
> - Check the availability of the device node in fwnode_graph_devcon_matches
> - Ensured valid access to "matches" in fwnode_graph_devcon_matches
> - Updated the documentation in fwnode_connection_find_match(es)
> - Dropped collected tags due to the new changes
>
> Changes in v11:
> - Added missing fwnode_handle_put in drivers/base/property.c
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 31 ++-
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 083a95791d3b..4426ac2b16ca 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1243,6 +1243,23 @@ static unsigned int fwnode_graph_devcon_matches(const
> struct fwnode_handle *fwno
> continue;
> }
>
> + ret = match(node, con_id, data);
> + fwnode_handle_put(node);
> + if (ret) {
> + if (matches)
> + matches[count] = ret;
> + count++;
> +
> + if (matches && count >= matches_len)
> + break;
> + }
> +
> + /*
> + * Some drivers may register devices for endpoints. Check
> + * the remote-endpoints for matches in addition to the remote
> + * port parent.
> + */
> + node = fwnode_graph_get_remote_endpoint(ep);
> ret = match(node, con_id, data);
> fwnode_handle_put(node);
> if (ret) {
> @@ -1293,8 +1310,11 @@ static unsigned int fwnode_devcon_matches(const struct
> fwnode_handle *fwnode,
> * @match: Function to check and convert the connection description
> *
> * Find a connection with unique identifier @con_id between @fwnode and
> another
> - * device node. @match will be used to convert the connection description to
> - * data the caller is expecting to be returned.
> + * device node. For fwnode graph connections, the graph endpoints are also
> + * checked. @match will be used to convert the connection description to data
> + * the caller is expecting to be returned.
> + *
> + * Return: The pointer to the matched node, or NULL on error.
> */
> void *fwnode_connection_find_match(const struct fwnode_handle *fwnode,
> const char *con_id, void *data,
> @@ -1325,9 +1345,10 @@ EXPORT_SYMBOL_GPL(fwnode_connection_find_match);
> * @matches_len: Length of @matches
> *
> * Find up to @matches_len connections with unique identifier @con_id between
> - * @fwnode and other device nodes. @match will be used to convert the
> - * connection description to data the caller is expecting to be returned
> - * through the @matches array.
> + * @fwnode and other device nodes. For fwnode graph connections, the graph
> + * endpoints are also checked. @match will be used to convert the connection
> + * description to data the caller is expecting to be returned through the
> + * @matches array.
> * If @matches is NULL @matches_len is ignored and the total number of
> resolved
> * matches is returned.
> *
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
--
heikki
Re: [PATCH v10 3/9] drm/display: Add Type-C switch helpers
Hi,
On Thu, Jan 12, 2023 at 12:20:58PM +0800, Pin-yen Lin wrote:
> Add helpers to register and unregister Type-C "switches" for bridges
> capable of switching their output between two downstream devices.
>
> The helper registers USB Type-C mode switches when the "mode-switch"
> and the "data-lanes" properties are available in Device Tree.
Let's not make this kind of helpers DT only, please. See below ...
> Signed-off-by: Pin-yen Lin
> Tested-by: Chen-Yu Tsai
> Reviewed-by: Chen-Yu Tsai
> Reviewed-by: AngeloGioacchino Del Regno
>
>
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
> - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
> - Print out the node name when errors on parsing DT
> - Use dev_dbg instead of dev_warn when no Type-C switch nodes available
> - Made the return path of drm_dp_register_mode_switch clearer
>
> Changes in v8:
> - Fixed the build issue when CONFIG_TYPEC=m
> - Fixed some style issues
>
> Changes in v7:
> - Extracted the common codes to a helper function
> - New in v7
>
> drivers/gpu/drm/display/drm_dp_helper.c | 134
> include/drm/display/drm_dp_helper.h | 17 +++
> 2 files changed, 151 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> b/drivers/gpu/drm/display/drm_dp_helper.c
> index 16565a0a5da6..a2ec40a621cb 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -30,11 +30,13 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel
> *panel, struct drm_dp_aux *aux)
> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>
> #endif
> +
> +#if IS_REACHABLE(CONFIG_TYPEC)
I think Jani already pointed out that that is wrong. Just move these
into a separate file and enable them silently in the Makefile when
TYPEC is enabled - so no separate Kconfig option.
> +static int drm_dp_register_mode_switch(struct device *dev, struct
> device_node *node,
static int drm_dp_register_mode_switch(struct device *dev, struct fwnode_handle
*fwnode,
> +struct drm_dp_typec_switch_desc
> *switch_desc,
> +void *data, typec_mux_set_fn_t mux_set)
> +{
> + struct drm_dp_typec_port_data *port_data;
> + struct typec_mux_desc mux_desc = {};
> + char name[32];
> + u32 dp_lanes[2];
> + int ret, num_lanes, port_num = -1;
> +
> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> + if (num_lanes <= 0) {
num_lanes = fwnode_property_read_u32_array(fwnode, "data-lanes", NULL,
0);
if (num_lanes <= 0 || num_lanes > 2)
> + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> + node->name, num_lanes);
> + return num_lanes;
> + }
> +
> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes,
> num_lanes);
ret = fwnode_property_read_u32_array(fwnode, "data-lanes", dp_lanes,
num_lanes);
> + if (ret) {
> + dev_err(dev, "Failed to read the data-lanes variable from %s:
> %d\n",
> + node->name, ret);
fwnode_get_name(fwnode), ret);
> + return ret;
> + }
> +
> + port_num = dp_lanes[0] / 2;
> +
> + port_data = _desc->typec_ports[port_num];
> + port_data->data = data;
> + mux_desc.fwnode = >fwnode;
mux_desc.fwnode = fwnode;
> + mux_desc.drvdata = port_data;
> + snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
snprintf(name, sizeof(name), "%s-%u", fwnode_get_name(fwnode),
port_num);
> + mux_desc.name = name;
> + mux_desc.set = mux_set;
> +
> + port_data->typec_mux = typec_mux_register(dev, _desc);
> + if (IS_ERR(port_data->typec_mux)) {
> + ret = PTR_ERR(port_data->typec_mux);
> + dev_err(dev, "Mode switch register for port %d failed: %d\n",
> + port_num, ret);
> +
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * drm_dp_register_typec_switches() - register Type-C switches
> + * @dev: Device that registers Type-C switches
> + * @port: Device node for the switch
> + * @switch_desc: A Type-C switch descriptor
> + * @data: Private data for the switches
> + * @mux_set: Callback function for typec_mux_set
> + *
> + * This function registers USB Type-C switches for DP bridges that can switch
> + * the output signal between their output pins.
> + *
> + * Currently only mode switches are implemented, and the function assumes the
> + * given @port device node has endpoints with "mode-switch" property.
> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> + * and register it as port 1 if "data-lanes" falls in 2/3.
> + */
> +int
Re: [PATCH v10 2/9] platform/chrome: cros_ec_typec: Purge blocking switch devlinks
On Thu, Jan 12, 2023 at 12:20:57PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani
>
> When using OF graph, the fw_devlink code will create links between the
> individual port driver (cros-ec-typec here) and the parent device for
> a Type-C switch (like mode-switch). Since the mode-switch will in turn
> have the usb-c-connector (i.e the child of the port driver) as a
> supplier, fw_devlink will not be able to resolve the cyclic dependency
> correctly.
>
> As a result, the mode-switch driver probe() never runs, so mode-switches
> are never registered. Because of that, the port driver probe constantly
> fails with -EPROBE_DEFER, because the Type-C connector class requires all
> switch devices to be registered prior to port registration.
>
> To break this deadlock and allow the mode-switch registration to occur,
> purge all the usb-c-connector nodes' absent suppliers. This eliminates
> the connector as a supplier for a switch and allows it to be probed.
>
> Signed-off-by: Prashant Malani
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Chen-Yu Tsai
> Tested-by: Chen-Yu Tsai
FWIW:
Acked-by: Heikki Krogerus
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v7:
> - Fix the long comment lines
>
> Changes in v6:
> - New in v6
>
> drivers/platform/chrome/cros_ec_typec.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c
> b/drivers/platform/chrome/cros_ec_typec.c
> index 2a7ff14dc37e..302474a647cc 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -382,6 +382,16 @@ static int cros_typec_init_ports(struct cros_typec_data
> *typec)
> return -EINVAL;
> }
>
> + /*
> + * OF graph may have set up some device links with switches,
> + * since connectors have their own compatible. Purge these
> + * to avoid a deadlock in switch probe (the switch mistakenly
> + * assumes the connector is a supplier).
> + */
> + if (dev_of_node(dev))
> + device_for_each_child_node(dev, fwnode)
> + fw_devlink_purge_absent_suppliers(fwnode);
> +
> /* DT uses "reg" to specify port number. */
> port_prop = dev->of_node ? "reg" : "port-number";
> device_for_each_child_node(dev, fwnode) {
> --
> 2.39.0.314.g84b9a713c41-goog
--
heikki
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani
>
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
>
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
>
> Signed-off-by: Prashant Malani
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Chen-Yu Tsai
> Tested-by: Chen-Yu Tsai
Acked-by: Heikki Krogerus
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..48877af4e444 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,21 @@ static unsigned int fwnode_graph_devcon_matches(struct
> fwnode_handle *fwnode,
> break;
> }
>
> + /*
> + * Some drivers may register devices for endpoints. Check
> + * the remote-endpoints for matches in addition to the remote
> + * port parent.
> + */
> + node = fwnode_graph_get_remote_endpoint(ep);
> + if (fwnode_device_is_available(node)) {
> + ret = match(node, con_id, data);
> + if (ret) {
> + if (matches)
> + matches[count] = ret;
> + count++;
> + }
> + }
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
> --
> 2.39.0.314.g84b9a713c41-goog
--
heikki
Re: [PATCH v7 08/13] usb: typec: tcpci_mt6370: Add MediaTek MT6370 tcpci driver
On Fri, Aug 05, 2022 at 03:06:05PM +0800, ChiaEn Wu wrote:
> From: ChiYuan Huang
>
> The MediaTek MT6370 is a highly-integrated smart power management IC,
> which includes a single cell Li-Ion/Li-Polymer switching battery
> charger, a USB Type-C & Power Delivery (PD) controller, dual
> Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> a display bias driver and a general LDO for portable devices.
>
> Add a support for the Type-C & Power Delivery controller in
> MediaTek MT6370 IC.
>
> Reviewed-by: AngeloGioacchino Del Regno
>
> Reviewed-by: Guenter Roeck
> Signed-off-by: ChiYuan Huang
> Signed-off-by: ChiaEn Wu
Reviewed-by: Heikki Krogerus
> ---
>
> v7
> - Revise 'devm_add_action_or_reset(dev, ...)' to one line
> - Revise 'return regmap_update_bits(...)' with using positive
> conditional
> ---
> drivers/usb/typec/tcpm/Kconfig| 11 ++
> drivers/usb/typec/tcpm/Makefile | 1 +
> drivers/usb/typec/tcpm/tcpci_mt6370.c | 207
> ++
> 3 files changed, 219 insertions(+)
> create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6370.c
>
> diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> index 073fd2e..e6b88ca 100644
> --- a/drivers/usb/typec/tcpm/Kconfig
> +++ b/drivers/usb/typec/tcpm/Kconfig
> @@ -35,6 +35,17 @@ config TYPEC_MT6360
> USB Type-C. It works with Type-C Port Controller Manager
> to provide USB PD and USB Type-C functionalities.
>
> +config TYPEC_TCPCI_MT6370
> + tristate "MediaTek MT6370 Type-C driver"
> + depends on MFD_MT6370
> + help
> + MediaTek MT6370 is a multi-functional IC that includes
> + USB Type-C. It works with Type-C Port Controller Manager
> + to provide USB PD and USB Type-C functionalities.
> +
> + This driver can also be built as a module. The module
> + will be called "tcpci_mt6370".
> +
> config TYPEC_TCPCI_MAXIM
> tristate "Maxim TCPCI based Type-C chip driver"
> help
> diff --git a/drivers/usb/typec/tcpm/Makefile b/drivers/usb/typec/tcpm/Makefile
> index 7d499f3..906d9dc 100644
> --- a/drivers/usb/typec/tcpm/Makefile
> +++ b/drivers/usb/typec/tcpm/Makefile
> @@ -6,4 +6,5 @@ typec_wcove-y := wcove.o
> obj-$(CONFIG_TYPEC_TCPCI)+= tcpci.o
> obj-$(CONFIG_TYPEC_RT1711H) += tcpci_rt1711h.o
> obj-$(CONFIG_TYPEC_MT6360) += tcpci_mt6360.o
> +obj-$(CONFIG_TYPEC_TCPCI_MT6370) += tcpci_mt6370.o
> obj-$(CONFIG_TYPEC_TCPCI_MAXIM) += tcpci_maxim.o
> diff --git a/drivers/usb/typec/tcpm/tcpci_mt6370.c
> b/drivers/usb/typec/tcpm/tcpci_mt6370.c
> new file mode 100644
> index 000..c5bb201
> --- /dev/null
> +++ b/drivers/usb/typec/tcpm/tcpci_mt6370.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Richtek Technology Corp.
> + *
> + * Author: ChiYuan Huang
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MT6370_REG_SYSCTRL8 0x9B
> +
> +#define MT6370_AUTOIDLE_MASK BIT(3)
> +
> +#define MT6370_VENDOR_ID 0x29CF
> +#define MT6370_TCPC_DID_A0x2170
> +
> +struct mt6370_priv {
> + struct device *dev;
> + struct regulator *vbus;
> + struct tcpci *tcpci;
> + struct tcpci_data tcpci_data;
> +};
> +
> +static const struct reg_sequence mt6370_reg_init[] = {
> + REG_SEQ(0xA0, 0x1, 1000),
> + REG_SEQ(0x81, 0x38, 0),
> + REG_SEQ(0x82, 0x82, 0),
> + REG_SEQ(0xBA, 0xFC, 0),
> + REG_SEQ(0xBB, 0x50, 0),
> + REG_SEQ(0x9E, 0x8F, 0),
> + REG_SEQ(0xA1, 0x5, 0),
> + REG_SEQ(0xA2, 0x4, 0),
> + REG_SEQ(0xA3, 0x4A, 0),
> + REG_SEQ(0xA4, 0x01, 0),
> + REG_SEQ(0x95, 0x01, 0),
> + REG_SEQ(0x80, 0x71, 0),
> + REG_SEQ(0x9B, 0x3A, 1000),
> +};
> +
> +static int mt6370_tcpc_init(struct tcpci *tcpci, struct tcpci_data *data)
> +{
> + u16 did;
> + int ret;
> +
> + ret = regmap_register_patch(data->regmap, mt6370_reg_init,
> + ARRAY_SIZE(mt6370_reg_init));
> + if (ret)
> + return ret;
> +
> + ret = regmap_raw_read(data->regmap, TCPC_BCD_DEV, , sizeof(u16));
> + if (ret)
> + return ret;
> +
> + if (did == MT6370_TCPC_DID_A)
> + return regmap_write(data->regmap, TCPC_FAULT_CTRL, 0x80);
> +
> + return 0;
> +}
> +
> +static int mt6370_tcpc_set_vconn(struct tcpc
Re: [PATCH 6/6] i2c: Make remove callback return void
0;
> }
>
> -static int tcpci_remove(struct i2c_client *client)
> +static void tcpci_remove(struct i2c_client *client)
> {
> struct tcpci_chip *chip = i2c_get_clientdata(client);
> int err;
> @@ -880,8 +880,6 @@ static int tcpci_remove(struct i2c_client *client)
> dev_warn(>dev, "Failed to disable irqs (%pe)\n",
> ERR_PTR(err));
>
> tcpci_unregister_port(chip->tcpci);
> -
> - return 0;
> }
>
> static const struct i2c_device_id tcpci_id[] = {
> diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.c
> b/drivers/usb/typec/tcpm/tcpci_maxim.c
> index df2505570f07..a11be5754128 100644
> --- a/drivers/usb/typec/tcpm/tcpci_maxim.c
> +++ b/drivers/usb/typec/tcpm/tcpci_maxim.c
> @@ -493,14 +493,12 @@ static int max_tcpci_probe(struct i2c_client *client,
> const struct i2c_device_id
> return ret;
> }
>
> -static int max_tcpci_remove(struct i2c_client *client)
> +static void max_tcpci_remove(struct i2c_client *client)
> {
> struct max_tcpci_chip *chip = i2c_get_clientdata(client);
>
> if (!IS_ERR_OR_NULL(chip->tcpci))
> tcpci_unregister_port(chip->tcpci);
> -
> - return 0;
> }
>
> static const struct i2c_device_id max_tcpci_id[] = {
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index b56a0880a044..9ad4924b4ba7 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -263,12 +263,11 @@ static int rt1711h_probe(struct i2c_client *client,
> return 0;
> }
>
> -static int rt1711h_remove(struct i2c_client *client)
> +static void rt1711h_remove(struct i2c_client *client)
> {
> struct rt1711h_chip *chip = i2c_get_clientdata(client);
>
> tcpci_unregister_port(chip->tcpci);
> - return 0;
> }
>
> static const struct i2c_device_id rt1711h_id[] = {
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index dfbba5ae9487..b637e8b378b3 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -857,15 +857,13 @@ static int tps6598x_probe(struct i2c_client *client)
> return ret;
> }
>
> -static int tps6598x_remove(struct i2c_client *client)
> +static void tps6598x_remove(struct i2c_client *client)
> {
> struct tps6598x *tps = i2c_get_clientdata(client);
>
> tps6598x_disconnect(tps, 0);
> typec_unregister_port(tps->port);
> usb_role_switch_put(tps->role_sw);
> -
> - return 0;
> }
>
> static const struct of_device_id tps6598x_of_match[] = {
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 6db7c8ddd51c..920b7e743f56 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -1398,7 +1398,7 @@ static int ucsi_ccg_probe(struct i2c_client *client,
> return status;
> }
>
> -static int ucsi_ccg_remove(struct i2c_client *client)
> +static void ucsi_ccg_remove(struct i2c_client *client)
> {
> struct ucsi_ccg *uc = i2c_get_clientdata(client);
>
> @@ -1408,8 +1408,6 @@ static int ucsi_ccg_remove(struct i2c_client *client)
> ucsi_unregister(uc->ucsi);
> ucsi_destroy(uc->ucsi);
> free_irq(uc->irq, uc);
> -
> - return 0;
> }
>
> static const struct i2c_device_id ucsi_ccg_device_id[] = {
> diff --git a/drivers/usb/typec/wusb3801.c b/drivers/usb/typec/wusb3801.c
> index e63509f8b01e..3cc7a15ecbd3 100644
> --- a/drivers/usb/typec/wusb3801.c
> +++ b/drivers/usb/typec/wusb3801.c
> @@ -399,7 +399,7 @@ static int wusb3801_probe(struct i2c_client *client)
> return ret;
> }
>
> -static int wusb3801_remove(struct i2c_client *client)
> +static void wusb3801_remove(struct i2c_client *client)
> {
> struct wusb3801 *wusb3801 = i2c_get_clientdata(client);
>
> @@ -411,8 +411,6 @@ static int wusb3801_remove(struct i2c_client *client)
>
> if (wusb3801->vbus_on)
> regulator_disable(wusb3801->vbus_supply);
> -
> - return 0;
> }
Reviewed-by: Heikki Krogerus
--
heikki
Re: [PATCH v4 2/7] usb: typec: mux: Add CONFIG guards for functions
On Wed, Jun 15, 2022 at 05:20:18PM +, Prashant Malani wrote:
> There are some drivers that can use the Type C mux API, but don't have
> to. Introduce CONFIG guards for the mux functions so that drivers can
> include the header file and not run into compilation errors on systems
> which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
> the Type C mux functions will be stub versions of the original calls.
>
> Reported-by: kernel test robot
> Reviewed-by: Nícolas F. R. A. Prado
> Tested-by: Nícolas F. R. A. Prado
> Signed-off-by: Prashant Malani
Reviewed-by: Heikki Krogerus
> ---
>
> Changes since v3:
> - No changes.
>
> Changes since v2:
> - Fix up return types for some of the stubs. Remove 1 unnecessary stub
> in the else condition.
> - Remove unnecessary IS_MODULE config guard.
> - Added Reviewed-by and Tested-by tags.
>
> Changes since v1:
> - Added static inline to stub functions.
> - Updated function signature of stub functions from "struct typec_mux"
> to "struct typec_mux_dev" in accordance with updates from commit
> 713fd49b430c ("usb: typec: mux: Introduce indirection")
>
> include/linux/usb/typec_mux.h | 44 ++-
> 1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
> index ee57781dcf28..9292f0e07846 100644
> --- a/include/linux/usb/typec_mux.h
> +++ b/include/linux/usb/typec_mux.h
> @@ -58,17 +58,13 @@ struct typec_mux_desc {
> void *drvdata;
> };
>
> +#if IS_ENABLED(CONFIG_TYPEC)
> +
> struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
> const struct typec_altmode_desc *desc);
> void typec_mux_put(struct typec_mux *mux);
> int typec_mux_set(struct typec_mux *mux, struct typec_mux_state *state);
>
> -static inline struct typec_mux *
> -typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc)
> -{
> - return fwnode_typec_mux_get(dev_fwnode(dev), desc);
> -}
> -
> struct typec_mux_dev *
> typec_mux_register(struct device *parent, const struct typec_mux_desc *desc);
> void typec_mux_unregister(struct typec_mux_dev *mux);
> @@ -76,4 +72,40 @@ void typec_mux_unregister(struct typec_mux_dev *mux);
> void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data);
> void *typec_mux_get_drvdata(struct typec_mux_dev *mux);
>
> +#else
> +
> +static inline struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle
> *fwnode,
> +const struct typec_altmode_desc *desc)
> +{
> + return NULL;
> +}
> +
> +static inline void typec_mux_put(struct typec_mux *mux) {}
> +
> +static inline int typec_mux_set(struct typec_mux *mux, struct
> typec_mux_state *state)
> +{
> + return 0;
> +}
> +
> +static inline struct typec_mux_dev *
> +typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline void typec_mux_unregister(struct typec_mux_dev *mux) {}
> +
> +static inline void typec_mux_set_drvdata(struct typec_mux_dev *mux, void
> *data) {}
> +static inline void *typec_mux_get_drvdata(struct typec_mux_dev *mux)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +#endif /* CONFIG_TYPEC */
> +
> +static inline struct typec_mux *
> +typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc)
> +{
> + return fwnode_typec_mux_get(dev_fwnode(dev), desc);
> +}
> +
> #endif /* __USB_TYPEC_MUX */
> --
> 2.36.1.476.g0c4daa206d-goog
--
heikki
Re: [PATCH v2 2/7] usb: typec: mux: Add CONFIG guards for functions
Hi,
On Thu, Jun 09, 2022 at 06:09:41PM +, Prashant Malani wrote:
> There are some drivers that can use the Type C mux API, but don't have
> to. Introduce CONFIG guards for the mux functions so that drivers can
> include the header file and not run into compilation errors on systems
> which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
> the Type C mux functions will be stub versions of the original calls.
>
> Reported-by: kernel test robot
> Signed-off-by: Prashant Malani
> ---
>
> Changes since v1:
> - Added static inline to stub functions.
> - Updated function signature of stub functions from "struct typec_mux"
> to "struct typec_mux_dev" in accordance with updates from commit
> 713fd49b430c ("usb: typec: mux: Introduce indirection")
>
> include/linux/usb/typec_mux.h | 38 +++
> 1 file changed, 38 insertions(+)
>
> diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
> index ee57781dcf28..9eda6146fd26 100644
> --- a/include/linux/usb/typec_mux.h
> +++ b/include/linux/usb/typec_mux.h
> @@ -58,6 +58,8 @@ struct typec_mux_desc {
> void *drvdata;
> };
>
> +#if IS_ENABLED(CONFIG_TYPEC) || IS_MODULE(CONFIG_TYPEC)
> +
> struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
> const struct typec_altmode_desc *desc);
> void typec_mux_put(struct typec_mux *mux);
> @@ -76,4 +78,40 @@ void typec_mux_unregister(struct typec_mux_dev *mux);
> void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data);
> void *typec_mux_get_drvdata(struct typec_mux_dev *mux);
>
> +#else
> +
> +static inline struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle
> *fwnode,
> +const struct typec_altmode_desc *desc)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
The mux is optional resource for the drivers - fwnode_typec_mux_get()
returns NULL if there is no mux for the caller - so it's better to
just return NULL from this stub.
> +static inline void typec_mux_put(struct typec_mux *mux) {}
> +
> +static inline int typec_mux_set(struct typec_mux *mux, struct
> typec_mux_state *state)
> +{
> + return -EOPNOTSUPP;
> +}
Return 0 in this case. That way this stub matches the function
behaviour:
...
if (IS_ERR_OR_NULL(mux))
return 0;
...
> +static inline struct typec_mux *
> +typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
You don't need this one. Just leave the original outside of the ifdef.
It's already an inline wrapper function.
> +static inline struct typec_mux_dev *
> +typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline void typec_mux_unregister(struct typec_mux_dev *mux) {}
> +
> +static inline void typec_mux_set_drvdata(struct typec_mux_dev *mux, void
> *data) {}
> +static inline void *typec_mux_get_drvdata(struct typec_mux_dev *mux)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +#endif /* CONFIG_TYPEC */
> +
> #endif /* __USB_TYPEC_MUX */
thanks,
--
heikki
Re: [PATCH v2 1/7] usb: typec: mux: Allow muxes to specify mode-switch
On Thu, Jun 09, 2022 at 06:09:40PM +, Prashant Malani wrote:
> Loosen the typec_mux_match() requirements so that searches where an
> alt mode is not specified, but the target mux device lists the
> "mode-switch" property, return a success.
>
> This is helpful in Type C port drivers which would like to get a pointer
> to the mux switch associated with a Type C port, but don't want to
> specify a particular alt mode.
>
> Signed-off-by: Prashant Malani
Reviewed-by: Heikki Krogerus
> ---
>
> Changes since v1:
> - No changes.
>
> drivers/usb/typec/mux.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index fd55c2c516a5..464330776cd6 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -281,9 +281,13 @@ static void *typec_mux_match(struct fwnode_handle
> *fwnode, const char *id,
> if (match)
> goto find_mux;
>
> - /* Accessory Mode muxes */
> if (!desc) {
> - match = fwnode_property_present(fwnode, "accessory");
> + /*
> + * Accessory Mode muxes & muxes which explicitly specify
> + * the required identifier can avoid SVID matching.
> + */
> + match = fwnode_property_present(fwnode, "accessory") ||
> + fwnode_property_present(fwnode, id);
> if (match)
> goto find_mux;
> return NULL;
> --
> 2.36.1.476.g0c4daa206d-goog
--
heikki
Re: [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
On Mon, May 02, 2022 at 09:53:13AM -0700, Bjorn Andersson wrote:
> In some implementations, such as the Qualcomm platforms, the display
> driver has no way to query the current HPD state and as such it's
> impossible to distinguish between disconnect and attention events.
>
> Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> state.
>
> Also push the test for unchanged state in the displayport altmode driver
> into the i915 driver, to allow other drivers to act upon each update.
>
> Signed-off-by: Bjorn Andersson
Acked-by: Heikki Krogerus
> ---
>
> Changes since v3:
> - Transition to drm_connector_status instead of custom hpd_state
>
> drivers/gpu/drm/drm_connector.c | 6 --
> drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/usb/typec/altmodes/displayport.c | 10 +++---
> include/drm/drm_connector.h | 6 --
> 5 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e86c69f0640f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2794,6 +2794,7 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> /**
> * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> connector
> * @connector_fwnode: fwnode_handle to report the event on
> + * @status: hot plug detect logical state
> *
> * On some hardware a hotplug event notification may come from outside the
> display
> * driver / device. An example of this is some USB Type-C setups where the
> hardware
> @@ -2803,7 +2804,8 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> * This function can be used to report these out-of-band events after
> obtaining
> * a drm_connector reference through calling drm_connector_find_by_fwnode().
> */
> -void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
> + enum drm_connector_status status)
> {
> struct drm_connector *connector;
>
> @@ -2812,7 +2814,7 @@ void drm_connector_oob_hotplug_event(struct
> fwnode_handle *connector_fwnode)
> return;
>
> if (connector->funcs->oob_hotplug_event)
> - connector->funcs->oob_hotplug_event(connector);
> + connector->funcs->oob_hotplug_event(connector, status);
>
> drm_connector_put(connector);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index e4a79c11fd25..56cc023f7bbd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4951,15 +4951,26 @@ static int intel_dp_connector_atomic_check(struct
> drm_connector *conn,
> return intel_modeset_synced_crtcs(state, conn);
> }
>
> -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
> +static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
> +enum drm_connector_status hpd_state)
> {
> struct intel_encoder *encoder =
> intel_attached_encoder(to_intel_connector(connector));
> struct drm_i915_private *i915 = to_i915(connector->dev);
> + bool hpd_high = hpd_state == connector_status_connected;
> + unsigned int hpd_pin = encoder->hpd_pin;
> + bool need_work = false;
>
> spin_lock_irq(>irq_lock);
> - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> + if (hpd_high != test_bit(hpd_pin,
> >hotplug.oob_hotplug_last_state)) {
> + i915->hotplug.event_bits |= BIT(hpd_pin);
> +
> + __assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
> hpd_high);
> + need_work = true;
> + }
> spin_unlock_irq(>irq_lock);
> - queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> +
> + if (need_work)
> + queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> }
>
> static const struct drm_connector_funcs intel_dp_connector_funcs = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 24111bf42ce0..96c088bb5522 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -135,6 +135,9 @@ struct i915_hotplug {
> /* Whether or not to count short HPD IRQs in HPD storms */
> u8 hpd_short_storm_enabled;
>
> + /* Last state rep
Re: [PATCH v4 2/5] drm: Add HPD state to drm_connector_oob_hotplug_event()
+Hans
Hans, do you have any comments?
On Mon, May 02, 2022 at 09:53:13AM -0700, Bjorn Andersson wrote:
> In some implementations, such as the Qualcomm platforms, the display
> driver has no way to query the current HPD state and as such it's
> impossible to distinguish between disconnect and attention events.
>
> Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> state.
>
> Also push the test for unchanged state in the displayport altmode driver
> into the i915 driver, to allow other drivers to act upon each update.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v3:
> - Transition to drm_connector_status instead of custom hpd_state
>
> drivers/gpu/drm/drm_connector.c | 6 --
> drivers/gpu/drm/i915/display/intel_dp.c | 17 ++---
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/usb/typec/altmodes/displayport.c | 10 +++---
> include/drm/drm_connector.h | 6 --
> 5 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e86c69f0640f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2794,6 +2794,7 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> /**
> * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> connector
> * @connector_fwnode: fwnode_handle to report the event on
> + * @status: hot plug detect logical state
> *
> * On some hardware a hotplug event notification may come from outside the
> display
> * driver / device. An example of this is some USB Type-C setups where the
> hardware
> @@ -2803,7 +2804,8 @@ struct drm_connector
> *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> * This function can be used to report these out-of-band events after
> obtaining
> * a drm_connector reference through calling drm_connector_find_by_fwnode().
> */
> -void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
> + enum drm_connector_status status)
> {
> struct drm_connector *connector;
>
> @@ -2812,7 +2814,7 @@ void drm_connector_oob_hotplug_event(struct
> fwnode_handle *connector_fwnode)
> return;
>
> if (connector->funcs->oob_hotplug_event)
> - connector->funcs->oob_hotplug_event(connector);
> + connector->funcs->oob_hotplug_event(connector, status);
>
> drm_connector_put(connector);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index e4a79c11fd25..56cc023f7bbd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4951,15 +4951,26 @@ static int intel_dp_connector_atomic_check(struct
> drm_connector *conn,
> return intel_modeset_synced_crtcs(state, conn);
> }
>
> -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
> +static void intel_dp_oob_hotplug_event(struct drm_connector *connector,
> +enum drm_connector_status hpd_state)
> {
> struct intel_encoder *encoder =
> intel_attached_encoder(to_intel_connector(connector));
> struct drm_i915_private *i915 = to_i915(connector->dev);
> + bool hpd_high = hpd_state == connector_status_connected;
> + unsigned int hpd_pin = encoder->hpd_pin;
> + bool need_work = false;
>
> spin_lock_irq(>irq_lock);
> - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> + if (hpd_high != test_bit(hpd_pin,
> >hotplug.oob_hotplug_last_state)) {
> + i915->hotplug.event_bits |= BIT(hpd_pin);
> +
> + __assign_bit(hpd_pin, >hotplug.oob_hotplug_last_state,
> hpd_high);
> + need_work = true;
> + }
> spin_unlock_irq(>irq_lock);
> - queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> +
> + if (need_work)
> + queue_delayed_work(system_wq, >hotplug.hotplug_work, 0);
> }
>
> static const struct drm_connector_funcs intel_dp_connector_funcs = {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 24111bf42ce0..96c088bb5522 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -135,6 +135,9 @@ struct i915_hotplug {
> /* Whether or not to count short HPD IRQs in HPD storms */
> u8 hpd_short_storm_enabled;
>
> + /* Last state reported by oob_hotplug_event for each encoder */
> + unsigned long oob_hotplug_last_state;
> +
> /*
>* if we get a HPD irq from DP and a HPD irq from non-DP
>* the non-DP HPD could block the workqueue on a mode config
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index
Re: [syzbot] WARNING in component_del
On Wed, Feb 09, 2022 at 04:51:44PM +0300, Pavel Skripkin wrote:
> Hi Heikki,
>
> On 2/9/22 16:42, Heikki Krogerus wrote:
> > On Tue, Feb 08, 2022 at 02:37:10AM -0800, syzbot wrote:
> > > syzbot has bisected this issue to:
> > >
> > > commit 8c67d06f3fd9639c44d8147483fb1c132d71388f
> > > Author: Heikki Krogerus
> > > Date: Thu Dec 23 08:23:49 2021 +
> > >
> > > usb: Link the ports to the connectors they are attached to
> > >
> > > bisection log:
> > > https://syzkaller.appspot.com/x/bisect.txt?x=1406316870
> > > start commit: 555f3d7be91a Merge tag '5.17-rc3-ksmbd-server-fixes' of
> > > gi..
> > > git tree: upstream
> > > final oops:
> > > https://syzkaller.appspot.com/x/report.txt?x=1606316870
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=1206316870
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=266de9da75c71a45
> > > dashboard link:
> > > https://syzkaller.appspot.com/bug?extid=60df062e1c41940cae0f
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15880d8470
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14de0c77b0
> > >
> > > Reported-by: syzbot+60df062e1c41940ca...@syzkaller.appspotmail.com
> > > Fixes: 8c67d06f3fd9 ("usb: Link the ports to the connectors they are
> > > attached to")
> > >
> > > For information about bisection process see:
> > > https://goo.gl/tpsmEJ#bisection
> >
> > I'm guessing the component_add() is failing in this case. So I guess
> > we need to consider the component_add() failures fatal in
> > drivers/usb/core/port.c, which is a bit of a bummer. I'll send the
> > fix.
> >
>
> Seems something similar to your approach is already posted
>
> https://lore.kernel.org/linux-usb/20220208170048.24718-1-fmdefrance...@gmail.com/
Ah, thanks! That one seems to leave the peer links, so it's broken...
Br,
--
heikki
Re: [syzbot] WARNING in component_del
On Tue, Feb 08, 2022 at 02:37:10AM -0800, syzbot wrote:
> syzbot has bisected this issue to:
>
> commit 8c67d06f3fd9639c44d8147483fb1c132d71388f
> Author: Heikki Krogerus
> Date: Thu Dec 23 08:23:49 2021 +
>
> usb: Link the ports to the connectors they are attached to
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1406316870
> start commit: 555f3d7be91a Merge tag '5.17-rc3-ksmbd-server-fixes' of gi..
> git tree: upstream
> final oops: https://syzkaller.appspot.com/x/report.txt?x=1606316870
> console output: https://syzkaller.appspot.com/x/log.txt?x=1206316870
> kernel config: https://syzkaller.appspot.com/x/.config?x=266de9da75c71a45
> dashboard link: https://syzkaller.appspot.com/bug?extid=60df062e1c41940cae0f
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15880d8470
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14de0c77b0
>
> Reported-by: syzbot+60df062e1c41940ca...@syzkaller.appspotmail.com
> Fixes: 8c67d06f3fd9 ("usb: Link the ports to the connectors they are attached
> to")
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
I'm guessing the component_add() is failing in this case. So I guess
we need to consider the component_add() failures fatal in
drivers/usb/core/port.c, which is a bit of a bummer. I'll send the
fix.
--
heikki
Re: [RFC] drm/msm/dp: Allow attaching a drm_panel
+Hans and Imre On Mon, Dec 06, 2021 at 02:31:40PM -0800, Bjorn Andersson wrote: > On Thu 07 Oct 03:17 PDT 2021, Heikki Krogerus wrote: > > On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote: > > > (CC+ Heikki) > [..] > > > On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson > > > wrote: > [..] > > void drm_connector_oob_hotplug_event(struct fwnode_handle > > *connector_fwnode); > > > > If your USB Type-C controller/port driver does not yet register the DP > > alt mode, the it's responsible of handling HPD separately by calling > > drm_connector_oob_hotplug_event() on its own. > > > > Finally found my way back to this topic and it doesn't look like I can > reuse the existing altmode code with the firmware interface provided by > Qualcomm, so I just hacked something up that invokes > drm_connector_oob_hotplug_event(). > > But I'm not able to make sense of what the expected usage is. Reading > altmode/displayport.c, it seems that I should only invoke > drm_connector_oob_hotplug_event() as HPD state toggles. > > I made a trial implementation of this, where my firmware interface > driver calls drm_connector_oob_hotplug_event() every time HPD state > changes and then in my oob_hotplug_event callback I flip the DP > controller between on and off. > > Unfortunately when I then connect my HDMI dongle, I get HPD state HIGH, > call the oob_hotplug_event, the DP driver powers up and concludes that > there's nothing connected to the dongle and goes to idle. I then connect > the HDMI cable to the dongle, the firmware sends me another message with > HPD irq and state HIGH, which I ignore because it's not a change in > state. > > In the end I hacked up drm_connector_oob_hotplug_event() to allow me to > pass the HPD state and this solves my problem. I can now distinguish > between connect, disconnect and attention. > > Can you please help shed some light on what I might be missing? Originally I wanted an API that we could use to pass all the details that we have in the USB Type-C drivers (that would be the configuration and status) to the GPU drivers, but Hans was against that because, if I remember correctly, the OOB hotplug event may need to be delivered to the GPU drivers in some cases even when the connector is not USB Type-C connector, and he wanted a common API. Hans, please correct me if I got it wrong. I think that the GPU drivers need to handle USB Type-C connectors separately one way or the other, but maybe the notification from the connector can continue to be generic - not USB Type-C specific. Imre proposed that the GPU drivers should be able to query the DisplayPort configuration and status from the USB Type-C drivers instead of the USB Type-C drivers just dumping the information together with the notification about some event (so connection, disconnection or attention) like I originally proposed. Imre, please correct me if I misunderstood you :-). I'm fine with anything, but we do need improvement here as you guys can see. thanks, -- heikki
Re: [RFC] drm/msm/dp: Allow attaching a drm_panel
Hi,
On Thu, Oct 07, 2021 at 09:15:12AM -0700, Bjorn Andersson wrote:
> The one thing that I still don't understand though is, if the typec_mux
> is used by the typec controller to inform _the_ mux about the function
> to be used, what's up with the complexity in typec_mux_match()? This is
> what lead me to believe that typec_mux was enabling/disabling individual
> altmodes, rather just flipping the physical switch at the bottom.
Ah, typec_mux_match() is a mess. I'm sorry about that. I think most of
the code in that function is not used by anybody. If I remember
correctly, all that complexity is attempting to solve some
hypothetical corner case(s). Probable a case where we have multiple
muxes per port to deal with.
I think it would probable be best to clean the function to the bare
minimum by keeping only the parts that are actually used today
(attached).
thanks,
--
heikki
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index c8340de0ed495..44f168c9bd9bf 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -193,56 +193,15 @@ static int mux_fwnode_match(struct device *dev, const
void *fwnode)
static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
void *data)
{
- const struct typec_altmode_desc *desc = data;
struct device *dev;
- bool match;
- int nval;
- u16 *val;
- int ret;
- int i;
/*
-* Check has the identifier already been "consumed". If it
-* has, no need to do any extra connection identification.
+* The connection identifier will be needed with device graph (OF
graph).
+* Device graph is not supported by this code yet, so bailing out.
*/
- match = !id;
- if (match)
- goto find_mux;
-
- /* Accessory Mode muxes */
- if (!desc) {
- match = fwnode_property_present(fwnode, "accessory");
- if (match)
- goto find_mux;
- return NULL;
- }
-
- /* Alternate Mode muxes */
- nval = fwnode_property_count_u16(fwnode, "svid");
- if (nval <= 0)
- return NULL;
-
- val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
- if (!val)
- return ERR_PTR(-ENOMEM);
-
- ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval);
- if (ret < 0) {
- kfree(val);
- return ERR_PTR(ret);
- }
-
- for (i = 0; i < nval; i++) {
- match = val[i] == desc->svid;
- if (match) {
- kfree(val);
- goto find_mux;
- }
- }
- kfree(val);
- return NULL;
+ if (id)
+ return ERR_PTR(-ENOTSUPP);
-find_mux:
dev = class_find_device(_mux_class, NULL, fwnode,
mux_fwnode_match);
Re: [RFC] drm/msm/dp: Allow attaching a drm_panel
Hi guys, On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote: > (CC+ Heikki) > > Hi, > > On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson wrote: > > > > Hi, > > > > On Tue, Oct 5, 2021 at 7:27 PM Bjorn Andersson > > wrote: > > > > > > > > For reference, this is how I thought one is supposed to tie the Type-C > > > > > controller to the display driver: > > > > > https://lore.kernel.org/all/20211005022451.2037405-1-bjorn.anders...@linaro.org/ > > > > > > > > OK, so I looked at that a bit. Fair warning that I've never looked at > > > > the type C code before today so anything I say could be totally wrong! > > > > :-) > > > > > > > > ...but I _think_ you're abusing the "mux" API for this. I think a type > > > > C port can have exactly 1 mux, right? Right now you are claiming to be > > > > _the_ mux in the DP driver, but what about for other alt modes? If > > > > those wanted to be notified about similar things it would be > > > > impossible because you're already _the_ mux, right? > > > > > > > > > > I actually don't think so, because I acquire the typec_mux handle by the > > > means of: > > > > > > mux_desc.svid = USB_TYPEC_DP_SID; > > > mux_desc.mode = USB_TYPEC_DP_MODE; > > > alt_port->mux = fwnode_typec_mux_get(fwnode, _desc); > > > > Hrm, I guess I need to go find that code. Ah, I see it in your WIP > > tree, but not posted anywhere. :-P The only code I can see calling > > fwnode_typec_mux_get() is `drivers/platform/chrome/cros_ec_typec.c`. > > In that code it passes NULL for the mux_desc and I'm nearly certain > > that it just handles one "mux" per connector despite the fact that it > > handles lots of different types of alternate modes. That doesn't mean > > that the cros_ec implementation is correct / finalized, but it's a > > reference point. > > > > > > > And in the DisplayPort node I provide svid = /bits/ 16 <0xff01>; > > > > > > So I will be able to reference multiple different altmode > > > implementors using this scheme. > > > > OK, so I'm trying to grok this more. Let's see. > > > > I'm looking at ucsi_glink_probe() and looking at the matching dts in > > your WIP tree [1] in "sc8180x-lenovo-flex-5g.dts" OK, so: > > > > 1. It's looping once per _connector_ by looping with > > `device_for_each_child_node(dev, fwnode)`. > > > > 2. For each connector, it has exactly one `alt_port` structure. > > > > 3. For each `alt_port` structure it has exactly one `mux`. > > > > ...so currently with your WIP tree there is one "mux" per type C connector. > > > > > > Perhaps what you're saying, though, is that the UCSI code in your WIP > > tree can/should be changed to support more than one mux per port. Then > > I guess it would have logic figuring out what muxes to notify about > > which things? ...and I guess that would mean that it's currently a bug > > that the ucsi_altmode_enable_usb() notifies "the DP type C mux" about > > USB changes? > > > > > > > > I _think_ a mux is supposed to be something more like > > > > `drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates > > > > the type C framework we're looking at here). There the phy can do all > > > > the work of remuxing things / flipping orientation / etc. I don't > > > > think it's a requirement that every SoC be able to do this remuxing > > > > itself but (if memory serves) rk3399 implemented it so we didn't have > > > > to do it on the TCPC and could use a cheaper solution there. > > > > > > > > > > I'm afraid I don't see how this interacts with a display controller. > > > > This was actually kinda my point. ;-) Specifically I think > > `phy-rockchip-typec.c` is the thing that's supposed to be a "mux". I > > think your display controller isn't a mux. Yeah, it's handy that muxes > > get told about DP HPD status, but that doesn't mean it's the right > > abstraction for you to implement. In my mental model, it's the same as > > implementing your "i2c" controller with a "pinctrl" driver. :-P > > > > > > > It > > > seems more like it's the phy side of things, what we have split between > > > the Type-C controller and the QMP phy to set the pins in the right > > > state. > > > > > > > In any case, my point is that I think there is supposed to be a > > > > _single_ mux per port that handles reassigning pins and that's what > > > > this API is for. > > > > > > > > > > If that's the case things such as typec_mux_match() is just completely > > > backwards. > > > > Yeah, I have no explanation for typec_mux_match(). Let me see if I can > > lure some type C folks into this discussion. > > This aligns with the model I have in my mind (not that that is > necessarily the right one). > I took that matching code to be meant to handle cases where the > firmware doesn't explicitly > define a "mode-switch" for the port (and so we look at the SVIDs > listed in the Mux fwnode descriptor). > > The matcher code does suggest there could be a mux for each alternate > mode. But then, how does the > bus code know which mux to set [2] ? In that code,
Re: [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v3)
On Tue, May 11, 2021 at 10:05:26AM +0300, Heikki Krogerus wrote: > On Wed, May 05, 2021 at 06:24:07PM +0200, Hans de Goede wrote: > > Hi All, > > > > Here is v3 of my patchset making DP over Type-C work on devices where the > > Type-C controller does not drive the HPD pin on the GPU, but instead > > we need to forward HPD events from the Type-C controller to the DRM driver. > > These look good to me. I can also test these next week if needed. I'll > give my Tested-by tag after that if these haven't been taken by > anybody by that. It's almost weird to see console output from the Type-C connector on my good old GPD printed to an actual display :-) At least in my tests, the DP alt mode driver now calls drm_connector_oob_hotplug_event() only when it should. This is pretty cool! FWIW: Tested-by: Heikki Krogerus thanks, -- heikki
Re: [PATCH 0/8] drm + usb-type-c: Add support for out-of-band hotplug notification (v3)
On Wed, May 05, 2021 at 06:24:07PM +0200, Hans de Goede wrote: > Hi All, > > Here is v3 of my patchset making DP over Type-C work on devices where the > Type-C controller does not drive the HPD pin on the GPU, but instead > we need to forward HPD events from the Type-C controller to the DRM driver. These look good to me. I can also test these next week if needed. I'll give my Tested-by tag after that if these haven't been taken by anybody by that. thanks, -- heikki
Re: [PATCH 8/8] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
On Wed, May 05, 2021 at 06:24:15PM +0200, Hans de Goede wrote:
> Use the new drm_connector_oob_hotplug_event() functions to let drm/kms
> drivers know about DisplayPort over Type-C hotplug events.
>
> Signed-off-by: Hans de Goede
Reviewed-by: Heikki Krogerus
> ---
> Changes in v3:
> - Only call drm_connector_oob_hotplug_event() on hpd status bit change
> - Adjust for drm_connector_oob_hotplug_event() no longer having a data
> argument
>
> Changes in v2:
> - Add missing depends on DRM to TYPEC_DP_ALTMODE Kconfig entry
> ---
> drivers/usb/typec/altmodes/Kconfig | 1 +
> drivers/usb/typec/altmodes/displayport.c | 23 +++
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/usb/typec/altmodes/Kconfig
> b/drivers/usb/typec/altmodes/Kconfig
> index 60d375e9c3c7..1a6b5e872b0d 100644
> --- a/drivers/usb/typec/altmodes/Kconfig
> +++ b/drivers/usb/typec/altmodes/Kconfig
> @@ -4,6 +4,7 @@ menu "USB Type-C Alternate Mode drivers"
>
> config TYPEC_DP_ALTMODE
> tristate "DisplayPort Alternate Mode driver"
> + depends on DRM
> help
> DisplayPort USB Type-C Alternate Mode allows DisplayPort
> displays and adapters to be attached to the USB Type-C
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index aa669b9cf70e..c1d8c23baa39 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -11,8 +11,10 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> +#include
> #include "displayport.h"
>
> #define DP_HEADER(_dp, ver, cmd) (VDO((_dp)->alt->svid, 1, ver, cmd)
> \
> @@ -57,11 +59,13 @@ struct dp_altmode {
> struct typec_displayport_data data;
>
> enum dp_state state;
> + bool hpd;
>
> struct mutex lock; /* device lock */
> struct work_struct work;
> struct typec_altmode *alt;
> const struct typec_altmode *port;
> + struct fwnode_handle *connector_fwnode;
> };
>
> static int dp_altmode_notify(struct dp_altmode *dp)
> @@ -125,6 +129,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8
> con)
> static int dp_altmode_status_update(struct dp_altmode *dp)
> {
> bool configured = !!DP_CONF_GET_PIN_ASSIGN(dp->data.conf);
> + bool hpd = !!(dp->data.status & DP_STATUS_HPD_STATE);
> u8 con = DP_STATUS_CONNECTION(dp->data.status);
> int ret = 0;
>
> @@ -137,6 +142,11 @@ static int dp_altmode_status_update(struct dp_altmode
> *dp)
> ret = dp_altmode_configure(dp, con);
> if (!ret)
> dp->state = DP_STATE_CONFIGURE;
> + } else {
> + if (dp->hpd != hpd) {
> + drm_connector_oob_hotplug_event(dp->connector_fwnode);
> + dp->hpd = hpd;
> + }
> }
>
> return ret;
> @@ -512,6 +522,7 @@ static const struct attribute_group dp_altmode_group = {
> int dp_altmode_probe(struct typec_altmode *alt)
> {
> const struct typec_altmode *port = typec_altmode_get_partner(alt);
> + struct fwnode_handle *fwnode;
> struct dp_altmode *dp;
> int ret;
>
> @@ -540,6 +551,11 @@ int dp_altmode_probe(struct typec_altmode *alt)
> alt->desc = "DisplayPort";
> alt->ops = _altmode_ops;
>
> + fwnode = dev_fwnode(alt->dev.parent->parent); /* typec_port fwnode */
> + dp->connector_fwnode = fwnode_find_reference(fwnode, "displayport", 0);
> + if (IS_ERR(dp->connector_fwnode))
> + dp->connector_fwnode = NULL;
> +
> typec_altmode_set_drvdata(alt, dp);
>
> dp->state = DP_STATE_ENTER;
> @@ -555,6 +571,13 @@ void dp_altmode_remove(struct typec_altmode *alt)
>
> sysfs_remove_group(>dev.kobj, _altmode_group);
> cancel_work_sync(>work);
> +
> + if (dp->connector_fwnode) {
> + if (dp->hpd)
> + drm_connector_oob_hotplug_event(dp->connector_fwnode);
> +
> + fwnode_handle_put(dp->connector_fwnode);
> + }
> }
> EXPORT_SYMBOL_GPL(dp_altmode_remove);
>
> --
> 2.31.1
--
heikki
Re: [PATCH 7/8] usb: typec: altmodes/displayport: Make dp_altmode_notify() more generic
On Wed, May 05, 2021 at 06:24:14PM +0200, Hans de Goede wrote:
> Make dp_altmode_notify() handle the dp->data.conf == 0 case too,
> rather then having separate code-paths for this in various places
> which call it.
>
> Signed-off-by: Hans de Goede
Reviewed-by: Heikki Krogerus
> ---
> drivers/usb/typec/altmodes/displayport.c | 35 +---
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index b7f094435b00..aa669b9cf70e 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -66,10 +66,17 @@ struct dp_altmode {
>
> static int dp_altmode_notify(struct dp_altmode *dp)
> {
> - u8 state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
> + unsigned long conf;
> + u8 state;
> +
> + if (dp->data.conf) {
> + state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
> + conf = TYPEC_MODAL_STATE(state);
> + } else {
> + conf = TYPEC_STATE_USB;
> + }
>
> - return typec_altmode_notify(dp->alt, TYPEC_MODAL_STATE(state),
> ->data);
> + return typec_altmode_notify(dp->alt, conf, >data);
> }
>
> static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
> @@ -137,21 +144,10 @@ static int dp_altmode_status_update(struct dp_altmode
> *dp)
>
> static int dp_altmode_configured(struct dp_altmode *dp)
> {
> - int ret;
> -
> sysfs_notify(>alt->dev.kobj, "displayport", "configuration");
> -
> - if (!dp->data.conf)
> - return typec_altmode_notify(dp->alt, TYPEC_STATE_USB,
> - >data);
> -
> - ret = dp_altmode_notify(dp);
> - if (ret)
> - return ret;
> -
> sysfs_notify(>alt->dev.kobj, "displayport", "pin_assignment");
>
> - return 0;
> + return dp_altmode_notify(dp);
> }
>
> static int dp_altmode_configure_vdm(struct dp_altmode *dp, u32 conf)
> @@ -172,13 +168,8 @@ static int dp_altmode_configure_vdm(struct dp_altmode
> *dp, u32 conf)
> }
>
> ret = typec_altmode_vdm(dp->alt, header, , 2);
> - if (ret) {
> - if (DP_CONF_GET_PIN_ASSIGN(dp->data.conf))
> - dp_altmode_notify(dp);
> - else
> - typec_altmode_notify(dp->alt, TYPEC_STATE_USB,
> - >data);
> - }
> + if (ret)
> + dp_altmode_notify(dp);
>
> return ret;
> }
> --
> 2.31.1
--
heikki
Re: [PATCH 8/9] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
Hi Hans,
On Mon, May 03, 2021 at 05:46:46PM +0200, Hans de Goede wrote:
> Use the new drm_connector_find_by_fwnode() and
> drm_connector_oob_hotplug_event() functions to let drm/kms drivers
> know about DisplayPort over Type-C hotplug events.
>
> Signed-off-by: Hans de Goede
> ---
> Changes in v2:
> - Add missing depends on DRM to TYPEC_DP_ALTMODE Kconfig entry
> ---
> drivers/usb/typec/altmodes/Kconfig | 1 +
> drivers/usb/typec/altmodes/displayport.c | 40 +++-
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/altmodes/Kconfig
> b/drivers/usb/typec/altmodes/Kconfig
> index 60d375e9c3c7..1a6b5e872b0d 100644
> --- a/drivers/usb/typec/altmodes/Kconfig
> +++ b/drivers/usb/typec/altmodes/Kconfig
> @@ -4,6 +4,7 @@ menu "USB Type-C Alternate Mode drivers"
>
> config TYPEC_DP_ALTMODE
> tristate "DisplayPort Alternate Mode driver"
> + depends on DRM
> help
> DisplayPort USB Type-C Alternate Mode allows DisplayPort
> displays and adapters to be attached to the USB Type-C
> diff --git a/drivers/usb/typec/altmodes/displayport.c
> b/drivers/usb/typec/altmodes/displayport.c
> index aa669b9cf70e..f00dfc5c14b6 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -11,8 +11,10 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> +#include
> #include "displayport.h"
>
> #define DP_HEADER(_dp, ver, cmd) (VDO((_dp)->alt->svid, 1, ver, cmd)
> \
> @@ -62,12 +64,30 @@ struct dp_altmode {
> struct work_struct work;
> struct typec_altmode *alt;
> const struct typec_altmode *port;
> + struct fwnode_handle *connector_fwnode;
> };
>
> +static void dp_altmode_notify_connector(struct dp_altmode *dp)
> +{
> + struct drm_connector_oob_hotplug_event_data data = { };
> + u8 pin_assign = DP_CONF_GET_PIN_ASSIGN(dp->data.conf);
> +
> + data.connected = dp->data.status & DP_STATUS_HPD_STATE;
> + data.orientation = typec_altmode_get_orientation(dp->alt);
> +
> + if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK)
> + data.dp_lanes = 4;
> + else if (pin_assign & DP_PIN_ASSIGN_MULTI_FUNC_MASK)
> + data.dp_lanes = 2;
> +
> + drm_connector_oob_hotplug_event(dp->connector_fwnode, );
> +}
> +
> static int dp_altmode_notify(struct dp_altmode *dp)
> {
> unsigned long conf;
> u8 state;
> + int ret;
>
> if (dp->data.conf) {
> state = get_count_order(DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
> @@ -76,7 +96,12 @@ static int dp_altmode_notify(struct dp_altmode *dp)
> conf = TYPEC_STATE_USB;
> }
>
> - return typec_altmode_notify(dp->alt, conf, >data);
> + ret = typec_altmode_notify(dp->alt, conf, >data);
> + if (ret)
> + return ret;
> +
> + dp_altmode_notify_connector(dp);
That is now going to be always called after configuration, right? The
HPD is not necessarily issued at that point.
I think that should be called from dp_altmode_status_update(), and
probable only if there really is HPD IRQ or HPD State changes... I
think?
> + return 0;
> }
thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification (v2)
On Tue, May 04, 2021 at 05:35:49PM +0200, Hans de Goede wrote:
> Hi,
>
> On 5/4/21 5:10 PM, Heikki Krogerus wrote:
> >> +/**
> >> + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> >> connector
> >> + * @connector: connector to report the event on
> >> + * @data: data related to the event
> >> + *
> >> + * On some hardware a hotplug event notification may come from outside
> >> the display
> >> + * driver / device. An example of this is some USB Type-C setups where
> >> the hardware
> >> + * muxes the DisplayPort data and aux-lines but does not pass the altmode
> >> HPD
> >> + * status bit to the GPU's DP HPD pin.
> >> + *
> >> + * This function can be used to report these out-of-band events after
> >> obtaining
> >> + * a drm_connector reference through calling
> >> drm_connector_find_by_fwnode().
> >> + */
> >> +void drm_connector_oob_hotplug_event(struct fwnode_handle
> >> *connector_fwnode,
> >> + struct
> >> drm_connector_oob_hotplug_event_data *data)
> >> +{
> >> + struct drm_connector *connector;
> >> +
> >> + connector = drm_connector_find_by_fwnode(connector_fwnode);
> >> + if (IS_ERR(connector))
> >> + return;
> >> +
> >> + if (connector->funcs->oob_hotplug_event)
> >> + connector->funcs->oob_hotplug_event(connector, data);
> >> +
> >> + drm_connector_put(connector);
> >> +}
> >> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event);
> >
> > So it does looks like the "data" parameter is not needed at all:
>
> Well Imre did indicate that having the number of lanes is useful, so
> for the next version I'll drop the orientation but I plan to keep
> the number of lanes if that is ok with you.
>
> Not having passing along this info was one of the reasons why my
> previous attempt at this was nacked, so dropping it all together
> feels wrong.
If you need to pass any information to the function, then you need to
pass all the information that we have. Don't start with abstraction.
First create a dedicated API, and then, only if we really have another
user for it, we can add an abstraction layer that both users can use.
All cases are going to be different. We don't know how the abstraction
/ generalisation should look like before we have at least two real
users (ideally more than two, actually). Right now we can not even say
for sure that generalising the API is even possible.
I would not make a huge deal out of this unless I wasn't myself being
told by guys like Greg KH in the past to drop my attempts to
"generalize" things from the beginning when I only had a single user.
By doing so you'll not only force all ends, the source of the data
(the typec drivers in this case) as well as the consumer (i915), to be
always changed together, it will also confuse things. We are not
always going to be able to tell the lane count for example like we can
with USB Type-C, so i915 can't really rely on that information.
Right now we also don't know what exact details i915 (or what ever GPU
driver) needs. We can only say for sure that some details are going to
be needed. Trying to guess and cherry-pick the details now does not
makes sense because of that reason too.
So just make this API USB Type-C DP Alt Mode specific API first, and
supply it everything we have.
thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/9] drm + usb-type-c: Add support for out-of-band hotplug notification (v2)
On Mon, May 03, 2021 at 05:46:38PM +0200, Hans de Goede wrote: > Hi All, > > Here is v2 of my work on making DP over Type-C work on devices where the > Type-C controller does not drive the HPD pin on the GPU, but instead > we need to forward HPD events from the Type-C controller to the DRM driver. > > Changes in v2: > - Replace the bogus "drm/connector: Make the drm_sysfs connector->kdev > device hold a reference to the connector" patch with: > "drm/connector: Give connector sysfs devices there own device_type" > the new patch is a dep for patch 2/9 see the patches > > - Stop using a class-dev-iter, instead at a global connector list > to drm_connector.c and use that to find the connector by the fwnode, > similar to how we already do this in drm_panel.c and drm_bridge.c > > - Make drm_connector_oob_hotplug_event() take a fwnode pointer as > argument, rather then a drm_connector pointer and let it do the > lookup itself. This allows making drm_connector_find_by_fwnode() a > drm-internal function and avoids code outside the drm subsystem > potentially holding on the a drm_connector reference for a longer > period. > > This series not only touches drm subsys files but it also touches > drivers/usb/typec/altmodes/typec_displayport.c, that file usually > does not see a whole lot of changes. So I believe it would be best > to just merge the entire series through drm-misc, Assuming we can > get an ack from Greg for merging the typec_displayport.c changes > this way. > > ### > > As already mentioned in the v1 cover-letter this series replaces > a previous attempt from quite some time ago. > For anyone interested here are the old (2019!) patches for this: > > https://patchwork.freedesktop.org/patch/288491/ > https://patchwork.freedesktop.org/patch/288493/ > https://patchwork.freedesktop.org/patch/288495/ > > Last time I posted this the biggest change requested was for more info to > be included in the event send to the DRM-subsystem, specifically sending > the following info was requested: > > 1. Which DP connector on the GPU the event is for > 2. How many lanes are available > 3. Connector orientation > > This series is basically an entirely new approach, which no longer > uses the notifier framework at all. Instead the Type-C code looksup > a connector based on a fwnode (this was suggested by Heikki Krogerus) > and then calls a new oob_hotplug_event drm_connector_func directly > on the connector, passing the requested info as argument. > > Info such as the orientation and the number of dp-lanes is now passed > to the drm_connector_oob_hotplug_event() function as requested in the > review of the old code, but nothing is done with it for now. > Using this info falls well outside of my knowledge of the i915 driver > so this is left to a follow-up patch (I will be available to test > patches for this). Thanks for taking care of these! It's really great that you spent the time to do this series. I'm already thinking about what we can add after these are in. I think support for re-configuration, so support for changing the pin-configuration in runtime is going to be needed soon after these. But first things first (sorry, I'm getting ahead of myself). thanks, -- heikki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification (v2)
> +/**
> + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to
> connector
> + * @connector: connector to report the event on
> + * @data: data related to the event
> + *
> + * On some hardware a hotplug event notification may come from outside the
> display
> + * driver / device. An example of this is some USB Type-C setups where the
> hardware
> + * muxes the DisplayPort data and aux-lines but does not pass the altmode HPD
> + * status bit to the GPU's DP HPD pin.
> + *
> + * This function can be used to report these out-of-band events after
> obtaining
> + * a drm_connector reference through calling drm_connector_find_by_fwnode().
> + */
> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
> + struct
> drm_connector_oob_hotplug_event_data *data)
> +{
> + struct drm_connector *connector;
> +
> + connector = drm_connector_find_by_fwnode(connector_fwnode);
> + if (IS_ERR(connector))
> + return;
> +
> + if (connector->funcs->oob_hotplug_event)
> + connector->funcs->oob_hotplug_event(connector, data);
> +
> + drm_connector_put(connector);
> +}
> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event);
So it does looks like the "data" parameter is not needed at all:
void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
{
struct drm_connector *connector;
connector = drm_connector_find_by_fwnode(connector_fwnode);
if (IS_ERR(connector))
return;
if (connector->funcs->oob_hotplug_event)
connector->funcs->oob_hotplug_event(connector);
drm_connector_put(connector);
}
thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries
Hi Andy,
> > +/* NOTE: The connector order must be final before this is called. */
> > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915)
> > +{
> > + struct drm_connector_list_iter conn_iter;
> > + struct drm_device *drm_dev = >drm;
> > + struct device *kdev = _dev->pdev->dev;
> > + struct fwnode_handle *fwnode = NULL;
> > + struct drm_connector *connector;
> > + struct acpi_device *adev;
> > +
> > + drm_connector_list_iter_begin(drm_dev, _iter);
> > + drm_for_each_connector_iter(connector, _iter) {
> > + /* Always getting the next, even when the last was not
> > used. */
> > + fwnode = device_get_next_child_node(kdev, fwnode);
> > + if (!fwnode)
> > + break;
>
> Who is dropping reference counting on fwnode ?
>
> I’m in the middle of a pile of fixes for fwnode refcounting when
> for_each_child or get_next_child is used. So, please double check you drop
> a reference.
Sorry Andy. This patch is from time before the software nodes
implementation of the get_next_child callback handled the ref counting
properly.
Br,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification
On Mon, May 03, 2021 at 04:35:29PM +0200, Hans de Goede wrote:
> Hi,
>
> On 5/3/21 10:00 AM, Heikki Krogerus wrote:
> > Hi Hans,
> >
> > On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
> >> +/**
> >> + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
> >> + *
> >> + * Contains data about out-of-band hotplug events, signalled through
> >> + * drm_connector_oob_hotplug_event().
> >> + */
> >> +struct drm_connector_oob_hotplug_event_data {
> >> + /**
> >> + * @connected: New connected status for the connector.
> >> + */
> >> + bool connected;
> >> + /**
> >> + * @dp_lanes: Number of available displayport lanes, 0 if unknown.
> >> + */
> >> + int dp_lanes;
> >> + /**
> >> + * @orientation: Connector orientation.
> >> + */
> >> + enum typec_orientation orientation;
> >> +};
> >
> > I don't think the orientation is relevant. It will always be "normal"
> > from DP PoW after muxing, no?
>
> That is what I thought to, but during the discussion of my previous attempt
> at this one of the i915 devs mentioned that in some cases the muxes manage
> to swap the lane order when the connector upside-down and at least the
> Intel GPUs can correct for this on the GPU side, so they asked for this
> info to be included.
>
> > I'm also not sure those deatils are enough in the long run. Based on
> > what I've understood from our graphics team guys, for example knowing
> > if multi-function is preferred may be important in some cases.
>
> The current data being passed is just intended as a starting point,
> this is purely a kernel internal API so we can easily add more
> data to the struct. As I mentioned in the cover-letter the current
> oob_hotplug handler which the i915 patch adds to the i915 driver does
> not actually do anything with the data. ATM it is purely there to
> demonstrate that the ability to pass relevant data is there now
> (which was an issue with the previous attempt). I believe the current
> code is fine as a PoC of "pass event data" once GPU drivers actually
> start doing something with the data we can extend or outright replace
> it without issues.
Ah, if there is nothing using that information yet, then just don't
pass it at all for now. As you said, it's kernel internal API, we can
change it later if needed.
> > All of that, and more, is already available in the Configuration VDO
> > Status VDO that the we have negotiated with the DP partner. Both those
> > VDOs are part of struct typec_displayport_data. I think we should
> > simply supply that structure to the DRM code instead of picking those
> > details out of it...
>
> I'm not sure I like the idea of passing the raw VDO, but if the
> DRM folks think that would be useful we can certainly add it.
Why are you against passing all the data that we have? What is the
benefit in picking only certain details out of an object that has a
standard format, and constructing a customised object for those
details instead?
thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification
Hi Hans,
On Wed, Apr 28, 2021 at 11:52:52PM +0200, Hans de Goede wrote:
> +/**
> + * struct drm_connector_oob_hotplug_event_data: OOB hotplug event data
> + *
> + * Contains data about out-of-band hotplug events, signalled through
> + * drm_connector_oob_hotplug_event().
> + */
> +struct drm_connector_oob_hotplug_event_data {
> + /**
> + * @connected: New connected status for the connector.
> + */
> + bool connected;
> + /**
> + * @dp_lanes: Number of available displayport lanes, 0 if unknown.
> + */
> + int dp_lanes;
> + /**
> + * @orientation: Connector orientation.
> + */
> + enum typec_orientation orientation;
> +};
I don't think the orientation is relevant. It will always be "normal"
from DP PoW after muxing, no?
I'm also not sure those deatils are enough in the long run. Based on
what I've understood from our graphics team guys, for example knowing
if multi-function is preferred may be important in some cases.
+Imre.
All of that, and more, is already available in the Configuration VDO
Status VDO that the we have negotiated with the DP partner. Both those
VDOs are part of struct typec_displayport_data. I think we should
simply supply that structure to the DRM code instead of picking those
details out of it...
> /**
> * struct drm_tv_connector_state - TV connector related states
> * @subconnector: selected subconnector
> @@ -1110,6 +1132,15 @@ struct drm_connector_funcs {
>*/
> void (*atomic_print_state)(struct drm_printer *p,
> const struct drm_connector_state *state);
> +
> + /**
> + * @oob_hotplug_event:
> + *
> + * This will get called when a hotplug-event for a drm-connector
> + * has been received from a source outside the display driver / device.
> + */
> + void (*oob_hotplug_event)(struct drm_connector *connector,
> + struct drm_connector_oob_hotplug_event_data
> *data);
So I would not try to generalise this like that. This callback should
be USB Type-C DP altmode specific:
void (*oob_hotplug_event)(struct drm_connector *connector,
struct typec_displayport_data *data);
Or like this if the orientation can really be reversed after muxing:
void (*oob_hotplug_event)(struct drm_connector *connector,
struct typec_altmode *altmode,
struct typec_displayport_data *data);
You can now check the orientation separately with
typec_altmode_get_orientation() if necessary.
thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 06/11] drm/bridge: ti-tfp410: switch to using fwnode_gpiod_get_index()
On Sat, Sep 21, 2019 at 02:12:28AM +0300, Laurent Pinchart wrote: > Hi Dmitry, > > (CC'ing Heikki as the original author of software nodes support) > > Thank you for the patch. > > On Wed, Sep 11, 2019 at 12:52:10AM -0700, Dmitry Torokhov wrote: > > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use > > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), bit > > s/bit/but/ > > > works with arbitrary firmware node. > > > > Signed-off-by: Dmitry Torokhov > > Reviewed-by: Laurent Pinchart > > On a side note, as I'm not very familiar with software nodes, I tried to > see how they are to be used, and it seems they are completely > undocumented :-( Heikki, is this something that could be fixed ? OK. I'll start writing API documentation for it. thanks, -- heikki
[PATCH 2/2] drm/i915: Associate ACPI connector nodes with connector entries
On Intel platforms we know that the ACPI connector device
node order will follow the order the driver (i915) decides.
The decision is made using the custom Intel ACPI OpRegion
(intel_opregion.c), though the driver does not actually know
that the values it sends to ACPI there are used for
associating a device node for the connectors, and assigning
address for them.
In reality that custom Intel ACPI OpRegion actually violates
ACPI specification (we supply dynamic information to objects
that are defined static, for example _ADR), however, it
makes assigning correct connector node for a connector entry
straightforward (it's one-on-one mapping).
Signed-off-by: Heikki Krogerus
---
drivers/gpu/drm/i915/intel_display.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 008560ef4db0..27aea2ef80ac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15526,6 +15526,45 @@ static int intel_initial_commit(struct drm_device *dev)
return ret;
}
+/* NOTE: The connector order must be final before this is called. */
+static void intel_assign_connector_fwnodes(struct drm_device *dev)
+{
+ struct drm_connector_list_iter conn_iter;
+ struct device *kdev = >pdev->dev;
+ struct fwnode_handle *fwnode = NULL;
+ struct drm_connector *connector;
+ struct acpi_device *adev;
+
+ drm_connector_list_iter_begin(dev, _iter);
+ drm_for_each_connector_iter(connector, _iter) {
+ /* Always getting the next, even when the last was not used. */
+ fwnode = device_get_next_child_node(kdev, fwnode);
+ if (!fwnode)
+ break;
+
+ switch (connector->connector_type) {
+ case DRM_MODE_CONNECTOR_LVDS:
+ case DRM_MODE_CONNECTOR_eDP:
+ case DRM_MODE_CONNECTOR_DSI:
+ /*
+* Integrated displays have a specific address 0x1f on
+* most Intel platforms, but not on all of them.
+*/
+ adev = acpi_find_child_device(ACPI_COMPANION(kdev),
+ 0x1f, 0);
+ if (adev) {
+ connector->fwnode = acpi_fwnode_handle(adev);
+ break;
+ }
+ /* fallthrough */
+ default:
+ connector->fwnode = fwnode;
+ break;
+ }
+ }
+ drm_connector_list_iter_end(_iter);
+}
+
int intel_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -15630,6 +15669,7 @@ int intel_modeset_init(struct drm_device *dev)
drm_modeset_lock_all(dev);
intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
+ intel_assign_connector_fwnodes(dev);
drm_modeset_unlock_all(dev);
for_each_intel_crtc(dev, crtc) {
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm: connector firmware nodes
Hi, If the system firmware supplies device nodes representing the display connectors on integrated video hardware, those nodes should probable always be associated with the display connector entries (struct drm_connector). With the USB Type-C DisplayPort alt mode, we will need a way to inform the correct drm connector entry about HPD IRQ, lane counts and other details. In ACPI (and most likely in DT too) the device node representing a DisplayPort behind USB Type-C connector should have a reference to the device node representing the USB Type-C connector (or vise versa). Once we have associated the DP connector device nodes with the drm connector entries, we can use those references to find the correct drm connector that the information the USB Type-C drivers are sending is meant for. Because I think the connector firmware nodes should be associated with the connector entries in any case (those nodes do seem to be supplying the connectors all kinds of resources, not only references to other components), I'm proposing this now instead of waiting for the USB Type-C patches. thanks, Heikki Krogerus (2): drm: Add fwnode member to the struct drm_connector drm/i915: Associate ACPI connector nodes with connector entries drivers/gpu/drm/drm_sysfs.c | 49 +++- drivers/gpu/drm/i915/intel_display.c | 40 +++ include/drm/drm_connector.h | 2 ++ 3 files changed, 76 insertions(+), 15 deletions(-) -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm: Add fwnode member to the struct drm_connector
If the system firmware supplies nodes that represent the connectors,
they need to be associated with the connector entries.
ACPI tables at least always supply a device node for every connector
on integrated video hardware - this is explained in ACPI
specification's ch. "Appendix B Video Extension". Many drivers appear
to already deal with those connector firmware nodes "indirectly"
in order to support custom Operation Regions, _DSM (device specific
method) and access other resources the nodes supply for the
connectors.
This commit will only add the fwnode member. It's first up to the
drivers to assign and take advantage of it. For convenience, the nodes
are assigned to the device entries that are used for exposing the
connectors to the user space via sysfs. That makes it possible to see
the link between the connector and its node also from user space. In
case of ACPI, the connector's sysfs directory will have a symlink
named "firmware_node" pointing to the node object directory in sysfs
if a node is associated with the connector.
Signed-off-by: Heikki Krogerus
---
drivers/gpu/drm/drm_sysfs.c | 49 +
include/drm/drm_connector.h | 2 ++
2 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ecb7b33002bb..7667939ef1c0 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -264,26 +264,50 @@ static const struct attribute_group
*connector_dev_groups[] = {
NULL
};
+static void drm_sysfs_release(struct device *dev)
+{
+ kfree(dev);
+}
+
int drm_sysfs_connector_add(struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
+ struct device *kdev;
+ int ret;
if (connector->kdev)
return 0;
- connector->kdev =
- device_create_with_groups(drm_class, dev->primary->kdev, 0,
- connector, connector_dev_groups,
- "card%d-%s", dev->primary->index,
- connector->name);
- DRM_DEBUG("adding \"%s\" to sysfs\n",
- connector->name);
+ kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
+ if (!kdev)
+ return -ENOMEM;
- if (IS_ERR(connector->kdev)) {
- DRM_ERROR("failed to register connector device: %ld\n",
PTR_ERR(connector->kdev));
- return PTR_ERR(connector->kdev);
+ device_initialize(kdev);
+ kdev->class = drm_class;
+ kdev->parent = dev->primary->kdev;
+ kdev->fwnode = connector->fwnode;
+ kdev->groups = connector_dev_groups;
+ kdev->release = drm_sysfs_release;
+ dev_set_drvdata(kdev, connector);
+
+ ret = dev_set_name(kdev, "card%d-%s", dev->primary->index,
+ connector->name);
+ if (ret) {
+ kfree(kdev);
+ return ret;
+ }
+
+ DRM_DEBUG("adding \"%s\" to sysfs\n", connector->name);
+
+ ret = device_add(kdev);
+ if (ret) {
+ DRM_ERROR("failed to register connector device: %d\n", ret);
+ put_device(kdev);
+ return ret;
}
+ connector->kdev = kdev;
+
/* Let userspace know we have a new connector */
drm_sysfs_hotplug_event(dev);
@@ -330,11 +354,6 @@ void drm_sysfs_hotplug_event(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_sysfs_hotplug_event);
-static void drm_sysfs_release(struct device *dev)
-{
- kfree(dev);
-}
-
struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
{
const char *minor_str;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c8061992d6cb..b8977c4eab14 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -886,6 +886,8 @@ struct drm_connector {
struct device *kdev;
/** @attr: sysfs attributes */
struct device_attribute *attr;
+ /** @fwnode: associated device node supplied by platform firmware */
+ struct fwnode_handle *fwnode;
/**
* @head:
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Thu, Feb 28, 2019 at 05:54:21PM +0100, Hans de Goede wrote: > Hi Heikki, > > On 28-02-19 15:47, Heikki Krogerus wrote: > > Hi Hans, > > > > On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 28-02-19 10:15, Heikki Krogerus wrote: > > > > > > > I've been thinking about this... Do we actually need to link the > > > > correct drm_connector to the Type-C connector? Perhaps we can make > > > > this work by just linking the GFX device to the Type-C connector. > > > > > > What use is it to the kms driver if it gets an event there is a DP > > > hotplug with x lanes and orientation foo, if we are not telling it > > > on which DP port it is ? kms devices already have multiple DP ports > > > and more then one could be hooked-up to type-c connectors. > > > > I was looking at this series. You walk trough every DP port in the > > system when the DP alt mode driver broadcasts the event, but maybe > > that's different. Never mind. > > Right, my "simple / naive" solution simply tells the kms driver to > check all DP ports for connection state changes, similar to how > running "xrandr" under Xorg causes the kms driver to re-check the > connection status of all ports. Actually running xrandr under Xorg > after plugging in the cable, is how I did my initial DP over Type-C > testing on the GPD win. > > But once we start passing extra-info, I believe the kms driver needs > to know to which connector that info belongs. > > > > > > > Well, I don't think we can deny the GPU driver (in this case) the > > > > information that we have and that is relevant to it, just because it > > > > seems difficult to deliver that information to the right location. > > > > > > Right, but this does not require a tight-coupling. My original > > > proposal can do this if we pass a data struct with an identifier > > > for the DP port for which the event is to the notifier. I suggest using > > > a string for identifier, something like: ":00:02.0/DP-1" this > > > event struct could then also contain all the info we want to pass. > > > > I do agree that we should not tie the objects (device entries) > > representing these components in kernel together, but I assume that we > > agree now that the connection between the two - the USB Type-C > > connector and the DisplayPort - must be described somewhere, either in > > firmware or build-in? So I guess we are talking implementation details > > here, right? > > Right. > > > If that is the case... > > > > That string identifier you proposed would basically provide the > > details about the connection, so if we know those details, we might as > > well use "normal" ways to describe the connection and just extract > > them in runtime in the function that our DP alt mode driver calls. I > > think the connection has to be defined in i915 on CHT in any case. > > Interesting, I think the connection should be described in the fwnode > for the fusb302 device for the CHT/GPD win case. Specifically I think > this fits well as a property of the dp altmode. OK, you are correct. I was stupidly still thinking about the driver loading order, but the order does not matter. > > The DP alt mode driver should definitely not need to pass anything > > else to the notifier other than handle to itself (actually, handle > > straight to the port device would be better) as an identifier. The > > notifier function needs to be the one that determines the actual > > connection using that handle. Even if the target DP is described using > > a string like you propose, then that string has to come from > > somewhere, most likely from a device property. The notifier function > > can just as well extract it, we don't need to pass it separately. > > > > Here's my suggestion for function prototype: > > > > int drm_typec_dp_notification(struct device *typec_port_dev, > >struct typec_displayport_data *data); > > How about instead of the port_dev we pass in the altmode object and > we have a method to get the fwnode for the altmode? Then the > drm_typec_dp_notification() function can get info from that fwnode > to implement the connection finding you describe below: We can pass the altmode object, np, but let's not decide which fwnode we'll ultimately use. I'm still leaning towards the connector node. > > So that function finds the connection between typec_port_dev and the > > correct DP in runtime,
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote: > Hi, > > On 27-02-19 12:16, Jani Nikula wrote: > > On Wed, 27 Feb 2019, Heikki Krogerus > > wrote: > > > One thing that this series does not consider is the DP lane count > > > problem. The GPU drivers (i915 in this case) does not know is four, > > > two or one DP lanes in use. > > > > Also, orientation. > > The orientation should simply be correct with Type-C over DP. The mux / > orientation-switch used will take care of (physically) swapping the > lanes if the connector is inserted upside-down. > > > > I guess that is not a critical issue since there is a workaround (I > > > think) where the driver basically does trial and error, but ideally we > > > should be able to tell i915 also the pin assignment that was > > > negotiated with the partner device so it knows the DP lane count. > > > > Yeah, if the information is there, we'd like to know. > > So far machines actually using a setup where the kernel does the > USB-PD / Type-C negotiation rather then this being handled in firmware > in say the Alpine Ridge controller, are very rare. > > AFAIK in the Alpine Ridge controller case, there is no communication > with the i915 driver, the only thing the Alpine Ridge controller does > which the everything done in the kernel approach does not, is that > it actually has a pin connected to the HDP pin of the displayport in > question. But that just lets the i915 driver know about hotplug-events, > not how many lanes are used. > > Currently I'm aware of only 2 x86 models which actually need the > hotplug event propagation from the Type-C subsystem to the i915 driver. > Do we really want to come up with a much more complex solution to > optimize for this corner case, while the much more common case > (Alpine Ridge controller) does not provide this info to the i915 driver? The HPD signal is often handled with a GPIO on Intel Platforms. Either the PD controller or EC controller, after receiving the Attention message, triggers the GPIO. On some systems though that GPIO method could not be used, so instead a side channel communication is used: the GFX device (so i915 driver) receives a specific custom interrupt. Unfortunately it now appears that there may be products coming where using the GPIO is not going to be possible, and also side channel communication is going to be out of the question. I've started an internal discussion inside Intel about the possibility to extend the UCSI specification to be able to support that kind of products. Alpine Ridge uses TI's Power Delivery controllers. The platforms that have Alpine Ridge TBT controller(s) often expose the USB Type-C connectors on them to the OS via UCSI, however, it appears the Alpine Ridge actually allows direct access to the PD controllers from the OS. That would mean we can supply the same information to the GPU drivers that we will deliver on CHT also on every platform that uses Alpine Ridge. > To give some idea of the complexity, first of all we need some > mechanism to let the kernel know which drm_connector is connected > to which Type-C port. For the 2 models I know if which need this, this > info is not available and we would need to hardcode it in the kernel > based on e.g. DMI matching. I've been thinking about this... Do we actually need to link the correct drm_connector to the Type-C connector? Perhaps we can make this work by just linking the GFX device to the Type-C connector. > Then once we have this link, we need to start thinking about probe > ordering. Likely we need the typec framework to find the drm_connector, > since the typec-partner device is only created when there actually is > a DP capable "dongle" connected, making doing it the other way around > tricky. Then the typec-partner device needs to get a reference or some > such to make sure the drm_connector does not fgo away during the lifetime > of the typec-partner device. No! We must not link the partner device with anything other than the Type-C connector. We link the USB Type-C connector to the DisplayPort, and we link the USB Type-C connector to the partner. The Type-C connector is the proxy here. > I would really like to avoid this, so if we want to send more info to > the i915 driver, I suggest we create some event struct for this which > gets passed to the notifier, this could include a string to > describe the DP connector which the Type-C connector is connected to > when the mux is set to DP mode, e.g. "i915/DP-1" should be unique > or probably better, use the PCI device name, so: ":00:02.0/DP-1" > > Then we still have a loose coupling avoiding lifetime issues, while > the receiving drm driver can check which connector the event is
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Thu, Feb 28, 2019 at 12:24:25PM +0100, Hans de Goede wrote: > Hi, > > On 28-02-19 10:15, Heikki Krogerus wrote: > > On Wed, Feb 27, 2019 at 04:45:32PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 27-02-19 12:16, Jani Nikula wrote: > > > > On Wed, 27 Feb 2019, Heikki Krogerus > > > > wrote: > > > > > One thing that this series does not consider is the DP lane count > > > > > problem. The GPU drivers (i915 in this case) does not know is four, > > > > > two or one DP lanes in use. > > > > > > > > Also, orientation. > > > > > > The orientation should simply be correct with Type-C over DP. The mux / > > > orientation-switch used will take care of (physically) swapping the > > > lanes if the connector is inserted upside-down. > > > > > > > > I guess that is not a critical issue since there is a workaround (I > > > > > think) where the driver basically does trial and error, but ideally we > > > > > should be able to tell i915 also the pin assignment that was > > > > > negotiated with the partner device so it knows the DP lane count. > > > > > > > > Yeah, if the information is there, we'd like to know. > > > > > > So far machines actually using a setup where the kernel does the > > > USB-PD / Type-C negotiation rather then this being handled in firmware > > > in say the Alpine Ridge controller, are very rare. > > > > > > AFAIK in the Alpine Ridge controller case, there is no communication > > > with the i915 driver, the only thing the Alpine Ridge controller does > > > which the everything done in the kernel approach does not, is that > > > it actually has a pin connected to the HDP pin of the displayport in > > > question. But that just lets the i915 driver know about hotplug-events, > > > not how many lanes are used. > > > > > > Currently I'm aware of only 2 x86 models which actually need the > > > hotplug event propagation from the Type-C subsystem to the i915 driver. > > > Do we really want to come up with a much more complex solution to > > > optimize for this corner case, while the much more common case > > > (Alpine Ridge controller) does not provide this info to the i915 driver? > > > > The HPD signal is often handled with a GPIO on Intel Platforms. Either > > the PD controller or EC controller, after receiving the Attention > > message, triggers the GPIO. On some systems though that GPIO method > > could not be used, so instead a side channel communication is used: > > the GFX device (so i915 driver) receives a specific custom interrupt. > > > > Unfortunately it now appears that there may be products coming where > > using the GPIO is not going to be possible, and also side channel > > communication is going to be out of the question. I've started an > > internal discussion inside Intel about the possibility to extend the > > UCSI specification to be able to support that kind of products. > > > > Alpine Ridge uses TI's Power Delivery controllers. The platforms that > > have Alpine Ridge TBT controller(s) often expose the USB Type-C > > connectors on them to the OS via UCSI, however, it appears the Alpine > > Ridge actually allows direct access to the PD controllers from the OS. > > That would mean we can supply the same information to the GPU drivers > > that we will deliver on CHT also on every platform that uses Alpine > > Ridge. > > Ok. > > > > To give some idea of the complexity, first of all we need some > > > mechanism to let the kernel know which drm_connector is connected > > > to which Type-C port. For the 2 models I know if which need this, this > > > info is not available and we would need to hardcode it in the kernel > > > based on e.g. DMI matching. > > > > I've been thinking about this... Do we actually need to link the > > correct drm_connector to the Type-C connector? Perhaps we can make > > this work by just linking the GFX device to the Type-C connector. > > What use is it to the kms driver if it gets an event there is a DP > hotplug with x lanes and orientation foo, if we are not telling it > on which DP port it is ? kms devices already have multiple DP ports > and more then one could be hooked-up to type-c connectors. I was looking at this series. You walk trough every DP port in the system when the DP alt mode driver broadcasts the event, but maybe that's different. Never mind. > > > Then once we have this link, we
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, Feb 27, 2019 at 01:16:27PM +0200, Jani Nikula wrote: > On Wed, 27 Feb 2019, Heikki Krogerus wrote: > > One thing that this series does not consider is the DP lane count > > problem. The GPU drivers (i915 in this case) does not know is four, > > two or one DP lanes in use. > > Also, orientation. > > > I guess that is not a critical issue since there is a workaround (I > > think) where the driver basically does trial and error, but ideally we > > should be able to tell i915 also the pin assignment that was > > negotiated with the partner device so it knows the DP lane count. > > Yeah, if the information is there, we'd like to know. With the > orientation, there's a worst case of sixth attempt of finding out > there's just one lane in a certain orientation. Couple that with link > rate selection (did it not work because too high link rate or because > the lanes are just not there?) we get pretty confused about what we > should try. The orientation is also considered in the alt mode API. We have a function for that typec_altmode_get_orientation(), but it of course requires that the caller has a handle to the alt mode object. So basically we would again need tight coupling between the DP connector and USB Type-C connector. Hans, I'm not so sure we should, or can, rule out the "tight coupling" option after all. thanks, -- heikki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Mon, Feb 25, 2019 at 02:20:34PM +0100, Hans de Goede wrote: > Hi All, > > On some Cherry Trail devices, DisplayPort over Type-C is supported through > a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed > datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this > case does the PD/alt-mode negotiation itself, rather then everything being > handled in firmware. > > So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch > to DP mode and sets the mux accordingly. In this setup the HPD pin is not > connected, so the i915 driver needs to respond to a software event and scan > the DP port for changes manually. > > Thanks to Heikki's great work on the DisplayPort altmode support in the > typec subsys, we now correctly tell the dongle to switch to DP altmode > and we correctly set the mux and orientation switches to connect the > DP lines to the Type-C connector. > > This just leaves sending an out-of-band hotplug event from the Type-C > subsystem to the i915 driver and then we've fully working DP over Type-C > on these devices. > > This series implements this. The first patch adds a generic mechanism > for oob hotplug events to be send to the drm subsys, the second patch > adds support for this mechanism to the i915 driver and the third patch > makes the typec displayport_altmode driver send these events. > > The commit message of the first patch explains why I've chosen to things > the way these patches do them. One thing that this series does not consider is the DP lane count problem. The GPU drivers (i915 in this case) does not know is four, two or one DP lanes in use. I guess that is not a critical issue since there is a workaround (I think) where the driver basically does trial and error, but ideally we should be able to tell i915 also the pin assignment that was negotiated with the partner device so it knows the DP lane count. My original idea was that we pass that struct typec_displayport_data as payload in the event. From the Configuration VDO the GPU drivers can then see the negotiated DP pin assignment and determine the lane count. thanks, -- heikki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote: > Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load > drm/kms drivers know about DisplayPort over Type-C hotplug events. > > Signed-off-by: Hans de Goede I'm OK with this. I'll wait for the v2 and see if I can test these. thanks, -- heikki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Lenovo ThinkVision X1 27" USB-C occasionally hangs the kernel (usbhid) and isn't setting a proper resolution when USB 3.0 (FHD) switched to USB 2.0 (UHD)
On Wed, Apr 11, 2018 at 02:09:29PM +0300, Heikki Krogerus wrote: > On Wed, Apr 11, 2018 at 08:28:44AM +, andrey.ara...@nixaid.com wrote: > > Thank you for the insights, Heikki! > > > > Please find the acpi.dump.tgz file is a attached. > > > > I do not have the USBC* and INT3515* devices, > > OK. That means we don't have any way to interface with the USB Type-C > ports directly unfortunately. The ports are quite simply not visible > to us. We can't do anything from USB side. > > The problem is in any case a graphics problem, so maybe it's better to > let those guys take a look at this. I think this MacBook Pro has AMD > GPU, so adding AMD driver maintainers and lists. Sorry Andrey, the previous mail did not get to you. I managed to change your email address somehow. :-) > > so I have attached the following file as well: > > > > # ls -1 /sys/bus/acpi/devices/*/status | while read dev; do echo $dev: > > $(cat $dev); done | gzip -c > /tmp/all-device-status.log.gz > > > > > > KR, > > Andrey Arapov > > > > April 10, 2018 4:35 PM, "Heikki Krogerus" <heikki.kroge...@linux.intel.com> > > wrote: > > > > > On Tue, Apr 10, 2018 at 09:05:07AM +, andrey.ara...@nixaid.com wrote: > > > > > >> Dear Linux Kernel Devs, > > >> > > >> I have recently got a Lenovo ThinkVision X1 27" monitor, it is connected > > >> to my > > >> laptop over a USB-C cable (DisplayPort). > > >> > > >> This monitor has a built-in USB hub with a toggle button, when pressed it > > >> shows two options on the screen: > > >> > > >> Press USB Switch to toggle between: > > >> A) 3840x2160 UHD USB 2.0 > > >> B) 1920x1080 FHD USB 3.0 > > >> > > >> By default it is always set to FHD, which is why Linux sees the > > >> 1920x1080 as a > > >> maximum possible resolution. > > >> > > >> Whenever I am changing it to the UHD, Linux is not changing the > > >> resolution to > > >> 3840x2160, instead it sets to somewhere around 2560x. > > >> > > >> To work this around, I am running the following commands manually: > > >> > > >> xrandr --newmode "3840x2160_60.00" 533.25 3840 3888 3920 4000 2160 2163 > > >> 2168 +hsync -vsync > > >> xrandr --addmode DisplayPort-2 3840x2160_60.00 > > >> xrandr --output DisplayPort-2 --mode 3840x2160_60.00 > > >> > > >> Then, when I was trying to switch it back to FHD and again back to UHD, > > >> sometimes it causes a kernel panic. The panic also happened when I was > > >> plugging the cable in/out and back again. What I was able to spot is > > >> that the > > >> last unloaded kernel module was usbhid. > > >> > > >> Please advise, what can I try to help investigating this issue further? > > >> > > >> I have attached the output from "dmesg -T" command as "4.16.1-dmesg.txt" > > >> file. > > >> The logs are related to when the monitor was connected via a USB-C cable > > >> to a > > >> DisplayPort-2, the default resolution picked was FHD (USB 3.0) and then > > >> I have > > >> pressed the toggle button to use UHD (USB 2.0), then have applied the > > >> "xrandr" > > >> commands and have closed the lid of my laptop so I would be using my > > >> monitor > > >> as a primary display. > > > > > > The board seems to support Thunderbold, however, in this case my guess is > > > that > > > the monitor is actually using just the DP alternate mode (Thundebolt can > > > pipe > > > DisplayPort protocol). > > > > > > I'm guessing the problem is related to the DisplayPort lane counts. With > > > the > > > highest resolutions you need all four lanes, but if you want to use USB3 > > > with > > > USB Type-C connectors at the same time, two have to be allocated for USB3 > > > leaving only two for DisplayPort allowing lower resolution. The problem > > > is that > > > the GPU drivers need to know how many lanes the DisplayPort has in use. > > > That > > > information will in normal case come from USB Type-C drivers. > > > Unfortunately we > > > do not have support for that in Linux kernel yet, but I have made a > > > proposal for > > > a solution. > > > > > > Let's start by checking details of your board. Can you send us acpidump > > > output? > > > > > > % acpidump -o > > > > > > Also, please check the status of these acpi devices: > > > > > > % cat /sys/bus/acpi/devices/USBC*/status > > > % cat /sys/bus/acpi/devices/INT3515*/status Cheers, -- heikki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 1/6] dt-bindings: add bindings for USB physical connector
On Mon, Mar 05, 2018 at 09:18:10AM +0100, Andrzej Hajda wrote:
> On 02.03.2018 14:13, Heikki Krogerus wrote:
> > Hi,
> >
> > On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
> >> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
> >> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
> >> +DisplayPort video lines are routed to the connector via SS mux in USB3
> >> PHY.
> >> +
> >> +ccic: s2mm005@33 {
> >> + ...
> >> + usb_con: connector {
> >> + compatible = "usb-c-connector";
> >> + label = "USB-C";
> > Is this child node really necessary? There will never be more then
> > one connector per CC line.
>
> But there can be more connectors/cc-lines per IC, for example EZ-PD CCG5[1].
OK, in that case the child node is of course needed.
> [1]:
> http://www.cypress.com/products/ez-pd-ccg5-two-port-usb-type-c-and-power-delivery
>
> >
> > We should prefer device_graph* functions over of_graph* and
> I guess you mean fwnode_graph* functions.
Yes.
> > acpi_graph* functions in the drivers so we don't have to handle the
> > same thing multiple times with separate APIs. Is it still possible if
> > there is that connector child node?
>
> Bindings proposed here are OF bindings, I suppose the most important is
> to follow OF specification and guidelines and these bindings tries to
> follow it.
> It looks like it should not be a problem for fwnode framework to handle
> such bindings, but it is just my guess. I have not seen any fwnode*
> specification I am not sure what is the real purpose of this framework,
> but it seems to be just in-kernel abstraction for different firmware
> standards (OF, ACPI), so even if it lacks at the moment some
> functionality it should not be a barrier for OF bindings.
Sure thing.
Thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 1/6] dt-bindings: add bindings for USB physical connector
Hi,
On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
> +
> +ccic: s2mm005@33 {
> + ...
> + usb_con: connector {
> + compatible = "usb-c-connector";
> + label = "USB-C";
Is this child node really necessary? There will never be more then
one connector per CC line.
We should prefer device_graph* functions over of_graph* and
acpi_graph* functions in the drivers so we don't have to handle the
same thing multiple times with separate APIs. Is it still possible if
there is that connector child node?
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + usb_con_hs: endpoint {
> + remote-endpoint = <_usbc_hs>;
> + };
> + };
> + port@1 {
> + reg = <1>;
> + usb_con_ss: endpoint {
> + remote-endpoint = <_phy_ss>;
> + };
> + };
> + port@2 {
> + reg = <2>;
> + usb_con_sbu: endpoint {
> + remote-endpoint = <_aux>;
> + };
> + };
> + };
> + };
> +};
Thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 15/15] usb: make device_type const
On Sat, Aug 19, 2017 at 01:52:26PM +0530, Bhumika Goyal wrote:
> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
>
> Signed-off-by: Bhumika Goyal <bhumi...@gmail.com>
Acked-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
> ---
> drivers/usb/common/ulpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 930e8f3..4aa5195 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -135,7 +135,7 @@ static void ulpi_dev_release(struct device *dev)
> kfree(to_ulpi_dev(dev));
> }
>
> -static struct device_type ulpi_dev_type = {
> +static const struct device_type ulpi_dev_type = {
> .name = "ulpi_device",
> .groups = ulpi_dev_attr_groups,
> .release = ulpi_dev_release,
Thanks,
--
heikki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
