linux-kernel - Re: [PATCH v6 3/3] w1: add UART w1 bus driver

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]
Date: Tue, 13 Feb 2024 08:02:32 +0100
From: Christoph Winklhofer <cj.winklhofer@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Rob Herring <robh+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Jonathan Corbet <corbet@....net>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v6 3/3] w1: add UART w1 bus driver

On Mon, Feb 12, 2024 at 04:30:00PM +0100, Krzysztof Kozlowski wrote:
> On 09/02/2024 07:22, Christoph Winklhofer via B4 Relay wrote:
> > From: Christoph Winklhofer <cj.winklhofer@...il.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:
> 
> 
> > +/*
> > + * 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;
> > +	u8 tx_byte;
> > +};
> > +
> > +/*
> > + * struct w1_uart_config - w1-uart device data
> 
> That's neither correct (device, not config) nor proper kerneldoc nor
> useful. Your comment repeats struct name. If you want to make it
> kerneldoc, go ahead, but then make it a full kerneldoc.
> 

Yes, sorry - will use the correct name.

> And obviously compile with W=1.
> 

You mean the padding error of mutex, I get it with W=3 and will fix it
by moving mutex up.

> > + *
> > + * @serdev: serial device
> > + * @bus: w1-bus master
> > + * @cfg_reset: config for 1-Wire reset
> > + * @cfg_touch_0: config for 1-Wire write-0 cycle
> > + * @cfg_touch_1: config for 1-Wire write-1 and read cycle
> > + * @rx_byte_received: completion for serdev receive
> > + * @rx_err: indicates an error in serdev-receive
> > + * @rx_byte: result byte from serdev-receive
> > + * @mutex: mutex to protected rx_err and rx_byte from concurrent access
> > + *         in w1-callbacks and serdev-receive.
> > + */
> > +struct w1_uart_device {
> > +	struct serdev_device *serdev;
> > +	struct w1_bus_master bus;
> > +
> > +	struct w1_uart_config cfg_reset;
> > +	struct w1_uart_config cfg_touch_0;
> > +	struct w1_uart_config cfg_touch_1;
> > +
> > +	struct completion rx_byte_received;
> > +	int rx_err;
> > +	u8 rx_byte;
> > +
> 
> How did you solve my comment and checkpatch warning from previous version:
> 
> CHECK: struct mutex definition without comment
> 

Thanks, I missed the option --strict in checkpatch.pl and dit not get
this warning. Will add a comment.

> > +	struct mutex mutex;
> > +};
> > +
> > +/*
> > + * struct w1_uart_limits - limits for 1-Wire operations
> > + *
> > + * @baudrate: Requested baud-rate to create 1-Wire timing pattern
> > + * @bit_min_us: minimum time for a bit (in us)
> > + * @bit_max_us: maximum time for a bit (in us)
> > + * @sample_us: timespan to sample 1-Wire response
> > + * @cycle_us: duration of the 1-Wire cycle
> > + */
> > +struct w1_uart_limits {
> > +	unsigned int baudrate;
> > +	unsigned int bit_min_us;
> > +	unsigned int bit_max_us;
> > +	unsigned int sample_us;
> > +	unsigned int cycle_us;
> 
> ...
> 
> > +/*
> > + * Configuration for write-1 and read cycle (touch bit 1)
> > + * - bit_min_us is 5us, add margin and use 6us
> > + * - limits for sample time 5us-15us, use 15us
> > + */
> > +static int w1_uart_set_config_touch_1(struct w1_uart_device *w1dev)
> > +{
> > +	struct serdev_device *serdev = w1dev->serdev;
> > +	struct device_node *np = serdev->dev.of_node;
> > +
> > +	struct w1_uart_limits limits = { .baudrate = 115200,
> > +					 .bit_min_us = 6,
> > +					 .bit_max_us = 15,
> > +					 .sample_us = 15,
> > +					 .cycle_us = 70 };
> > +
> > +	of_property_read_u32(np, "write-1-bps", &limits.baudrate);
> > +
> > +	return w1_uart_set_config(serdev, &limits, &w1dev->cfg_touch_1);
> > +}
> > +
> > +/*
> > + * Configure and open the serial device
> > + */
> > +static int w1_uart_serdev_open(struct w1_uart_device *w1dev)
> > +{
> > +	struct serdev_device *serdev = w1dev->serdev;
> > +	struct device *dev = &serdev->dev;
> > +	int ret;
> > +
> > +	/* serdev is automatically closed on unbind or driver remove */
> 
> Drop comment, that's obvious. That's what devm* functions are for.
> 
> 

Ok.

> > +	ret = devm_serdev_device_open(dev, serdev);
> 
> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> > +	if (ret < 0) {
> > +		dev_err(dev, "set parity failed\n");
> > +		return ret;
> > +	}
> 
> 
> Best regards,
> Krzysztof
> 

Thanks!
Christoph