From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Sender: Johan Hovold Date: Thu, 18 Jan 2018 13:23:55 +1100 From: Johan Hovold To: Ulrich Hecht Cc: linux-serial@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-bluetooth@vger.kernel.org, robh@kernel.org, martin.blumenstingl@googlemail.com, johan@kernel.org, marcel@holtmann.org, gregkh@linuxfoundation.org, magnus.damm@gmail.com, laurent.pinchart@ideasonboard.com, wsa@the-dreams.de, geert@linux-m68k.org Subject: Re: [PATCH] serdev: add method to set parity Message-ID: <20180118022355.GA3286@localhost> References: <1516203739-4705-1-git-send-email-ulrich.hecht+renesas@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1516203739-4705-1-git-send-email-ulrich.hecht+renesas@gmail.com> List-ID: On Wed, Jan 17, 2018 at 04:42:19PM +0100, Ulrich Hecht wrote: > Adds serdev_device_set_parity() and an implementation for ttyport. Perhaps you can mention that the interface uses an enum and the three settings that you add here. > Signed-off-by: Ulrich Hecht > --- > Broken out of the "[PATCH 0/6] serdev multiplexing support" series > because this kind of functionality is apparently also required > for "[RFC v2 0/9] Realtek Bluetooth serdev support (H5 protocol)", > which contains an earlier revision of this patch. Thanks for submitting this separately. > drivers/tty/serdev/core.c | 12 ++++++++++++ > drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++++++ > include/linux/serdev.h | 10 ++++++++++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c > index 5dc88f6..1f25896 100644 > --- a/drivers/tty/serdev/core.c > +++ b/drivers/tty/serdev/core.c > @@ -290,6 +290,18 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear) > } > EXPORT_SYMBOL_GPL(serdev_device_set_tiocm); > > +int serdev_device_set_parity(struct serdev_device *serdev, > + enum serdev_parity parity) > +{ > + struct serdev_controller *ctrl = serdev->ctrl; > + > + if (!ctrl || !ctrl->ops->set_parity) > + return -ENOTSUPP; > + > + return ctrl->ops->set_parity(ctrl, parity); > +} > +EXPORT_SYMBOL_GPL(serdev_device_set_parity); Please place the parity functions (and fields) after the set_flow equivalents so that we keep the line-setting functionality grouped together. > +static int ttyport_set_parity(struct serdev_controller *ctrl, > + enum serdev_parity parity) > +{ > + struct serport *serport = serdev_controller_get_drvdata(ctrl); > + struct tty_struct *tty = serport->tty; > + struct ktermios ktermios = tty->termios; > + > + ktermios.c_cflag &= ~(PARENB | PARODD); You should also clear CMSPAR. > + if (parity != SERDEV_PARITY_NONE) { > + ktermios.c_cflag |= PARENB; > + if (parity == SERDEV_PARITY_ODD) > + ktermios.c_cflag |= PARODD; > + } > + > + return tty_set_termios(tty, &ktermios); Note that tty_set_termios() always return 0. You need to verify that you got what you requested by looking at the termios after the function returns (and possibly return -EINVAL). Not all drivers support (all) parity modes. Note that we currently do not implement proper error handling for flow control, but that should be fixed. Looks good otherwise. Thanks, Johan From mboxrd@z Thu Jan 1 00:00:00 1970 From: Johan Hovold Subject: Re: [PATCH] serdev: add method to set parity Date: Thu, 18 Jan 2018 13:23:55 +1100 Message-ID: <20180118022355.GA3286@localhost> References: <1516203739-4705-1-git-send-email-ulrich.hecht+renesas@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1516203739-4705-1-git-send-email-ulrich.hecht+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: linux-bluetooth-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ulrich Hecht Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org, johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org, magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org, wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org, geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org List-Id: linux-serial@vger.kernel.org On Wed, Jan 17, 2018 at 04:42:19PM +0100, Ulrich Hecht wrote: > Adds serdev_device_set_parity() and an implementation for ttyport. Perhaps you can mention that the interface uses an enum and the three settings that you add here. > Signed-off-by: Ulrich Hecht > --- > Broken out of the "[PATCH 0/6] serdev multiplexing support" series > because this kind of functionality is apparently also required > for "[RFC v2 0/9] Realtek Bluetooth serdev support (H5 protocol)", > which contains an earlier revision of this patch. Thanks for submitting this separately. > drivers/tty/serdev/core.c | 12 ++++++++++++ > drivers/tty/serdev/serdev-ttyport.c | 18 ++++++++++++++++++ > include/linux/serdev.h | 10 ++++++++++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c > index 5dc88f6..1f25896 100644 > --- a/drivers/tty/serdev/core.c > +++ b/drivers/tty/serdev/core.c > @@ -290,6 +290,18 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear) > } > EXPORT_SYMBOL_GPL(serdev_device_set_tiocm); > > +int serdev_device_set_parity(struct serdev_device *serdev, > + enum serdev_parity parity) > +{ > + struct serdev_controller *ctrl = serdev->ctrl; > + > + if (!ctrl || !ctrl->ops->set_parity) > + return -ENOTSUPP; > + > + return ctrl->ops->set_parity(ctrl, parity); > +} > +EXPORT_SYMBOL_GPL(serdev_device_set_parity); Please place the parity functions (and fields) after the set_flow equivalents so that we keep the line-setting functionality grouped together. > +static int ttyport_set_parity(struct serdev_controller *ctrl, > + enum serdev_parity parity) > +{ > + struct serport *serport = serdev_controller_get_drvdata(ctrl); > + struct tty_struct *tty = serport->tty; > + struct ktermios ktermios = tty->termios; > + > + ktermios.c_cflag &= ~(PARENB | PARODD); You should also clear CMSPAR. > + if (parity != SERDEV_PARITY_NONE) { > + ktermios.c_cflag |= PARENB; > + if (parity == SERDEV_PARITY_ODD) > + ktermios.c_cflag |= PARODD; > + } > + > + return tty_set_termios(tty, &ktermios); Note that tty_set_termios() always return 0. You need to verify that you got what you requested by looking at the termios after the function returns (and possibly return -EINVAL). Not all drivers support (all) parity modes. Note that we currently do not implement proper error handling for flow control, but that should be fixed. Looks good otherwise. Thanks, Johan