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

[Hugo Villeneuve]

On Wed, 3 Apr 2024 21:04:29 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Wed, Apr 3, 2024 at 8:59 PM Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:
> > 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
> 
> Hmm... How do the other drivers have this separation? So, check
> several examples (and check _the recently added_) in IIO and follow
> that. If they have CORE visible, follow it.

In iio/accel, there is a roughly equal mix of everything. But all
examples that automatically select CORE do not list any specific
variants, probably because of the maintenance burden that this would
impose like I mentioned.

The most recent one is bmi088, which has only one KConfig
selection available, with I2C/SPI parts automatically selected, and
with the benefit of only having to list all variants once. The only
minor drawback of course is having to (almost always) compile an
interface (I2C or SPI) which won't be used.

I don't mind going this way for V4.


> 
> ...
> 
> > > > 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.
> 
> This file doesn't use the struct device, it uses an opaque pointer. In
> C for this the forward declaration is enough, no need to include a
> whole mess from device.h.

Ok.

Hugo.


> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 


-- 
Hugo Villeneuve