Received-SPF: pass (google.com: domain of linux-kernel+bounces-130413-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240402174353.256627-1-hugo@hugovil.com> <20240402174353.256627-4-hugo@hugovil.com>
 <CAHp75VdZ5yYVx7Df7G4X4Y7ZvJ3LAdq=A0fVNzNfMcdywJC-dQ@mail.gmail.com>
 <20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com> <CAHp75VcEj64aFb0nT8uQitkNSdWWqz7pmAqSGaGwy-tA=df0EA@mail.gmail.com>
 <20240403135910.2ef852670c184927340d08d7@hugovil.com>
In-Reply-To: <20240403135910.2ef852670c184927340d08d7@hugovil.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Wed, 3 Apr 2024 21:04:29 +0300
Message-ID: <CAHp75Vdm0RAX_xD41VvXuv3GG0SkHffqw7v=hs_6m4Ev1WZjMQ@mail.gmail.com>
Subject: Re: [PATCH v3 3/5] serial: sc16is7xx: split into core and I2C/SPI
 parts (core)
To: Hugo Villeneuve <hugo@hugovil.com>
Cc: gregkh@linuxfoundation.org, jirislaby@kernel.org, 
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, 
	Hugo Villeneuve <hvilleneuve@dimonoff.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 3, 2024 at 8:59=E2=80=AFPM Hugo Villeneuve <hugo@hugovil.com> w=
rote:
> On Wed, 3 Apr 2024 20:44:05 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Apr 3, 2024 at 7:35=E2=80=AFPM Hugo Villeneuve <hugo@hugovil.co=
m> wrote:
> > > On Tue, 2 Apr 2024 22:40:07 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Apr 2, 2024 at 8:45=E2=80=AFPM Hugo Villeneuve <hugo@hugovi=
l.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, simil=
ar
> > > 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.

..

> > > 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.

--=20
With Best Regards,
Andy Shevchenko