Hi Greg, Thanks for the reviews. > -----Original Message----- > From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > Sent: Thursday, July 16, 2020 12:10 AM > To: Mani, Rajmohan <rajmohan.mani@xxxxxxxxx> > Cc: Darren Hart <dvhart@xxxxxxxxxxxxx>; Andy Shevchenko > <andy@xxxxxxxxxxxxx>; Mika Westerberg > <mika.westerberg@xxxxxxxxxxxxxxx>; Dmitry Torokhov > <dmitry.torokhov@xxxxxxxxx>; Lee Jones <lee.jones@xxxxxxxxxx>; Ayman > Bagabas <ayman.bagabas@xxxxxxxxx>; Masahiro Yamada > <masahiroy@xxxxxxxxxx>; Joseph, Jithu <jithu.joseph@xxxxxxxxx>; Blaž > Hrastnik <blaz@xxxxxxx>; Srinivas Pandruvada > <srinivas.pandruvada@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > platform-driver-x86@xxxxxxxxxxxxxxx; Heikki Krogerus > <heikki.krogerus@xxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; > pmalani@xxxxxxxxxxxx; bleung@xxxxxxxxxxxx > Subject: Re: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) > driver > > On Wed, Jul 15, 2020 at 05:33:09PM -0700, Rajmohan Mani wrote: > > Input Output Manager (IOM) is part of the Tiger Lake SoC that > > configures the Type-C Sub System (TCSS). IOM is a micro controller > > that handles Type-C topology, configuration and PM functions of > > various Type-C devices connected on the platform. > > > > This driver helps read relevant information such as Type-C port status > > (whether a device is connected to a Type-C port or not) and the > > activity type on the Type-C ports (such as USB, Display Port, > > Thunderbolt), for consumption by other drivers. > > > > Currently intel_iom_port_status() API is exported by this driver, that > > has information about the Type-C port status and port activity type. > > > > Signed-off-by: Rajmohan Mani <rajmohan.mani@xxxxxxxxx> > > --- > > drivers/platform/x86/Kconfig | 16 +++ > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/intel_iom.c | 133 ++++++++++++++++++++ > > include/linux/platform_data/x86/intel_iom.h | 62 +++++++++ > > Why do you need a .h file for a single .c file that no one else shares this data? > Just put it all in the .c file please. > The APIs exported by this driver, are used by the caller (Intel PMC USB mux control driver), hence the need for header file. > > 4 files changed, 212 insertions(+) > > create mode 100644 drivers/platform/x86/intel_iom.c create mode > > 100644 include/linux/platform_data/x86/intel_iom.h > > > > diff --git a/drivers/platform/x86/Kconfig > > b/drivers/platform/x86/Kconfig index 0581a54cf562..271feddb20ef 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -816,6 +816,22 @@ config INTEL_INT0002_VGPIO > > To compile this driver as a module, choose M here: the module will > > be called intel_int0002_vgpio. > > > > +config INTEL_IOM > > + tristate "Intel Input Output Manager (IOM) driver" > > + depends on ACPI && PCI > > + help > > + This driver helps read relevant information such as Type-C port > > + status (whether a device is connected to a Type-C port or not) > > + and the activity type on the Type-C ports (such as USB, Display > > + Port, Thunderbolt), for consumption by other drivers. > > + > > + Currently intel_iom_port_status() API is exported by this driver, > > + that has information about the Type-C port status and port activity > > + type. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called intel_iom. > > + > > config INTEL_MENLOW > > tristate "Thermal Management driver for Intel menlow platform" > > depends on ACPI_THERMAL > > diff --git a/drivers/platform/x86/Makefile > > b/drivers/platform/x86/Makefile index 2b85852a1a87..d71e4620a7c6 > > 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -76,6 +76,7 @@ intel_cht_int33fe-objs := > intel_cht_int33fe_common.o \ > > intel_cht_int33fe_microb.o > > obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o > > obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o > > +obj-$(CONFIG_INTEL_IOM) += intel_iom.o > > obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o > > obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o > > obj-$(CONFIG_INTEL_VBTN) += intel-vbtn.o > > diff --git a/drivers/platform/x86/intel_iom.c > > b/drivers/platform/x86/intel_iom.c > > new file mode 100644 > > index 000000000000..ece0fe720b2d > > --- /dev/null > > +++ b/drivers/platform/x86/intel_iom.c > > @@ -0,0 +1,133 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Intel Core SoC Input Output Manager (IOM) driver. > > + * > > + * This driver provides access to the Input Output Manager (IOM) > > +(that > > + * is part of Tiger Lake SoC) registers that can be used to know > > +about > > + * Type-C Sub System related information (such as Type-C port status, > > + * activity type on Type-C ports). > > + * > > + * Copyright (C) 2020, Intel Corporation > > + * Author: Rajmohan Mani <rajmohan.mani@xxxxxxxxx> */ > > + > > +#include <linux/io.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/platform_data/x86/intel_iom.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > + > > +#define IOM_PORT_STATUS_OFFSET 0x560 > > + > > +struct intel_iom { > > + struct device *dev; > > + void __iomem *regbar; > > +}; > > + > > +static struct intel_iom iom_dev; > > Why just one? Why is this static? > There is just one IOM device in the system. > > + > > +/** > > + * intel_iom_get() - Get IOM device instance > > + * > > + * This function returns the IOM device instance. This also ensures > > +that > > + * this driver cannot be unloaded while the caller has the instance. > > + * > > + * Call intel_iom_put() to release the instance. > > + * > > + * Returns IOM device instance on success or error pointer otherwise. > > + */ > > +struct intel_iom *intel_iom_get(void) { > > + struct device *dev = get_device(iom_dev.dev); > > Wht if dev is NULL? > Ack. Will add a check for NULL. > > + > > + /* Prevent this driver from being unloaded while in use */ > > + if (!try_module_get(dev->driver->owner)) { > > Why are you poking around in a random device's driver's owner? > > That's not ok. And probably totally racy. > > Handle module reference counts properly, not like this. > Ack. Will use THIS_MODULE here. > And why does it even matter that you incremented the module reference > count? What is that "protecting" you from? > To prevent this driver from being unloaded, while it is being used. > > + put_device(iom_dev.dev); > > + return ERR_PTR(-EBUSY); > > + } > > + > > + return &iom_dev; > > +} > > +EXPORT_SYMBOL_GPL(intel_iom_get); > > Who calls this function? > Intel PMC USB mux control driver, in this case. The callers are expected to call intel_iom_get() before using the intel_iom_port_status(), after which intel_iom_put() can be called to release the IOM device instance. > > + > > +/** > > + * intel_iom_put() - Put IOM device instance > > + * @iom: IOM device instance > > + * > > + * This function releases the IOM device instance created using > > + * intel_iom_get() and allows the driver to be unloaded. > > + * > > + * Call intel_iom_put() to release the instance. > > + */ > > +void intel_iom_put(struct intel_iom *iom) { > > + if (!iom) > > + return; > > + > > + module_put(iom->dev->driver->owner); > > And if the device doesn't have a driver? boom :( > > Don't do this. > Ack. Will use THIS_MODULE here. > > + put_device(iom->dev); > > +} > > +EXPORT_SYMBOL_GPL(intel_iom_put); > > + > > +/** > > + * intel_iom_port_status() - Get status bits for the Type-C port > > + * @iom: IOM device instance > > + * @port: Type-C port number > > + * @status: pointer to receive the status bits > > + * > > + * Returns 0 on success, error otherwise. > > + */ > > +int intel_iom_port_status(struct intel_iom *iom, u8 port, u32 > > +*status) { > > + void __iomem *reg; > > + > > + if (!iom) > > + return -ENODEV; > > + > > + if (!status || (port > IOM_MAX_PORTS - 1)) > > + return -EINVAL; > > + > > + reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN * > port; > > + > > + *status = ioread32(reg); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(intel_iom_port_status); > > So the whole driver is here just to read, directly from memory, a single > 32 bit value? Yes. > Doesn't that seem like a lot of overkill? Why can't the caller just > do this themselves? > Ack. Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC. So I thought it made sense to keep these 2 devices / drivers apart, despite the overkill. Heikki also agreed with this approach, given the above. > greg k-h