[PATCH net-next v2 0/3] net: dsa: realtek: support reset controller

[PATCH net-next v2 0/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

v1->v2:
- Introduce a dedicated commit for removing the reset-gpios requirement.
- Place bindings patches before code changes.
- Remove the 'reset-names' property.
- Move the example from the commit message to realtek.yaml.
- Split the reset function into _assert/_deassert variants.
- Modify reset functions to return a warning instead of a value.
- Utilize devm_reset_control_get_optional to prevent failure when
  reset_control is missing.
- Use 'true' and 'false' for boolean values.
- Remove the CONFIG_RESET_CONTROLLER check as stub methods are sufficient
  when undefined.

[PATCH net-next v2 1/3] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Luiz Angelo Daros de Luca]

The 'reset-gpios' should not be mandatory. although they might be
required for some devices if the switch reset was left asserted by a
previous driver, such as the bootloader.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/net/dsa/realtek.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index cce692f57b08..46e113df77c8 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -127,7 +127,6 @@ else:
     - mdc-gpios
     - mdio-gpios
     - mdio
-    - reset-gpios
 
 required:
   - compatible
-- 
2.42.0

Re: [PATCH net-next v2 1/3] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Arınç ÜNAL]

On 27.10.2023 22:00, Luiz Angelo Daros de Luca wrote:
> The 'reset-gpios' should not be mandatory. although they might be
> required for some devices if the switch reset was left asserted by a
> previous driver, such as the bootloader.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Arınç

Re: [PATCH net-next v2 1/3] dt-bindings: net: dsa: realtek: reset-gpios is not required

[Rob Herring]

On Fri, 27 Oct 2023 16:00:55 -0300, Luiz Angelo Daros de Luca wrote:
> The 'reset-gpios' should not be mandatory. although they might be
> required for some devices if the switch reset was left asserted by a
> previous driver, such as the bootloader.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/net/dsa/realtek.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH net-next v2 2/3] dt-bindings: net: dsa: realtek: add reset controller

[Luiz Angelo Daros de Luca]

Realtek switches can use a reset controller instead of reset-gpios.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: devicetree@vger.kernel.org
---
 .../devicetree/bindings/net/dsa/realtek.yaml  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index 46e113df77c8..ef7b27c3b1a3 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -59,6 +59,9 @@ properties:
     description: GPIO to be used to reset the whole device
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
   realtek,disable-leds:
     type: boolean
     description: |
@@ -385,3 +388,75 @@ examples:
                     };
             };
       };
+
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    platform {
+            switch {
+                    compatible = "realtek,rtl8365mb";
+                    mdc-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>;
+                    mdio-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>;
+
+                    resets = <&rst 8>;
+
+                    ethernet-ports {
+                            #address-cells = <1>;
+                            #size-cells = <0>;
+
+                            ethernet-port@0 {
+                                    reg = <0>;
+                                    label = "wan";
+                                    phy-handle = <&ethphy-0>;
+                            };
+                            ethernet-port@1 {
+                                    reg = <1>;
+                                    label = "lan1";
+                                    phy-handle = <&ethphy-1>;
+                            };
+                            ethernet-port@2 {
+                                    reg = <2>;
+                                    label = "lan2";
+                                    phy-handle = <&ethphy-2>;
+                            };
+                            ethernet-port@3 {
+                                    reg = <3>;
+                                    label = "lan3";
+                                    phy-handle = <&ethphy-3>;
+                            };
+                            ethernet-port@4 {
+                                    reg = <4>;
+                                    label = "lan4";
+                                    phy-handle = <&ethphy-4>;
+                            };
+                            ethernet-port@5 {
+                                    reg = <5>;
+                                    ethernet = <&eth0>;
+                                    phy-mode = "rgmii";
+                                    fixed-link {
+                                            speed = <1000>;
+                                            full-duplex;
+                                    };
+                            };
+                    };
+
+                    mdio {
+                            compatible = "realtek,smi-mdio";
+                            #address-cells = <1>;
+                            #size-cells = <0>;
+
+                            ethphy-0: ethernet-phy@0 {
+                                    reg = <0>;
+                            };
+                            ethphy-1: ethernet-phy@1 {
+                                    reg = <1>;
+                            };
+                            ethphy-2: ethernet-phy@2 {
+                                    reg = <2>;
+                            };
+                            ethphy-3: ethernet-phy@3 {
+                                    reg = <3>;
+                            };
+                    };
+            };
+    };
-- 
2.42.0

Re: [PATCH net-next v2 2/3] dt-bindings: net: dsa: realtek: add reset controller

[Arınç ÜNAL]

On 27.10.2023 22:00, Luiz Angelo Daros de Luca wrote:
> Realtek switches can use a reset controller instead of reset-gpios.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Arınç

Re: [PATCH net-next v2 2/3] dt-bindings: net: dsa: realtek: add reset controller

[Rob Herring]

