[PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Sean Nyekjaer]

This series adds support for setting the nWKRQ voltage.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes in v2:
- Converted tcan4x5x.txt to DT schema. In
  https://lore.kernel.org/linux-can/20241105-convert-tcan-v2-1-4b320f3fcf99@geanix.com/
- Reworked ti,nwkrq-voltage-sel, to DH schema style.
- Link to v1: https://lore.kernel.org/r/20241031-tcan-wkrqv-v1-0-823dbd12fe4a@geanix.com

---
Sean Nyekjaer (2):
      can: tcan4x5x: add option for selecting nWKRQ voltage
      dt-bindings: can: tcan4x5x: Document the ti,nwkrq-voltage-sel option

 .../devicetree/bindings/net/can/ti,tcan4x5x.yaml   | 13 ++++++++
 drivers/net/can/m_can/tcan4x5x-core.c              | 35 ++++++++++++++++++++++
 drivers/net/can/m_can/tcan4x5x.h                   |  2 ++
 3 files changed, 50 insertions(+)
---
base-commit: 2b2a9a08f8f0b904ea2bc61db3374421b0f944a6
change-id: 20241030-tcan-wkrqv-c62b906005da
prerequisite-change-id: 20241105-convert-tcan-4b516424ecf6:v2
prerequisite-patch-id: a652b1a16dadd5ff525d0b58fec56f605a976aa3

Best regards,
-- 
Sean Nyekjaer <sean@geanix.com>

[PATCH v2 1/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Sean Nyekjaer]

nWKRQ supports an output voltage of either the internal reference voltage
(3.6V) or the reference voltage of the digital interface 0 - 6V.
Add the devicetree option ti,nwkrq-voltage-sel to be able to select
between them.
Default is kept as the internal reference voltage.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/net/can/m_can/tcan4x5x.h      |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 2f73bf3abad889c222f15c39a3d43de1a1cf5fbb..264bba830be50033347056da994102f8b614e51b 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -92,6 +92,8 @@
 #define TCAN4X5X_MODE_STANDBY BIT(6)
 #define TCAN4X5X_MODE_NORMAL BIT(7)
 
+#define TCAN4X5X_NWKRQ_VOLTAGE_MASK BIT(19)
+
 #define TCAN4X5X_DISABLE_WAKE_MSK	(BIT(31) | BIT(30))
 #define TCAN4X5X_DISABLE_INH_MSK	BIT(9)
 
@@ -267,6 +269,11 @@ static int tcan4x5x_init(struct m_can_classdev *cdev)
 	if (ret)
 		return ret;
 
+	ret = regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
+				 TCAN4X5X_NWKRQ_VOLTAGE_MASK, tcan4x5x->nwkrq_voltage);
+	if (ret)
+		return ret;
+
 	return ret;
 }
 
@@ -318,6 +325,28 @@ static const struct tcan4x5x_version_info
 	return &tcan4x5x_versions[TCAN4X5X];
 }
 
+static int tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
+{
+	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
+	struct device_node *np = cdev->dev->of_node;
+	u8 prop;
+	int ret;
+
+	ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
+	if (!ret) {
+		if (prop <= 1)
+			tcan4x5x->nwkrq_voltage = prop;
+		else
+			dev_warn(cdev->dev,
+				 "nwkrq-voltage-sel have invalid option: %u\n",
+				 prop);
+	} else {
+		tcan4x5x->nwkrq_voltage = 0;
+	}
+
+	return 0;
+}
+
 static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
 			      const struct tcan4x5x_version_info *version_info)
 {
@@ -453,6 +482,12 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 		goto out_power;
 	}
 
+	ret = tcan4x5x_get_dt_data(mcan_class);
+	if (ret) {
+		dev_err(&spi->dev, "Getting dt data failed %pe\n", ERR_PTR(ret));
+		goto out_power;
+	}
+
 	tcan4x5x_check_wake(priv);
 
 	ret = tcan4x5x_write_tcan_reg(mcan_class, TCAN4X5X_INT_EN, 0);
