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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux