Received-SPF: pass (google.com: domain of linux-kernel+bounces-142673-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 12 Apr 2024 08:58:30 -0400
From: Parker Newman <parker@finest.io>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: 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: <20240412085830.54935b1b@SWDEV2.connecttech.local>
In-Reply-To: <2024041247-clamor-bottom-ae36@gregkh>
References: <cover.1712863999.git.pnewman@connecttech.com>
	<d16cb88f916914278e125023c856bbf85d0908c1.1712863999.git.pnewman@connecttech.com>
	<2024041247-clamor-bottom-ae36@gregkh>
Organization: Connect Tech Inc.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:1PjHRcd3OtE=;9U5Afp4m+Fn+eIwhgszcOUPhjRK
 f0EWuKcShEJSQjMHGjXydB0iIDRWDuMxZqr/GLbPg0cCyucn/rDxKqJPXj/cfYOtsP6aZNljq
 VqXwbUU4O92h1NR/lj/vTpvS8199GHPGBJG9GcTOl07Av7drZMPpxjmFDPpdRRvG+P2iSreCc
 3HafG4q4ByYKZx2qZuEcqxVF8NaHxEMhAW2tfc2E3P/30z1QXvGMiciU9e2M6H7czvWCinXHN
 QugUL/UJfCL4hmxN2KRrbXmAGhKvT/OKTDT6SuJQiWLQllzMXuuC1KNLf6zU6N4c89IKfPYmg
 aqIXXqadm3qaJJF1fbhRVA4Yx/OPQfLgkxWP3VANa8q5vel0iXdACgYsdR9cjtkIIe0zRr73x
 XN0N/Wi6Pk7IrdpK+rMh8uXmHc37j3vHOQFX28fGDuumUad3GeEERSwpmG5ZZSiNdPaskEtLo
 jNLrPFWk9FTIy7jC5Sf7rnlCKcvlXJ/nr6akDOCOjblxpNESHhaGr69a/Zo/ijSt4pGnSXaYh
 U3Ndps75+7rT22yi4HwrXLHqDNlGLc+bcpIGXct2E294ZjrGE9DcKIODY/AcTTDm+GXvEvjgZ
 nKeuuljqM7BoXnHxexgPB+Ck9bQ4Gv7VDNr6TpDtxFqrEbH+GObRq8Og0gJQ1Sr4Br/YkcwWp
 fA72J52x1lgkY990fjQ8fIIkf8wzhmZ1Zf21scYCdrtxJymX8WN5XLAkD066buJbq/9prRba1
 NsAzg2m7JmNA3PybnMwfaks209FEygPL2T0roF/2R0/grsIpRvJVPI=

On Fri, 12 Apr 2024 07:26:49 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Apr 11, 2024 at 04:25:40PM -0400, parker@finest.io wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > - Adds support for reading a word from the Exar EEPROM.
> > - Adds exar_write_reg/exar_read_reg for reading and writing to the UAR=
T's
> > config registers.
>
> First off, thanks for splitting this up, looks much better.
>
> Some minor nits here:
>
> >
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 110 +++++++++++++++++++++++++++=
+
> >  1 file changed, 110 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/=
8250/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 Polar=
ity */
> >
> > +/* 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
>
> Use tabs after the define name and before the value?

There should be tabs there... I will re-tab them in v3 to make sure.

>
> > +
> > +
> >  /*
> >   * IOT2040 MPIO wiring semantics:
> >   *
> > @@ -195,6 +205,106 @@ struct exar8250 {
> >  	int			line[];
> >  };
> >
> > +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 i=
nt reg)
> > +{
> > +	if (!priv || !priv->virt)
> > +		return 0;
>
> How can either of these ever happen?  You control when this is called,
> right?  So just make sure that isn't an issue.

I think I was a bit over paranoid here. I was considering there are other
3rd party cards supported in this driver that use uart_port->private_data
differently or don't set it. I agree they aren't really needed in the end.

> > +
> > +	return readb(priv->virt + reg);
> > +}
> > +
> > +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> > +{
> > +	uint8_t value =3D 0x00;
>
> This is the kernel, please use kernel types, not userspace types (i.e.
> u8 not uint8_t).  Yes, there are lots of places in the kernel that have
> userspace types, but let's not add to the mess please.

Yes there is some confusion on which should be used.
Thanks for the clarification. I will convert in v3.

>
> > +
> > +	if (enable)
> > +		value |=3D UART_EXAR_REGB_EECS;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
>
> Why wait this amount of time?  A comment would be nice.  Why not just
> do a read to ensure the write happened instead?

The Exar UART uses a bit-bang I2C interface for the EEPROM so a delay is
needed for the I2C bit time (2us =3D 500khz). This is legacy code so I wil=
l
double check this is actually needed and add comments if it is.

> > +}
> > +
> > +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> > +{
> > +	uint8_t value =3D UART_EXAR_REGB_EECS;
>
> Same comment about the type, here and everywhere else.
>
> > +
> > +	if (bit)
> > +		value |=3D UART_EXAR_REGB_EEDI;
> > +
> > +	//Clock out the bit on the i2c interface
>
> Comments using // are fine, but please put a space after the "//" to
> make them readable

I will fix.

> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
>
> Same commment about the time value, here and everywhere else.  Why slow
> things down if you don't have to?
>
> thanks,
>
> greg k-h

Thanks for the feedback!
- Parker