On Fri, Oct 27, 2023 at 04:00:56PM -0300, Luiz Angelo Daros de Luca wrote:
> Realtek switches can use a reset controller instead of reset-gpios.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../devicetree/bindings/net/dsa/realtek.yaml  | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index 46e113df77c8..ef7b27c3b1a3 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -59,6 +59,9 @@ properties:
>      description: GPIO to be used to reset the whole device
>      maxItems: 1
>  
> +  resets:
> +    maxItems: 1
> +
>    realtek,disable-leds:
>      type: boolean
>      description: |
> @@ -385,3 +388,75 @@ examples:
>                      };
>              };
>        };
> +
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    platform {
> +            switch {
> +                    compatible = "realtek,rtl8365mb";
> +                    mdc-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> +                    mdio-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>;
> +
> +                    resets = <&rst 8>;
> +
> +                    ethernet-ports {
> +                            #address-cells = <1>;
> +                            #size-cells = <0>;
> +
> +                            ethernet-port@0 {
> +                                    reg = <0>;
> +                                    label = "wan";
> +                                    phy-handle = <&ethphy-0>;
> +                            };
> +                            ethernet-port@1 {
> +                                    reg = <1>;
> +                                    label = "lan1";
> +                                    phy-handle = <&ethphy-1>;
> +                            };
> +                            ethernet-port@2 {
> +                                    reg = <2>;
> +                                    label = "lan2";
> +                                    phy-handle = <&ethphy-2>;
> +                            };
> +                            ethernet-port@3 {
> +                                    reg = <3>;
> +                                    label = "lan3";
> +                                    phy-handle = <&ethphy-3>;
> +                            };
> +                            ethernet-port@4 {
> +                                    reg = <4>;
> +                                    label = "lan4";
> +                                    phy-handle = <&ethphy-4>;
> +                            };
> +                            ethernet-port@5 {
> +                                    reg = <5>;
> +                                    ethernet = <&eth0>;
> +                                    phy-mode = "rgmii";
> +                                    fixed-link {
> +                                            speed = <1000>;
> +                                            full-duplex;
> +                                    };
> +                            };
> +                    };
> +
> +                    mdio {
> +                            compatible = "realtek,smi-mdio";
> +                            #address-cells = <1>;
> +                            #size-cells = <0>;
> +
> +                            ethphy-0: ethernet-phy@0 {

You didn't test your binding (make dt_binding_check).

'-' is not valid in labels.


Why do we have a whole other example just for 'resets' instead of 
'reset-gpios'? That's not really worth it.

Rob

Re: [PATCH net-next v2 2/3] dt-bindings: net: dsa: realtek: add reset controller

[Luiz Angelo Daros de Luca]

> On Fri, Oct 27, 2023 at 04:00:56PM -0300, Luiz Angelo Daros de Luca wrote:
> > Realtek switches can use a reset controller instead of reset-gpios.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > ---
> >  .../devicetree/bindings/net/dsa/realtek.yaml  | 75 +++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> > index 46e113df77c8..ef7b27c3b1a3 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> > @@ -59,6 +59,9 @@ properties:
> >      description: GPIO to be used to reset the whole device
> >      maxItems: 1
> >
> > +  resets:
> > +    maxItems: 1
> > +
> >    realtek,disable-leds:
> >      type: boolean
> >      description: |
> > @@ -385,3 +388,75 @@ examples:
> >                      };
> >              };
> >        };
> > +
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    platform {
> > +            switch {
> > +                    compatible = "realtek,rtl8365mb";
> > +                    mdc-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> > +                    mdio-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>;
> > +
> > +                    resets = <&rst 8>;
> > +
> > +                    ethernet-ports {
> > +                            #address-cells = <1>;
> > +                            #size-cells = <0>;
> > +
> > +                            ethernet-port@0 {
> > +                                    reg = <0>;
> > +                                    label = "wan";
> > +                                    phy-handle = <&ethphy-0>;
> > +                            };
> > +                            ethernet-port@1 {
> > +                                    reg = <1>;
> > +                                    label = "lan1";
> > +                                    phy-handle = <&ethphy-1>;
> > +                            };
> > +                            ethernet-port@2 {
> > +                                    reg = <2>;
> > +                                    label = "lan2";
> > +                                    phy-handle = <&ethphy-2>;
> > +                            };
> > +                            ethernet-port@3 {
> > +                                    reg = <3>;
> > +                                    label = "lan3";
> > +                                    phy-handle = <&ethphy-3>;
> > +                            };
> > +                            ethernet-port@4 {
> > +                                    reg = <4>;
> > +                                    label = "lan4";
> > +                                    phy-handle = <&ethphy-4>;
> > +                            };
> > +                            ethernet-port@5 {
> > +                                    reg = <5>;
> > +                                    ethernet = <&eth0>;
> > +                                    phy-mode = "rgmii";
> > +                                    fixed-link {
> > +                                            speed = <1000>;
> > +                                            full-duplex;
> > +                                    };
> > +                            };
> > +                    };
> > +
> > +                    mdio {
> > +                            compatible = "realtek,smi-mdio";
> > +                            #address-cells = <1>;
> > +                            #size-cells = <0>;
> > +
> > +                            ethphy-0: ethernet-phy@0 {
>
> You didn't test your binding (make dt_binding_check).
>
> '-' is not valid in labels.
>

My bad. I (wrongly) fixed that in the realtek.example.dtb content,
which is derived from the realtek.yaml.
As it has a newer mtime, it was not updated with dt_binding_check.

>
> Why do we have a whole other example just for 'resets' instead of
> 'reset-gpios'? That's not really worth it.

However, as it is not worth it, I'll drop it.

> Rob

[PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

The 'reset-gpios' will not work when the switch reset is controlled by a
reset controller.

Although the reset is optional and the driver performs a soft reset
during setup, if the initial reset state was asserted, the driver will
not detect it.

The reset controller will take precedence over the reset GPIO.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++----
 drivers/net/dsa/realtek/realtek-smi.c  | 49 ++++++++++++++++++++++---
 drivers/net/dsa/realtek/realtek.h      |  2 +
 3 files changed, 89 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 292e6d087e8b..aad94e49d4c9 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
 	.disable_locking = true,
 };
 
+static void realtek_mdio_reset_assert(struct realtek_priv *priv)
+{
+	int ret;
+
+	if (priv->reset_ctl) {
+		ret = reset_control_assert(priv->reset_ctl);
+		if (ret)
+			dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
+				 ret);
+
+		return;
+	}
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, true);
+}
+
+static void realtek_mdio_reset_deassert(struct realtek_priv *priv)
+{
+	int ret;
+
+	if (priv->reset_ctl) {
+		ret = reset_control_deassert(priv->reset_ctl);
+		if (ret)
+			dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
+				 ret);
+
+		return;
+	}
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, false);
+}
+
 static int realtek_mdio_probe(struct mdio_device *mdiodev)
 {
 	struct realtek_priv *priv;
@@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
 
 	dev_set_drvdata(dev, priv);
 
-	/* TODO: if power is software controlled, set up any regulators here */
 	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
 
+	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(priv->reset_ctl)) {
+		ret = PTR_ERR(priv->reset_ctl);
+		return dev_err_probe(dev, ret, "failed to get reset control\n");
+	}
+
 	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->reset)) {
 		dev_err(dev, "failed to get RESET GPIO\n");
 		return PTR_ERR(priv->reset);
 	}
-
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
+	if (priv->reset_ctl || priv->reset) {
+		realtek_mdio_reset_assert(priv);
 		dev_dbg(dev, "asserted RESET\n");
 		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
+		realtek_mdio_reset_deassert(priv);
 		msleep(REALTEK_HW_START_DELAY);
 		dev_dbg(dev, "deasserted RESET\n");
 	}
@@ -246,8 +284,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
 	dsa_unregister_switch(priv->ds);
 
 	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_mdio_reset_assert(priv);
 }
 
 static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index bfd11591faf4..a99e53b5b662 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
 	return ret;
 }
 
+static void realtek_smi_reset_assert(struct realtek_priv *priv)
+{
+	int ret;
+
+	if (priv->reset_ctl) {
+		ret = reset_control_assert(priv->reset_ctl);
+		if (ret)
+			dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
+				 ret);
+
+		return;
+	}
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, true);
+}
+
+static void realtek_smi_reset_deassert(struct realtek_priv *priv)
+{
+	int ret;
+
+	if (priv->reset_ctl) {
+		ret = reset_control_deassert(priv->reset_ctl);
+		if (ret)
+			dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
+				 ret);
+
+		return;
+	}
+
+	if (priv->reset)
+		gpiod_set_value(priv->reset, false);
+}
+
 static int realtek_smi_probe(struct platform_device *pdev)
 {
 	const struct realtek_variant *var;
@@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, priv);
 	spin_lock_init(&priv->lock);
 
-	/* TODO: if power is software controlled, set up any regulators here */
+	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(priv->reset_ctl)) {
+		ret = PTR_ERR(priv->reset_ctl);
+		return dev_err_probe(dev, ret, "failed to get reset control\n");
+	}
 
 	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(priv->reset)) {
 		dev_err(dev, "failed to get RESET GPIO\n");
 		return PTR_ERR(priv->reset);
 	}
-	if (priv->reset) {
-		gpiod_set_value(priv->reset, 1);
+	if (priv->reset_ctl || priv->reset) {
+		realtek_smi_reset_assert(priv);
 		dev_dbg(dev, "asserted RESET\n");
 		msleep(REALTEK_HW_STOP_DELAY);
-		gpiod_set_value(priv->reset, 0);
+		realtek_smi_reset_deassert(priv);
 		msleep(REALTEK_HW_START_DELAY);
 		dev_dbg(dev, "deasserted RESET\n");
 	}
@@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
 		of_node_put(priv->slave_mii_bus->dev.of_node);
 
 	/* leave the device reset asserted */
-	if (priv->reset)
-		gpiod_set_value(priv->reset, 1);
+	realtek_smi_reset_assert(priv);
 }
 
 static void realtek_smi_shutdown(struct platform_device *pdev)
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index 4fa7c6ba874a..516450837b74 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
 #include <net/dsa.h>
+#include <linux/reset.h>
 
 #define REALTEK_HW_STOP_DELAY		25	/* msecs */
 #define REALTEK_HW_START_DELAY		100	/* msecs */
@@ -48,6 +49,7 @@ struct rtl8366_vlan_4k {
 
 struct realtek_priv {
 	struct device		*dev;
+	struct reset_control    *reset_ctl;
 	struct gpio_desc	*reset;
 	struct gpio_desc	*mdc;
 	struct gpio_desc	*mdio;
-- 
2.42.0

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Andrew Lunn]

> -	/* TODO: if power is software controlled, set up any regulators here */

Please don't remove these comments. The switch might be powered using
a regulator. And this does look like the correct place to get the
regulator, and turn it on before doing the reset. Somebody thought
such boards might exist...

> -	/* TODO: if power is software controlled, set up any regulators here */

etc.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Fri, Oct 27, 2023 at 04:00:57PM -0300, Luiz Angelo Daros de Luca wrote:
> The 'reset-gpios' will not work when the switch reset is controlled by a
> reset controller.
> 
> Although the reset is optional and the driver performs a soft reset
> during setup, if the initial reset state was asserted, the driver will
> not detect it.
> 
> The reset controller will take precedence over the reset GPIO.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++----
>  drivers/net/dsa/realtek/realtek-smi.c  | 49 ++++++++++++++++++++++---
>  drivers/net/dsa/realtek/realtek.h      |  2 +
>  3 files changed, 89 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..aad94e49d4c9 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
>  	.disable_locking = true,
>  };
>  
> +static void realtek_mdio_reset_assert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_assert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> +				 ret);

