| Message ID | 3e671b6c0d11a2d0c292947675ed087eaaa5445e.1712863999.git.pnewman@connecttech.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: exar: add Connect Tech serial cards to Exar driver | expand |
On Thu, Apr 11, 2024 at 04:25:41PM -0400, parker@finest.io wrote: > +/** > + * exar_mpio_config() - Configure an EXar MPIO as input or output > + * @priv: Device's private structure > + * @mpio_num: MPIO number/offset to configure > + * @output: Configure as output if true, inout if false > + * > + * Configure a single MPIO as an input or output and disable trisate. > + * If configuring as output it is reccomended to set value with > + * exar_mpio_set prior to calling this function to ensure default state. > + * > + * Return: 0 on success, negative error code on failure > + */ > +static int exar_mpio_config(struct exar8250 *priv, > + unsigned int mpio_num, bool output) When you have a bool in a function, every time you read the code you have to go and figure out what that boolean means. Have 2 functions: exar_mpio_config_input() exar_mpio_config_output() and then have THEM call this function with the bool set or not. That way when reading the code you know exactly what is happening. Same with other functions in this patch. Naming is hard, make it easy please. thanks, greg k-h
On Fri, 12 Apr 2024 07:29:16 +0200 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Apr 11, 2024 at 04:25:41PM -0400, parker@finest.io wrote: > > +/** > > + * exar_mpio_config() - Configure an EXar MPIO as input or output > > + * @priv: Device's private structure > > + * @mpio_num: MPIO number/offset to configure > > + * @output: Configure as output if true, inout if false > > + * > > + * Configure a single MPIO as an input or output and disable trisate. > > + * If configuring as output it is reccomended to set value with > > + * exar_mpio_set prior to calling this function to ensure default state. > > + * > > + * Return: 0 on success, negative error code on failure > > + */ > > +static int exar_mpio_config(struct exar8250 *priv, > > + unsigned int mpio_num, bool output) > > When you have a bool in a function, every time you read the code you > have to go and figure out what that boolean means. > > Have 2 functions: > exar_mpio_config_input() > exar_mpio_config_output() > > and then have THEM call this function with the bool set or not. That > way when reading the code you know exactly what is happening. > > Same with other functions in this patch. Naming is hard, make it easy > please. Good feedback thanks. > thanks, > > greg k-h
On Fri, 12 Apr 2024, Parker Newman wrote: > On Fri, 12 Apr 2024 13:20:41 +0300 (EEST) > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > On Thu, 11 Apr 2024, parker@finest.io wrote: > > > > > From: Parker Newman <pnewman@connecttech.com> > > > > > > Adds support for configuring and setting a single MPIO > > > > > > Signed-off-by: Parker Newman <pnewman@connecttech.com> > > > --- > > > drivers/tty/serial/8250/8250_exar.c | 88 +++++++++++++++++++++++++++++ > > > 1 file changed, 88 insertions(+) > > > > > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c > > > index 49d690344e65..9915a99cb7c6 100644 > > > --- a/drivers/tty/serial/8250/8250_exar.c > > > +++ b/drivers/tty/serial/8250/8250_exar.c > > > @@ -305,6 +305,94 @@ static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr) > > > return data; > > > } > > > > > > +/** > > > + * exar_mpio_config() - Configure an EXar MPIO as input or output > > > + * @priv: Device's private structure > > > + * @mpio_num: MPIO number/offset to configure > > > + * @output: Configure as output if true, inout if false > > > + * > > > + * Configure a single MPIO as an input or output and disable trisate. > > > > tristate > > > > > + * If configuring as output it is reccomended to set value with > > > + * exar_mpio_set prior to calling this function to ensure default state. > > > > Use () if talking about function. > > > > > + * > > > + * Return: 0 on success, negative error code on failure > > > + */ > > > +static int exar_mpio_config(struct exar8250 *priv, > > > + unsigned int mpio_num, bool output) > > > +{ > > > + uint8_t sel_reg; //MPIO Select register (input/output) > > > + uint8_t tri_reg; //MPIO Tristate register > > > + uint8_t value; > > > + unsigned int bit; > > > + > > > + if (mpio_num < 8) { > > > + sel_reg = UART_EXAR_MPIOSEL_7_0; > > > + tri_reg = UART_EXAR_MPIO3T_7_0; > > > + bit = mpio_num; > > > + } else if (mpio_num >= 8 && mpio_num < 16) { > > > + sel_reg = UART_EXAR_MPIOSEL_15_8; > > > + tri_reg = UART_EXAR_MPIO3T_15_8; > > > + bit = mpio_num - 8; > > > + } else { > > > + return -EINVAL; > > > + } > > > + > > > + //Disable MPIO pin tri-state > > > + value = exar_read_reg(priv, tri_reg); > > > + value &= ~(BIT(bit)); > > > > Use more meaningful variable name than "bit", it could perhaps even avoid > > the need to use the comment if the code is self-explanary with better > > variable name. > > > > > + exar_write_reg(priv, tri_reg, value); > > > + > > > + value = exar_read_reg(priv, sel_reg); > > > + //Set MPIO as input (1) or output (0) > > > > Unnecessary comment. > > > > > + if (output) > > > + value &= ~(BIT(bit)); > > > > Unnecessary parenthesis. > > > > > + else > > > + value |= BIT(bit); > > > + > > > + exar_write_reg(priv, sel_reg, value); > > > > Don't leave empty line into RMW sequence. > > > > > + > > > + return 0; > > > +} > > > +/** > > > + * exar_mpio_set() - Set an Exar MPIO output high or low > > > + * @priv: Device's private structure > > > + * @mpio_num: MPIO number/offset to set > > > + * @high: Set MPIO high if true, low if false > > > + * > > > + * Set a single MPIO high or low. exar_mpio_config must also be called > > > + * to configure the pin as an output. > > > + * > > > + * Return: 0 on success, negative error code on failure > > > + */ > > > +static int exar_mpio_set(struct exar8250 *priv, > > > + unsigned int mpio_num, bool high) > > > +{ > > > + uint8_t reg; > > > + uint8_t value; > > > + unsigned int bit; > > > + > > > + if (mpio_num < 8) { > > > + reg = UART_EXAR_MPIOSEL_7_0; > > > + bit = mpio_num; > > > + } else if (mpio_num >= 8 && mpio_num < 16) { > > > + reg = UART_EXAR_MPIOSEL_15_8; > > > + bit = mpio_num - 8; > > > + } else { > > > + return -EINVAL; > > > + } > > > + > > > + value = exar_read_reg(priv, reg); > > > + > > > + if (high) > > > + value |= BIT(bit); > > > + else > > > + value &= ~(BIT(bit)); > > > > Extra parenthesis. > > > > > + > > > + exar_write_reg(priv, reg, value); > > > > Again, I'd put this kind of simple RMW sequence without newlines. > > > > > + > > > + return 0; > > > +} > > I will fix above. > > > There are zero users of these functions so I couldn't review if two > > functions are really needed, or if the difference could be simply handled > > using a boolean parameter. > > > > The functions are used by code in other patches in this series. > > I kept exar_mpio_set() and exar_mpio_config() separate because we plan on > adding support for other features in the future that require reading and > writing MPIO. Ok. After getting up to the point where the callers were, I started to understand things somewhat better so keeping them separate seems fine with how I ended up understanding things. But please put these functions into the patch which is using them when you reorganize the series.
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c index 49d690344e65..9915a99cb7c6 100644 --- a/drivers/tty/serial/8250/8250_exar.c +++ b/drivers/tty/serial/8250/8250_exar.c @@ -305,6 +305,94 @@ static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr) return data; } +/** + * exar_mpio_config() - Configure an EXar MPIO as input or output + * @priv: Device's private structure + * @mpio_num: MPIO number/offset to configure + * @output: Configure as output if true, inout if false + * + * Configure a single MPIO as an input or output and disable trisate. + * If configuring as output it is reccomended to set value with + * exar_mpio_set prior to calling this function to ensure default state. + * + * Return: 0 on success, negative error code on failure + */ +static int exar_mpio_config(struct exar8250 *priv, + unsigned int mpio_num, bool output) +{ + uint8_t sel_reg; //MPIO Select register (input/output) + uint8_t tri_reg; //MPIO Tristate register + uint8_t value; + unsigned int bit; + + if (mpio_num < 8) { + sel_reg = UART_EXAR_MPIOSEL_7_0; + tri_reg = UART_EXAR_MPIO3T_7_0; + bit = mpio_num; + } else if (mpio_num >= 8 && mpio_num < 16) { + sel_reg = UART_EXAR_MPIOSEL_15_8; + tri_reg = UART_EXAR_MPIO3T_15_8; + bit = mpio_num - 8; + } else { + return -EINVAL; + } + + //Disable MPIO pin tri-state + value = exar_read_reg(priv, tri_reg); + value &= ~(BIT(bit)); + exar_write_reg(priv, tri_reg, value); + + value = exar_read_reg(priv, sel_reg); + //Set MPIO as input (1) or output (0) + if (output) + value &= ~(BIT(bit)); + else + value |= BIT(bit); + + exar_write_reg(priv, sel_reg, value); + + return 0; +} +/** + * exar_mpio_set() - Set an Exar MPIO output high or low + * @priv: Device's private structure + * @mpio_num: MPIO number/offset to set + * @high: Set MPIO high if true, low if false + * + * Set a single MPIO high or low. exar_mpio_config must also be called + * to configure the pin as an output. + * + * Return: 0 on success, negative error code on failure + */ +static int exar_mpio_set(struct exar8250 *priv, + unsigned int mpio_num, bool high) +{ + uint8_t reg; + uint8_t value; + unsigned int bit; + + if (mpio_num < 8) { + reg = UART_EXAR_MPIOSEL_7_0; + bit = mpio_num; + } else if (mpio_num >= 8 && mpio_num < 16) { + reg = UART_EXAR_MPIOSEL_15_8; + bit = mpio_num - 8; + } else { + return -EINVAL; + } + + value = exar_read_reg(priv, reg); + + if (high) + value |= BIT(bit); + else + value &= ~(BIT(bit)); + + exar_write_reg(priv, reg, value); + + return 0; +} + static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old) { /*