Received-SPF: pass (google.com: domain of linux-kernel+bounces-142688-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 12 Apr 2024 09:06:54 -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>, linux-kernel@vger.kernel.org,
 linux-serial@vger.kernel.org, Parker Newman <pnewman@connecttech.com>
Subject: Re: [PATCH v2 2/7] serial: exar: add support for reading from Exar
 EEPROM
Message-ID: <20240412090654.54c43612@SWDEV2.connecttech.local>
In-Reply-To: <2b817d47-52f6-bc9b-cb24-4f540101ea50@linux.intel.com>
References: <cover.1712863999.git.pnewman@connecttech.com>
	<d16cb88f916914278e125023c856bbf85d0908c1.1712863999.git.pnewman@connecttech.com>
	<2b817d47-52f6-bc9b-cb24-4f540101ea50@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:90UXfPez/NA=;NUO32OZvhgDrW+EPC8F4hUdjKdr
 Tjvcfu08S4skfqDiCOmkR4B1Wj26FGNfIRptEHcMgzGx5cJzVio72HpyH4dtBW5u+5Pf2F1xz
 uITYxNDICrkhzveAugTfpbjHJjDIxq19MPGtRTPK56eOHfKE0nnvXUp3IwnQzlZFdALBsMET8
 XVwntZTeL8FDxagq1euMFVZrpGuAgVgDqO7aILo/cQpZQCTL/afU/OB+OkY0o9TJdHjPwB1aJ
 PUurYnED9fqEAyYgMSSWADySXl2YqTHGuQWuNr1Y2ZKRyZpdRg2KP/ZcOf2/BRanmTDuKqYXu
 UxpfnsvSLY9wbasETPwbi6fSTt+nfca4d+ptCHESeRlHDXxY6J3da1uGdZ4XRy/p8njCDeBp3
 l7HOdrb8Cj5v0R0BlrqSESzyzwoXQmRbbQI0s7dSOOZadeoLjp7Li/4bn87hnl8hnUZYPh2qE
 uTMxjEkc38jFQn9ENAVLK3GofUtJB3dQSoh3xCcKnaBBow4JElAV32gspadY4RgP6/OWexpBW
 V2YlMhYQw9oXMeZkOZqKgFPiZyIjzFZiTTyhylRMtKncwYXMqFaK5L2KUELkXv3lj3VezKBf2
 pti6GK81wVEyq/m/dkC1EpcADkxOnJq4XtyugOrNm5/ziwnB8G+oKHWzZkPZzRUgY276Mctz9
 2LmeQlH/Gh7gJa1G1+ozBLGolyZ0maYhQAhjfk8+4BL4UvdimVHj+keb09n/jY8RVthWsBGJz
 HmOfAJ02/q4oYyYrQcLovCGBFeZHmv0mlHsHtMd2l2gweI8wIvtHDE=

On Fri, 12 Apr 2024 13:36:42 +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 reading a word from the Exar EEPROM.
> > - Adds exar_write_reg/exar_read_reg for reading and writing to the UART=
's
> > config registers.
> >=20
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 110 ++++++++++++++++++++++++++++
> >  1 file changed, 110 insertions(+)
> >=20
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8=
250/8250_exar.c
> > index 4d1e07343d0b..49d690344e65 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -128,6 +128,16 @@
> >  #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
> >  #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polari=
ty */
> >=20
> > +/* EEPROM registers */
> > +#define UART_EXAR_REGB                  0x8e
> > +#define UART_EXAR_REGB_EECK             BIT(4)
> > +#define UART_EXAR_REGB_EECS             BIT(5)
> > +#define UART_EXAR_REGB_EEDI             BIT(6)
> > +#define UART_EXAR_REGB_EEDO             BIT(7)
> > +#define UART_EXAR_REGB_EE_ADDR_SIZE     6
> > +#define UART_EXAR_REGB_EE_DATA_SIZE     16
> > +
> > + =20
>=20
> Extra new line.
>=20
> >  /*
> >   * IOT2040 MPIO wiring semantics:
> >   *
> > @@ -195,6 +205,106 @@ struct exar8250 {
> >  	int			line[];
> >  };
> >=20
> > +static inline void exar_write_reg(struct exar8250 *priv,
> > +				unsigned int reg, uint8_t value)
> > +{
> > +	if (!priv || !priv->virt)
> > +		return;
> > +
> > +	writeb(value, priv->virt + reg);
> > +}
> > +
> > +static inline uint8_t exar_read_reg(struct exar8250 *priv, unsigned in=
t reg)
> > +{
> > +	if (!priv || !priv->virt)
> > +		return 0;
> > +
> > +	return readb(priv->virt + reg);
> > +}
> > +
> > +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> > +{
> > +	uint8_t value =3D 0x00;
> > +
> > +	if (enable)
> > +		value |=3D UART_EXAR_REGB_EECS; =20
>=20
> You could just do:
> 	u8 value;
>=20
> 	value =3D enable ? UART_EXAR_REGB_EECS : 0;
>=20
> Or even:
>=20
> 	exar_write_reg(priv, UART_EXAR_REGB, enable ? UART_EXAR_REGB_EECS : 0);
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +}
> > +
> > +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> > +{
> > +	uint8_t value =3D UART_EXAR_REGB_EECS;
> > +
> > +	if (bit)
> > +		value |=3D UART_EXAR_REGB_EEDI;
> > +
> > +	//Clock out the bit on the i2c interface
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +
> > +	value |=3D UART_EXAR_REGB_EECK;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +}
> > +
> > +static inline uint8_t exar_ee_read_bit(struct exar8250 *priv)
> > +{
> > +	uint8_t regb;
> > +	uint8_t value =3D UART_EXAR_REGB_EECS;
> > +
> > +	//Clock in the bit on the i2c interface
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +
> > +	value |=3D UART_EXAR_REGB_EECK;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +
> > +	regb =3D exar_read_reg(priv, UART_EXAR_REGB);
> > +
> > +	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
> > +}
> > +
> > +/**
> > + * exar_ee_read() - Read a word from the EEPROM
> > + * @priv: Device's private structure
> > + * @ee_addr: Offset of EEPROM to read word from
> > + *
> > + * Read a single 16bit word from an Exar UART's EEPROM =20
>=20
> Add missing .
>=20
> > + *
> > + * Return: EEPROM word on success, negative error code on failure =20
>=20
> This function does not return any -Exx code as far as I can see??
>=20
> > + */
> > +static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
> > +{
> > +	int i;
> > +	int data =3D 0;
> > +
> > +	exar_ee_select(priv, true);
> > +
> > +	//Send read command (opcode 110)
> > +	exar_ee_write_bit(priv, 1);
> > +	exar_ee_write_bit(priv, 1);
> > +	exar_ee_write_bit(priv, 0);
> > +
> > +	//Send address to read from
> > +	for (i =3D 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>=3D 1)
> > +		exar_ee_write_bit(priv, (ee_addr & i));
> > +
> > +	//Read data 1 bit at a time
> > +	for (i =3D 0; i <=3D UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> > +		data <<=3D 1;
> > +		data |=3D exar_ee_read_bit(priv);
> > +	}
> > +
> > +	exar_ee_select(priv, false);
> > +
> > +	return data;
> > +} =20
>=20

I will fix all of these.=20
Thanks,
-Parker