Received-SPF: pass (google.com: domain of linux-kernel+bounces-128595-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240402174353.256627-1-hugo@hugovil.com> <20240402174353.256627-4-hugo@hugovil.com>
In-Reply-To: <20240402174353.256627-4-hugo@hugovil.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Tue, 2 Apr 2024 22:40:07 +0300
Message-ID: <CAHp75VdZ5yYVx7Df7G4X4Y7ZvJ3LAdq=A0fVNzNfMcdywJC-dQ@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 Tue, Apr 2, 2024 at 8:45=E2=80=AFPM Hugo Villeneuve <hugo@hugovil.com> w=
rote:
>
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Split the common code from sc16is7xx driver and move the I2C and SPI bus
> parts into interface-specific source files.
>
> sc16is7xx becomes the core functions which can support multiple bus
> interfaces like I2C and SPI.
>
> No functional change intended.

..

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

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

..

> +EXPORT_SYMBOL_GPL(sc16is74x_devtype);

Is it namespaced? Please make sure we are not polluting the global
namespace with these.

..

> +#ifndef _SC16IS7XX_H_
> +#define _SC16IS7XX_H_
> +
> +#include <linux/device.h>

Not used (by this file).

> +#include <linux/mod_devicetable.h>

> +#include <linux/regmap.h>

> +#include <linux/serial_core.h>

Not used.

> +#include <linux/types.h>

> +extern const struct of_device_id __maybe_unused sc16is7xx_dt_ids[];

No __maybe_unused. Just have it always be added.

> +const char *sc16is7xx_regmap_name(u8 port_id);
> +
> +unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id);
> +
> +int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *=
devtype,
> +                   struct regmap *regmaps[], int irq);

> +void sc16is7xx_remove(struct device *dev);

Will require forward declaration

#include ...

struct device;

> +#endif /* _SC16IS7XX_H_ */

..

> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>

Follow the IWYU principle (include what you use).

..

> +               return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match=
 device\n");

+ dev_printk.h

..

> +static int __init sc16is7xx_i2c_init(void)
> +{
> +       return i2c_add_driver(&sc16is7xx_i2c_driver);
> +}
> +module_init(sc16is7xx_i2c_init);
> +
> +static void __exit sc16is7xx_i2c_exit(void)
> +{
> +       i2c_del_driver(&sc16is7xx_i2c_driver);
> +}
> +module_exit(sc16is7xx_i2c_exit);

This is now module_i2c_driver().

..

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SC16IS7xx I2C interface driver");

+ MODULE_IMPORT_NS()

..

> +++ b/drivers/tty/serial/sc16is7xx_spi.c

Similar/same comments as per i2c counterpart.

--=20
With Best Regards,
Andy Shevchenko