Received-SPF: pass (google.com: domain of linux-kernel+bounces-142756-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 12 Apr 2024 16:44:55 +0300 (EEST)
To: Parker Newman <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: <20240412093613.0cbb4362@SWDEV2.connecttech.local>
Message-ID: <90e76b4c-10f8-2c06-2c86-51bdf822f968@linux.intel.com>
References: <cover.1712863999.git.pnewman@connecttech.com> <3e671b6c0d11a2d0c292947675ed087eaaa5445e.1712863999.git.pnewman@connecttech.com> <b057b1e2-1cf9-2f20-2453-b359a1e89f01@linux.intel.com> <20240412093613.0cbb4362@SWDEV2.connecttech.local>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323328-2064743525-1712929495=:1014"

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-2064743525-1712929495=:1014
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Fri, 12 Apr 2024, Parker Newman wrote:

> On Fri, 12 Apr 2024 13:20:41 +0300 (EEST)
> Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com> wrote:
>=20
> > 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=
/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, u=
int8_t ee_addr)
> > >  =09return 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=
=2E =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 st=
ate. =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,
> > > +=09=09=09unsigned int mpio_num, bool output)
> > > +{
> > > +=09uint8_t sel_reg; //MPIO Select register (input/output)
> > > +=09uint8_t tri_reg; //MPIO Tristate register
> > > +=09uint8_t value;
> > > +=09unsigned int bit;
> > > +
> > > +=09if (mpio_num < 8) {
> > > +=09=09sel_reg =3D UART_EXAR_MPIOSEL_7_0;
> > > +=09=09tri_reg =3D UART_EXAR_MPIO3T_7_0;
> > > +=09=09bit =3D mpio_num;
> > > +=09} else if (mpio_num >=3D 8 && mpio_num < 16) {
> > > +=09=09sel_reg =3D UART_EXAR_MPIOSEL_15_8;
> > > +=09=09tri_reg =3D UART_EXAR_MPIO3T_15_8;
> > > +=09=09bit =3D mpio_num - 8;
> > > +=09} else {
> > > +=09=09return -EINVAL;
> > > +=09}
> > > +
> > > +=09//Disable MPIO pin tri-state
> > > +=09value =3D exar_read_reg(priv, tri_reg);
> > > +=09value &=3D ~(BIT(bit)); =20
> >=20
> > Use more meaningful variable name than "bit", it could perhaps even avo=
id=20
> > the need to use the comment if the code is self-explanary with better=
=20
> > variable name.
> >=20
> > > +=09exar_write_reg(priv, tri_reg, value);
> > > +
> > > +=09value =3D exar_read_reg(priv, sel_reg);
> > > +=09//Set MPIO as input (1) or output (0) =20
> >=20
> > Unnecessary comment.
> >=20
> > > +=09if (output)
> > > +=09=09value &=3D ~(BIT(bit)); =20
> >=20
> > Unnecessary parenthesis.
> >=20
> > > +=09else
> > > +=09=09value |=3D BIT(bit);
> > > +
> > > +=09exar_write_reg(priv, sel_reg, value); =20
> >=20
> > Don't leave empty line into RMW sequence.
> >=20
> > > +
> > > +=09return 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 call=
ed
> > > + * to configure the pin as an output.
> > > + *
> > > + * Return: 0 on success, negative error code on failure
> > > + */
> > > +static int exar_mpio_set(struct exar8250 *priv,
> > > +=09=09unsigned int mpio_num, bool high)
> > > +{
> > > +=09uint8_t reg;
> > > +=09uint8_t value;
> > > +=09unsigned int bit;
> > > +
> > > +=09if (mpio_num < 8) {
> > > +=09=09reg =3D UART_EXAR_MPIOSEL_7_0;
> > > +=09=09bit =3D mpio_num;
> > > +=09} else if (mpio_num >=3D 8 && mpio_num < 16) {
> > > +=09=09reg =3D UART_EXAR_MPIOSEL_15_8;
> > > +=09=09bit =3D mpio_num - 8;
> > > +=09} else {
> > > +=09=09return -EINVAL;
> > > +=09}
> > > +
> > > +=09value =3D exar_read_reg(priv, reg);
> > > +
> > > +=09if (high)
> > > +=09=09value |=3D BIT(bit);
> > > +=09else
> > > +=09=09value &=3D ~(BIT(bit)); =20
> >=20
> > Extra parenthesis.
> >=20
> > > +
> > > +=09exar_write_reg(priv, reg, value); =20
> >=20
> > Again, I'd put this kind of simple RMW sequence without newlines.
> >=20
> > > +
> > > +=09return 0;
> > > +} =20
>=20
> I will fix above.=20
>=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 handl=
ed=20
> > using a boolean parameter.
> >=20
>=20
> The functions are used by code in other patches in this series.=20
>=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

Ok. After getting up to the point where the callers were, I started to=20
understand things somewhat better so keeping them separate seems fine=20
with how I ended up understanding things.

But please put these functions into the patch which is using them when you=
=20
reorganize the series.

--=20
 i.

--8323328-2064743525-1712929495=:1014--