diff --git a/drivers/net/can/m_can/tcan4x5x.h b/drivers/net/can/m_can/tcan4x5x.h
index e62c030d3e1e5a713c997e7c8ecad4a44aff4e6a..04ebe5c64f4f7056a62e72e717cb85dd3817ab9c 100644
--- a/drivers/net/can/m_can/tcan4x5x.h
+++ b/drivers/net/can/m_can/tcan4x5x.h
@@ -42,6 +42,8 @@ struct tcan4x5x_priv {
 
 	struct tcan4x5x_map_buf map_buf_rx;
 	struct tcan4x5x_map_buf map_buf_tx;
+
+	u8 nwkrq_voltage;
 };
 
 static inline void

-- 
2.46.2

[PATCH v2 2/2] dt-bindings: can: tcan4x5x: Document the ti,nwkrq-voltage-sel option

[Sean Nyekjaer]

nWKRQ supports an output voltage of either the internal reference voltage
(3.6V) or the reference voltage of the digital interface 0 - 6V.
Add the devicetree option ti,nwkrq-voltage-sel to be able to select
between them.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
index f1d18a5461e05296998ae9bf09bdfa1226580131..a77c560868d689e92ded08b9deb43e5a2b89bf2b 100644
--- a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
+++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
@@ -106,6 +106,18 @@ properties:
       Must be half or less of "clocks" frequency.
     maximum: 18000000
 
+  ti,nwkrq-voltage-sel:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      nWKRQ Pin GPO buffer voltage rail configuration.
+      The option of this properties will tell which
+      voltage rail is used for the nWKRQ Pin.
+    oneOf:
+      - description: Internal voltage rail
+        const: 0
+      - description: VIO voltage rail
+        const: 1
+
   wakeup-source:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
@@ -157,6 +169,7 @@ examples:
             device-state-gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
             device-wake-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
             reset-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
+            ti,nwkrq-voltage-sel = /bits/ 8 <0>;
             wakeup-source;
         };
     };

-- 
2.46.2

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Jakub Kicinski]

On Mon, 11 Nov 2024 09:54:48 +0100 Sean Nyekjaer wrote:
> This series adds support for setting the nWKRQ voltage.

There is no need to CC netdev@ on pure drivers/net/can changes.
Since these changes are not tagged in any way I have to manually
go and drop all of them from our patchwork.

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Sean Nyekjaer]

Hi Jakub,

On Mon, Nov 11, 2024 at 10:10:11AM +0100, Jakub Kicinski wrote:
> On Mon, 11 Nov 2024 09:54:48 +0100 Sean Nyekjaer wrote:
> > This series adds support for setting the nWKRQ voltage.
> 
> There is no need to CC netdev@ on pure drivers/net/can changes.
> Since these changes are not tagged in any way I have to manually
> go and drop all of them from our patchwork.

Oh sorry for that.
I'm using b4's --auto-to-cc feature, any way to fix that?

Br,
/Sean

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

On 12.11.2024 07:39:12, Sean Nyekjaer wrote:
> On Mon, Nov 11, 2024 at 10:10:11AM +0100, Jakub Kicinski wrote:
> > On Mon, 11 Nov 2024 09:54:48 +0100 Sean Nyekjaer wrote:
> > > This series adds support for setting the nWKRQ voltage.
> > 
> > There is no need to CC netdev@ on pure drivers/net/can changes.
> > Since these changes are not tagged in any way I have to manually
> > go and drop all of them from our patchwork.

Does the prefix "can-next" help, i.e.:

| [PATCH can-next v2 0/2]

which can be configured via:

| b4 prep --set-prefixes "can-next"

> Oh sorry for that.
> I'm using b4's --auto-to-cc feature, any way to fix that?

You can manually trim the list of Cc: using:

| b4 prep --edit-cover

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] dt-bindings: can: tcan4x5x: Document the ti,nwkrq-voltage-sel option

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

On 11.11.2024 09:54:50, Sean Nyekjaer wrote:
> nWKRQ supports an output voltage of either the internal reference voltage
> (3.6V) or the reference voltage of the digital interface 0 - 6V.
> Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> between them.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> index f1d18a5461e05296998ae9bf09bdfa1226580131..a77c560868d689e92ded08b9deb43e5a2b89bf2b 100644
> --- a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> +++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> @@ -106,6 +106,18 @@ properties:
>        Must be half or less of "clocks" frequency.
>      maximum: 18000000
>  
> +  ti,nwkrq-voltage-sel:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      nWKRQ Pin GPO buffer voltage rail configuration.
> +      The option of this properties will tell which
> +      voltage rail is used for the nWKRQ Pin.
> +    oneOf:
> +      - description: Internal voltage rail
> +        const: 0
> +      - description: VIO voltage rail
> +        const: 1

We usually don't want to put register values into the DT. Is 0, i.e. the
internal voltage rail the default? Is using a boolean better here?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On 11.11.2024 09:54:48, Sean Nyekjaer wrote:
> This series adds support for setting the nWKRQ voltage.

IIRC the yaml change should be made before the driver change. Please
make the yaml changes the 1st patch in the series.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] dt-bindings: can: tcan4x5x: Document the ti,nwkrq-voltage-sel option

[Sean Nyekjaer]

Hi Marc,

On Tue, Nov 12, 2024 at 08:35:43AM +0100, Marc Kleine-Budde wrote:
> On 11.11.2024 09:54:50, Sean Nyekjaer wrote:
> > nWKRQ supports an output voltage of either the internal reference voltage
> > (3.6V) or the reference voltage of the digital interface 0 - 6V.
> > Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> > between them.
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> > index f1d18a5461e05296998ae9bf09bdfa1226580131..a77c560868d689e92ded08b9deb43e5a2b89bf2b 100644
> > --- a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> > @@ -106,6 +106,18 @@ properties:
> >        Must be half or less of "clocks" frequency.
> >      maximum: 18000000
> >  
> > +  ti,nwkrq-voltage-sel:
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    description:
> > +      nWKRQ Pin GPO buffer voltage rail configuration.
> > +      The option of this properties will tell which
> > +      voltage rail is used for the nWKRQ Pin.
> > +    oneOf:
> > +      - description: Internal voltage rail
> > +        const: 0
> > +      - description: VIO voltage rail
> > +        const: 1
> 
> We usually don't want to put register values into the DT. Is 0, i.e. the
> internal voltage rail the default? Is using a boolean better here?
> 
> regards,
> Marc
> 

Thanks for the review :)

Can you come up with a sane naming?
A boolean that equals true when it's set to VIO voltage? Or the other
way around?

/Sean

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Sean Nyekjaer]

Hi Marc,

On Tue, Nov 12, 2024 at 08:38:26AM +0100, Marc Kleine-Budde wrote:
> On 11.11.2024 09:54:48, Sean Nyekjaer wrote:
> > This series adds support for setting the nWKRQ voltage.
> 
> IIRC the yaml change should be made before the driver change. Please
> make the yaml changes the 1st patch in the series.
> 
> Marc
> 

I know, so I have added, prerequisite-change-id as pr the b4 manual.

/Sean

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On 12.11.2024 08:44:00, Sean Nyekjaer wrote:
> Hi Marc,
> 
> On Tue, Nov 12, 2024 at 08:38:26AM +0100, Marc Kleine-Budde wrote:
> > On 11.11.2024 09:54:48, Sean Nyekjaer wrote:
> > > This series adds support for setting the nWKRQ voltage.
> > 
> > IIRC the yaml change should be made before the driver change. Please
> > make the yaml changes the 1st patch in the series.
> > 
> > Marc
> > 
> 
> I know, so I have added, prerequisite-change-id as pr the b4 manual.

I mean the order of patches in this series. First the yaml patch, then
the code change.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Sean Nyekjaer]

Hi Marc,

