On Wed, 3 Apr 2024 20:44:05 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Wed, Apr 3, 2024 at 7:35 PM Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:
> > On Tue, 2 Apr 2024 22:40:07 +0300
> > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > > On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@xxxxxxxxxxx> 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.
Ok, I see. But a drawback of doing this is that for each interface
(I2C/SPI), you would need to list (duplicate) all the devices
supported. For now, that list is only in one place,
for the generic SERIAL_SC16IS7XX_CORE section:
...
config SERIAL_SC16IS7XX_CORE
tristate "SC16IS7xx serial support"
select SERIAL_CORE
help
Core driver for NXP SC16IS7xx serial ports.
Supported ICs are:
SC16IS740
SC16IS741
SC16IS750
...
>
> ...
>
> > > > +#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.
Yes I have looked at it, and saw that the forward declaration of
"struct device" opaque pointer is in it, and this is why I was
including it. But I will remove it if you wish.
>
> ...
>
> > > 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.
Ok
>
> > and maybe I can add <linux/string.h> for memcpy().
>
> For sure, yes.
Ok.
Thanks for your comments.
Hugo.
