2023-07-12 08:55:19

by Souradeep Chowdhury

[permalink] [raw]
Subject: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
The notifier chain is used to check the role switch status of EUD. Since EUD can
function only in device mode, other modules trying to do role-switch on the same
port have to first check the EUD status by calling this notifier chain and based
on the status proceed or block their role-switching step. The modules can call
the notifier through the call eud_notifier_call_chain and pass their own
role switch state as the argument. This chain will also be able to handle the
scenario of multiple modules switching roles on the same port since this can
create a priority and ordering among them for conflict resolution.

Souradeep Chowdhury (3):
usb: misc: Add the interface for notifier call for Embedded USB
Debugger(EUD)
usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver
MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)

MAINTAINERS | 1 +
drivers/usb/misc/qcom_eud.c | 52 ++++++++++++++++++++++++++--
drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++
3 files changed, 61 insertions(+), 2 deletions(-)
create mode 100644 drivers/usb/misc/qcom_eud_notifier.h

--
2.17.1



2023-07-12 08:55:55

by Souradeep Chowdhury

[permalink] [raw]
Subject: [PATCH V1 2/3] usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver

Add the notifier call chain to EUD driver. The notifier call chain
is added to check the role-switch status of EUD. When multiple
modules are switching roles on the same port, they need to call this
notifier to check the role-switch status of EUD. If EUD is disabled,
then the modules can go for the role-switch, otherwise it needs to
be blocked. The notifier chain can be used to link multiple modules
switching roles on the same port and create a ordering, priority and
conflict resolution among them. The wrapper functions are defined here
which can be used to register a notifier block to the chain, deregister
it and also call the chain.

Signed-off-by: Souradeep Chowdhury <[email protected]>
---
drivers/usb/misc/qcom_eud.c | 52 +++++++++++++++++++++++++++++++++++--
1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 7f371ea1248c..e6c97a2cf2df 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -11,10 +11,13 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
#include <linux/usb/role.h>
+#include "qcom_eud_notifier.h"

#define EUD_REG_INT1_EN_MASK 0x0024
#define EUD_REG_INT_STATUS_1 0x0044
@@ -22,14 +25,16 @@
#define EUD_REG_VBUS_INT_CLR 0x0080
#define EUD_REG_CSR_EUD_EN 0x1014
#define EUD_REG_SW_ATTACH_DET 0x1018
-#define EUD_REG_EUD_EN2 0x0000
+#define EUD_REG_EUD_EN2 0x0000

#define EUD_ENABLE BIT(0)
-#define EUD_INT_PET_EUD BIT(0)
+#define EUD_INT_PET_EUD BIT(0)
#define EUD_INT_VBUS BIT(2)
#define EUD_INT_SAFE_MODE BIT(4)
#define EUD_INT_ALL (EUD_INT_VBUS | EUD_INT_SAFE_MODE)