On Tue, Nov 12, 2024 at 08:52:14AM +0100, Marc Kleine-Budde wrote:
> On 12.11.2024 08:44:00, Sean Nyekjaer wrote:
> > Hi Marc,
> > 
> > On Tue, Nov 12, 2024 at 08:38:26AM +0100, Marc Kleine-Budde wrote:
> > > On 11.11.2024 09:54:48, Sean Nyekjaer wrote:
> > > > This series adds support for setting the nWKRQ voltage.
> > > 
> > > IIRC the yaml change should be made before the driver change. Please
> > > make the yaml changes the 1st patch in the series.
> > > 
> > > Marc
> > > 
> > 
> > I know, so I have added, prerequisite-change-id as pr the b4 manual.
> 
> I mean the order of patches in this series. First the yaml patch, then
> the code change.
> 
> regards,
> Marc
> 

Oh, noted thanks!

/Sean

Re: [PATCH v2 2/2] dt-bindings: can: tcan4x5x: Document the ti,nwkrq-voltage-sel option

[Rob Herring]

On Tue, Nov 12, 2024 at 08:40:56AM +0100, Sean Nyekjaer wrote:
> Hi Marc,
> 
> On Tue, Nov 12, 2024 at 08:35:43AM +0100, Marc Kleine-Budde wrote:
> > On 11.11.2024 09:54:50, Sean Nyekjaer wrote:
> > > nWKRQ supports an output voltage of either the internal reference voltage
> > > (3.6V) or the reference voltage of the digital interface 0 - 6V.
> > > Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> > > between them.
> > > 
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---
> > >  Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> > > index f1d18a5461e05296998ae9bf09bdfa1226580131..a77c560868d689e92ded08b9deb43e5a2b89bf2b 100644
> > > --- a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> > > +++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> > > @@ -106,6 +106,18 @@ properties:
> > >        Must be half or less of "clocks" frequency.
> > >      maximum: 18000000
> > >  
> > > +  ti,nwkrq-voltage-sel:
> > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > +    description:
> > > +      nWKRQ Pin GPO buffer voltage rail configuration.
> > > +      The option of this properties will tell which
> > > +      voltage rail is used for the nWKRQ Pin.
> > > +    oneOf:
> > > +      - description: Internal voltage rail
> > > +        const: 0
> > > +      - description: VIO voltage rail
> > > +        const: 1
> > 
> > We usually don't want to put register values into the DT. Is 0, i.e. the
> > internal voltage rail the default? Is using a boolean better here?
> > 
> > regards,
> > Marc
> > 
> 
> Thanks for the review :)
> 
> Can you come up with a sane naming?
> A boolean that equals true when it's set to VIO voltage? Or the other
> way around?

Make the property named/present for the less common case if there is 
one. That might not be known here.

Rob

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Jakub Kicinski]

On Tue, 12 Nov 2024 08:16:02 +0100 Marc Kleine-Budde wrote:
> > > There is no need to CC netdev@ on pure drivers/net/can changes.
> > > Since these changes are not tagged in any way I have to manually
> > > go and drop all of them from our patchwork.  
> 
> Does the prefix "can-next" help, i.e.:
> 
> | [PATCH can-next v2 0/2]
> 
> which can be configured via:
> 
> | b4 prep --set-prefixes "can-next"

Yup, prefix would make it easy for us to automatically discard !

> > Oh sorry for that.
> > I'm using b4's --auto-to-cc feature, any way to fix that?  
> 
> You can manually trim the list of Cc: using:
> 
> | b4 prep --edit-cover

My bad actually, I didn't realize we don't have an X: entries
on net/can/ under general networking in MAINTAINERS.

Would you mind if I added them?

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Vincent Mailhol]

On Thu. 14 Nov. 2024 at 12:37, Jakub Kicinski <kuba@kernel.org> wrote:
> My bad actually, I didn't realize we don't have an X: entries
> on net/can/ under general networking in MAINTAINERS.
>
> Would you mind if I added them?

OK for me. I guess you want to add the exclusion for both the

  CAN NETWORK DRIVERS

and the

  CAN NETWORK LAYER

entries in MAINTAINERS.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 1/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Vincent Mailhol]

