[v3,1/7] tty/serial: Add GPIOLIB helpers for controlling modem lines

Commit Message

Richard Genoud Feb. 17, 2014, 4:57 p.m. UTC

This patch add some helpers to control modem lines (CTS/RTS/DSR...) via
GPIO.
This will be useful for many boards which have a serial controller that
only handle CTS/RTS pins (or even just RX/TX).

Signed-off-by: Richard Genoud <[e-mail]>
---
 Documentation/serial/driver            |  21 +++++++
 drivers/tty/serial/Kconfig             |   3 +
 drivers/tty/serial/Makefile            |   3 +
 drivers/tty/serial/serial_mctrl_gpio.c | 112 +++++++++++++++++++++++++++++++++
 drivers/tty/serial/serial_mctrl_gpio.h |  92 +++++++++++++++++++++++++++
 5 files changed, 231 insertions(+)
 create mode 100644 drivers/tty/serial/serial_mctrl_gpio.c
 create mode 100644 drivers/tty/serial/serial_mctrl_gpio.h

Comments

Alexander Shiyan Feb. 17, 2014, 6:37 p.m. UTC | #1

Hello.

A few comments below..

???????????, 17 ??????? 2014, 17:57 +01:00 ?? Richard Genoud <[e-mail]>:
> This patch add some helpers to control modem lines (CTS/RTS/DSR...) via
> GPIO.
> This will be useful for many boards which have a serial controller that
> only handle CTS/RTS pins (or even just RX/TX).
> 
> Signed-off-by: Richard Genoud <[e-mail]>
> ---
...
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
...
> +static const struct {
> +	const char *name;
> +	unsigned int mctrl;
> +	bool dir_out;
> +} mctrl_gpios_desc[] = {
> +	{ "cts", TIOCM_CTS, false, },
> +	{ "dsr", TIOCM_DSR, false, },
> +	{ "dcd", TIOCM_CD, false, },
> +	{ "ri", TIOCM_RI, false, },
> +	{ "rts", TIOCM_RTS, true, },
> +	{ "dtr", TIOCM_DTR, true, },
> +	{ "out1", TIOCM_OUT1, true, },
> +	{ "out2", TIOCM_OUT2, true, },
> +};
> +
> +void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
> +{
> +	enum mctrl_gpio_idx i;
> +
> +	for (i = 0; i < UART_GPIO_MAX; i++)

Use ARRAY_SIZE(mctrl_gpios_desc) here and elsewhere below.

> +		if (!IS_ERR_OR_NULL(gpios->gpio[i]) &&
> +		    mctrl_gpios_desc[i].dir_out)
> +			gpiod_set_value(gpios->gpio[i],
> +					!!(mctrl & mctrl_gpios_desc[i].mctrl));
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_set);
> +
> +unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
> +{

Why you want to put mctrl as parameter here?
Let's return mctrl from GPIOs, then handle this value as you want int the driver.

> +	enum mctrl_gpio_idx i;
> +
> +	for (i = 0; i < UART_GPIO_MAX; i++) {
> +		if (!IS_ERR_OR_NULL(gpios->gpio[i]) &&
> +		    !mctrl_gpios_desc[i].dir_out) {
> +			if (gpiod_get_value(gpios->gpio[i]))
> +				*mctrl |= mctrl_gpios_desc[i].mctrl;
> +			else
> +				*mctrl &= ~mctrl_gpios_desc[i].mctrl;
> +		}
> +	}
> +
> +	return *mctrl;
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_get);
> +

> +int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
> +{

I suggest to allocate "gpios" here and return pointer to this structure
or ERR_PTR(). Additionally, as I mentioned before, add "index" variable
to specify port number.

> +	enum mctrl_gpio_idx i;
> +	int err = 0;
> +	int ret = 0;
> +
> +	for (i = 0; i < UART_GPIO_MAX; i++) {
> +		gpios->gpio[i] = devm_gpiod_get(dev, mctrl_gpios_desc[i].name);
> +
> +		/*
> +		 * The GPIOs are maybe not all filled,
> +		 * this is not an error.
> +		 */
> +		if (IS_ERR_OR_NULL(gpios->gpio[i]))
> +			continue;
> +
> +		if (mctrl_gpios_desc[i].dir_out)
> +			err = gpiod_direction_output(gpios->gpio[i], 0);
> +		else
> +			err = gpiod_direction_input(gpios->gpio[i]);
> +		if (err) {
> +			dev_err(dev, "Unable to set direction for %s GPIO",
> +				mctrl_gpios_desc[i].name);
> +			devm_gpiod_put(dev, gpios->gpio[i]);
> +			gpios->gpio[i] = NULL;
> +			ret--;
> +		}
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mctrl_gpio_init);
...
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
...
> +enum mctrl_gpio_idx {
> +	UART_GPIO_CTS,
> +	UART_GPIO_DSR,
> +	UART_GPIO_DCD,
> +	UART_GPIO_RI,
> +	UART_GPIO_RTS,
> +	UART_GPIO_DTR,
> +	UART_GPIO_OUT1,
> +	UART_GPIO_OUT2,
> +	UART_GPIO_MAX,
> +};
> +
> +struct mctrl_gpios {
> +	struct gpio_desc *gpio[UART_GPIO_MAX];

struct gpio_desc *gpio;

...
> +static inline
> +int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
> +{
> +	return -UART_GPIO_MAX;

return -ENOSYS;

...

Thanks.
---

Richard Genoud Feb. 18, 2014, 9:59 a.m. UTC | #2

On 17/02/2014 19:37, Alexander Shiyan wrote:
> Hello.
Hi !
> 
> A few comments below..
> 
> ???????????, 17 ??????? 2014, 17:57 +01:00 ?? Richard Genoud <[e-mail]>:
>> This patch add some helpers to control modem lines (CTS/RTS/DSR...) via
>> GPIO.
>> This will be useful for many boards which have a serial controller that
>> only handle CTS/RTS pins (or even just RX/TX).
>>
>> Signed-off-by: Richard Genoud <[e-mail]>
>> ---
> ...
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> ...
>> +static const struct {
>> +	const char *name;
>> +	unsigned int mctrl;
>> +	bool dir_out;
>> +} mctrl_gpios_desc[] = {
>> +	{ "cts", TIOCM_CTS, false, },
>> +	{ "dsr", TIOCM_DSR, false, },
>> +	{ "dcd", TIOCM_CD, false, },
>> +	{ "ri", TIOCM_RI, false, },
>> +	{ "rts", TIOCM_RTS, true, },
>> +	{ "dtr", TIOCM_DTR, true, },
>> +	{ "out1", TIOCM_OUT1, true, },
>> +	{ "out2", TIOCM_OUT2, true, },
>> +};
>> +
>> +void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
>> +{
>> +	enum mctrl_gpio_idx i;
>> +
>> +	for (i = 0; i < UART_GPIO_MAX; i++)
> 
> Use ARRAY_SIZE(mctrl_gpios_desc) here and elsewhere below.
Could you explain why ARRAY_SIZE(mctrl_gpios_desc) seems better than
UART_GPIO_MAX ?

> 
>> +		if (!IS_ERR_OR_NULL(gpios->gpio[i]) &&
>> +		    mctrl_gpios_desc[i].dir_out)
>> +			gpiod_set_value(gpios->gpio[i],
>> +					!!(mctrl & mctrl_gpios_desc[i].mctrl));
>> +}
>> +EXPORT_SYMBOL_GPL(mctrl_gpio_set);
>> +
>> +unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
>> +{
> 
> Why you want to put mctrl as parameter here?
> Let's return mctrl from GPIOs, then handle this value as you want int the driver.
It's because I like when it's simple :).
Use case:
Your USART controller handles CTS/RTS, and you add DTR/DSR as gpios.
In get_mctrl() callback, with current implementation, you'll have
something like this: (cf atmel_get_mctrl() for a real life example)
{
unsigned int mctrl;
mctrl = usart_controller_get_mctrl(); /* driver specific */
return mctrl_gpio_get(gpios, &mctrl);
}
If I use as you suggest mctrl_gpio_get(struct mctrl_gpios *gpios), we'll
have:
{
unsigned int usart_mctrl;
unsigned int gpio_mctrl;
int i;

usart_mctrl = usart_controller_get_mctrl();
gpio_mctrl = mctrl_gpio_get(gpios);
if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_CTS]) {
	if (gpio_mctrl & TIOCM_CTS)
		usart_mctrl |= TIOCM_CTS;
	else
		usart_mctrl &= ~TIOCM_CTS;
}
if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_DSR]) {
	if (gpio_mctrl & TIOCM_DSR)
		usart_mctrl |= TIOCM_DSR;
	else
		usart_mctrl &= ~TIOCM_DSR;
}
etc...
}
Because when gpio_mctrl is returned, I don't know if a flag is not set
because a gpio is at 1 or because the gpio has not been requested.
In the later case, the value should not override the value retrieved by
the controller.

