From: Alexander Shiyan <shc_work@mail.ru> To: Richard Genoud <richard.genoud@gmail.com> Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, "Nicolas Ferre" <nicolas.ferre@atmel.com>, "Linus Walleij" <linus.walleij@linaro.org>, linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 1/7] tty/serial: Add GPIOLIB helpers for controlling modem lines Date: Tue, 18 Feb 2014 19:26:20 +0400 [thread overview] Message-ID: <20140218192620.12ad4b94cb63135e8665cf45@mail.ru> (raw) In-Reply-To: <53032F1C.2040607@gmail.com> On Tue, 18 Feb 2014 10:59:56 +0100 Richard Genoud <richard.genoud@gmail.com> wrote: > On 17/02/2014 19:37, Alexander Shiyan wrote: > > Hello. > Hi ! > > > > A few comments below.. > > > > Понедельник, 17 февраля 2014, 17:57 +01:00 от Richard Genoud <richard.genoud@gmail.com>: > >> 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 <richard.genoud@gmail.com> > >> --- > > ... > >> 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); ... -- Alexander Shiyan <shc_work@mail.ru> -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: shc_work@mail.ru (Alexander Shiyan) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 1/7] tty/serial: Add GPIOLIB helpers for controlling modem lines Date: Tue, 18 Feb 2014 19:26:20 +0400 [thread overview] Message-ID: <20140218192620.12ad4b94cb63135e8665cf45@mail.ru> (raw) In-Reply-To: <53032F1C.2040607@gmail.com> On Tue, 18 Feb 2014 10:59:56 +0100 Richard Genoud <richard.genoud@gmail.com> wrote: > On 17/02/2014 19:37, Alexander Shiyan wrote: > > Hello. > Hi ! > > > > A few comments below.. > > > > ???????????, 17 ??????? 2014, 17:57 +01:00 ?? Richard Genoud <richard.genoud@gmail.com>: > >> 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 <richard.genoud@gmail.com> > >> --- > > ... > >> 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); ... -- Alexander Shiyan <shc_work@mail.ru>
next prev parent reply other threads:[~2014-02-18 15:26 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-02-17 16:57 [PATCH v3 0/7] tty/serial: Add helpers to use GPIOs to control modem lines and implement atmel_serial.c Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-17 16:57 ` [PATCH v3 1/7] tty/serial: Add GPIOLIB helpers for controlling modem lines Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-17 18:37 ` Alexander Shiyan 2014-02-17 18:37 ` Alexander Shiyan 2014-02-18 9:59 ` Richard Genoud 2014-02-18 9:59 ` Richard Genoud 2014-02-18 15:26 ` Alexander Shiyan [this message] 2014-02-18 15:26 ` Alexander Shiyan 2014-02-20 11:20 ` Richard Genoud 2014-02-20 11:20 ` Richard Genoud 2014-02-17 16:57 ` [PATCH v3 2/7] tty/serial: at91: use dev_err instead of printk Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-17 16:57 ` [PATCH v3 3/7] tty/serial: at91: remove unused open/close hooks Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-17 16:57 ` [PATCH v3 4/7] tty/serial: at91: use mctrl_gpio helpers Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-18 15:04 ` Alexander Shiyan 2014-02-18 15:04 ` Alexander Shiyan 2014-02-18 15:09 ` Richard Genoud 2014-02-18 15:09 ` Richard Genoud 2014-02-17 16:57 ` [PATCH v3 5/7] ARM: at91: gpio: implement get_direction Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-24 14:42 ` Linus Walleij 2014-02-24 14:42 ` Linus Walleij 2014-02-17 16:57 ` [PATCH v3 6/7] pinctrl: at91: " Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-24 14:44 ` Linus Walleij 2014-02-24 14:44 ` Linus Walleij 2014-02-24 14:56 ` Richard Genoud 2014-02-24 14:56 ` Richard Genoud 2014-02-25 9:34 ` Linus Walleij 2014-02-25 9:34 ` Linus Walleij 2014-02-17 16:57 ` [PATCH v3 7/7] tty/serial: at91: add interrupts for modem control lines Richard Genoud 2014-02-17 16:57 ` Richard Genoud 2014-02-17 17:53 ` [PATCH v3 0/7] tty/serial: Add helpers to use GPIOs to control modem lines and implement atmel_serial.c Alexander Shiyan 2014-02-17 17:53 ` Alexander Shiyan 2014-02-18 9:59 ` Richard Genoud 2014-02-18 9:59 ` Richard Genoud
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140218192620.12ad4b94cb63135e8665cf45@mail.ru \ --to=shc_work@mail.ru \ --cc=gregkh@linuxfoundation.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-serial@vger.kernel.org \ --cc=nicolas.ferre@atmel.com \ --cc=richard.genoud@gmail.com \ --cc=u.kleine-koenig@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.