Received-SPF: pass (google.com: domain of linux-kernel+bounces-142739-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 12 Apr 2024 09:36:13 -0400
From: Parker Newman <parker@finest.io>
To: Ilpo =?UTF-8?B?SsOkcnZpbmVu?= <ilpo.jarvinen@linux.intel.com>
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
Message-ID: <20240412093613.0cbb4362@SWDEV2.connecttech.local>
In-Reply-To: <b057b1e2-1cf9-2f20-2453-b359a1e89f01@linux.intel.com>
References: <cover.1712863999.git.pnewman@connecttech.com>
	<3e671b6c0d11a2d0c292947675ed087eaaa5445e.1712863999.git.pnewman@connecttech.com>
	<b057b1e2-1cf9-2f20-2453-b359a1e89f01@linux.intel.com>
Organization: Connect Tech Inc.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:z11ttMU+Sow=;yZ1j1QLlQ7kqOHJfM1vaNxLdMci
 vEF5vMlPI5vB6+orJavTlmip6jjKG60mb1gQJ0Qrgi6mz23izlJ5LsV33fBBgj6PdBMBvqGv4
 dG9dLOnvTtJp39klqP+oLQWdQWLOYvH7z4y3Vb0Log+56US4/K49lWWsT7/LZ4CPv456rd+rE
 MdcCvkd6gcbdtC7KwHi4xHwD4p8W1dIIsFaPx4choiXr3V50x0BCplUc5EeGCuXQ+loA08Q5F
 XdTGOx6gG+DfB3m/v8gciVzHKYRYDVQa1UsmfEFp0ewe7G1VbWeej0RJ41IbiLsiSTShESWaF
 /vjA8QFdc4lxXga/xSzbOyyEYmRwh875xp6LUtrfvDDDXo1Cji+Gw8y2zvqKbuVi4wjQnNefQ
 3wFLIEIT3U+eDZ9d/ULXitEN0CKEE8UDFUmMgUR41MShUMvb06FwhjJwMgkieRMS8ZdHWrH0a
 kb01iQZSWiFrt/ky9dG8jueWfJHGvGowiAGHUIcHElDVNhMsPKQNGcLSVJ+PPNqY9We0rUtMT
 Owt5/BD4k66ZNcJ9OW56hGB0egKr+SfpaIIQjz3oXTBaGQ6+/oMhKApWVsQ5hXO86CQwXV3bQ
 /y8xXDtvNDzV1VgMQLvje0LNrqy4qCuWIn6D5DT02JDJ1ytJTsuivhOkB0rq85YRILDq4/oN4
 0HOHT8QSQ1vkCJRjawRkxFrVykn0mgXKXlxA/ghxr7Wq6uwxnn+F7BrsSduGc+rIyNxaxmMbn
 stiwrOWV8bj1jb05URYsRPMvWFbBSmd4dHOGWUsAtUY+kjfMW9u2yM=

On Fri, 12 Apr 2024 13:20:41 +0300 (EEST)
Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 11 Apr 2024, parker@finest.io wrote:
>=20
> > From: Parker Newman <pnewman@connecttech.com>
> >=20
> > Adds support for configuring and setting a single MPIO
> >=20
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 88 +++++++++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> >=20
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8=
250/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, uin=
t8_t ee_addr)
> >  	return data;
> >  }
> >=20
> > +/**
> > + * 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. =
=20
>=20
> tristate
>=20
> > + * If configuring as output it is reccomended to set value with
> > + * exar_mpio_set prior to calling this function to ensure default stat=
e. =20
>=20
> Use () if talking about function.
>=20
> > + *
> > + * 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 =3D UART_EXAR_MPIOSEL_7_0;
> > +		tri_reg =3D UART_EXAR_MPIO3T_7_0;
> > +		bit =3D mpio_num;
> > +	} else if (mpio_num >=3D 8 && mpio_num < 16) {
> > +		sel_reg =3D UART_EXAR_MPIOSEL_15_8;
> > +		tri_reg =3D UART_EXAR_MPIO3T_15_8;
> > +		bit =3D mpio_num - 8;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	//Disable MPIO pin tri-state
> > +	value =3D exar_read_reg(priv, tri_reg);
> > +	value &=3D ~(BIT(bit)); =20
>=20
> Use more meaningful variable name than "bit", it could perhaps even avoid=
=20
> the need to use the comment if the code is self-explanary with better=20
> variable name.
>=20
> > +	exar_write_reg(priv, tri_reg, value);
> > +
> > +	value =3D exar_read_reg(priv, sel_reg);
> > +	//Set MPIO as input (1) or output (0) =20
>=20
> Unnecessary comment.
>=20
> > +	if (output)
> > +		value &=3D ~(BIT(bit)); =20
>=20
> Unnecessary parenthesis.
>=20
> > +	else
> > +		value |=3D BIT(bit);
> > +
> > +	exar_write_reg(priv, sel_reg, value); =20
>=20
> Don't leave empty line into RMW sequence.
>=20
> > +
> > +	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 =3D UART_EXAR_MPIOSEL_7_0;
> > +		bit =3D mpio_num;
> > +	} else if (mpio_num >=3D 8 && mpio_num < 16) {
> > +		reg =3D UART_EXAR_MPIOSEL_15_8;
> > +		bit =3D mpio_num - 8;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	value =3D exar_read_reg(priv, reg);
> > +
> > +	if (high)
> > +		value |=3D BIT(bit);
> > +	else
> > +		value &=3D ~(BIT(bit)); =20
>=20
> Extra parenthesis.
>=20
> > +
> > +	exar_write_reg(priv, reg, value); =20
>=20
> Again, I'd put this kind of simple RMW sequence without newlines.
>=20
> > +
> > +	return 0;
> > +} =20

I will fix above.=20

> There are zero users of these functions so I couldn't review if two=20
> functions are really needed, or if the difference could be simply handled=
=20
> using a boolean parameter.
>=20

The functions are used by code in other patches in this series.=20

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=20
writing MPIO.=20

Thanks,
-Parker


=20