From: Parker Newman <[email protected]>
- 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.
Signed-off-by: Parker Newman <[email protected]>
---
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
+
+
/*
* 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;
+
+ return readb(priv->virt + reg);
+}
+
+static inline void exar_ee_select(struct exar8250 *priv, bool enable)
+{
+ uint8_t value = 0x00;
+
+ if (enable)
+ value |= UART_EXAR_REGB_EECS;
+
+ 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 = UART_EXAR_REGB_EECS;
+
+ if (bit)
+ value |= UART_EXAR_REGB_EEDI;
+
+ //Clock out the bit on the i2c interface
+ exar_write_reg(priv, UART_EXAR_REGB, value);
+ udelay(2);
+
+ value |= 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 = UART_EXAR_REGB_EECS;
+
+ //Clock in the bit on the i2c interface
+ exar_write_reg(priv, UART_EXAR_REGB, value);
+ udelay(2);
+
+ value |= UART_EXAR_REGB_EECK;
+
+ exar_write_reg(priv, UART_EXAR_REGB, value);
+ udelay(2);
+
+ regb = 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
+ *
+ * Return: EEPROM word on success, negative error code on failure
+ */
+static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
+{
+ int i;
+ int data = 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 = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
+ exar_ee_write_bit(priv, (ee_addr & i));
+
+ //Read data 1 bit at a time
+ for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
+ data <<= 1;
+ data |= exar_ee_read_bit(priv);
+ }
+
+ exar_ee_select(priv, false);
+
+ return data;
+}
+
static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
{
/*
--
2.43.2
On Thu, Apr 11, 2024 at 04:25:40PM -0400, [email protected] wrote:
> From: Parker Newman <[email protected]>
>
> - 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 <[email protected]>
> ---
> 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
On Thu, 11 Apr 2024, [email protected] wrote:
> From: Parker Newman <[email protected]>
>
> - 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.
>
> Signed-off-by: Parker Newman <[email protected]>
> ---
> 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
> +
> +
Extra new line.
> /*
> * 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;
> +
> + return readb(priv->virt + reg);
> +}
> +
> +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> +{
> + uint8_t value = 0x00;
> +
> + if (enable)
> + value |= UART_EXAR_REGB_EECS;
You could just do:
u8 value;
value = enable ? UART_EXAR_REGB_EECS : 0;
Or even:
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 = UART_EXAR_REGB_EECS;
> +
> + if (bit)
> + value |= UART_EXAR_REGB_EEDI;
> +
> + //Clock out the bit on the i2c interface
> + exar_write_reg(priv, UART_EXAR_REGB, value);
> + udelay(2);
> +
> + value |= 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 = UART_EXAR_REGB_EECS;
> +
> + //Clock in the bit on the i2c interface
> + exar_write_reg(priv, UART_EXAR_REGB, value);
> + udelay(2);
> +
> + value |= UART_EXAR_REGB_EECK;
> +
> + exar_write_reg(priv, UART_EXAR_REGB, value);
> + udelay(2);
> +
> + regb = 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
Add missing .
> + *
> + * Return: EEPROM word on success, negative error code on failure
This function does not return any -Exx code as far as I can see??
> + */
> +static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
> +{
> + int i;
> + int data = 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 = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> + exar_ee_write_bit(priv, (ee_addr & i));
> +
> + //Read data 1 bit at a time
> + for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> + data <<= 1;
> + data |= exar_ee_read_bit(priv);
> + }
> +
> + exar_ee_select(priv, false);
> +
> + return data;
> +}
--
i.
On Fri, 12 Apr 2024 07:26:49 +0200
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Apr 11, 2024 at 04:25:40PM -0400, [email protected] wrote:
> > From: Parker Newman <[email protected]>
> >
> > - 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 <[email protected]>
> > ---
> > 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?
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 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.
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 = 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 |= 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 = 500khz). This is legacy code so I will
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 = 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
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
On Fri, 12 Apr 2024 13:36:42 +0300 (EEST)
Ilpo Järvinen <[email protected]> wrote:
> On Thu, 11 Apr 2024, [email protected] wrote:
>
> > From: Parker Newman <[email protected]>
> >
> > - 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.
> >
> > Signed-off-by: Parker Newman <[email protected]>
> > ---
> > 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
> > +
> > +
>
> Extra new line.
>
> > /*
> > * 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;
> > +
> > + return readb(priv->virt + reg);
> > +}
> > +
> > +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> > +{
> > + uint8_t value = 0x00;
> > +
> > + if (enable)
> > + value |= UART_EXAR_REGB_EECS;
>
> You could just do:
> u8 value;
>
> value = enable ? UART_EXAR_REGB_EECS : 0;
>
> Or even:
>
> 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 = UART_EXAR_REGB_EECS;
> > +
> > + if (bit)
> > + value |= UART_EXAR_REGB_EEDI;
> > +
> > + //Clock out the bit on the i2c interface
> > + exar_write_reg(priv, UART_EXAR_REGB, value);
> > + udelay(2);
> > +
> > + value |= 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 = UART_EXAR_REGB_EECS;
> > +
> > + //Clock in the bit on the i2c interface
> > + exar_write_reg(priv, UART_EXAR_REGB, value);
> > + udelay(2);
> > +
> > + value |= UART_EXAR_REGB_EECK;
> > +
> > + exar_write_reg(priv, UART_EXAR_REGB, value);
> > + udelay(2);
> > +
> > + regb = 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
>
> Add missing .
>
> > + *
> > + * Return: EEPROM word on success, negative error code on failure
>
> This function does not return any -Exx code as far as I can see??
>
> > + */
> > +static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
> > +{
> > + int i;
> > + int data = 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 = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> > + exar_ee_write_bit(priv, (ee_addr & i));
> > +
> > + //Read data 1 bit at a time
> > + for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> > + data <<= 1;
> > + data |= exar_ee_read_bit(priv);
> > + }
> > +
> > + exar_ee_select(priv, false);
> > +
> > + return data;
> > +}
>
I will fix all of these.
Thanks,
-Parker