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]

Message-ID: <466d7be4-6ca1-4eb2-a59b-a3f0a846a2df@linaro.org>
Date: Mon, 12 Feb 2024 16:30:00 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: cj.winklhofer@...il.com, 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>
Cc: 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 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.

And obviously compile with W=1.

> + *
> + * @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

> +	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.


> +	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