Re: [PATCH v5 2/2] phy: add basic support for NXPs TJA1145 CAN transceiver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver fails to enable the hardware power supplies (`vcc-supply`, `vio-supply`, `vbat-supply`) before communicating with the device.
- [High] The driver does not observe the required mode transition delay when powering on the PHY.
- [Medium] Missing rollback to standby mode in the error path of `tja1145_phy_power_on`.
--

commit 288134e179992cb2e690ff7a97502e72f04e9d5a
Author: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

phy: add basic support for NXPs TJA1145 CAN transceiver

This commit adds basic driver support for the NXP TJA1145 CAN transceiver.
It implements the PHY operations to bring the hardware up and down by
switching between normal and standby modes using SPI commands.

> diff --git a/drivers/phy/phy-nxp-tja1145.c b/drivers/phy/phy-nxp-tja1145.c
> new file mode 100644
> index 0000000000000..1e8bd169743ab
> --- /dev/null
> +++ b/drivers/phy/phy-nxp-tja1145.c

[ ... ]

> +static int tja1145_phy_power_on(struct phy *phy)
> +{
> +	struct regmap *map = phy_get_drvdata(phy);
> +	int ret;
> +
> +	/*
> +	 * Switch operating mode to normal which is the active operating mode.
> +	 * In this mode, the device is fully operational.
> +	 */
> +	ret = regmap_update_bits(map, TJA1145_MODE_CTRL, TJA1145_MODE_CTRL_MC,
> +				 TJA1145_MODE_CTRL_NORMAL);
> +	if (ret)
> +		return ret;

According to the hardware datasheet, transitioning the transceiver to normal
mode can take up to 40 us. Does this sequence need a hardware transition
delay before sending the active mode command?

With a fast SPI bus, the next command might arrive before the hardware state
machine finishes transitioning, which could cause the active mode command to
be ignored.

> +
> +	/*
> +	 * Switch to CAN operating mode active where the PHY can transmit and
> +	 * receive data.
> +	 */
> +	return regmap_update_bits(map, TJA1145_CAN_CTRL, TJA1145_CAN_CTRL_CMC,
> +				  TJA1145_CAN_CTRL_ACTIVE);

If this second regmap_update_bits() fails, the function returns an error
without reverting the device to standby mode.

Since the PHY framework keeps the power_count at 0 on an error return,
phy_power_off() will not be called during shutdown or suspend. Could this
leave the hardware permanently stuck in normal mode, drawing power
unintentionally?

> +}

[ ... ]

> +static int tja1145_probe(struct spi_device *spi)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &spi->dev;
> +	struct regmap *map;
> +	struct phy *phy;
> +	int ret;
> +
> +	map = devm_regmap_init_spi(spi, &tja1145_regmap_config);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map), "failed to init regmap\n");
> +
> +	ret = tja1145_check_ident(dev, map);

Will this fail to communicate with the hardware on platforms where power
supplies are dynamically controlled?

The device tree binding defines vcc-supply, vio-supply, and vbat-supply as
power sources, but they do not appear to be requested or enabled in this
function before tja1145_check_ident() is called.

This might leave the PHY unpowered, causing the SPI read to return garbage
data and fail the probe with -ENODEV.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to identify device\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-tja1145-support-v5-0-38720a7ee63e@liebherr.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy