On Tue, 18 Feb 2014 10:59:56 +0100
Richard Genoud <richard.genoud@xxxxxxxxx> 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@xxxxxxxxx>:
> >> 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@xxxxxxxxx>
> >> ---
> > ...
> >> 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@xxxxxxx>
--
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
