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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5083 bytes --]

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.

-- 
 i.