+static RAW_NOTIFIER_HEAD(eud_nh);
+
struct eud_chip {
struct device *dev;
struct usb_role_switch *role_sw;
@@ -41,6 +46,42 @@ struct eud_chip {
bool usb_attached;
};

+int eud_register_notify(struct notifier_block *nb)
+{
+ return raw_notifier_chain_register(&eud_nh, nb);
+}
+EXPORT_SYMBOL_GPL(eud_register_notify);
+
+void eud_unregister_notify(struct notifier_block *nb)
+{
+ raw_notifier_chain_unregister(&eud_nh, nb);
+}
+EXPORT_SYMBOL_GPL(eud_unregister_notify);
+
+void eud_notifier_call_chain(unsigned long role_switch_state)
+{
+ raw_notifier_call_chain(&eud_nh, role_switch_state, NULL);
+}
+EXPORT_SYMBOL_GPL(eud_notifier_call_chain);
+
+static int eud_vbus_spoof_attach_detach(struct notifier_block *nb, unsigned long event,
+ void *data)
+{
+ struct device_node *eud = of_find_compatible_node(NULL, NULL, "qcom,eud");
+ struct device *eud_device = bus_find_device_by_of_node(&platform_bus_type, eud);
+ struct eud_chip *eud_data = dev_get_drvdata(eud_device);
+
+ if (eud_data->enabled && event != USB_ROLE_DEVICE)
+ return NOTIFY_BAD;
+ else
+ return NOTIFY_OK;
+}
+
+static struct notifier_block eud_notifier = {
+ .notifier_call = eud_vbus_spoof_attach_detach,
+ .priority = 1,
+};
+
static int enable_eud(struct eud_chip *priv)
{
writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
@@ -196,6 +237,10 @@ static int eud_probe(struct platform_device *pdev)
return dev_err_probe(chip->dev, ret,
"failed to add role switch release action\n");

+ ret = eud_register_notify(&eud_notifier);
+ if (ret)
+ return dev_err_probe(chip->dev, ret, "failed to register notifier\n");
+
chip->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(chip->base))
return PTR_ERR(chip->base);
@@ -226,6 +271,9 @@ static void eud_remove(struct platform_device *pdev)

device_init_wakeup(&pdev->dev, false);
disable_irq_wake(chip->irq);
+ eud_unregister_notify(&eud_notifier);
+
+ return 0;
}

static const struct of_device_id eud_dt_match[] = {
--
2.17.1


2023-07-12 08:59:35

by Souradeep Chowdhury

[permalink] [raw]
Subject: [PATCH V1 3/3] MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)

Add the entry for Embedded USB Debugger(EUD) header file which contains
interface definitions for the EUD notifier chain.

Signed-off-by: Souradeep Chowdhury <[email protected]>
---
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3be1bdfe8ecc..6d395cc6f45c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17288,6 +17288,7 @@ L: [email protected]
S: Maintained
F: Documentation/ABI/testing/sysfs-driver-eud
F: Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
+F: drivers/usb/misc/qcom_eud_notifier.h
F: drivers/usb/misc/qcom_eud.c

QCOM IPA DRIVER
--
2.17.1


2023-07-12 17:12:24

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

On Wed, Jul 12, 2023 at 01:52:37PM +0530, Souradeep Chowdhury wrote:
> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
> The notifier chain is used to check the role switch status of EUD. Since EUD can
> function only in device mode, other modules trying to do role-switch on the same
> port have to first check the EUD status by calling this notifier chain and based
> on the status proceed or block their role-switching step. The modules can call
> the notifier through the call eud_notifier_call_chain and pass their own
> role switch state as the argument. This chain will also be able to handle the
> scenario of multiple modules switching roles on the same port since this can
> create a priority and ordering among them for conflict resolution.

You are adding a new api that no one is actually using, so why would we
accept this at all?

And how can we actually review it without any real users?

thanks,

greg k-h

2023-07-12 17:20:44

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver



On 12.07.2023 10:22, Souradeep Chowdhury wrote:
> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
> The notifier chain is used to check the role switch status of EUD. Since EUD can
> function only in device mode, other modules trying to do role-switch on the same
> port have to first check the EUD status by calling this notifier chain and based
> on the status proceed or block their role-switching step. The modules can call
> the notifier through the call eud_notifier_call_chain and pass their own
> role switch state as the argument. This chain will also be able to handle the
> scenario of multiple modules switching roles on the same port since this can
> create a priority and ordering among them for conflict resolution.
>
> Souradeep Chowdhury (3):
> usb: misc: Add the interface for notifier call for Embedded USB
> Debugger(EUD)
> usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver
> MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)
Please actually CC all maintainers, as present in the MAINTAINERS file..

Consider using b4:

https://b4.docs.kernel.org/en/latest/index.html

Konrad
>
> MAINTAINERS | 1 +
> drivers/usb/misc/qcom_eud.c | 52 ++++++++++++++++++++++++++--
> drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++
> 3 files changed, 61 insertions(+), 2 deletions(-)
> create mode 100644 drivers/usb/misc/qcom_eud_notifier.h
>
> --
> 2.17.1
>

