| Message ID | 1463988658-9183-3-git-send-email-yegorslists@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Yegor, On Mon, May 23, 2016 at 09:30:56AM +0200, yegorslists@googlemail.com wrote: > From: Yegor Yefremov <yegorslists@googlemail.com> > > mctrl_gpio_get_outputs() returns the state of following signals: > > RTS, DTR > > Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com> > --- > drivers/tty/serial/serial_mctrl_gpio.c | 18 ++++++++++++++++++ > drivers/tty/serial/serial_mctrl_gpio.h | 15 ++++++++++++++- > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c > index e8dd509..a868595 100644 > --- a/drivers/tty/serial/serial_mctrl_gpio.c > +++ b/drivers/tty/serial/serial_mctrl_gpio.c > @@ -86,6 +86,24 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) > } > EXPORT_SYMBOL_GPL(mctrl_gpio_get); > > +unsigned int > +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) > +{ > + enum mctrl_gpio_idx i; > + > + for (i = 0; i < UART_GPIO_MAX; i++) { > + if (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; Given that get_value for outputs isn't well defined I'd prefer if there were no usecase for this. This is intended for probe-time, right? Otherwise the state of these lines should be known. > + } > + } > + > + return *mctrl; > +} > +EXPORT_SYMBOL_GPL(mctrl_gpio_get_outputs); > + > struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) > { > struct mctrl_gpios *gpios; > diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h > index 332a33a..fa000bc 100644 > --- a/drivers/tty/serial/serial_mctrl_gpio.h > +++ b/drivers/tty/serial/serial_mctrl_gpio.h > @@ -48,12 +48,19 @@ struct mctrl_gpios; > void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl); > > /* > - * Get state of the modem control output lines from GPIOs. > + * Get state of the modem control input lines from GPIOs. This looks right, should be a separate patch or mentioned in the commit log. > +unsigned int > +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl); Personally I'd prefer the following formatting: unsigned int mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) I didn't check though which of these two is more consistent in that file. Best regards Uwe
On 05/23/2016 01:59 AM, Uwe Kleine-König wrote: > Hello Yegor, > > On Mon, May 23, 2016 at 09:30:56AM +0200, yegorslists@googlemail.com wrote: >> From: Yegor Yefremov <yegorslists@googlemail.com> >> >> mctrl_gpio_get_outputs() returns the state of following signals: >> >> RTS, DTR >> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com> >> --- >> drivers/tty/serial/serial_mctrl_gpio.c | 18 ++++++++++++++++++ >> drivers/tty/serial/serial_mctrl_gpio.h | 15 ++++++++++++++- >> 2 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c >> index e8dd509..a868595 100644 >> --- a/drivers/tty/serial/serial_mctrl_gpio.c >> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >> @@ -86,6 +86,24 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) >> } >> EXPORT_SYMBOL_GPL(mctrl_gpio_get); >> >> +unsigned int >> +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) >> +{ >> + enum mctrl_gpio_idx i; >> + >> + for (i = 0; i < UART_GPIO_MAX; i++) { >> + if (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; > > Given that get_value for outputs isn't well defined I'd prefer if there > were no usecase for this. This is intended for probe-time, right? > Otherwise the state of these lines should be known. It should be known to both gpio and this emulation layer already, and it's an oversight if it doesn't. Why create a write-only interface? As if it's not hard enough when hardware registers are write-only... >> + } >> + } >> + >> + return *mctrl; >> +} >> +EXPORT_SYMBOL_GPL(mctrl_gpio_get_outputs); >> + >> struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) >> { >> struct mctrl_gpios *gpios; >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h >> index 332a33a..fa000bc 100644 >> --- a/drivers/tty/serial/serial_mctrl_gpio.h >> +++ b/drivers/tty/serial/serial_mctrl_gpio.h >> @@ -48,12 +48,19 @@ struct mctrl_gpios; >> void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl); >> >> /* >> - * Get state of the modem control output lines from GPIOs. >> + * Get state of the modem control input lines from GPIOs. > > This looks right, should be a separate patch or mentioned in the commit > log. > >> +unsigned int >> +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl); > > Personally I'd prefer the following formatting: > > unsigned int mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, > unsigned int *mctrl) > > I didn't check though which of these two is more consistent in that > file. > > Best regards > Uwe > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Mon, May 23, 2016 at 10:08:26AM -0700, Peter Hurley wrote: > On 05/23/2016 01:59 AM, Uwe Kleine-König wrote: > > Hello Yegor, > > > > On Mon, May 23, 2016 at 09:30:56AM +0200, yegorslists@googlemail.com wrote: > >> From: Yegor Yefremov <yegorslists@googlemail.com> > >> > >> mctrl_gpio_get_outputs() returns the state of following signals: > >> > >> RTS, DTR > >> > >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com> > >> --- > >> drivers/tty/serial/serial_mctrl_gpio.c | 18 ++++++++++++++++++ > >> drivers/tty/serial/serial_mctrl_gpio.h | 15 ++++++++++++++- > >> 2 files changed, 32 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c > >> index e8dd509..a868595 100644 > >> --- a/drivers/tty/serial/serial_mctrl_gpio.c > >> +++ b/drivers/tty/serial/serial_mctrl_gpio.c > >> @@ -86,6 +86,24 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) > >> } > >> EXPORT_SYMBOL_GPL(mctrl_gpio_get); > >> > >> +unsigned int > >> +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) > >> +{ > >> + enum mctrl_gpio_idx i; > >> + > >> + for (i = 0; i < UART_GPIO_MAX; i++) { > >> + if (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; > > > > Given that get_value for outputs isn't well defined I'd prefer if there > > were no usecase for this. This is intended for probe-time, right? > > Otherwise the state of these lines should be known. > > It should be known to both gpio and this emulation layer already, > and it's an oversight if it doesn't. So in which situations is this function supposed to be called? (OK, I could check how it is used in patch 4, but I think explaining it here in a comment to that function would be nice.) > Why create a write-only interface? As if it's not hard enough when > hardware registers are write-only... There are different possible semantics and so the different hardware implementations differ. Some return what the gpio was set to, some return the real level of the pin, some return always 0 and I'm sure if I ask a few hardware engineers they can come up with at least two further creative ideas. Best regards Uwe
On 05/23/2016 10:54 AM, Uwe Kleine-König wrote: > Hello, > > On Mon, May 23, 2016 at 10:08:26AM -0700, Peter Hurley wrote: >> On 05/23/2016 01:59 AM, Uwe Kleine-König wrote: >>> Hello Yegor, >>> >>> On Mon, May 23, 2016 at 09:30:56AM +0200, yegorslists@googlemail.com wrote: >>>> From: Yegor Yefremov <yegorslists@googlemail.com> >>>> >>>> mctrl_gpio_get_outputs() returns the state of following signals: >>>> >>>> RTS, DTR >>>> >>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com> >>>> --- >>>> drivers/tty/serial/serial_mctrl_gpio.c | 18 ++++++++++++++++++ >>>> drivers/tty/serial/serial_mctrl_gpio.h | 15 ++++++++++++++- >>>> 2 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c >>>> index e8dd509..a868595 100644 >>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c >>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >>>> @@ -86,6 +86,24 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) >>>> } >>>> EXPORT_SYMBOL_GPL(mctrl_gpio_get); >>>> >>>> +unsigned int >>>> +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) >>>> +{ >>>> + enum mctrl_gpio_idx i; >>>> + >>>> + for (i = 0; i < UART_GPIO_MAX; i++) { >>>> + if (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; >>> >>> Given that get_value for outputs isn't well defined I'd prefer if there >>> were no usecase for this. This is intended for probe-time, right? >>> Otherwise the state of these lines should be known. >> >> It should be known to both gpio and this emulation layer already, >> and it's an oversight if it doesn't. > > So in which situations is this function supposed to be called? > (OK, I could check how it is used in patch 4, but I think explaining it > here in a comment to that function would be nice.) > >> Why create a write-only interface? As if it's not hard enough when >> hardware registers are write-only... > > There are different possible semantics and so the different hardware > implementations differ. Some return what the gpio was set to, some > return the real level of the pin, some return always 0 and I'm sure if I > ask a few hardware engineers they can come up with at least two further > creative ideas. I don't care about generic gpio. This is an emulation layer for uarts; show me a single uart design where a r/w register read of an output pin does anything other than return the last value written. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 23, 2016 at 10:59 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello Yegor, > > On Mon, May 23, 2016 at 09:30:56AM +0200, yegorslists@googlemail.com wrote: >> From: Yegor Yefremov <yegorslists@googlemail.com> >> >> mctrl_gpio_get_outputs() returns the state of following signals: >> >> RTS, DTR >> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com> >> --- >> drivers/tty/serial/serial_mctrl_gpio.c | 18 ++++++++++++++++++ >> drivers/tty/serial/serial_mctrl_gpio.h | 15 ++++++++++++++- >> 2 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c >> index e8dd509..a868595 100644 >> --- a/drivers/tty/serial/serial_mctrl_gpio.c >> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >> @@ -86,6 +86,24 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) >> } >> EXPORT_SYMBOL_GPL(mctrl_gpio_get); >> >> +unsigned int >> +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) >> +{ >> + enum mctrl_gpio_idx i; >> + >> + for (i = 0; i < UART_GPIO_MAX; i++) { >> + if (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; > > Given that get_value for outputs isn't well defined I'd prefer if there > were no usecase for this. This is intended for probe-time, right? > Otherwise the state of these lines should be known. > >> + } >> + } >> + >> + return *mctrl; >> +} >> +EXPORT_SYMBOL_GPL(mctrl_gpio_get_outputs); >> + >> struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) >> { >> struct mctrl_gpios *gpios; >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h >> index 332a33a..fa000bc 100644 >> --- a/drivers/tty/serial/serial_mctrl_gpio.h >> +++ b/drivers/tty/serial/serial_mctrl_gpio.h >> @@ -48,12 +48,19 @@ struct mctrl_gpios; >> void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl); >> >> /* >> - * Get state of the modem control output lines from GPIOs. >> + * Get state of the modem control input lines from GPIOs. > > This looks right, should be a separate patch or mentioned in the commit > log. I'll mention it in the commit log. >> +unsigned int >> +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl); > > Personally I'd prefer the following formatting: > > unsigned int mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, > unsigned int *mctrl) > > I didn't check though which of these two is more consistent in that > file. I've used this formatting because it was consistent with the rest of the routine definitions in this file. Yegor -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c index e8dd509..a868595 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.c +++ b/drivers/tty/serial/serial_mctrl_gpio.c @@ -86,6 +86,24 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) } EXPORT_SYMBOL_GPL(mctrl_gpio_get); +unsigned int +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) +{ + enum mctrl_gpio_idx i; + + for (i = 0; i < UART_GPIO_MAX; i++) { + if (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_outputs); + struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) { struct mctrl_gpios *gpios; diff --git a/drivers/tty/serial/serial_mctrl_gpio.h b/drivers/tty/serial/serial_mctrl_gpio.h index 332a33a..fa000bc 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.h +++ b/drivers/tty/serial/serial_mctrl_gpio.h @@ -48,12 +48,19 @@ struct mctrl_gpios; void mctrl_gpio_set(struct mctrl_gpios *gpios, unsigned int mctrl); /* - * Get state of the modem control output lines from GPIOs. + * Get state of the modem control input lines from GPIOs. * The mctrl flags are updated and returned. */ unsigned int mctrl_gpio_get(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_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl); + +/* * Returns the associated struct gpio_desc to the modem line gidx */ struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios, @@ -107,6 +114,12 @@ unsigned int mctrl_gpio_get(struct mctrl_gpios *gpios, unsigned int *mctrl) return *mctrl; } +static inline unsigned int +mctrl_gpio_get_outputs(struct mctrl_gpios *gpios, unsigned int *mctrl) +{ + return *mctrl; +} + static inline struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios, enum mctrl_gpio_idx gidx)