[v2,3/7] serial: exar: add support for config/set single MPIO

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(+)

--
2.43.2

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.