Received-SPF: pass (google.com: domain of linux-kernel+bounces-142484-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 12 Apr 2024 13:20:41 +0300 (EEST)
To: parker@finest.io
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
    Jiri Slaby <jirislaby@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
    linux-serial <linux-serial@vger.kernel.org>, 
    Parker Newman <pnewman@connecttech.com>
Subject: Re: [PATCH v2 3/7] serial: exar: add support for config/set single
 MPIO
In-Reply-To: <3e671b6c0d11a2d0c292947675ed087eaaa5445e.1712863999.git.pnewman@connecttech.com>
Message-ID: <b057b1e2-1cf9-2f20-2453-b359a1e89f01@linux.intel.com>
References: <cover.1712863999.git.pnewman@connecttech.com> <3e671b6c0d11a2d0c292947675ed087eaaa5445e.1712863999.git.pnewman@connecttech.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

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;
> +}

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.

-- 
 i.