linux-kernel - Re: [PATCH v3 3/5] serial: sc16is7xx: split into core and I2C/SPI parts (core)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcEj64aFb0nT8uQitkNSdWWqz7pmAqSGaGwy-tA=df0EA@mail.gmail.com>
Date: Wed, 3 Apr 2024 20:44:05 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Hugo Villeneuve <hugo@...ovil.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	Hugo Villeneuve <hvilleneuve@...onoff.com>
Subject: Re: [PATCH v3 3/5] serial: sc16is7xx: split into core and I2C/SPI
 parts (core)

On Wed, Apr 3, 2024 at 7:35 PM Hugo Villeneuve <hugo@...ovil.com> wrote:
> On Tue, 2 Apr 2024 22:40:07 +0300
> Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> > On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@...ovil.com> wrote:

..

> > > -config SERIAL_SC16IS7XX_CORE
> > > -       tristate
> > > -
> > >  config SERIAL_SC16IS7XX
> > >         tristate "SC16IS7xx serial support"
> > >         select SERIAL_CORE
> > > -       depends on (SPI_MASTER && !I2C) || I2C
> > > +       depends on SPI_MASTER || I2C
> >
> > Is it?
>
> See discussion below, but I would remove the SPI/I2C depends. And I
> would rename SERIAL_SC16IS7XX to SERIAL_SC16IS7XX_CORE.
>
> >
> > >         help
> > >           Core driver for NXP SC16IS7xx serial ports.
> > >           Supported ICs are:
> > > @@ -1042,22 +1039,18 @@ config SERIAL_SC16IS7XX
> > >           drivers below.
> > >
> > >  config SERIAL_SC16IS7XX_I2C
> > > -       bool "SC16IS7xx for I2C interface"
> > > +       tristate "SC16IS7xx for I2C interface"
> > >         depends on SERIAL_SC16IS7XX
> > >         depends on I2C
> > > -       select SERIAL_SC16IS7XX_CORE if SERIAL_SC16IS7XX
> > > -       select REGMAP_I2C if I2C
> > > -       default y
> > > +       select REGMAP_I2C
> > >         help
> > > -         Enable SC16IS7xx driver on I2C bus,
> > > -         enabled by default to support oldconfig.
> > > +         Enable SC16IS7xx driver on I2C bus.
> > >
> > >  config SERIAL_SC16IS7XX_SPI
> > > -       bool "SC16IS7xx for spi interface"
> > > +       tristate "SC16IS7xx for SPI interface"
> > >         depends on SERIAL_SC16IS7XX
> > >         depends on SPI_MASTER
> > > -       select SERIAL_SC16IS7XX_CORE if SERIAL_SC16IS7XX
> > > -       select REGMAP_SPI if SPI_MASTER
> > > +       select REGMAP_SPI
> > >         help
> > >           Enable SC16IS7xx driver on SPI bus.
> >
> > Hmm... What I was thinking about is more like dropping
> >  the SERIAL_SC16IS7XX and having I2C/SPI to select the core.
> >
> > See many examples under drivers/iio on how it's done.
>
> Ok, I found this example:
> bf96f6e80cef ("iio: accel: kxsd9: Split out SPI transport")
>
> In it, the SPI part doesn't select the core, but depends on it, similar
> to what I have done. I find this approach more interesting for
> embedded systems as you can enable/disable I2C or SPI part if you
> need only one interface.

Yes, but what I mean is to have i2c/spi symbols visible and if user
selects one of them or both the core is automatically being selected.

..

> > > +#include <linux/device.h>
> >
> > Not used (by this file).
>
> I was assuming that this file was for "struct device"?

But it does not use it. It uses an opaque pointer, for which the
forward declaration is enough to have.

..

> > > +void sc16is7xx_remove(struct device *dev);
> >
> > Will require forward declaration
> >
> > #include ...
> >
> > struct device;
>
> Isn't it provided by <linux/device.h> ?

But why? Have you looked into device.h? It's a mess. You don't need it
in this header.

..

> > Follow the IWYU principle (include what you use).
>
> Ok, I tried to follow it, I do think those 4 includes are required in
> this file, no?

I haven't deeply checked, I believe for the next version you will
provide a better list.

> and maybe I can add <linux/string.h> for memcpy().

For sure, yes.

--
With Best Regards,
Andy Shevchenko