On Mon. 11 Nov. 2024 at 17:55, Sean Nyekjaer <sean@geanix.com> wrote:
> nWKRQ supports an output voltage of either the internal reference voltage
> (3.6V) or the reference voltage of the digital interface 0 - 6V.
> Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> between them.
> Default is kept as the internal reference voltage.
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/net/can/m_can/tcan4x5x.h      |  2 ++
>  2 files changed, 37 insertions(+)
>
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 2f73bf3abad889c222f15c39a3d43de1a1cf5fbb..264bba830be50033347056da994102f8b614e51b 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -92,6 +92,8 @@
>  #define TCAN4X5X_MODE_STANDBY BIT(6)
>  #define TCAN4X5X_MODE_NORMAL BIT(7)
>
> +#define TCAN4X5X_NWKRQ_VOLTAGE_MASK BIT(19)
> +
>  #define TCAN4X5X_DISABLE_WAKE_MSK      (BIT(31) | BIT(30))
>  #define TCAN4X5X_DISABLE_INH_MSK       BIT(9)
>
> @@ -267,6 +269,11 @@ static int tcan4x5x_init(struct m_can_classdev *cdev)
>         if (ret)
>                 return ret;
>
> +       ret = regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
> +                                TCAN4X5X_NWKRQ_VOLTAGE_MASK, tcan4x5x->nwkrq_voltage);
> +       if (ret)
> +               return ret;
> +
>         return ret;
>  }
>
> @@ -318,6 +325,28 @@ static const struct tcan4x5x_version_info
>         return &tcan4x5x_versions[TCAN4X5X];
>  }
>
> +static int tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
> +{
> +       struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> +       struct device_node *np = cdev->dev->of_node;
> +       u8 prop;
> +       int ret;
> +
> +       ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
> +       if (!ret) {
> +               if (prop <= 1)
> +                       tcan4x5x->nwkrq_voltage = prop;
> +               else
> +                       dev_warn(cdev->dev,
> +                                "nwkrq-voltage-sel have invalid option: %u\n",
> +                                prop);
> +       } else {
> +               tcan4x5x->nwkrq_voltage = 0;
> +       }

If the

  if (prop <= 1)

condition fails, you print a warning, but you are not assigning a
value to tcan4x5x->nwkrq_voltage. Is this intentional?

What about:

        tcan4x5x->nwkrq_voltage = 0;
        ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
        if (!ret) {
                if (prop <= 1)
                        tcan4x5x->nwkrq_voltage = prop;
                else
                        dev_warn(cdev->dev,
                                 "nwkrq-voltage-sel have invalid option: %u\n",
                                 prop);
        }

so that you make sure that tcan4x5x->nwkrq_voltage always gets a
default zero value? Else, if you can make sure that tcan4x5x is always
zero initialized, you can just drop the

        tcan4x5x->nwkrq_voltage = 0;

thing.

> +       return 0;
> +}
> +
>  static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
>                               const struct tcan4x5x_version_info *version_info)
>  {
> @@ -453,6 +482,12 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>                 goto out_power;
>         }
>
> +       ret = tcan4x5x_get_dt_data(mcan_class);
> +       if (ret) {
> +               dev_err(&spi->dev, "Getting dt data failed %pe\n", ERR_PTR(ret));
> +               goto out_power;
> +       }
> +
>         tcan4x5x_check_wake(priv);
>
>         ret = tcan4x5x_write_tcan_reg(mcan_class, TCAN4X5X_INT_EN, 0);
> diff --git a/drivers/net/can/m_can/tcan4x5x.h b/drivers/net/can/m_can/tcan4x5x.h
> index e62c030d3e1e5a713c997e7c8ecad4a44aff4e6a..04ebe5c64f4f7056a62e72e717cb85dd3817ab9c 100644
> --- a/drivers/net/can/m_can/tcan4x5x.h
> +++ b/drivers/net/can/m_can/tcan4x5x.h
> @@ -42,6 +42,8 @@ struct tcan4x5x_priv {
>
>         struct tcan4x5x_map_buf map_buf_rx;
>         struct tcan4x5x_map_buf map_buf_tx;
> +
> +       u8 nwkrq_voltage;
>  };
>
>  static inline void
>
> --
> 2.46.2
>
>

