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
Date: Fri, 12 Apr 2024 08:58:30 -0400 [thread overview]
Message-ID: <20240412085830.54935b1b@SWDEV2.connecttech.local> (raw)
In-Reply-To: <2024041247-clamor-bottom-ae36@gregkh>
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 UART'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 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
next prev parent reply other threads:[~2024-04-12 12:58 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 20:25 [PATCH v2 0/7] serial: exar: add Connect Tech serial cards to Exar driver parker
2024-04-11 20:25 ` [PATCH v2 1/7] serial: exar: adding missing CTI and Exar PCI ids parker
2024-04-11 20:25 ` [PATCH v2 2/7] serial: exar: add support for reading from Exar EEPROM parker
2024-04-12 5:26 ` Greg Kroah-Hartman
2024-04-12 12:58 ` Parker Newman [this message]
2024-04-12 10:36 ` Ilpo Järvinen
2024-04-12 13:06 ` Parker Newman
2024-04-11 20:25 ` [PATCH v2 3/7] serial: exar: add support for config/set single MPIO parker
2024-04-12 5:29 ` Greg Kroah-Hartman
2024-04-12 13:05 ` Parker Newman
2024-04-12 10:20 ` Ilpo Järvinen
2024-04-12 13:36 ` Parker Newman
2024-04-12 13:44 ` Ilpo Järvinen
2024-04-11 20:25 ` [PATCH v2 4/7] serial: exar: add optional board_setup function parker
2024-04-12 10:08 ` Ilpo Järvinen
2024-04-12 13:50 ` Parker Newman
2024-04-11 20:25 ` [PATCH v2 5/7] serial: exar: add some CTI helper functions parker
2024-04-12 10:48 ` Ilpo Järvinen
2024-04-12 13:57 ` Parker Newman
2024-04-12 14:09 ` Ilpo Järvinen
2024-04-11 20:25 ` [PATCH v2 6/7] serial: exar: add CTI board and port setup functions parker
2024-04-12 10:57 ` Ilpo Järvinen
2024-04-12 15:19 ` Parker Newman
2024-04-12 15:28 ` Greg Kroah-Hartman
2024-04-12 15:39 ` Parker Newman
2024-04-11 20:25 ` [PATCH v2 7/7] serial: exar: fix: fix crash during shutdown if setup fails parker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240412085830.54935b1b@SWDEV2.connecttech.local \
--to=parker@finest.io \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=pnewman@connecttech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).