[PATCH] serdev: add method to set parity

[PATCH] serdev: add method to set parity

[Ulrich Hecht]

Adds serdev_device_set_parity() and an implementation for ttyport.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
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.

CU
Uli


 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);
+
 static int serdev_drv_probe(struct device *dev)
 {
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index a5abb05..710e9a9 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -224,6 +224,23 @@ static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
 	return tty->driver->ops->tiocmset(tty, set, clear);
 }
 
+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);
+	if (parity != SERDEV_PARITY_NONE) {
+		ktermios.c_cflag |= PARENB;
+		if (parity == SERDEV_PARITY_ODD)
+			ktermios.c_cflag |= PARODD;
+	}
+
+	return tty_set_termios(tty, &ktermios);
+}
+
 static const struct serdev_controller_ops ctrl_ops = {
 	.write_buf = ttyport_write_buf,
 	.write_flush = ttyport_write_flush,
@@ -235,6 +252,7 @@ static const struct serdev_controller_ops ctrl_ops = {
 	.wait_until_sent = ttyport_wait_until_sent,
 	.get_tiocm = ttyport_get_tiocm,
 	.set_tiocm = ttyport_set_tiocm,
+	.set_parity = ttyport_set_parity,
 };
 
 struct device *serdev_tty_port_register(struct tty_port *port,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 48d8ce2..4dd3cb7 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -78,6 +78,12 @@ static inline struct serdev_device_driver *to_serdev_device_driver(struct device
 	return container_of(d, struct serdev_device_driver, driver);
 }
 
+enum serdev_parity {
+	SERDEV_PARITY_NONE,
+	SERDEV_PARITY_EVEN,
+	SERDEV_PARITY_ODD,
+};
+
 /*
  * serdev controller structures
  */
@@ -92,6 +98,7 @@ struct serdev_controller_ops {
 	void (*wait_until_sent)(struct serdev_controller *, long);
 	int (*get_tiocm)(struct serdev_controller *);
 	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
+	int (*set_parity)(struct serdev_controller *, enum serdev_parity);
 };
 
 /**
@@ -301,6 +308,9 @@ static inline int serdev_device_set_rts(struct serdev_device *serdev, bool enabl
 		return serdev_device_set_tiocm(serdev, 0, TIOCM_RTS);
 }
 
+int serdev_device_set_parity(struct serdev_device *serdev,
+			     enum serdev_parity parity);
+
 /*
  * serdev hooks into TTY core
  */
-- 
2.7.4

Re: [PATCH] serdev: add method to set parity

[Johan Hovold]

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 <ulrich.hecht+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> 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

Re: [PATCH] serdev: add method to set parity

[Marcel Holtmann]

Hi Ulrich,

>> 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 <ulrich.hecht+renesas@gmail.com>
>> ---
>> 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.

can I get an updated patch addressing the comments and also including the Reviewed-by tags.

Regards

Marcel

Re: [PATCH] serdev: add method to set parity

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4490 bytes --]

Hi,

On Wed, Jan 17, 2018 at 04:42:19PM +0100, Ulrich Hecht wrote:
> Adds serdev_device_set_parity() and an implementation for ttyport.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

-- Sebastian

> 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.
> 
> CU
> Uli
> 
> 
>  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);
> +
>  static int serdev_drv_probe(struct device *dev)
>  {
>  	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index a5abb05..710e9a9 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
> @@ -224,6 +224,23 @@ static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
>  	return tty->driver->ops->tiocmset(tty, set, clear);
>  }
>  
> +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);
> +	if (parity != SERDEV_PARITY_NONE) {
> +		ktermios.c_cflag |= PARENB;
> +		if (parity == SERDEV_PARITY_ODD)
> +			ktermios.c_cflag |= PARODD;
> +	}
> +
> +	return tty_set_termios(tty, &ktermios);
> +}
> +
>  static const struct serdev_controller_ops ctrl_ops = {
>  	.write_buf = ttyport_write_buf,
>  	.write_flush = ttyport_write_flush,
> @@ -235,6 +252,7 @@ static const struct serdev_controller_ops ctrl_ops = {
>  	.wait_until_sent = ttyport_wait_until_sent,
>  	.get_tiocm = ttyport_get_tiocm,
>  	.set_tiocm = ttyport_set_tiocm,
> +	.set_parity = ttyport_set_parity,
>  };
>  
>  struct device *serdev_tty_port_register(struct tty_port *port,
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 48d8ce2..4dd3cb7 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -78,6 +78,12 @@ static inline struct serdev_device_driver *to_serdev_device_driver(struct device
>  	return container_of(d, struct serdev_device_driver, driver);
>  }
>  
> +enum serdev_parity {
> +	SERDEV_PARITY_NONE,
> +	SERDEV_PARITY_EVEN,
> +	SERDEV_PARITY_ODD,
> +};
> +
>  /*
>   * serdev controller structures
>   */
> @@ -92,6 +98,7 @@ struct serdev_controller_ops {
>  	void (*wait_until_sent)(struct serdev_controller *, long);
>  	int (*get_tiocm)(struct serdev_controller *);
>  	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
> +	int (*set_parity)(struct serdev_controller *, enum serdev_parity);
>  };
>  
>  /**
> @@ -301,6 +308,9 @@ static inline int serdev_device_set_rts(struct serdev_device *serdev, bool enabl
>  		return serdev_device_set_tiocm(serdev, 0, TIOCM_RTS);
>  }
>  
> +int serdev_device_set_parity(struct serdev_device *serdev,
> +			     enum serdev_parity parity);
> +
>  /*
>   * serdev hooks into TTY core
>   */
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]