2023-07-12 18:40:49

by Bhupesh Sharma

[permalink] [raw]
Subject: Re: [PATCH V1 3/3] MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)

On Wed, 12 Jul 2023 at 13:58, Souradeep Chowdhury
<[email protected]> wrote:
>
> Add the entry for Embedded USB Debugger(EUD) header file which contains
> interface definitions for the EUD notifier chain.
>
> Signed-off-by: Souradeep Chowdhury <[email protected]>
> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3be1bdfe8ecc..6d395cc6f45c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17288,6 +17288,7 @@ L: [email protected]
> S: Maintained
> F: Documentation/ABI/testing/sysfs-driver-eud
> F: Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> +F: drivers/usb/misc/qcom_eud_notifier.h
> F: drivers/usb/misc/qcom_eud.c

You can simplify it to 'drivers/usb/misc/qcom_eud*' instead, for
avoiding repeatedly changing this when new files are added in future.

Thanks,
Bhupesh

2023-07-12 18:56:24

by Bhupesh Sharma

[permalink] [raw]
Subject: Re: [PATCH V1 2/3] usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver

Hi Souradeep,

On Wed, 12 Jul 2023 at 13:58, Souradeep Chowdhury
<[email protected]> wrote:
>
> Add the notifier call chain to EUD driver. The notifier call chain
> is added to check the role-switch status of EUD. When multiple
> modules are switching roles on the same port, they need to call this
> notifier to check the role-switch status of EUD. If EUD is disabled,
> then the modules can go for the role-switch, otherwise it needs to
> be blocked. The notifier chain can be used to link multiple modules
> switching roles on the same port and create a ordering, priority and
> conflict resolution among them. The wrapper functions are defined here
> which can be used to register a notifier block to the chain, deregister
> it and also call the chain.
>
> Signed-off-by: Souradeep Chowdhury <[email protected]>
> ---
> drivers/usb/misc/qcom_eud.c | 52 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 7f371ea1248c..e6c97a2cf2df 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -11,10 +11,13 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/sysfs.h>
> #include <linux/usb/role.h>
> +#include "qcom_eud_notifier.h"
>
> #define EUD_REG_INT1_EN_MASK 0x0024
> #define EUD_REG_INT_STATUS_1 0x0044
> @@ -22,14 +25,16 @@
> #define EUD_REG_VBUS_INT_CLR 0x0080
> #define EUD_REG_CSR_EUD_EN 0x1014
> #define EUD_REG_SW_ATTACH_DET 0x1018
> -#define EUD_REG_EUD_EN2 0x0000
> +#define EUD_REG_EUD_EN2 0x0000
>
> #define EUD_ENABLE BIT(0)
> -#define EUD_INT_PET_EUD BIT(0)

These indentation issues are already addressed in my EUD patches.
Please rebase your patches on the same to reuse those.

