Messages in this thread | | | Date | Wed, 3 Apr 2024 12:35:01 -0400 | From | Hugo Villeneuve <> | Subject | Re: [PATCH v3 3/5] serial: sc16is7xx: split into core and I2C/SPI parts (core) |
| |
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
| |