Re: [PATCH v2 1/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Sean Nyekjaer]

Hi Vincent,

On Thu, Nov 14, 2024 at 01:53:34PM +0100, Vincent Mailhol wrote:
> On Mon. 11 Nov. 2024 at 17:55, Sean Nyekjaer <sean@geanix.com> wrote:
> > nWKRQ supports an output voltage of either the internal reference voltage
> > (3.6V) or the reference voltage of the digital interface 0 - 6V.
> > Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> > between them.
> > Default is kept as the internal reference voltage.
> >
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/net/can/m_can/tcan4x5x-core.c | 35 +++++++++++++++++++++++++++++++++++
> >  drivers/net/can/m_can/tcan4x5x.h      |  2 ++
> >  2 files changed, 37 insertions(+)
> >

[...]

> >
> > +static int tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
> > +{
> > +       struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> > +       struct device_node *np = cdev->dev->of_node;
> > +       u8 prop;
> > +       int ret;
> > +
> > +       ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
> > +       if (!ret) {
> > +               if (prop <= 1)
> > +                       tcan4x5x->nwkrq_voltage = prop;
> > +               else
> > +                       dev_warn(cdev->dev,
> > +                                "nwkrq-voltage-sel have invalid option: %u\n",
> > +                                prop);
> > +       } else {
> > +               tcan4x5x->nwkrq_voltage = 0;
> > +       }
> 
> If the
> 
>   if (prop <= 1)
> 
> condition fails, you print a warning, but you are not assigning a
> value to tcan4x5x->nwkrq_voltage. Is this intentional?
> 
> What about:
> 
>         tcan4x5x->nwkrq_voltage = 0;
>         ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
>         if (!ret) {
>                 if (prop <= 1)
>                         tcan4x5x->nwkrq_voltage = prop;
>                 else
>                         dev_warn(cdev->dev,
>                                  "nwkrq-voltage-sel have invalid option: %u\n",
>                                  prop);
>         }
> 
> so that you make sure that tcan4x5x->nwkrq_voltage always gets a
> default zero value? Else, if you can make sure that tcan4x5x is always
> zero initialized, you can just drop the
> 
>         tcan4x5x->nwkrq_voltage = 0;
> 
> thing.

Thanks for the review.
You are right, so I reworked this for v3:
https://lore.kernel.org/r/20241112-tcan-wkrqv-v3-0-c66423fba26d@geanix.com

> 
> > +       return 0;
> > +}
> > +

[...]

> >
> >

/Sean

Re: [PATCH v2 1/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Vincent Mailhol]