Instead of "Error: %i" you can say ".. reset control: %pe\n", ERR_PTR(ret)
which will print the error as a symbolic error name (if CONFIG_SYMBOLIC_ERRNAME=y)
rather than just a numeric value.

Also, I don't know if this is explicit in the coding style, but I
believe it is more consistent if single function calls are enveloped in
curly braces if they span multiple lines, like so:

		if (ret) {
			dev_warn(priv->dev,
				 "Failed to assert the switch reset control: %pe",
				 ERR_PTR(ret));
		}

Also, please note that netdev still prefers the 80 character line limit.

> +
> +		return;
> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, true);
> +}
> +
> +static void realtek_mdio_reset_deassert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_deassert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> +				 ret);
> +
> +		return;

Is there a particular reason why this has to ignore a reset GPIO if
present, rather than fall through, checking for the latter as well?

> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, false);
> +}
> +
>  static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv;
> @@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  
>  	dev_set_drvdata(dev, priv);
>  
> -	/* TODO: if power is software controlled, set up any regulators here */

As Andrew mentions, this commit does not make power software-controlled,
so don't remove this.

>  	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
>  
> +	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> +	if (IS_ERR(priv->reset_ctl)) {
> +		ret = PTR_ERR(priv->reset_ctl);
> +		return dev_err_probe(dev, ret, "failed to get reset control\n");
> +	}
> +
>  	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->reset)) {
>  		dev_err(dev, "failed to get RESET GPIO\n");
>  		return PTR_ERR(priv->reset);
>  	}
> -
> -	if (priv->reset) {
> -		gpiod_set_value(priv->reset, 1);
> +	if (priv->reset_ctl || priv->reset) {
> +		realtek_mdio_reset_assert(priv);
>  		dev_dbg(dev, "asserted RESET\n");
>  		msleep(REALTEK_HW_STOP_DELAY);
> -		gpiod_set_value(priv->reset, 0);
> +		realtek_mdio_reset_deassert(priv);
>  		msleep(REALTEK_HW_START_DELAY);
>  		dev_dbg(dev, "deasserted RESET\n");
>  	}
> @@ -246,8 +284,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
>  	dsa_unregister_switch(priv->ds);
>  
>  	/* leave the device reset asserted */
> -	if (priv->reset)
> -		gpiod_set_value(priv->reset, 1);
> +	realtek_mdio_reset_assert(priv);
>  }
>  
>  static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index bfd11591faf4..a99e53b5b662 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  	return ret;
>  }
>  
> +static void realtek_smi_reset_assert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_assert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> +				 ret);
> +
> +		return;
> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, true);
> +}
> +
> +static void realtek_smi_reset_deassert(struct realtek_priv *priv)
> +{
> +	int ret;
> +
> +	if (priv->reset_ctl) {
> +		ret = reset_control_deassert(priv->reset_ctl);
> +		if (ret)
> +			dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> +				 ret);
> +
> +		return;
> +	}
> +
> +	if (priv->reset)
> +		gpiod_set_value(priv->reset, false);
> +}
> +

To respond here, in a single email, to your earlier question (sorry):
https://lore.kernel.org/netdev/CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com/

| Both interface modules, realtek-smi and realtek-mdio, do not share
| code, except for the realtek.h header file. I don't know if it is
| worth it to put the code in a new shared module. What is the best
| practice here? Create a realtek_common.c linked to both modules?

The answer is: I ran "meld" between realtek-mdio.c and realtek-smi.c,
and the probe, remove and shutdown functions are surprisingly similar
already, and perhaps might become even more similar in the future.
I think it is worth introducing a common kernel module for both
interface drivers as a preliminary patch, rather than keeping duplicated
probe/remove/shutdown code.

>  static int realtek_smi_probe(struct platform_device *pdev)
>  {
>  	const struct realtek_variant *var;
> @@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  	dev_set_drvdata(dev, priv);
>  	spin_lock_init(&priv->lock);
>  
> -	/* TODO: if power is software controlled, set up any regulators here */
> +	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> +	if (IS_ERR(priv->reset_ctl)) {
> +		ret = PTR_ERR(priv->reset_ctl);
> +		return dev_err_probe(dev, ret, "failed to get reset control\n");
> +	}
>  
>  	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->reset)) {
>  		dev_err(dev, "failed to get RESET GPIO\n");
>  		return PTR_ERR(priv->reset);
>  	}
> -	if (priv->reset) {
> -		gpiod_set_value(priv->reset, 1);
> +	if (priv->reset_ctl || priv->reset) {
> +		realtek_smi_reset_assert(priv);
>  		dev_dbg(dev, "asserted RESET\n");
>  		msleep(REALTEK_HW_STOP_DELAY);
> -		gpiod_set_value(priv->reset, 0);
> +		realtek_smi_reset_deassert(priv);
>  		msleep(REALTEK_HW_START_DELAY);
>  		dev_dbg(dev, "deasserted RESET\n");
>  	}
> @@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
>  		of_node_put(priv->slave_mii_bus->dev.of_node);

slave_mii_bus was renamed to user_mii_bus, and this prevents the
application of the patch currently, so you will need to respin. But I
think net-next is going to close soon for 2 weeks, so either you respin
as RFC or you wait until it reopens.

>  
>  	/* leave the device reset asserted */
> -	if (priv->reset)
> -		gpiod_set_value(priv->reset, 1);
> +	realtek_smi_reset_assert(priv);
>  }
>  

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

> On Fri, Oct 27, 2023 at 04:00:57PM -0300, Luiz Angelo Daros de Luca wrote:
> > The 'reset-gpios' will not work when the switch reset is controlled by a
> > reset controller.
> >
> > Although the reset is optional and the driver performs a soft reset
> > during setup, if the initial reset state was asserted, the driver will
> > not detect it.
> >
> > The reset controller will take precedence over the reset GPIO.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > ---
> >  drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++----
> >  drivers/net/dsa/realtek/realtek-smi.c  | 49 ++++++++++++++++++++++---
> >  drivers/net/dsa/realtek/realtek.h      |  2 +
> >  3 files changed, 89 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 292e6d087e8b..aad94e49d4c9 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
> >       .disable_locking = true,
> >  };
> >
> > +static void realtek_mdio_reset_assert(struct realtek_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     if (priv->reset_ctl) {
> > +             ret = reset_control_assert(priv->reset_ctl);
> > +             if (ret)
> > +                     dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> > +                              ret);
>
> Instead of "Error: %i" you can say ".. reset control: %pe\n", ERR_PTR(ret)
> which will print the error as a symbolic error name (if CONFIG_SYMBOLIC_ERRNAME=y)
> rather than just a numeric value.
>
> Also, I don't know if this is explicit in the coding style, but I
> believe it is more consistent if single function calls are enveloped in
> curly braces if they span multiple lines, like so:
>
>                 if (ret) {
>                         dev_warn(priv->dev,
>                                  "Failed to assert the switch reset control: %pe",
>                                  ERR_PTR(ret));
>                 }
>
> Also, please note that netdev still prefers the 80 character line limit.

Sure.

> > +
> > +             return;
> > +     }
> > +
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, true);
> > +}
> > +
> > +static void realtek_mdio_reset_deassert(struct realtek_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     if (priv->reset_ctl) {
> > +             ret = reset_control_deassert(priv->reset_ctl);
> > +             if (ret)
> > +                     dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> > +                              ret);
> > +
> > +             return;
>
> Is there a particular reason why this has to ignore a reset GPIO if
> present, rather than fall through, checking for the latter as well?

Something like this, disregard white space issues?

static void realtek_smi_reset_assert(struct realtek_priv *priv)
{
       int ret;

       if (priv->reset_ctl) {
               ret = reset_control_assert(priv->reset_ctl);
               if (!ret)
                       return;

               dev_warn(priv->dev,
                        "Failed to assert the switch reset control: %pe\n",
                        ERR_PTR(ret));
       }

       if (priv->reset)
               gpiod_set_value(priv->reset, true);
}

> > +     }
> > +
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, false);
> > +}
> > +
> >  static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >  {
> >       struct realtek_priv *priv;
> > @@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
> >
> >       dev_set_drvdata(dev, priv);
> >
> > -     /* TODO: if power is software controlled, set up any regulators here */
>
> As Andrew mentions, this commit does not make power software-controlled,
> so don't remove this.

I'll return it and move specific this TODO after the leds_disabled as
it should be before the reset. The one in realtek-smi was in the right
position.

> >       priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
> >
> > +     priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> > +     if (IS_ERR(priv->reset_ctl)) {
> > +             ret = PTR_ERR(priv->reset_ctl);
> > +             return dev_err_probe(dev, ret, "failed to get reset control\n");
> > +     }
> > +
> >       priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> >       if (IS_ERR(priv->reset)) {
> >               dev_err(dev, "failed to get RESET GPIO\n");
> >               return PTR_ERR(priv->reset);
> >       }
> > -
> > -     if (priv->reset) {
> > -             gpiod_set_value(priv->reset, 1);
> > +     if (priv->reset_ctl || priv->reset) {
> > +             realtek_mdio_reset_assert(priv);
> >               dev_dbg(dev, "asserted RESET\n");
> >               msleep(REALTEK_HW_STOP_DELAY);
> > -             gpiod_set_value(priv->reset, 0);
> > +             realtek_mdio_reset_deassert(priv);
> >               msleep(REALTEK_HW_START_DELAY);
> >               dev_dbg(dev, "deasserted RESET\n");
> >       }
> > @@ -246,8 +284,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev)
> >       dsa_unregister_switch(priv->ds);
> >
> >       /* leave the device reset asserted */
> > -     if (priv->reset)
> > -             gpiod_set_value(priv->reset, 1);
> > +     realtek_mdio_reset_assert(priv);
> >  }
> >
> >  static void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> > index bfd11591faf4..a99e53b5b662 100644
> > --- a/drivers/net/dsa/realtek/realtek-smi.c
> > +++ b/drivers/net/dsa/realtek/realtek-smi.c
> > @@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
> >       return ret;
> >  }
> >
> > +static void realtek_smi_reset_assert(struct realtek_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     if (priv->reset_ctl) {
> > +             ret = reset_control_assert(priv->reset_ctl);
> > +             if (ret)
> > +                     dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i",
> > +                              ret);
> > +
> > +             return;
> > +     }
> > +
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, true);
> > +}
> > +
> > +static void realtek_smi_reset_deassert(struct realtek_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     if (priv->reset_ctl) {
> > +             ret = reset_control_deassert(priv->reset_ctl);
> > +             if (ret)
> > +                     dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
> > +                              ret);
> > +
> > +             return;
> > +     }
> > +
> > +     if (priv->reset)
> > +             gpiod_set_value(priv->reset, false);
> > +}
> > +
>
> To respond here, in a single email, to your earlier question (sorry):
> https://lore.kernel.org/netdev/CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com/
>
> | Both interface modules, realtek-smi and realtek-mdio, do not share
> | code, except for the realtek.h header file. I don't know if it is
> | worth it to put the code in a new shared module. What is the best
> | practice here? Create a realtek_common.c linked to both modules?
>
> The answer is: I ran "meld" between realtek-mdio.c and realtek-smi.c,
> and the probe, remove and shutdown functions are surprisingly similar
> already, and perhaps might become even more similar in the future.
> I think it is worth introducing a common kernel module for both
> interface drivers as a preliminary patch, rather than keeping duplicated
> probe/remove/shutdown code.

The remove/shutdown are probably similar to any other DSA driver. I
think the extra code around a shared code in a new module would be
bigger than the duplicated code.

realtek-mdio is an MDIO driver while realtek-smi is a platform driver
implementing a bitbang protocol. They might never be used together in
a system to share RAM and not even installed together in small
systems. If I do need to share the code, I would just link it twice.
Would something like this be acceptable?

drivers/net/dsa/realtek/Makefile
-obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o
-obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o
+obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o realtek_common.o
+obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o realtek_common.o

drivers/net/dsa/realtek/realtek.h:
+void realtek_reset_assert(struct realtek_priv *priv);
+void realtek_reset_deassert(struct realtek_priv *priv);

realtek_common:
+void realtek_reset_assert(struct realtek_priv *priv) {}
+void realtek_reset_deassert(struct realtek_priv *priv) {}

If that realtek_common grows, we could convert it into a module. For
now, it would just introduce extra complexity.

> >  static int realtek_smi_probe(struct platform_device *pdev)
> >  {
> >       const struct realtek_variant *var;
> > @@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev)
> >       dev_set_drvdata(dev, priv);
> >       spin_lock_init(&priv->lock);
> >
> > -     /* TODO: if power is software controlled, set up any regulators here */
> > +     priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
> > +     if (IS_ERR(priv->reset_ctl)) {
> > +             ret = PTR_ERR(priv->reset_ctl);
> > +             return dev_err_probe(dev, ret, "failed to get reset control\n");
> > +     }
> >
> >       priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> >       if (IS_ERR(priv->reset)) {
> >               dev_err(dev, "failed to get RESET GPIO\n");
> >               return PTR_ERR(priv->reset);
> >       }
> > -     if (priv->reset) {
> > -             gpiod_set_value(priv->reset, 1);
> > +     if (priv->reset_ctl || priv->reset) {
> > +             realtek_smi_reset_assert(priv);
> >               dev_dbg(dev, "asserted RESET\n");
> >               msleep(REALTEK_HW_STOP_DELAY);
> > -             gpiod_set_value(priv->reset, 0);
> > +             realtek_smi_reset_deassert(priv);
> >               msleep(REALTEK_HW_START_DELAY);
> >               dev_dbg(dev, "deasserted RESET\n");
> >       }
> > @@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev)
> >               of_node_put(priv->slave_mii_bus->dev.of_node);
>
> slave_mii_bus was renamed to user_mii_bus, and this prevents the
> application of the patch currently, so you will need to respin. But I
> think net-next is going to close soon for 2 weeks, so either you respin
> as RFC or you wait until it reopens.

I'll wait. I hope the comments on this thread might be enough to get
this patch sorted out.

>
> >
> >       /* leave the device reset asserted */
> > -     if (priv->reset)
> > -             gpiod_set_value(priv->reset, 1);
> > +     realtek_smi_reset_assert(priv);
> >  }
> >

Regards,

Luiz

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

Hi Vladimir,

> realtek-mdio is an MDIO driver while realtek-smi is a platform driver
> implementing a bitbang protocol. They might never be used together in
> a system to share RAM and not even installed together in small
> systems. If I do need to share the code, I would just link it twice.
> Would something like this be acceptable?
>
> drivers/net/dsa/realtek/Makefile
> -obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o
> -obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o
> +obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o realtek_common.o
> +obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o realtek_common.o

Just a follow up.

It is not that simple to include a .c file into an existing single
file module. It looks like you need to rename the original file as all
linked objects must not conflict with the module name. The kernel
build seems to create a new object file for each module. Is there a
clearer way? I think #include a common .c file would not be
acceptable.

I tested your shared module suggestion. It is the clearest one but it
also increased the overall size quite a bit. Even linking two objects
seems to use the double of space used by the functions alone. These
are some values (mips)

drivers/net/dsa/realtek/realtek_common.o  5744  without exports
drivers/net/dsa/realtek/realtek_common.o 33472  exporting the two
reset functions (assert/deassert)

drivers/net/dsa/realtek/realtek-mdio.o   18756  without the reset
funcs (to be used as a module)
drivers/net/dsa/realtek/realtek-mdio.o   20480  including the
realtek_common.c (#include <realtek_common.c>)
drivers/net/dsa/realtek/realtek-mdio.o   22696  linking the realtek_common.o

drivers/net/dsa/realtek/realtek-smi.o    30712  without the reset
funcs (to be used as a module)
drivers/net/dsa/realtek/realtek-smi.o    34604  linking the realtek_common.o

drivers/net/dsa/realtek/realtek-mdio.ko  28800  without the reset
funcs (it will use realtek_common.ko)
drivers/net/dsa/realtek/realtek-mdio.ko  32736  linking the realtek_common.o

drivers/net/dsa/realtek/realtek-smi.ko   40708  without the reset
funcs (it will use realtek_common.ko)
drivers/net/dsa/realtek/realtek-smi.ko   44612  linking the realtek_common.o

In summary, we get about 1.5kb of code with the extra functions,
almost 4kb if we link a common object containing the functions and
33kb if we use a module for those two functions.

I can go with any option. I just need to know which one would be
accepted to update my patches.
1) keep duplicated functions on each file
2) share the code including the .c on both
3) share the code linking a common object in both modules (and
renaming existing .c files)
4) create a new module used by both modules.

