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

[Hugo Villeneuve <hugo@hugovil.com>]

On Tue, 2 Apr 2024 22:40:07 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

Hi Andy,

> On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > 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?

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.

> 
> ...
> 
> > +EXPORT_SYMBOL_GPL(sc16is74x_devtype);
> 
> Is it namespaced? Please make sure we are not polluting the global
> namespace with these.

Ok, will add namespace support to all exports.


> 
> ...
> 
> > +#ifndef _SC16IS7XX_H_
> > +#define _SC16IS7XX_H_
> > +
> > +#include <linux/device.h>
> 
> Not used (by this file).

I was assuming that this file was for "struct device"?


> 
> > +#include <linux/mod_devicetable.h>
> 
> > +#include <linux/regmap.h>
> 
> > +#include <linux/serial_core.h>
> 
> Not used.

Ok, will drop for V4.


> 
> > +#include <linux/types.h>
> 
> > +extern const struct of_device_id __maybe_unused sc16is7xx_dt_ids[];
> 
> No __maybe_unused. Just have it always be added.

Ok.


> 
> > +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;

Isn't it provided by <linux/device.h> ?


> 
> > +#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).

Ok, I tried to follow it, I do think those 4 includes are required in
this file, no?

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


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

Ok.


> 
> ...
> 
> > +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().

Ok. Great, simplifies things.


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

Ok.


> 
> ...
> 
> > +++ b/drivers/tty/serial/sc16is7xx_spi.c
> 
> Similar/same comments as per i2c counterpart.

Ok.


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


-- 
Hugo Villeneuve