On Thu. 14 Nov. 2024 at 16:33, Sean Nyekjaer <sean@geanix.com> wrote:
> Hi Vincent,
> On Thu, Nov 14, 2024 at 01:53:34PM +0100, Vincent Mailhol wrote:
> > On Mon. 11 Nov. 2024 at 17:55, Sean Nyekjaer <sean@geanix.com> wrote:
> > > nWKRQ supports an output voltage of either the internal reference voltage
> > > (3.6V) or the reference voltage of the digital interface 0 - 6V.
> > > Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> > > between them.
> > > Default is kept as the internal reference voltage.
> > >
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---
> > >  drivers/net/can/m_can/tcan4x5x-core.c | 35 +++++++++++++++++++++++++++++++++++
> > >  drivers/net/can/m_can/tcan4x5x.h      |  2 ++
> > >  2 files changed, 37 insertions(+)
> > >
>
> [...]
>
> > >
> > > +static int tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
> > > +{
> > > +       struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> > > +       struct device_node *np = cdev->dev->of_node;
> > > +       u8 prop;
> > > +       int ret;
> > > +
> > > +       ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
> > > +       if (!ret) {
> > > +               if (prop <= 1)
> > > +                       tcan4x5x->nwkrq_voltage = prop;
> > > +               else
> > > +                       dev_warn(cdev->dev,
> > > +                                "nwkrq-voltage-sel have invalid option: %u\n",
> > > +                                prop);
> > > +       } else {
> > > +               tcan4x5x->nwkrq_voltage = 0;
> > > +       }
> >
> > If the
> >
> >   if (prop <= 1)
> >
> > condition fails, you print a warning, but you are not assigning a
> > value to tcan4x5x->nwkrq_voltage. Is this intentional?
> >
> > What about:
> >
> >         tcan4x5x->nwkrq_voltage = 0;
> >         ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
> >         if (!ret) {
> >                 if (prop <= 1)
> >                         tcan4x5x->nwkrq_voltage = prop;
> >                 else
> >                         dev_warn(cdev->dev,
> >                                  "nwkrq-voltage-sel have invalid option: %u\n",
> >                                  prop);
> >         }
> >
> > so that you make sure that tcan4x5x->nwkrq_voltage always gets a
> > default zero value? Else, if you can make sure that tcan4x5x is always
> > zero initialized, you can just drop the
> >
> >         tcan4x5x->nwkrq_voltage = 0;
> >
> > thing.
>
> Thanks for the review.
> You are right, so I reworked this for v3:
> https://lore.kernel.org/r/20241112-tcan-wkrqv-v3-0-c66423fba26d@geanix.com

I see. So this v2 was already outdated at the time of my review. Well,
glad to hear that this was proactively fixed in the v3 and sorry for
missing that.

Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

On 13.11.2024 19:37:09, Jakub Kicinski wrote:
> On Tue, 12 Nov 2024 08:16:02 +0100 Marc Kleine-Budde wrote:
> > > > There is no need to CC netdev@ on pure drivers/net/can changes.
> > > > Since these changes are not tagged in any way I have to manually
> > > > go and drop all of them from our patchwork.  
> > 
> > Does the prefix "can-next" help, i.e.:
> > 
> > | [PATCH can-next v2 0/2]
> > 
> > which can be configured via:
> > 
> > | b4 prep --set-prefixes "can-next"
> 
> Yup, prefix would make it easy for us to automatically discard !
> 
> > > Oh sorry for that.
> > > I'm using b4's --auto-to-cc feature, any way to fix that?  
> > 
> > You can manually trim the list of Cc: using:
> > 
> > | b4 prep --edit-cover
> 
> My bad actually, I didn't realize we don't have an X: entries
> on net/can/ under general networking in MAINTAINERS.
> 
> Would you mind if I added them?

Makes sense! I didn'k know that X: exists and that it makes total sense
here.

Feel free to add my:

Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On 14.11.2024 13:41:12, Vincent Mailhol wrote:
> On Thu. 14 Nov. 2024 at 12:37, Jakub Kicinski <kuba@kernel.org> wrote:
> > My bad actually, I didn't realize we don't have an X: entries
> > on net/can/ under general networking in MAINTAINERS.
                      ^^^^^^^^^^^^^^^^^^
> >
> > Would you mind if I added them?
> 
> OK for me. I guess you want to add the exclusion for both the
> 
>   CAN NETWORK DRIVERS
> 
> and the
> 
>   CAN NETWORK LAYER
> 
> entries in MAINTAINERS.

I thinks, it's the other way round.

General networking gets an X: for driver/net/can and driver/can/ and the
include files.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3781 bytes --]

