On Tue, 2 Apr 2024 22:40:07 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
Hi Andy,
> On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:
> >
> > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> >
> > 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
