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@xxxxxxxxx>
> >
> > 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
