Received-SPF: pass (google.com: domain of linux-kernel+bounces-19935-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 8 Jan 2024 19:03:21 +0100
From: Christoph Winklhofer <cj.winklhofer@gmail.com>
To: Jiri Slaby <jirislaby@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>, Rob Herring <robh@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH v4 3/3] w1: add UART w1 bus driver
Message-ID: <ZZw46ZQ5JoxlWflG@cjw-notebook>
References: <20240106-w1-uart-v4-0-7fe1378a8b3e@gmail.com>
 <20240106-w1-uart-v4-3-7fe1378a8b3e@gmail.com>
 <5ff1d706-9f06-4eb6-bc86-75f933e54118@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5ff1d706-9f06-4eb6-bc86-75f933e54118@kernel.org>

On Mon, Jan 08, 2024 at 07:18:31AM +0100, Jiri Slaby wrote:
> On 06. 01. 24, 17:02, Christoph Winklhofer via B4 Relay wrote:
> > From: Christoph Winklhofer <cj.winklhofer@gmail.com>
> > 
> > Add a UART 1-Wire bus driver. The driver utilizes the UART interface via
> > the Serial Device Bus to create the 1-Wire timing patterns. The driver
> > was tested on a "Raspberry Pi 3B" with a DS18B20 and on a "Variscite
> > DART-6UL" with a DS18S20 temperature sensor.
> > 
> > The 1-Wire timing pattern and the corresponding UART baud-rate with the
> > interpretation of the transferred bytes are described in the document:
> > 
> > Link: https://www.analog.com/en/technical-articles/using-a-uart-to-implement-a-1wire-bus-master.html
> > 
> > In short, the UART peripheral must support full-duplex and operate in
> > open-drain mode. The timing patterns are generated by a specific
> > combination of baud-rate and transmitted byte, which corresponds to a
> > 1-Wire read bit, write bit or reset.
> ...
> > --- /dev/null
> > +++ b/drivers/w1/masters/w1-uart.c
> > @@ -0,0 +1,398 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * w1-uart - UART 1-Wire bus driver
> > + *
> > + * Uses the UART interface (via Serial Device Bus) to create the 1-Wire
> > + * timing patterns. Implements the following 1-Wire master interface:
> > + *
> > + * - reset_bus: requests baud-rate 9600
> > + *
> > + * - touch_bit: requests baud-rate 115200
> > + *
> > + * Author: Christoph Winklhofer <cj.winklhofer@gmail.com>
> > + */
> > +
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/serdev.h>
> > +#include <linux/w1.h>
> > +
> > +#define W1_UART_TIMEOUT msecs_to_jiffies(500)
> > +
> > +/*
> > + * struct w1_uart_config - configuration for 1-Wire operation
> > + *
> > + * @baudrate: baud-rate returned from serdev
> > + * @delay_us: delay to complete a 1-Wire cycle (in us)
> > + * @tx_byte: byte to generate 1-Wire timing pattern
> > + */
> > +struct w1_uart_config {
> > +	unsigned int baudrate;
> > +	unsigned int delay_us;
> > +	unsigned char tx_byte;
> 
> If it is a "byte", it should be u8.
> 
will change this and all others to u8.

...
> > +
> > +static inline unsigned int baud_to_bit_ns(unsigned int baud)
> > +{
> > +	return 1000000000 / baud;
> 
> NSEC_PER_SEC
> 
> > +}
> > +
> > +static inline unsigned int to_ns(unsigned int us)
> > +{
> > +	return us * 1000;
> 
> NSEC_PER_USEC
> 
and use the correct constants.

...
> > +}
> > +
> > +/*
> > + * Set baud-rate, delay and tx-byte to create a 1-Wire pulse and adapt
> > + * the tx-byte according to the actual baud-rate.
> > + *
> > + * Reject when:
> > + * - time for a bit outside min/max range
> > + * - a 1-Wire response is not detectable for sent byte
> > + */
> > +static int w1_uart_set_config(struct serdev_device *serdev,
> > +			      const struct w1_uart_limits *limits,
> > +			      struct w1_uart_config *w1cfg)
> > +{
...
> > +	/* 1-Wire response detectable for sent byte */
> > +	if (limits->sample_us > 0 &&
> > +	    bit_ns * 8 < low_ns + to_ns(limits->sample_us))
> 
> BITS_PER_BYTE
> 
ok, change it (it is the time for the UART data-frame).
> > +		return -EINVAL;
> > +
> > +	/* delay to complete 1-Wire cycle, include start and stop-bit */
> > +	w1cfg->delay_us = 0;
> > +	if (bit_ns * 10 < to_ns(limits->cycle_us))
> 
> What is this 10? Dub it.
> 
> > +		w1cfg->delay_us =
> > +			(to_ns(limits->cycle_us) - bit_ns * 10) / 1000;
> 
> And this 10?
> 
> The end: / NSEC_PER_USEC
> 
will be more explicit (it is the time for the UART packet:
BITS_PER_BYTE + 2 (start and stop-bit).

...
> > +static int w1_uart_serdev_receive_buf(struct serdev_device *serdev,
> > +				      const unsigned char *buf, size_t count)
> 
> serdev already uses u8 * here. You are basing on the top of some old tree.
Yes, this patch is based on the w1-next branch of
  git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-w1.git
was not sure from where to start. I guess that this change is probably in
the w1-tree after the next stable release.
> 
> regards,
> -- 
> js
> suse labs
> 
Thanks Jiri for the review!

Kind regards,
Christoph