The devices that would use this driver have very restricted storage
space. Every kbyte counts.

Regards,

Luiz

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Wed, Nov 01, 2023 at 04:55:19PM -0300, Luiz Angelo Daros de Luca wrote:
> Hi Vladimir,
> 
> > realtek-mdio is an MDIO driver while realtek-smi is a platform driver
> > implementing a bitbang protocol. They might never be used together in
> > a system to share RAM and not even installed together in small
> > systems. If I do need to share the code, I would just link it twice.
> > Would something like this be acceptable?
> >
> > drivers/net/dsa/realtek/Makefile
> > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o
> > -obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o
> > +obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o realtek_common.o
> > +obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o realtek_common.o

You cannot link realtek_common.o into multiple .ko files. It generates
build warnings and it is being phased out.
https://patchwork.kernel.org/project/netdevbpf/cover/20221119225650.1044591-1-alobakin@pm.me/

> Just a follow up.
> 
> It is not that simple to include a .c file into an existing single
> file module. It looks like you need to rename the original file as all
> linked objects must not conflict with the module name. The kernel
> build seems to create a new object file for each module. Is there a
> clearer way? I think #include a common .c file would not be
> acceptable.
> 
> I tested your shared module suggestion. It is the clearest one but it
> also increased the overall size quite a bit. Even linking two objects
> seems to use the double of space used by the functions alone. These
> are some values (mips)
> 
> drivers/net/dsa/realtek/realtek_common.o  5744  without exports
> drivers/net/dsa/realtek/realtek_common.o 33472  exporting the two reset functions (assert/deassert)
> 
> drivers/net/dsa/realtek/realtek-mdio.o   18756  without the reset funcs (to be used as a module)
> drivers/net/dsa/realtek/realtek-mdio.o   20480  including the realtek_common.c (#include <realtek_common.c>)
> drivers/net/dsa/realtek/realtek-mdio.o   22696  linking the realtek_common.o
> 
> drivers/net/dsa/realtek/realtek-smi.o    30712  without the reset funcs (to be used as a module)
> drivers/net/dsa/realtek/realtek-smi.o    34604  linking the realtek_common.o
> 
> drivers/net/dsa/realtek/realtek-mdio.ko  28800  without the reset funcs (it will use realtek_common.ko)
> drivers/net/dsa/realtek/realtek-mdio.ko  32736  linking the realtek_common.o
> 
> drivers/net/dsa/realtek/realtek-smi.ko   40708  without the reset funcs (it will use realtek_common.ko)
> drivers/net/dsa/realtek/realtek-smi.ko   44612  linking the realtek_common.o
> 
> In summary, we get about 1.5kb of code with the extra functions,
> almost 4kb if we link a common object containing the functions and
> 33kb if we use a module for those two functions.
> 
> I can go with any option. I just need to know which one would be
> accepted to update my patches.
> 1) keep duplicated functions on each file

Disadvantage: need to update the same functions twice, it becomes
possible for them to diverge, increases maintenance difficulty.

> 2) share the code including the .c on both

Including a .c file with a preprocessor #include is fragile, has to be
placed very carefully, etc. So it is also a time bomb from a maintenance
PoV. Maybe a header with static inline function definitions would be
worth considering, although I don't think it's common practice to do
this.

> 3) share the code linking a common object in both modules (and
> renaming existing .c files)

Sharing object files is being phased out.

> 4) create a new module used by both modules.

I am suspicious of the numbers you provided above. What needs to be
compared is the reduction in size of realtek_mdio.ko and realtek_smi.ko,
compared to the size of the new realtek_common.ko. Also, this starts
being more and more worthwhile as more code goes into realtek_common.ko,
and I also mentioned a common probe/remove/shutdown as being viable
candidates. Looking at your figures, I'm not sure at which ones I need
to look in order to compute that metric.

> The devices that would use this driver have very restricted storage
> space. Every kbyte counts.

Well, in that case you could compile the code as built into the kernel,
and the module overhead would go away.

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Linus Walleij]

On Wed, Nov 1, 2023 at 8:55 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> > drivers/net/dsa/realtek/Makefile
> > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o
> > -obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o
> > +obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o realtek_common.o
> > +obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o realtek_common.o
>
> Just a follow up.
>
> It is not that simple to include a .c file into an existing single
> file module. It looks like you need to rename the original file as all
> linked objects must not conflict with the module name. The kernel
> build seems to create a new object file for each module. Is there a
> clearer way? I think #include a common .c file would not be
> acceptable.

I don't know if this is an answer to your question, but look at what I did in

drivers/usb/fotg210/Makefile:

# This setup links the different object files into one single
# module so we don't have to EXPORT() a lot of internal symbols
# or create unnecessary submodules.
fotg210-objs-y                          += fotg210-core.o
fotg210-objs-$(CONFIG_USB_FOTG210_HCD)  += fotg210-hcd.o
fotg210-objs-$(CONFIG_USB_FOTG210_UDC)  += fotg210-udc.o
fotg210-objs                            := $(fotg210-objs-y)
obj-$(CONFIG_USB_FOTG210)               += fotg210.o

Everything starting with CONFIG_* is a Kconfig option obviously.

The final module is just one file named fotg210.ko no matter whether
HCD (host controller), UDC (device controller) or both parts were
compiled into it. Often you just need one of them, sometimes you may
need both.

It's a pretty clean example of how you do this "one module from
several optional parts" using Kbuild.

It's not super-intuitive, copy/paste/modify is a viable way to get to this.

Yours,
Linus Walleij

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Thu, Nov 02, 2023 at 03:59:48PM +0100, Linus Walleij wrote:
> I don't know if this is an answer to your question, but look at what I did in
> 
> drivers/usb/fotg210/Makefile:
> 
> # This setup links the different object files into one single
> # module so we don't have to EXPORT() a lot of internal symbols
> # or create unnecessary submodules.
> fotg210-objs-y                          += fotg210-core.o
> fotg210-objs-$(CONFIG_USB_FOTG210_HCD)  += fotg210-hcd.o
> fotg210-objs-$(CONFIG_USB_FOTG210_UDC)  += fotg210-udc.o
> fotg210-objs                            := $(fotg210-objs-y)
> obj-$(CONFIG_USB_FOTG210)               += fotg210.o
> 
> Everything starting with CONFIG_* is a Kconfig option obviously.
> 
> The final module is just one file named fotg210.ko no matter whether
> HCD (host controller), UDC (device controller) or both parts were
> compiled into it. Often you just need one of them, sometimes you may
> need both.
> 
> It's a pretty clean example of how you do this "one module from
> several optional parts" using Kbuild.

To be clear, something like this is what you mean, right?

diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
index 060165a85fb7..857a039fb0f1 100644
--- a/drivers/net/dsa/realtek/Kconfig
+++ b/drivers/net/dsa/realtek/Kconfig
@@ -15,39 +15,37 @@ menuconfig NET_DSA_REALTEK
 
 if NET_DSA_REALTEK
 
+config NET_DSA_REALTEK_INTERFACE
+	tristate
+	help
+	  Common interface driver for accessing Realtek switches, either
+	  through MDIO or SMI.
+
 config NET_DSA_REALTEK_MDIO
-	tristate "Realtek MDIO interface driver"
-	depends on OF
-	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
-	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
-	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
+	tristate "Realtek MDIO interface support"
 	help
 	  Select to enable support for registering switches configured
 	  through MDIO.
 
 config NET_DSA_REALTEK_SMI
-	tristate "Realtek SMI interface driver"
-	depends on OF
-	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
-	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
-	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
+	bool "Realtek SMI interface support"
 	help
 	  Select to enable support for registering switches connected
 	  through SMI.
 
 config NET_DSA_REALTEK_RTL8365MB
 	tristate "Realtek RTL8365MB switch subdriver"
-	imply NET_DSA_REALTEK_SMI
-	imply NET_DSA_REALTEK_MDIO
+	select NET_DSA_REALTEK_INTERFACE
 	select NET_DSA_TAG_RTL8_4
+	depends on OF
 	help
 	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
 
 config NET_DSA_REALTEK_RTL8366RB
 	tristate "Realtek RTL8366RB switch subdriver"
-	imply NET_DSA_REALTEK_SMI
-	imply NET_DSA_REALTEK_MDIO
+	select NET_DSA_REALTEK_INTERFACE
 	select NET_DSA_TAG_RTL4_A
+	depends on OF
 	help
 	  Select to enable support for Realtek RTL8366RB.
 
diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
index 0aab57252a7c..35b7734c0ad0 100644
--- a/drivers/net/dsa/realtek/Makefile
+++ b/drivers/net/dsa/realtek/Makefile
@@ -1,6 +1,15 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_NET_DSA_REALTEK_MDIO) 	+= realtek-mdio.o
-obj-$(CONFIG_NET_DSA_REALTEK_SMI) 	+= realtek-smi.o
+
+obj-$(CONFIG_NET_DSA_REALTEK_INTERFACE) := realtek-interface.o
+
+realtek-interface-objs			:= realtek-interface-common.o
+ifdef CONFIG_NET_DSA_REALTEK_MDIO
+realtek-interface-objs			+= realtek-mdio.o
+endif
+ifdef CONFIG_NET_DSA_REALTEK_SMI
+realtek-interface-objs			+= realtek-smi.o
+endif
+
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
 rtl8366-objs 				:= rtl8366-core.o rtl8366rb.o
 obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
diff --git a/drivers/net/dsa/realtek/realtek-interface-common.c b/drivers/net/dsa/realtek/realtek-interface-common.c
new file mode 100644
index 000000000000..bb7c77cdb9e2
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-interface-common.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/module.h>
+
+#include "realtek-mdio.h"
+#include "realtek-smi.h"
+
+static int __init realtek_interface_init(void)
+{
+	int err;
+
+	err = realtek_mdio_init();
+	if (err)
+		return err;
+
+	err = realtek_smi_init();
+	if (err) {
+		realtek_smi_exit();
+		return err;
+	}
+
+	return 0;
+}
+module_init(realtek_interface_init);
+
+static void __exit realtek_interface_exit(void)
+{
+	realtek_smi_exit();
+	realtek_mdio_exit();
+}
+module_exit(realtek_interface_exit);
+
+MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
+MODULE_DESCRIPTION("Driver for interfacing with Realtek switches via MDIO or SMI");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 292e6d087e8b..6997dec14de2 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0+
-/* Realtek MDIO interface driver
+/* Realtek MDIO interface support
  *
  * ASICs we intend to support with this driver:
  *
@@ -19,12 +19,12 @@
  * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org>
  */
 
-#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/overflow.h>
 #include <linux/regmap.h>
 
 #include "realtek.h"
+#include "realtek-mdio.h"
 
 /* Read/write via mdiobus */
 #define REALTEK_MDIO_CTRL0_REG		31
@@ -283,8 +283,12 @@ static struct mdio_driver realtek_mdio_driver = {
 	.shutdown = realtek_mdio_shutdown,
 };
 
-mdio_module_driver(realtek_mdio_driver);
+int realtek_mdio_init(void)
+{
+	return mdio_driver_register(&realtek_mdio_driver);
+}
 
-MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
-MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
-MODULE_LICENSE("GPL");
+void realtek_mdio_exit(void)
+{
+	mdio_driver_unregister(&realtek_mdio_driver);
+}
diff --git a/drivers/net/dsa/realtek/realtek-mdio.h b/drivers/net/dsa/realtek/realtek-mdio.h
new file mode 100644
index 000000000000..941b4ef9d531
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-mdio.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_MDIO_H
+#define _REALTEK_MDIO_H
+
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO)
+
+int realtek_mdio_init(void);
+void realtek_mdio_exit(void);
+
+#else
+
+static inline int realtek_mdio_init(void)
+{
+	return 0;
+}
+
+static inline void realtek_mdio_exit(void)
+{
+}
+
+#endif
+
+#endif
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 755546ed8db6..4c282bfc884d 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0+
-/* Realtek Simple Management Interface (SMI) driver
+/* Realtek Simple Management Interface (SMI) interface
  * It can be discussed how "simple" this interface is.
  *
  * The SMI protocol piggy-backs the MDIO MDC and MDIO signals levels
@@ -26,7 +26,6 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/device.h>
 #include <linux/spinlock.h>
 #include <linux/skbuff.h>
@@ -40,6 +39,7 @@
 #include <linux/if_bridge.h>
 
 #include "realtek.h"
+#include "realtek-smi.h"
 
 #define REALTEK_SMI_ACK_RETRY_COUNT		5
 
@@ -560,8 +560,13 @@ static struct platform_driver realtek_smi_driver = {
 	.remove_new = realtek_smi_remove,
 	.shutdown = realtek_smi_shutdown,
 };
-module_platform_driver(realtek_smi_driver);
 
-MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
-MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
-MODULE_LICENSE("GPL");
+int realtek_smi_init(void)
+{
+	return platform_driver_register(&realtek_smi_driver);
+}
+
+void realtek_smi_exit(void)
+{
+	platform_driver_unregister(&realtek_smi_driver);
+}
diff --git a/drivers/net/dsa/realtek/realtek-smi.h b/drivers/net/dsa/realtek/realtek-smi.h
new file mode 100644
index 000000000000..9a4838321f94
--- /dev/null
+++ b/drivers/net/dsa/realtek/realtek-smi.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _REALTEK_SMI_H
+#define _REALTEK_SMI_H
+
+#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI)
+
+int realtek_smi_init(void);
+void realtek_smi_exit(void);
+
+#else
+
+static inline int realtek_smi_init(void)
+{
+	return 0;
+}
+
+static inline void realtek_smi_exit(void)
+{
+}
+
+#endif
+
+#endif
-- 
2.34.1


It looks pretty reasonable to me. More stuff could go into
realtek-interface-common.c, that could be called directly from
realtek-smi.c and realtek-mdio.c without exporting anything.

I've eliminated the possibility for the SMI and MDIO options to be
anything other than y or n, because only a single interface module
(the common one) exists, and the y/n/m quality of that is
implied/selected by the drivers which depend on it. I hope I wasn't too
trigger-happy with this.

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Thu, Nov 02, 2023 at 05:55:21PM +0200, Vladimir Oltean wrote:
> diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> index 060165a85fb7..857a039fb0f1 100644
> --- a/drivers/net/dsa/realtek/Kconfig
> +++ b/drivers/net/dsa/realtek/Kconfig
> @@ -15,39 +15,37 @@ menuconfig NET_DSA_REALTEK
>  
>  if NET_DSA_REALTEK
>  
> +config NET_DSA_REALTEK_INTERFACE
> +	tristate
> +	help
> +	  Common interface driver for accessing Realtek switches, either
> +	  through MDIO or SMI.
> +
>  config NET_DSA_REALTEK_MDIO
> -	tristate "Realtek MDIO interface driver"
> -	depends on OF
> -	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
> -	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
> -	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
> +	tristate "Realtek MDIO interface support"

I meant to also make "config NET_DSA_REALTEK_MDIO" a bool and not tristate.

>  	help
>  	  Select to enable support for registering switches configured
>  	  through MDIO.
>  
>  config NET_DSA_REALTEK_SMI
> -	tristate "Realtek SMI interface driver"
> -	depends on OF
> -	depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
> -	depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
> -	depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
> +	bool "Realtek SMI interface support"
>  	help
>  	  Select to enable support for registering switches connected
>  	  through SMI.
>  
>  config NET_DSA_REALTEK_RTL8365MB
>  	tristate "Realtek RTL8365MB switch subdriver"
> -	imply NET_DSA_REALTEK_SMI
> -	imply NET_DSA_REALTEK_MDIO
> +	select NET_DSA_REALTEK_INTERFACE
>  	select NET_DSA_TAG_RTL8_4
> +	depends on OF
>  	help
>  	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
>  
>  config NET_DSA_REALTEK_RTL8366RB
>  	tristate "Realtek RTL8366RB switch subdriver"
> -	imply NET_DSA_REALTEK_SMI
> -	imply NET_DSA_REALTEK_MDIO
> +	select NET_DSA_REALTEK_INTERFACE
>  	select NET_DSA_TAG_RTL4_A
> +	depends on OF
>  	help
>  	  Select to enable support for Realtek RTL8366RB.

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Thu, Nov 02, 2023 at 05:55:21PM +0200, Vladimir Oltean wrote:
> +static int __init realtek_interface_init(void)
> +{
> +	int err;
> +
> +	err = realtek_mdio_init();
> +	if (err)
> +		return err;
> +
> +	err = realtek_smi_init();
> +	if (err) {
> +		realtek_smi_exit();

One more correction, this was supposed to be realtek_mdio_exit().

> +		return err;
> +	}
> +
> +	return 0;
> +}
> +module_init(realtek_interface_init);

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Linus Walleij]

On Thu, Nov 2, 2023 at 4:55 PM Vladimir Oltean <olteanv@gmail.com> wrote:

> To be clear, something like this is what you mean, right?

Hey, it's 90% done already! :D
And we do away with all the hopeless IMPLY stuff.

> +realtek-interface-objs                 := realtek-interface-common.o
> +ifdef CONFIG_NET_DSA_REALTEK_MDIO
> +realtek-interface-objs                 += realtek-mdio.o
> +endif
> +ifdef CONFIG_NET_DSA_REALTEK_SMI
> +realtek-interface-objs                 += realtek-smi.o
> +endif

I would try to do

realtek-interface-objs-y                := realtek-interface-common.o
realtek-interface-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
realtek-interface-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
realtek-interface-objs := $(realtek-interface-objs-y)

I think it's equivalent just more compact?

Other than that it looks like what I would have done myself.

Yours,
Linus Walleij

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

> On Thu, Nov 02, 2023 at 03:59:48PM +0100, Linus Walleij wrote:
> > I don't know if this is an answer to your question, but look at what I did in
> >
> > drivers/usb/fotg210/Makefile:
> >
> > # This setup links the different object files into one single
> > # module so we don't have to EXPORT() a lot of internal symbols
> > # or create unnecessary submodules.
> > fotg210-objs-y                          += fotg210-core.o
> > fotg210-objs-$(CONFIG_USB_FOTG210_HCD)  += fotg210-hcd.o
> > fotg210-objs-$(CONFIG_USB_FOTG210_UDC)  += fotg210-udc.o
> > fotg210-objs                            := $(fotg210-objs-y)
> > obj-$(CONFIG_USB_FOTG210)               += fotg210.o
> >
> > Everything starting with CONFIG_* is a Kconfig option obviously.
> >
> > The final module is just one file named fotg210.ko no matter whether
> > HCD (host controller), UDC (device controller) or both parts were
> > compiled into it. Often you just need one of them, sometimes you may
> > need both.
> >
> > It's a pretty clean example of how you do this "one module from
> > several optional parts" using Kbuild.
>
> To be clear, something like this is what you mean, right?
>
> diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> index 060165a85fb7..857a039fb0f1 100644
> --- a/drivers/net/dsa/realtek/Kconfig
> +++ b/drivers/net/dsa/realtek/Kconfig
> @@ -15,39 +15,37 @@ menuconfig NET_DSA_REALTEK
>
>  if NET_DSA_REALTEK
>
> +config NET_DSA_REALTEK_INTERFACE
> +       tristate
> +       help
> +         Common interface driver for accessing Realtek switches, either
> +         through MDIO or SMI.
> +
>  config NET_DSA_REALTEK_MDIO
> -       tristate "Realtek MDIO interface driver"
> -       depends on OF
> -       depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
> -       depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
> -       depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
> +       tristate "Realtek MDIO interface support"
>         help
>           Select to enable support for registering switches configured
>           through MDIO.
>
>  config NET_DSA_REALTEK_SMI
> -       tristate "Realtek SMI interface driver"
> -       depends on OF
> -       depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
> -       depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
> -       depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
> +       bool "Realtek SMI interface support"
>         help
>           Select to enable support for registering switches connected
>           through SMI.
>
>  config NET_DSA_REALTEK_RTL8365MB
>         tristate "Realtek RTL8365MB switch subdriver"
> -       imply NET_DSA_REALTEK_SMI
> -       imply NET_DSA_REALTEK_MDIO
> +       select NET_DSA_REALTEK_INTERFACE
>         select NET_DSA_TAG_RTL8_4
> +       depends on OF
>         help
>           Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
>
>  config NET_DSA_REALTEK_RTL8366RB
>         tristate "Realtek RTL8366RB switch subdriver"
> -       imply NET_DSA_REALTEK_SMI
> -       imply NET_DSA_REALTEK_MDIO
> +       select NET_DSA_REALTEK_INTERFACE
>         select NET_DSA_TAG_RTL4_A
> +       depends on OF
>         help
>           Select to enable support for Realtek RTL8366RB.
>
> diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
> index 0aab57252a7c..35b7734c0ad0 100644
> --- a/drivers/net/dsa/realtek/Makefile
> +++ b/drivers/net/dsa/realtek/Makefile
> @@ -1,6 +1,15 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_NET_DSA_REALTEK_MDIO)     += realtek-mdio.o
> -obj-$(CONFIG_NET_DSA_REALTEK_SMI)      += realtek-smi.o
> +
> +obj-$(CONFIG_NET_DSA_REALTEK_INTERFACE) := realtek-interface.o
> +
> +realtek-interface-objs                 := realtek-interface-common.o
> +ifdef CONFIG_NET_DSA_REALTEK_MDIO
> +realtek-interface-objs                 += realtek-mdio.o
> +endif
> +ifdef CONFIG_NET_DSA_REALTEK_SMI
> +realtek-interface-objs                 += realtek-smi.o
> +endif
> +
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
>  rtl8366-objs                           := rtl8366-core.o rtl8366rb.o
>  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
> diff --git a/drivers/net/dsa/realtek/realtek-interface-common.c b/drivers/net/dsa/realtek/realtek-interface-common.c
> new file mode 100644
> index 000000000000..bb7c77cdb9e2
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-interface-common.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/module.h>
> +
> +#include "realtek-mdio.h"
> +#include "realtek-smi.h"
> +
> +static int __init realtek_interface_init(void)
> +{
> +       int err;
> +
> +       err = realtek_mdio_init();
> +       if (err)
> +               return err;
> +
> +       err = realtek_smi_init();
> +       if (err) {
> +               realtek_smi_exit();
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +module_init(realtek_interface_init);
> +
> +static void __exit realtek_interface_exit(void)
> +{
> +       realtek_smi_exit();
> +       realtek_mdio_exit();
> +}
> +module_exit(realtek_interface_exit);
> +
> +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
> +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
> +MODULE_DESCRIPTION("Driver for interfacing with Realtek switches via MDIO or SMI");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..6997dec14de2 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0+
> -/* Realtek MDIO interface driver
> +/* Realtek MDIO interface support
>   *
>   * ASICs we intend to support with this driver:
>   *
> @@ -19,12 +19,12 @@
>   * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org>
>   */
>
> -#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/overflow.h>
>  #include <linux/regmap.h>
>
>  #include "realtek.h"
> +#include "realtek-mdio.h"
>
>  /* Read/write via mdiobus */
>  #define REALTEK_MDIO_CTRL0_REG         31
> @@ -283,8 +283,12 @@ static struct mdio_driver realtek_mdio_driver = {
>         .shutdown = realtek_mdio_shutdown,
>  };
>
> -mdio_module_driver(realtek_mdio_driver);
> +int realtek_mdio_init(void)
> +{
> +       return mdio_driver_register(&realtek_mdio_driver);
> +}
>
> -MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>");
> -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
> -MODULE_LICENSE("GPL");
> +void realtek_mdio_exit(void)
> +{
> +       mdio_driver_unregister(&realtek_mdio_driver);
> +}
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.h b/drivers/net/dsa/realtek/realtek-mdio.h
> new file mode 100644
> index 000000000000..941b4ef9d531
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-mdio.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _REALTEK_MDIO_H
> +#define _REALTEK_MDIO_H
> +
> +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO)
> +
> +int realtek_mdio_init(void);
> +void realtek_mdio_exit(void);
> +
> +#else
> +
> +static inline int realtek_mdio_init(void)
> +{
> +       return 0;
> +}
> +
> +static inline void realtek_mdio_exit(void)
> +{
> +}
> +
> +#endif
> +
> +#endif
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 755546ed8db6..4c282bfc884d 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0+
> -/* Realtek Simple Management Interface (SMI) driver
> +/* Realtek Simple Management Interface (SMI) interface
>   * It can be discussed how "simple" this interface is.
>   *
>   * The SMI protocol piggy-backs the MDIO MDC and MDIO signals levels
> @@ -26,7 +26,6 @@
>   */
>
>  #include <linux/kernel.h>
> -#include <linux/module.h>
>  #include <linux/device.h>
>  #include <linux/spinlock.h>
>  #include <linux/skbuff.h>
> @@ -40,6 +39,7 @@
>  #include <linux/if_bridge.h>
>
>  #include "realtek.h"
> +#include "realtek-smi.h"
>
>  #define REALTEK_SMI_ACK_RETRY_COUNT            5
>
> @@ -560,8 +560,13 @@ static struct platform_driver realtek_smi_driver = {
>         .remove_new = realtek_smi_remove,
>         .shutdown = realtek_smi_shutdown,
>  };
> -module_platform_driver(realtek_smi_driver);
>
> -MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
> -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
> -MODULE_LICENSE("GPL");
> +int realtek_smi_init(void)
> +{
> +       return platform_driver_register(&realtek_smi_driver);
> +}
> +
> +void realtek_smi_exit(void)
> +{
> +       platform_driver_unregister(&realtek_smi_driver);
> +}
> diff --git a/drivers/net/dsa/realtek/realtek-smi.h b/drivers/net/dsa/realtek/realtek-smi.h
> new file mode 100644
> index 000000000000..9a4838321f94
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-smi.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _REALTEK_SMI_H
> +#define _REALTEK_SMI_H
> +
> +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI)
> +
> +int realtek_smi_init(void);
> +void realtek_smi_exit(void);
> +
> +#else
> +
> +static inline int realtek_smi_init(void)
> +{
> +       return 0;
> +}
> +
> +static inline void realtek_smi_exit(void)
> +{
> +}
> +
> +#endif
> +
> +#endif
> --
> 2.34.1
>
>
> It looks pretty reasonable to me. More stuff could go into
> realtek-interface-common.c, that could be called directly from
> realtek-smi.c and realtek-mdio.c without exporting anything.
>
> I've eliminated the possibility for the SMI and MDIO options to be
> anything other than y or n, because only a single interface module
> (the common one) exists, and the y/n/m quality of that is
> implied/selected by the drivers which depend on it. I hope I wasn't too
> trigger-happy with this.