> +#define EUD_INT_PET_EUD BIT(0)
> #define EUD_INT_VBUS BIT(2)
> #define EUD_INT_SAFE_MODE BIT(4)
> #define EUD_INT_ALL (EUD_INT_VBUS | EUD_INT_SAFE_MODE)
>
> +static RAW_NOTIFIER_HEAD(eud_nh);
> +
> struct eud_chip {
> struct device *dev;
> struct usb_role_switch *role_sw;
> @@ -41,6 +46,42 @@ struct eud_chip {
> bool usb_attached;
> };
>
> +int eud_register_notify(struct notifier_block *nb)
> +{
> + return raw_notifier_chain_register(&eud_nh, nb);
> +}
> +EXPORT_SYMBOL_GPL(eud_register_notify);
> +
> +void eud_unregister_notify(struct notifier_block *nb)
> +{
> + raw_notifier_chain_unregister(&eud_nh, nb);
> +}
> +EXPORT_SYMBOL_GPL(eud_unregister_notify);
> +
> +void eud_notifier_call_chain(unsigned long role_switch_state)
> +{
> + raw_notifier_call_chain(&eud_nh, role_switch_state, NULL);
> +}
> +EXPORT_SYMBOL_GPL(eud_notifier_call_chain);

Probably I missed it, but you have not provided any example users of
these APIs in the patchset or reference to another one which shows how
these APIs are used.

> +static int eud_vbus_spoof_attach_detach(struct notifier_block *nb, unsigned long event,
> + void *data)
> +{
> + struct device_node *eud = of_find_compatible_node(NULL, NULL, "qcom,eud");
> + struct device *eud_device = bus_find_device_by_of_node(&platform_bus_type, eud);
> + struct eud_chip *eud_data = dev_get_drvdata(eud_device);
> +
> + if (eud_data->enabled && event != USB_ROLE_DEVICE)
> + return NOTIFY_BAD;
> + else
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block eud_notifier = {
> + .notifier_call = eud_vbus_spoof_attach_detach,
> + .priority = 1,

Why do you need a 'priority = 1' here, it can be 0 or even lower?

Thanks,
Bhupesh

> +};
> +
> static int enable_eud(struct eud_chip *priv)
> {
> writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
> @@ -196,6 +237,10 @@ static int eud_probe(struct platform_device *pdev)
> return dev_err_probe(chip->dev, ret,
> "failed to add role switch release action\n");
>
> + ret = eud_register_notify(&eud_notifier);
> + if (ret)
> + return dev_err_probe(chip->dev, ret, "failed to register notifier\n");
> +
> chip->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(chip->base))
> return PTR_ERR(chip->base);
> @@ -226,6 +271,9 @@ static void eud_remove(struct platform_device *pdev)
>
> device_init_wakeup(&pdev->dev, false);
> disable_irq_wake(chip->irq);
> + eud_unregister_notify(&eud_notifier);
> +
> + return 0;
> }
>
> static const struct of_device_id eud_dt_match[] = {
> --
> 2.17.1
>

2023-07-13 08:47:04

by Souradeep Chowdhury

[permalink] [raw]
Subject: Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver



On 7/12/2023 10:10 PM, Greg KH wrote:
> On Wed, Jul 12, 2023 at 01:52:37PM +0530, Souradeep Chowdhury wrote:
>> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
>> The notifier chain is used to check the role switch status of EUD. Since EUD can
>> function only in device mode, other modules trying to do role-switch on the same
>> port have to first check the EUD status by calling this notifier chain and based
>> on the status proceed or block their role-switching step. The modules can call
>> the notifier through the call eud_notifier_call_chain and pass their own
>> role switch state as the argument. This chain will also be able to handle the
>> scenario of multiple modules switching roles on the same port since this can
>> create a priority and ordering among them for conflict resolution.
>
> You are adding a new api that no one is actually using, so why would we
> accept this at all?
>
> And how can we actually review it without any real users?

Ack. The next version of this will be posted along with the usage
of these apis for ordering the role-switch.

>
> thanks,
>
> greg k-h

2023-07-13 09:03:30

by Souradeep Chowdhury

[permalink] [raw]
Subject: Re: [PATCH V1 2/3] usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver



On 7/12/2023 11:52 PM, Bhupesh Sharma wrote:
> Hi Souradeep,
>
> On Wed, 12 Jul 2023 at 13:58, Souradeep Chowdhury
> <[email protected]> wrote:
>>
>> Add the notifier call chain to EUD driver. The notifier call chain
>> is added to check the role-switch status of EUD. When multiple
>> modules are switching roles on the same port, they need to call this
>> notifier to check the role-switch status of EUD. If EUD is disabled,
>> then the modules can go for the role-switch, otherwise it needs to
>> be blocked. The notifier chain can be used to link multiple modules
>> switching roles on the same port and create a ordering, priority and
>> conflict resolution among them. The wrapper functions are defined here
>> which can be used to register a notifier block to the chain, deregister
>> it and also call the chain.
>>
>> Signed-off-by: Souradeep Chowdhury <[email protected]>
>> ---
>> drivers/usb/misc/qcom_eud.c | 52 +++++++++++++++++++++++++++++++++++--
>> 1 file changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
>> index 7f371ea1248c..e6c97a2cf2df 100644
>> --- a/drivers/usb/misc/qcom_eud.c
>> +++ b/drivers/usb/misc/qcom_eud.c
>> @@ -11,10 +11,13 @@
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> #include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/sysfs.h>
>> #include <linux/usb/role.h>
>> +#include "qcom_eud_notifier.h"
>>
>> #define EUD_REG_INT1_EN_MASK 0x0024
>> #define EUD_REG_INT_STATUS_1 0x0044
>> @@ -22,14 +25,16 @@
>> #define EUD_REG_VBUS_INT_CLR 0x0080
>> #define EUD_REG_CSR_EUD_EN 0x1014
>> #define EUD_REG_SW_ATTACH_DET 0x1018
>> -#define EUD_REG_EUD_EN2 0x0000
>> +#define EUD_REG_EUD_EN2 0x0000
>>
>> #define EUD_ENABLE BIT(0)
>> -#define EUD_INT_PET_EUD BIT(0)
>
> These indentation issues are already addressed in my EUD patches.
> Please rebase your patches on the same to reuse those
Ack

>
>> +#define EUD_INT_PET_EUD BIT(0)
>> #define EUD_INT_VBUS BIT(2)
>> #define EUD_INT_SAFE_MODE BIT(4)
>> #define EUD_INT_ALL (EUD_INT_VBUS | EUD_INT_SAFE_MODE)
>>
>> +static RAW_NOTIFIER_HEAD(eud_nh);
>> +
>> struct eud_chip {
>> struct device *dev;
>> struct usb_role_switch *role_sw;
>> @@ -41,6 +46,42 @@ struct eud_chip {
>> bool usb_attached;
>> };
>>
>> +int eud_register_notify(struct notifier_block *nb)
>> +{
>> + return raw_notifier_chain_register(&eud_nh, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(eud_register_notify);
>> +
>> +void eud_unregister_notify(struct notifier_block *nb)
>> +{
>> + raw_notifier_chain_unregister(&eud_nh, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(eud_unregister_notify);
>> +
>> +void eud_notifier_call_chain(unsigned long role_switch_state)
>> +{
>> + raw_notifier_call_chain(&eud_nh, role_switch_state, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(eud_notifier_call_chain);
>
> Probably I missed it, but you have not provided any example users of
> these APIs in the patchset or reference to another one which shows how
> these APIs are used.

Ack, the usage for this will be posted in the next version.

>
>> +static int eud_vbus_spoof_attach_detach(struct notifier_block *nb, unsigned long event,
>> + void *data)
>> +{
>> + struct device_node *eud = of_find_compatible_node(NULL, NULL, "qcom,eud");
>> + struct device *eud_device = bus_find_device_by_of_node(&platform_bus_type, eud);
>> + struct eud_chip *eud_data = dev_get_drvdata(eud_device);
>> +
>> + if (eud_data->enabled && event != USB_ROLE_DEVICE)
>> + return NOTIFY_BAD;
>> + else
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block eud_notifier = {
>> + .notifier_call = eud_vbus_spoof_attach_detach,
>> + .priority = 1,
>
> Why do you need a 'priority = 1' here, it can be 0 or even lower?

Priority is 0 by default for a notifier block, since eud notifier
needs to be the first to be called in the chain to check the
role-switch status, I have kept the priority as 1 here.

>
> Thanks,
> Bhupesh
>
>> +};
>> +
>> static int enable_eud(struct eud_chip *priv)
>> {
>> writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
>> @@ -196,6 +237,10 @@ static int eud_probe(struct platform_device *pdev)
>> return dev_err_probe(chip->dev, ret,
>> "failed to add role switch release action\n");
>>
>> + ret = eud_register_notify(&eud_notifier);
>> + if (ret)
>> + return dev_err_probe(chip->dev, ret, "failed to register notifier\n");
>> +
>> chip->base = devm_platform_ioremap_resource(pdev, 0);
>> if (IS_ERR(chip->base))
>> return PTR_ERR(chip->base);
>> @@ -226,6 +271,9 @@ static void eud_remove(struct platform_device *pdev)
>>
>> device_init_wakeup(&pdev->dev, false);
>> disable_irq_wake(chip->irq);
>> + eud_unregister_notify(&eud_notifier);
>> +
>> + return 0;
>> }
>>
>> static const struct of_device_id eud_dt_match[] = {
>> --
>> 2.17.1
>>

2023-07-13 09:28:10

by Souradeep Chowdhury

[permalink] [raw]
Subject: Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver



On 7/12/2023 10:13 PM, Konrad Dybcio wrote:
>
>
> On 12.07.2023 10:22, Souradeep Chowdhury wrote:
>> This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
>> The notifier chain is used to check the role switch status of EUD. Since EUD can
>> function only in device mode, other modules trying to do role-switch on the same
>> port have to first check the EUD status by calling this notifier chain and based
>> on the status proceed or block their role-switching step. The modules can call
>> the notifier through the call eud_notifier_call_chain and pass their own
>> role switch state as the argument. This chain will also be able to handle the
>> scenario of multiple modules switching roles on the same port since this can
>> create a priority and ordering among them for conflict resolution.
>>
>> Souradeep Chowdhury (3):
>> usb: misc: Add the interface for notifier call for Embedded USB
>> Debugger(EUD)
>> usb: misc: Add notifier call chain to Embedded USB Debug(EUD) driver
>> MAINTAINERS: Add the header file entry for Embedded USB debugger(EUD)
> Please actually CC all maintainers, as present in the MAINTAINERS file..
>
> Consider using b4:
>
> https://b4.docs.kernel.org/en/latest/index.html

Ack

>
> Konrad
>>
>> MAINTAINERS | 1 +
>> drivers/usb/misc/qcom_eud.c | 52 ++++++++++++++++++++++++++--
>> drivers/usb/misc/qcom_eud_notifier.h | 10 ++++++
>> 3 files changed, 61 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/usb/misc/qcom_eud_notifier.h
>>
>> --
>> 2.17.1
>>

2023-07-13 13:56:10

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH V1 0/3] Add notifier call chain to Embedded USB Debug(EUD) driver

On Thu, Jul 13, 2023 at 01:55:34PM +0530, Souradeep Chowdhury wrote:
>
>
> On 7/12/2023 10:10 PM, Greg KH wrote:
> > On Wed, Jul 12, 2023 at 01:52:37PM +0530, Souradeep Chowdhury wrote:
> > > This patch series adds the notifier chain to the Embedded USB Debug(EUD) driver.
> > > The notifier chain is used to check the role switch status of EUD. Since EUD can
> > > function only in device mode, other modules trying to do role-switch on the same
> > > port have to first check the EUD status by calling this notifier chain and based
> > > on the status proceed or block their role-switching step. The modules can call
> > > the notifier through the call eud_notifier_call_chain and pass their own
> > > role switch state as the argument. This chain will also be able to handle the
> > > scenario of multiple modules switching roles on the same port since this can
> > > create a priority and ordering among them for conflict resolution.
> >
> > You are adding a new api that no one is actually using, so why would we
> > accept this at all?
> >
> > And how can we actually review it without any real users?
>
> Ack. The next version of this will be posted along with the usage
> of these apis for ordering the role-switch.

Also note that I hate notifier call chains, so you better have a very
good reason for why they are required and can't be done in any other
normal way instead :)

good luck!

greg k-h