On 14.11.2024 13:53:34, Vincent Mailhol wrote:
> On Mon. 11 Nov. 2024 at 17:55, Sean Nyekjaer <sean@geanix.com> wrote:
> > nWKRQ supports an output voltage of either the internal reference voltage
> > (3.6V) or the reference voltage of the digital interface 0 - 6V.
> > Add the devicetree option ti,nwkrq-voltage-sel to be able to select
> > between them.
> > Default is kept as the internal reference voltage.
> >
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/net/can/m_can/tcan4x5x-core.c | 35 +++++++++++++++++++++++++++++++++++
> >  drivers/net/can/m_can/tcan4x5x.h      |  2 ++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> > index 2f73bf3abad889c222f15c39a3d43de1a1cf5fbb..264bba830be50033347056da994102f8b614e51b 100644
> > --- a/drivers/net/can/m_can/tcan4x5x-core.c
> > +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> > @@ -92,6 +92,8 @@
> >  #define TCAN4X5X_MODE_STANDBY BIT(6)
> >  #define TCAN4X5X_MODE_NORMAL BIT(7)
> >
> > +#define TCAN4X5X_NWKRQ_VOLTAGE_MASK BIT(19)
> > +
> >  #define TCAN4X5X_DISABLE_WAKE_MSK      (BIT(31) | BIT(30))
> >  #define TCAN4X5X_DISABLE_INH_MSK       BIT(9)
> >
> > @@ -267,6 +269,11 @@ static int tcan4x5x_init(struct m_can_classdev *cdev)
> >         if (ret)
> >                 return ret;
> >
> > +       ret = regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
> > +                                TCAN4X5X_NWKRQ_VOLTAGE_MASK, tcan4x5x->nwkrq_voltage);
> > +       if (ret)
> > +               return ret;
> > +
> >         return ret;
> >  }
> >
> > @@ -318,6 +325,28 @@ static const struct tcan4x5x_version_info
> >         return &tcan4x5x_versions[TCAN4X5X];
> >  }
> >
> > +static int tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
> > +{
> > +       struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> > +       struct device_node *np = cdev->dev->of_node;
> > +       u8 prop;
> > +       int ret;
> > +
> > +       ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
> > +       if (!ret) {
> > +               if (prop <= 1)
> > +                       tcan4x5x->nwkrq_voltage = prop;
> > +               else
> > +                       dev_warn(cdev->dev,
> > +                                "nwkrq-voltage-sel have invalid option: %u\n",
> > +                                prop);
> > +       } else {
> > +               tcan4x5x->nwkrq_voltage = 0;
> > +       }
> 
> If the
> 
>   if (prop <= 1)
> 
> condition fails, you print a warning, but you are not assigning a
> value to tcan4x5x->nwkrq_voltage. Is this intentional?
> 
> What about:
> 
>         tcan4x5x->nwkrq_voltage = 0;
>         ret = of_property_read_u8(np, "ti,nwkrq-voltage-sel", &prop);
>         if (!ret) {
>                 if (prop <= 1)
>                         tcan4x5x->nwkrq_voltage = prop;
>                 else
>                         dev_warn(cdev->dev,
>                                  "nwkrq-voltage-sel have invalid option: %u\n",
>                                  prop);
>         }
> 
> so that you make sure that tcan4x5x->nwkrq_voltage always gets a
> default zero value? Else, if you can make sure that tcan4x5x is always
> zero initialized, you can just drop the

The tcan4x5x_priv is allocated in the netdev priv, which is initialized
with 0x0.

> 
>         tcan4x5x->nwkrq_voltage = 0;
> 
> thing.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] can: tcan4x5x: add option for selecting nWKRQ voltage

[Vincent Mailhol]

On 14/11/2024 at 18:03, Marc Kleine-Budde wrote:
> On 14.11.2024 13:41:12, Vincent Mailhol wrote:
>> On Thu. 14 Nov. 2024 at 12:37, Jakub Kicinski <kuba@kernel.org> wrote:
>>> My bad actually, I didn't realize we don't have an X: entries
>>> on net/can/ under general networking in MAINTAINERS.
>                        ^^^^^^^^^^^^^^^^^^
>>>
>>> Would you mind if I added them?
>>
>> OK for me. I guess you want to add the exclusion for both the
>>
>>    CAN NETWORK DRIVERS
>>
>> and the
>>
>>    CAN NETWORK LAYER
>>
>> entries in MAINTAINERS.
> 
> I thinks, it's the other way round.
> 
> General networking gets an X: for driver/net/can and driver/can/ and the
> include files.

Indeed. Now that you say it, makes perfect sense.

@Jakub, similar to Marc, feel free to add my Acked-by tag when you send 
such a patch.


Yours sincerely,
Vincent Mailhol