netdev - Re: [PATCH v1] can: mcp251xfd: Enable transceiver using gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ac470f9-c918-49d5-8b71-0b7c6ed2c83e@intel.com>
Date: Tue, 6 Aug 2024 13:03:00 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Anup Kulkarni <quic_anupkulk@...cinc.com>
CC: <quic_msavaliy@...cinc.com>, <quic_vdadhani@...cinc.com>,
	<mkl@...gutronix.de>, <manivannan.sadhasivam@...aro.org>,
	<thomas.kopp@...rochip.com>, <mailhol.vincent@...adoo.fr>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <linux-can@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] can: mcp251xfd: Enable transceiver using gpio

On 8/6/24 11:03, Anup Kulkarni wrote:
> Ensure the CAN transceiver is active during mcp251xfd_open() and
> inactive during mcp251xfd_close() by utilizing
> mcp251xfd_transceiver_mode(). Adjust GPIO_0 to switch between
> NORMAL and STANDBY modes of transceiver.
> 
> Signed-off-by: Anup Kulkarni <quic_anupkulk@...cinc.com>
> ---
>   .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 32 +++++++++++++++++++
>   drivers/net/can/spi/mcp251xfd/mcp251xfd.h     |  7 ++++
>   2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index 3e7526274e34..3b56dc1721a5 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -153,6 +153,25 @@ static inline int mcp251xfd_vdd_disable(const struct mcp251xfd_priv *priv)
>   	return regulator_disable(priv->reg_vdd);
>   }
>   
> +static int
> +mcp251xfd_transceiver_mode(const struct mcp251xfd_priv *priv,
> +			   const enum mcp251xfd_xceiver_mode mode)
> +{
> +	int val, pmode, latch;
> +
> +	if (mode == MCP251XFD_XCVR_NORMAL_MODE) {
> +		pmode = MCP251XFD_REG_IOCON_PM0;
> +		latch = 0;
> +	} else if (mode == MCP251XFD_XCVR_STBY_MODE) {
> +		pmode = MCP251XFD_REG_IOCON_PM0;
> +		latch = MCP251XFD_REG_IOCON_LAT0;
> +	} else {
> +		return -EINVAL;
> +	}

@pmode is always the same, no need for separate assignment
this if-else chain will be better as an switch statement

> +	val = (pmode | latch) << priv->transceiver_pin;

put a newline here

> +	return regmap_write(priv->map_reg, MCP251XFD_REG_IOCON, val);
> +}
> +
>   static inline int
>   mcp251xfd_transceiver_enable(const struct mcp251xfd_priv *priv)
>   {
> @@ -1620,6 +1639,10 @@ static int mcp251xfd_open(struct net_device *ndev)
>   	if (err)
>   		goto out_transceiver_disable;
>   
> +	err = mcp251xfd_transceiver_mode(priv, MCP251XFD_XCVR_NORMAL_MODE);
> +	if (err)
> +		goto out_transceiver_disable;
> +
>   	clear_bit(MCP251XFD_FLAGS_DOWN, priv->flags);
>   	can_rx_offload_enable(&priv->offload);
>   
> @@ -1668,6 +1691,7 @@ static int mcp251xfd_open(struct net_device *ndev)
>   
>   static int mcp251xfd_stop(struct net_device *ndev)
>   {
> +	int err;
>   	struct mcp251xfd_priv *priv = netdev_priv(ndev);
>   
>   	netif_stop_queue(ndev);
> @@ -1678,6 +1702,9 @@ static int mcp251xfd_stop(struct net_device *ndev)
>   	free_irq(ndev->irq, priv);
>   	destroy_workqueue(priv->wq);
>   	can_rx_offload_disable(&priv->offload);
> +	err = mcp251xfd_transceiver_mode(priv, MCP251XFD_XCVR_STBY_MODE);
> +	if (err)
> +		return err;

perhaps it would be better to continue here anyway?

>   	mcp251xfd_chip_stop(priv, CAN_STATE_STOPPED);
>   	mcp251xfd_transceiver_disable(priv);
>   	mcp251xfd_ring_free(priv);
> @@ -2051,6 +2078,11 @@ static int mcp251xfd_probe(struct spi_device *spi)
>   					     "Failed to get clock-frequency!\n");
>   	}
>   
> +	err = device_property_read_u32(&spi->dev, "gpio-transceiver-pin", &priv->transceiver_pin);
> +		if (err)
> +			return dev_err_probe(&spi->dev, err,
> +					     "Failed to get gpio transceiver pin!\n");
> +

remember to fix it as Christophe J. has pointed out

>   	/* Sanity check */
>   	if (freq < MCP251XFD_SYSCLOCK_HZ_MIN ||
>   	    freq > MCP251XFD_SYSCLOCK_HZ_MAX) {
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> index dcbbd2b2fae8..14b086814bdb 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> @@ -614,6 +614,12 @@ enum mcp251xfd_flags {
>   	__MCP251XFD_FLAGS_SIZE__
>   };
>   
> +enum mcp251xfd_xceiver_mode {
> +	MCP251XFD_XCVR_NORMAL_MODE,
> +	MCP251XFD_XCVR_STBY_MODE,
> +	MCP251XFD_XCVR_MODE_NONE

no need to add NONE mode if you don't make any use of it,
also the name is not consistent with the other modes

> +};
> +
>   struct mcp251xfd_priv {
>   	struct can_priv can;
>   	struct can_rx_offload offload;
> @@ -670,6 +676,7 @@ struct mcp251xfd_priv {
>   
>   	struct mcp251xfd_devtype_data devtype_data;
>   	struct can_berr_counter bec;
> +	u32 transceiver_pin;
>   };
>   
>   #define MCP251XFD_IS(_model) \