Re: [PATCH v2 2/7] serial: exar: add support for reading from Exar EEPROM

[Greg Kroah-Hartman]

On Thu, Apr 11, 2024 at 04:25:40PM -0400, parker@xxxxxxxxx wrote:
> From: Parker Newman <pnewman@xxxxxxxxxxxxxxx>
> 
> - 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.

First off, thanks for splitting this up, looks much better.

Some minor nits here:

> 
> Signed-off-by: Parker Newman <pnewman@xxxxxxxxxxxxxxx>
> ---
>  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 Polarity */
> 
> +/* 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?

> +
> +
>  /*
>   * 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 int 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.

> +
> +	return readb(priv->virt + reg);
> +}
> +
> +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> +{
> +	uint8_t value = 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.

> +
> +	if (enable)
> +		value |= 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?

> +}
> +
> +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> +{
> +	uint8_t value = UART_EXAR_REGB_EECS;

Same comment about the type, here and everywhere else.

> +
> +	if (bit)
> +		value |= 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

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