Vladimir, you did all the work ;-) Thanks!

I implemented this code (with the fixes), and this is the result:

 30272 drivers/net/dsa/realtek/realtek-mdio.ko
 42176 drivers/net/dsa/realtek/realtek-smi.ko
114544 drivers/net/dsa/realtek/rtl8365mb.ko
115080 drivers/net/dsa/realtek/rtl8366.ko

 87264 drivers/net/dsa/realtek/realtek-interface.ko
114544 drivers/net/dsa/realtek/rtl8365mb.ko
115080 drivers/net/dsa/realtek/rtl8366.ko

It is still strange why merging both modules into a single
realtek-interface.ko results in a slightly larger file. Anyway, the
difference is not that significant. I still think that some systems
will miss that extra 30kb or 40kb for the interface code they don't
need.

Your proposed Kconfig does not attempt to avoid a realtek-interface
without both interfaces or without support for both switch families.
Is it possible in Kconfig to force it to, at least, select one of the
interfaces and one of the switches? Is it okay to leave it
unconstrained?

If merging the modules is the accepted solution, it makes me wonder if
rtl8365mb.ko and rtl8366.ko should get merged as well into a single
realtek-switch.ko. They are a hard dependency for realtek-interface.ko
(previously on each interface module). If the kernel is custom-built,
it would still be possible to exclude one switch family at build time.

I'll use these modules in OpenWrt, which builds a single kernel for a
bunch of devices. Is there a way to weakly depend on a module,
allowing the system to load only a single subdriver? Is it worth it?

Regards,

Luiz

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Linus Walleij]

On Mon, Nov 6, 2023 at 11:37 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> Your proposed Kconfig does not attempt to avoid a realtek-interface
> without both interfaces or without support for both switch families.
> Is it possible in Kconfig to force it to, at least, select one of the
> interfaces and one of the switches? Is it okay to leave it
> unconstrained?

Can't you just remove the help text under
NET_DSA_REALTEK_INTERFACE so it becomes a hidden
option? The other options just select it anyway.

> If merging the modules is the accepted solution, it makes me wonder if
> rtl8365mb.ko and rtl8366.ko should get merged as well into a single
> realtek-switch.ko. They are a hard dependency for realtek-interface.ko
> (previously on each interface module). If the kernel is custom-built,
> it would still be possible to exclude one switch family at build time.

That's not a good idea, because we want to be able to load
a single module into the kernel to support a single switch
family at runtime. If you have a kernel that boots on several
systems and some of them have one of the switches and
some of them have another switch, I think you see the problem
with this approach.

> I'll use these modules in OpenWrt, which builds a single kernel for a
> bunch of devices. Is there a way to weakly depend on a module,
> allowing the system to load only a single subdriver? Is it worth it?

Last time I looked actually having DSA:s as loadable modules
didn't work so well, so they are all compiled in. In OpenWrt
I didn't find any DSA modules packaged as modules. But maybe
I didn't try hard enough. IIRC the problem is that it needs to
also have a tag module (for NET_DSA_TAG_*) and that didn't
modularize so well.

Yours,
Linus Walleij

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Luiz Angelo Daros de Luca]

> > Your proposed Kconfig does not attempt to avoid a realtek-interface
> > without both interfaces or without support for both switch families.
> > Is it possible in Kconfig to force it to, at least, select one of the
> > interfaces and one of the switches? Is it okay to leave it
> > unconstrained?
>
> Can't you just remove the help text under
> NET_DSA_REALTEK_INTERFACE so it becomes a hidden
> option? The other options just select it anyway.

Without a text after the tristate, it will already be hidden. However,
we can still ask to build a module with no SMI and MDIO.

> > If merging the modules is the accepted solution, it makes me wonder if
> > rtl8365mb.ko and rtl8366.ko should get merged as well into a single
> > realtek-switch.ko. They are a hard dependency for realtek-interface.ko
> > (previously on each interface module). If the kernel is custom-built,
> > it would still be possible to exclude one switch family at build time.
>
> That's not a good idea, because we want to be able to load
> a single module into the kernel to support a single switch
> family at runtime. If you have a kernel that boots on several
> systems and some of them have one of the switches and
> some of them have another switch, I think you see the problem
> with this approach.

We already have this situation. As the interface module uses
rtl8366rb_variant and rtl8365mb_variant, we cannot select one or the
other at runtime.

rtl8365mb              14802  1 realtek_smi
rtl8366                20870  1 realtek_smi
tag_rtl4_a              1522  1

If we build it with support for both switches, both modules need to be
loaded together.

Somehow initializing the switch selectively autoloads the tag module.
Is it possible to have something like this for subdrivers?

> > I'll use these modules in OpenWrt, which builds a single kernel for a
> > bunch of devices. Is there a way to weakly depend on a module,
> > allowing the system to load only a single subdriver? Is it worth it?
>
> Last time I looked actually having DSA:s as loadable modules
> didn't work so well, so they are all compiled in. In OpenWrt
> I didn't find any DSA modules packaged as modules. But maybe
> I didn't try hard enough. IIRC the problem is that it needs to
> also have a tag module (for NET_DSA_TAG_*) and that didn't
> modularize so well.

It does work, even the tag module. As I mentioned, the tag modules are
even loaded on demand. You just need to load it in the correct
sequence.

>
> Yours,
> Linus Walleij

Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller

[Vladimir Oltean]

On Mon, Oct 30, 2023 at 09:30:45PM -0300, Luiz Angelo Daros de Luca wrote:
> The remove/shutdown are probably similar to any other DSA driver. I
> think the extra code around a shared code in a new module would be
> bigger than the duplicated code.

When you start looking at the duplicated parsing of vendor-specific OF
properties like "realtek,disable-leds", I am starting to question that.
There are also differences in the handling of the user_mii_bus, which
stem from the duplicate implementations, which are hard to justify if
you are saying that the only difference is that the switch is controlled
through a different interface and it is otherwise the same.