From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1f6BIY-0002nB-3i for barebox@lists.infradead.org; Wed, 11 Apr 2018 08:40:11 +0000 Date: Wed, 11 Apr 2018 10:39:57 +0200 From: Sascha Hauer Message-ID: <20180411083957.pvagr7h4my4lyfpo@pengutronix.de> References: <20180326130915.8726-1-andrew.smirnov@gmail.com> <20180326130915.8726-3-andrew.smirnov@gmail.com> <20180403065436.p2vec7ntdja55hts@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "barebox" Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 2/3] console: Add simplified 'serdev' framework from Linux kernel To: Andrey Smirnov Cc: Barebox List On Mon, Apr 09, 2018 at 09:40:46AM -0700, Andrey Smirnov wrote: > On Mon, Apr 2, 2018 at 11:54 PM, Sascha Hauer wrote: > > Hi Andrey, > > > > Some comments inside. > > > > > > On Mon, Mar 26, 2018 at 06:09:14AM -0700, Andrey Smirnov wrote: > >> Port 'serdev' UART-slave deivce framework found in recent Linux > >> kernels (post 4.13) in order to be able to port 'serdev' slave drivers > >> from Linux. > >> > >> Signed-off-by: Andrey Smirnov > >> @@ -323,6 +324,17 @@ int console_register(struct console_device *newcdev) > >> dev->parent = newcdev->dev; > >> platform_device_register(dev); > >> > >> + newcdev->open_count = 0; > >> + > >> + /* > >> + * If our console deive is a serdev, we skip the creation of > > > > s/deive/device/ > > Will fix in v2. > > > > >> + * corresponding entry in /dev as well as registration in > >> + * console_list and just go straigh to populating child > > > > s/straigh/straight/ > > Ditto. > > > > >> + * devices. > >> + */ > >> + if (serdev_node) > >> + return of_platform_populate(serdev_node, NULL, dev); > > > > How is this going to be used? A serdev driver binds to the serdev_node > > and then it probably needs to get a pointer to the console device, > > right? How does the driver accomplish this? > > > > Serdev slave driver doesn't hold explicit pointer to console device, > instead accessing it via point to serdev_device. The latter could > obtained by calling to_serdev_device(dev->parent), where dev is > device_d given to slave driver's probe function. > > > >> +/** > >> + * struct serdev_device - Basic representation of an serdev device > >> + * > >> + * @dev: Corresponding device > >> + * @fifo: Circular buffer used for console draining > >> + * @buf: Buffer used to pass Rx data to consumers > >> + * @poller Async poller used to poll this serdev > >> + * @polling_interval: Async poller periodicity > >> + * @polling_window: Duration of a single busy loop poll > >> + * @receive_buf: Function called with data received from device; > >> + * returns number of bytes accepted; > >> + */ > >> +struct serdev_device { > >> + struct device_d *dev; > >> + struct kfifo *fifo; > >> + unsigned char *buf; > >> + struct poller_async poller; > >> + uint64_t polling_interval; > >> + uint64_t polling_window; > >> + > >> + int (*receive_buf)(struct serdev_device *, const unsigned char *, > >> + size_t); > >> +}; > >> + > >> +int serdev_device_open(struct serdev_device *); > >> +unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int); > >> +int serdev_device_write(struct serdev_device *, const unsigned char *, > >> + size_t, unsigned long); > > > > So a serdev driver uses serdev_device_write() to send characters out. To > > receive characters it has to implement serdev_device->receive_buf, > > right? > > Right. I tried to implement exactly the same API that Linux's serdev > API provides. > > > What kind of devices did you implement this for? > > I ported serdev in support of porting the driver for RAVE SP which is > a small microcontroller device found many ZII board including RDU2. It > implement a whole bunch of various functionality including watchdog, > parameter EEPROM, sensor access, backlight control, button input event > generation, etc. > > > For devices which send data without request (GPS?) this seems the way to go. For > > others a synchronous receive function might be good, no? > > > > I didn't implement anything like that mostly because Linux serdev API > doesn't and any ported driver wouldn't have any need for those > functions. But in general, I am not sure how useful synchronous > receive function would be. In my experience, devices like that usually > implement some binary transport protocol with packetization/escape > sequences on top of UART, which usually requires a state machine > operating with byte granularity as the data comes in to parse > responses correctly and synchronous APIs are not extremely useful for > that kind of a use-case. > > FWIW, since serdev API is integrated into poller infrastructure it is > pretty trivial to write blocking code with it. Here's how I use it in > my driver to implement request-response type of a function: > > rave_sp_write(sp, data, data_size); > /* > * is_timeout will implicitly poll serdev via poller > * infrastructure > */ > while (!is_timeout(start, SECOND) && !reply.received) > ; I understand that synchronous receiving might not be that useful. Given how simple it is we could add a synchronous receive wrapper function just for the sake of completeness, even if it only provides an example how the code can be used. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox