Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc: linux-can@vger.kernel.org, "Stefan Mätje" <Stefan.Maetje@esd.eu>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min
Date: Mon, 16 Aug 2021 10:42:35 +0200	[thread overview]
Message-ID: <20210816084235.fr7fzau2ce7zl4d4@pengutronix.de> (raw)
In-Reply-To: <20210815033248.98111-3-mailhol.vincent@wanadoo.fr>

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

On 15.08.2021 12:32:43, Vincent Mailhol wrote:
> ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
> compensation" that "the configuration range for [the] SSP position
> shall be at least 0 to 63 minimum time quanta."
> 
> Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
> TDCO to hold zero value in order to honor SSP's minimum possible
> value.
> 
> However, current implementation assigned special meaning to TDCV and
> TDCO's zero values:
>   * TDCV = 0 -> TDCV is automatically measured by the transceiver.
>   * TDCO = 0 -> TDC is off.
> 
> In order to allow for those values to really be zero and to maintain
> current features, we introduce two new flags:
>   * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
>     automatic measurement of TDCV.
>   * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
>     manual configuration of TDCV. N.B.: current implementation failed
>     to provide an option for the driver to indicate that only manual
>     mode was supported.
> 
> TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
> CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
> can_tdc_is_enabled() which is also introduced in this patch.

Nitpick: We can only say that TDC is disabled, if the driver supports
the TDC interface at all, which is the case if tdc_const is set.

> Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
> struct can_tdc_const. While we are not convinced that those three
> fields could be anything else than zero, we can imagine that some
> controllers might specify a lower bound on these. Thus, those minimums
> are really added "just in case".

I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
-64...63.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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