another solution would be to use a mask filled at startup:
init_gpios(...)
{
unsigned int driver_port_gpio_input_mask = 0;
mctrl_gpio_init(dev, &p->gpios);

if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_CTS])
	driver_port_gpio_mask |= TIOCM_CTS;
if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_DSR])
	driver_port_gpio_mask |= TIOCM_DSR;
if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_DCD])
	driver_port_gpio_mask |= TIOCM_DCD;
if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_RI])
	driver_port_gpio_mask |= TIOCM_RI;
}
and then, in get_mctrl():
{
unsigned int mctrl;
int i;

mctrl = usart_controller_get_mctrl();
mctrl &= ~driver_port_gpio_input_mask;
mctrl |= mctrl_gpio_get(gpios);
return mctrl;
}

But both solutions seems to me more complicated than the first one.


>> +	enum mctrl_gpio_idx i;
>> +
>> +	for (i = 0; i < UART_GPIO_MAX; i++) {
>> +		if (!IS_ERR_OR_NULL(gpios->gpio[i]) &&
>> +		    !mctrl_gpios_desc[i].dir_out) {
>> +			if (gpiod_get_value(gpios->gpio[i]))
>> +				*mctrl |= mctrl_gpios_desc[i].mctrl;
>> +			else
>> +				*mctrl &= ~mctrl_gpios_desc[i].mctrl;
>> +		}
>> +	}
>> +
>> +	return *mctrl;
>> +}
>> +EXPORT_SYMBOL_GPL(mctrl_gpio_get);
>> +
> 
>> +int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
>> +{
> 
> I suggest to allocate "gpios" here and return pointer to this structure
> or ERR_PTR(). Additionally, as I mentioned before, add "index" variable
> to specify port number.

I don't understand the benefit of dynamically allocating something that
has a fixed size...
Or maybe in the case no GPIO are used, the array is not allocated, and
I'll have to add "if (!gpios)" test in other functions. That could save
some bytes.

Could you explain a little more your idea of ""index" variable to
specify port number" ?
I'm not sure to get it.

>> +	enum mctrl_gpio_idx i;
>> +	int err = 0;
>> +	int ret = 0;
>> +
>> +	for (i = 0; i < UART_GPIO_MAX; i++) {
>> +		gpios->gpio[i] = devm_gpiod_get(dev, mctrl_gpios_desc[i].name);
>> +
>> +		/*
>> +		 * The GPIOs are maybe not all filled,
>> +		 * this is not an error.
>> +		 */
>> +		if (IS_ERR_OR_NULL(gpios->gpio[i]))
>> +			continue;
>> +
>> +		if (mctrl_gpios_desc[i].dir_out)
>> +			err = gpiod_direction_output(gpios->gpio[i], 0);
>> +		else
>> +			err = gpiod_direction_input(gpios->gpio[i]);
>> +		if (err) {
>> +			dev_err(dev, "Unable to set direction for %s GPIO",
>> +				mctrl_gpios_desc[i].name);
>> +			devm_gpiod_put(dev, gpios->gpio[i]);
>> +			gpios->gpio[i] = NULL;
>> +			ret--;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mctrl_gpio_init);
> ...
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
> ...
>> +enum mctrl_gpio_idx {
>> +	UART_GPIO_CTS,
>> +	UART_GPIO_DSR,
>> +	UART_GPIO_DCD,
>> +	UART_GPIO_RI,
>> +	UART_GPIO_RTS,
>> +	UART_GPIO_DTR,
>> +	UART_GPIO_OUT1,
>> +	UART_GPIO_OUT2,
>> +	UART_GPIO_MAX,
>> +};
>> +
>> +struct mctrl_gpios {
>> +	struct gpio_desc *gpio[UART_GPIO_MAX];
> 
> struct gpio_desc *gpio;
> 
> ...
>> +static inline
>> +int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
>> +{
>> +	return -UART_GPIO_MAX;
> 
> return -ENOSYS;
yes, that's right.

> 
> ...
> 
> Thanks.
Thanks for your review !

Richard.

Alexander Shiyan Feb. 18, 2014, 3:26 p.m. UTC | #3

On Tue, 18 Feb 2014 10:59:56 +0100
Richard Genoud <[e-mail]> wrote:

> On 17/02/2014 19:37, Alexander Shiyan wrote:
> > Hello.
> Hi !
> > 
> > A few comments below..
> > 
> > ???????????, 17 ??????? 2014, 17:57 +01:00 ?? Richard Genoud <[e-mail]>:
> >> This patch add some helpers to control modem lines (CTS/RTS/DSR...) via
> >> GPIO.
> >> This will be useful for many boards which have a serial controller that
> >> only handle CTS/RTS pins (or even just RX/TX).
> >>
> >> Signed-off-by: Richard Genoud <[e-mail]>
> >> ---
> > ...
> >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> > ...
> >> +static const struct {
> >> +	const char *name;
> >> +	unsigned int mctrl;
> >> +	bool dir_out;
> >> +} mctrl_gpios_desc[] = {
> >> +	{ "cts", TIOCM_CTS, false, },
> >> +	{ "dsr", TIOCM_DSR, false, },
> >> +	{ "dcd", TIOCM_CD, false, },
> >> +	{ "ri", TIOCM_RI, false, },
> >> +	{ "rts", TIOCM_RTS, true, },
> >> +	{ "dtr", TIOCM_DTR, true, },
> >> +	{ "out1", TIOCM_OUT1, true, },
> >> +	{ "out2", TIOCM_OUT2, true, },
> >> +};
> >> +
> >> +void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
> >> +{
> >> +	enum mctrl_gpio_idx i;
> >> +
> >> +	for (i = 0; i < UART_GPIO_MAX; i++)
> > 
> > Use ARRAY_SIZE(mctrl_gpios_desc) here and elsewhere below.
> Could you explain why ARRAY_SIZE(mctrl_gpios_desc) seems better than
> UART_GPIO_MAX ?

Because you iterate through the array.
I do not see the use of UART_GPIO_MAX in this module. You use UART_GPIO_MAX
in the at91 serial driver only, here we must be sure that we are within the
boundaries of the array.

...
> >> +unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
> >> +{
> > 
> > Why you want to put mctrl as parameter here?
> > Let's return mctrl from GPIOs, then handle this value as you want int the driver.
> It's because I like when it's simple :).
> Use case:
> Your USART controller handles CTS/RTS, and you add DTR/DSR as gpios.
> In get_mctrl() callback, with current implementation, you'll have
> something like this: (cf atmel_get_mctrl() for a real life example)
> {
> unsigned int mctrl;
> mctrl = usart_controller_get_mctrl(); /* driver specific */
> return mctrl_gpio_get(gpios, &mctrl);
> }
> If I use as you suggest mctrl_gpio_get(struct mctrl_gpios *gpios), we'll
> have:
> {
> unsigned int usart_mctrl;
> unsigned int gpio_mctrl;
> int i;
> 
> usart_mctrl = usart_controller_get_mctrl();
> gpio_mctrl = mctrl_gpio_get(gpios);
> if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_CTS]) {
> 	if (gpio_mctrl & TIOCM_CTS)
> 		usart_mctrl |= TIOCM_CTS;
> 	else
> 		usart_mctrl &= ~TIOCM_CTS;
> }
> if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_DSR]) {
> 	if (gpio_mctrl & TIOCM_DSR)
> 		usart_mctrl |= TIOCM_DSR;
> 	else
> 		usart_mctrl &= ~TIOCM_DSR;
> }

No, just like this:
{
  unsigned int mctrl = usart_controller_get_mctrl(); /* driver specific */
  mctrl |= mctrl_gpio_get(gpios, mctrl) & ~(TIOCM_CTS | TIOCM_DSR);
  return mctrl;
}

or in reverse order:
{
  unsigned int mctrl = mctrl_gpio_get(gpios, mctrl);
  return mctrl | usart_controller_get_mctrl(); /* driver specific */
}

...
> >> +int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
> >> +{
> > 
> > I suggest to allocate "gpios" here and return pointer to this structure
> > or ERR_PTR(). Additionally, as I mentioned before, add "index" variable
> > to specify port number.
> 
> I don't understand the benefit of dynamically allocating something that
> has a fixed size...
> Or maybe in the case no GPIO are used, the array is not allocated, and
> I'll have to add "if (!gpios)" test in other functions. That could save
> some bytes.

Yes, but basically this able to use just a pointer in your driver data,
this will not depend on GPIOLIB, since without GPIOLIB we do not know storage
size of struct gpio_desc.

> Could you explain a little more your idea of ""index" variable to
> specify port number" ?
> I'm not sure to get it.

Index can be used for drivers which allocate more than one port for one device.
In your implementation you should just replace devm_gpiod_get() to
devm_gpiod_get_index() and add an additional parameter to mctrl_gpio_init().
For at91 serial this parameter should be 0.

> 
> >> +	enum mctrl_gpio_idx i;
> >> +	int err = 0;
> >> +	int ret = 0;
> >> +
> >> +	for (i = 0; i < UART_GPIO_MAX; i++) {
> >> +		gpios->gpio[i] = devm_gpiod_get(dev, mctrl_gpios_desc[i].name);
...

Richard Genoud Feb. 20, 2014, 11:20 a.m. UTC | #4

On 18/02/2014 16:26, Alexander Shiyan wrote:
> On Tue, 18 Feb 2014 10:59:56 +0100
> Richard Genoud <[e-mail]> wrote:
> 
>> On 17/02/2014 19:37, Alexander Shiyan wrote:
>>> Hello.
>> Hi !
>>>
>>> A few comments below..
>>>
>>> ???????????, 17 ??????? 2014, 17:57 +01:00 ?? Richard Genoud <[e-mail]>:
>>>> This patch add some helpers to control modem lines (CTS/RTS/DSR...) via
>>>> GPIO.
>>>> This will be useful for many boards which have a serial controller that
>>>> only handle CTS/RTS pins (or even just RX/TX).
>>>>
>>>> Signed-off-by: Richard Genoud <[e-mail]>
>>>> ---
>>> ...
>>>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
>>> ...
>>>> +static const struct {
>>>> +	const char *name;
>>>> +	unsigned int mctrl;
>>>> +	bool dir_out;
>>>> +} mctrl_gpios_desc[] = {
>>>> +	{ "cts", TIOCM_CTS, false, },
>>>> +	{ "dsr", TIOCM_DSR, false, },
>>>> +	{ "dcd", TIOCM_CD, false, },
>>>> +	{ "ri", TIOCM_RI, false, },
>>>> +	{ "rts", TIOCM_RTS, true, },
>>>> +	{ "dtr", TIOCM_DTR, true, },
>>>> +	{ "out1", TIOCM_OUT1, true, },
>>>> +	{ "out2", TIOCM_OUT2, true, },
>>>> +};
>>>> +
>>>> +void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
>>>> +{
>>>> +	enum mctrl_gpio_idx i;
>>>> +
>>>> +	for (i = 0; i < UART_GPIO_MAX; i++)
>>>
>>> Use ARRAY_SIZE(mctrl_gpios_desc) here and elsewhere below.
>> Could you explain why ARRAY_SIZE(mctrl_gpios_desc) seems better than
>> UART_GPIO_MAX ?
> 
> Because you iterate through the array.
> I do not see the use of UART_GPIO_MAX in this module. You use UART_GPIO_MAX
> in the at91 serial driver only, here we must be sure that we are within the
> boundaries of the array.
Well, the loop iterates through the gpios->gpio[] array which size is
UART_GPIO_MAX, and uses also the mctrl_gpios_desc[] which size is implicit.
Anyway, if you absolutely want this change, I'll do it.

(Or I could also force the length of mctrl_gpios_desc[] like that:
static const struct {
	const char *name;
	unsigned int mctrl;
	bool dir_out;
} mctrl_gpios_desc[UART_GPIO_MAX] = {
	{ "cts", TIOCM_CTS, false, },
	{ "dsr", TIOCM_DSR, false, },
	{ "dcd", TIOCM_CD, false, },
	{ "ri", TIOCM_RI, false, },
	{ "rts", TIOCM_RTS, true, },
	{ "dtr", TIOCM_DTR, true, },
	{ "out1", TIOCM_OUT1, true, },
	{ "out2", TIOCM_OUT2, true, },
};
This way, all loops will be correct. )

> ...
>>>> +unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
>>>> +{
>>>
>>> Why you want to put mctrl as parameter here?
>>> Let's return mctrl from GPIOs, then handle this value as you want int the driver.
>> It's because I like when it's simple :).
>> Use case:
>> Your USART controller handles CTS/RTS, and you add DTR/DSR as gpios.
>> In get_mctrl() callback, with current implementation, you'll have
>> something like this: (cf atmel_get_mctrl() for a real life example)
>> {
>> unsigned int mctrl;
>> mctrl = usart_controller_get_mctrl(); /* driver specific */
>> return mctrl_gpio_get(gpios, &mctrl);
>> }
>> If I use as you suggest mctrl_gpio_get(struct mctrl_gpios *gpios), we'll
>> have:
>> {
>> unsigned int usart_mctrl;
>> unsigned int gpio_mctrl;
>> int i;
>>
>> usart_mctrl = usart_controller_get_mctrl();
>> gpio_mctrl = mctrl_gpio_get(gpios);
>> if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_CTS]) {
>> 	if (gpio_mctrl & TIOCM_CTS)
>> 		usart_mctrl |= TIOCM_CTS;
>> 	else
>> 		usart_mctrl &= ~TIOCM_CTS;
>> }
>> if (!IS_ERR_OR_NULL(gpios->gpio[UART_GPIO_DSR]) {
>> 	if (gpio_mctrl & TIOCM_DSR)
>> 		usart_mctrl |= TIOCM_DSR;
>> 	else
>> 		usart_mctrl &= ~TIOCM_DSR;
>> }
> 
> No, just like this:
> {
>   unsigned int mctrl = usart_controller_get_mctrl(); /* driver specific */
>   mctrl |= mctrl_gpio_get(gpios, mctrl) & ~(TIOCM_CTS | TIOCM_DSR);
>   return mctrl;
> }
Hum, I think you made a mistake here.
The "mctrl_gpio_get(gpios, mctrl) & ~(TIOCM_CTS | TIOCM_DSR)" doesn't make
any sense. Or I don't understand what you want to do...

> 
> or in reverse order:
> {
>   unsigned int mctrl = mctrl_gpio_get(gpios, mctrl);
>   return mctrl | usart_controller_get_mctrl(); /* driver specific */
> }
Ok, I didn't explain everything on that:
In atmel_serial (at least), the function usart_controller_get_mctrl()
is in fact a read on a register (Channel Status Register)  + conversion
from the register flags to TIOCM_* flags.
That's pretty classical.

BUT when, for example CTS, is muxed as a GPIO and not as the peripheral
function, the value read for the register is undefined (I've seen it
changed randomly). (and it's not very surprising)

So, the value(s) of declared GPIO have to supersede the value(s)
retrieved from the controller.

Thus, a simple OR won't do the trick.

If you really think that mctrl_gpio_get() as it is, is over-complicated,
I could do something like:
static inline unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios) {
  int mctrl = 0;
  return mctrl_gpio_update(gpios, &mctrl);
}

unsigned int mctrl_gpio_update(struct mctrl_gpios *gpios, unsigned int *mctrl)
{
/* like the old mctrl_gpio_get() */
}

But is it really worth it ?

> 
> ...
>>>> +int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
>>>> +{
>>>
>>> I suggest to allocate "gpios" here and return pointer to this structure
>>> or ERR_PTR(). Additionally, as I mentioned before, add "index" variable
>>> to specify port number.
>>
>> I don't understand the benefit of dynamically allocating something that
>> has a fixed size...
>> Or maybe in the case no GPIO are used, the array is not allocated, and
>> I'll have to add "if (!gpios)" test in other functions. That could save
>> some bytes.
> 
> Yes, but basically this able to use just a pointer in your driver data,
> this will not depend on GPIOLIB, since without GPIOLIB we do not know storage
> size of struct gpio_desc.
Ok, convinced.

> 
>> Could you explain a little more your idea of ""index" variable to
>> specify port number" ?
>> I'm not sure to get it.
> 
> Index can be used for drivers which allocate more than one port for one device.
> In your implementation you should just replace devm_gpiod_get() to
> devm_gpiod_get_index() and add an additional parameter to mctrl_gpio_init().
> For at91 serial this parameter should be 0.
Understood.

>>
>>>> +	enum mctrl_gpio_idx i;
>>>> +	int err = 0;
>>>> +	int ret = 0;
>>>> +
>>>> +	for (i = 0; i < UART_GPIO_MAX; i++) {
>>>> +		gpios->gpio[i] = devm_gpiod_get(dev, mctrl_gpios_desc[i].name);
> ...
> 


Thanks !

Richard.

Patch

diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index c3a7689a90e6..23aa32667d68 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -429,3 +429,24 @@  thus:
 		struct uart_port	port;
 		int			my_stuff;
 	};
+
+Modem control lines via GPIO
+----------------------------
+
+Some helpers are provided in order to set/get modem control lines via GPIO.
+
+mctrl_gpio_init(dev, gpios):
+	This will get the {cts,rts,...}-gpios from device tree if they are
+	present and request them, set direction etc. devm_* functions are
+	used, so there's no need to call mctrl_gpio_free().
+
+mctrl_gpio_free(dev, gpios):
+	This will free the requested gpios in mctrl_gpio_init().
+	As devm_* function are used, there's generally no need to call
+	this function.
+
+mctrl_gpio_set(gpios, mctrl):
+	This will sets the gpios according to the mctrl state.
+
+mctrl_gpio_get(gpios, mctrl):
+	This will update mctrl with the gpios values.
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index a3815eaed421..4fe8ca16f492 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1509,4 +1509,7 @@  config SERIAL_ST_ASC_CONSOLE
 
 endmenu
 
+config SERIAL_MCTRL_GPIO
+	tristate
+
 endif # TTY
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 3680854fef41..bcf31da267dd 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -87,3 +87,6 @@  obj-$(CONFIG_SERIAL_EFM32_UART) += efm32-uart.o
 obj-$(CONFIG_SERIAL_ARC)	+= arc_uart.o
 obj-$(CONFIG_SERIAL_RP2)	+= rp2.o
 obj-$(CONFIG_SERIAL_FSL_LPUART)	+= fsl_lpuart.o
+
+# GPIOLIB helpers for modem control lines
+obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
new file mode 100644
index 000000000000..bd8e4a7c981c
--- /dev/null
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -0,0 +1,112 @@ 
+/*
+ * Helpers for controlling modem lines via GPIO
+ *
+ * Copyright (C) 2014 Paratronic S.A.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <uapi/asm-generic/termios.h>
+
+#include "serial_mctrl_gpio.h"
+
+static const struct {
+	const char *name;
+	unsigned int mctrl;
+	bool dir_out;
+} mctrl_gpios_desc[] = {
+	{ "cts", TIOCM_CTS, false, },
+	{ "dsr", TIOCM_DSR, false, },
+	{ "dcd", TIOCM_CD, false, },
+	{ "ri", TIOCM_RI, false, },
+	{ "rts", TIOCM_RTS, true, },
+	{ "dtr", TIOCM_DTR, true, },
+	{ "out1", TIOCM_OUT1, true, },
+	{ "out2", TIOCM_OUT2, true, },
+};
+
+void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
+{
+	enum mctrl_gpio_idx i;
+
+	for (i = 0; i < UART_GPIO_MAX; i++)
+		if (!IS_ERR_OR_NULL(gpios->gpio[i]) &&
+		    mctrl_gpios_desc[i].dir_out)
+			gpiod_set_value(gpios->gpio[i],
+					!!(mctrl & mctrl_gpios_desc[i].mctrl));
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_set);
+
+unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
+{
+	enum mctrl_gpio_idx i;
+
+	for (i = 0; i < UART_GPIO_MAX; i++) {
+		if (!IS_ERR_OR_NULL(gpios->gpio[i]) &&
+		    !mctrl_gpios_desc[i].dir_out) {
+			if (gpiod_get_value(gpios->gpio[i]))
+				*mctrl |= mctrl_gpios_desc[i].mctrl;
+			else
+				*mctrl &= ~mctrl_gpios_desc[i].mctrl;
+		}
+	}
+
+	return *mctrl;
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_get);
+
+int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
+{
+	enum mctrl_gpio_idx i;
+	int err = 0;
+	int ret = 0;
+
+	for (i = 0; i < UART_GPIO_MAX; i++) {
+		gpios->gpio[i] = devm_gpiod_get(dev, mctrl_gpios_desc[i].name);
+
+		/*
+		 * The GPIOs are maybe not all filled,
+		 * this is not an error.
+		 */
+		if (IS_ERR_OR_NULL(gpios->gpio[i]))
+			continue;
+
+		if (mctrl_gpios_desc[i].dir_out)
+			err = gpiod_direction_output(gpios->gpio[i], 0);
+		else
+			err = gpiod_direction_input(gpios->gpio[i]);
+		if (err) {
+			dev_err(dev, "Unable to set direction for %s GPIO",
+				mctrl_gpios_desc[i].name);
+			devm_gpiod_put(dev, gpios->gpio[i]);
+			gpios->gpio[i] = NULL;
+			ret--;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_init);
+
+void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
+{
+	enum mctrl_gpio_idx i;
+
+	for (i = 0; i < UART_GPIO_MAX; i++)
+		if (!IS_ERR_OR_NULL(gpios->gpio[i])) {
+			devm_gpiod_put(dev, gpios->gpio[i]);
+			gpios->gpio[i] = NULL;
+		}
+}
+EXPORT_SYMBOL_GPL(mctrl_gpio_free);
diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h
new file mode 100644
index 000000000000..e9797323d531
--- /dev/null
+++ b/drivers/tty/serial/serial_mctrl_gpio.h
@@ -0,0 +1,92 @@ 
+/*
+ * Helpers for controlling modem lines via GPIO
+ *
+ * Copyright (C) 2014 Paratronic S.A.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __SERIAL_MCTRL_GPIO__
+#define __SERIAL_MCTRL_GPIO__
+
+#include <linux/gpio/consumer.h>
+
+enum mctrl_gpio_idx {
+	UART_GPIO_CTS,
+	UART_GPIO_DSR,
+	UART_GPIO_DCD,
+	UART_GPIO_RI,
+	UART_GPIO_RTS,
+	UART_GPIO_DTR,
+	UART_GPIO_OUT1,
+	UART_GPIO_OUT2,
+	UART_GPIO_MAX,
+};
+
+struct mctrl_gpios {
+	struct gpio_desc *gpio[UART_GPIO_MAX];
+};
+
+#ifdef CONFIG_GPIOLIB
+
+/*
+ * Set state of the modem control output lines via GPIOs.
+ */
+void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl);
+
+/*
+ * Get state of the modem control output lines from GPIOs.
+ * The mctrl flags are updated and returned.
+ */
+unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl);
+
+/*
+ * Request and set direction of modem control lines GPIOs.
+ * devm_* functions are used, so there's no need to call mctrl_gpio_free().
+ * Returns 0 if ok, -nb_err if setting direction failed.
+ */
+int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios);
+
+/*
+ * Free all modem control lines GPIOs.
+ * Normally, this function will not be called, as the GPIOs will
+ * be disposed of by the resource management code.
+ */
+void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios);
+
+#else /* GPIOLIB */
+
+static inline
+void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl)
+{
+}
+
+static inline
+unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl)
+{
+	return 0;
+}
+
+static inline
+int mctrl_gpio_init(struct device *dev, struct mctrl_gpios *gpios)
+{
+	return -UART_GPIO_MAX;
+}
+
+static inline
+void mctrl_gpio_free(struct device *dev, struct mctrl_gpios *gpios)
+{
+}
+
+#endif /* GPIOLIB */
+
+#endif