[PATCH v1 0/3] pinctrl: intel: Add generic platform driver

[PATCH v1 0/3] pinctrl: intel: Add generic platform driver

[Andy Shevchenko]

New Intel platforms one-by-one will be switching to use generic
approach for the pin control description in ACPI, hence this driver.

It depends on the patches in current Linux Next (will be part of
v6.7-rc1) and this series
https://lore.kernel.org/r/20231030120734.2831419-1-andriy.shevchenko@linux.intel.com.

Andy Shevchenko (3):
  pinctrl: intel: Revert "Unexport intel_pinctrl_probe()"
  pinctrl: intel: Add a generic Intel pin control platform driver
  pinctrl: intel: Add ACPI ID to the generic platform driver

 drivers/pinctrl/intel/Kconfig                 |   8 +
 drivers/pinctrl/intel/Makefile                |   1 +
 .../pinctrl/intel/pinctrl-intel-platform.c    | 224 ++++++++++++++++++
 drivers/pinctrl/intel/pinctrl-intel.c         |   5 +-
 drivers/pinctrl/intel/pinctrl-intel.h         |   3 +
 5 files changed, 239 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/intel/pinctrl-intel-platform.c

-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 1/3] pinctrl: intel: Revert "Unexport intel_pinctrl_probe()"

[Andy Shevchenko]

In order to prepare for a new coming driver export the original
intel_pinctrl_probe() again.

This reverts commit 0dd519e3784b13befa1cdfeff847a0885b06650f.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 5 +++--
 drivers/pinctrl/intel/pinctrl-intel.h | 3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index f1d8f7e0d9b7..f04781915631 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1506,8 +1506,8 @@ static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl,
 	return PTR_ERR_OR_ZERO(pwm);
 }
 
-static int intel_pinctrl_probe(struct platform_device *pdev,
-			       const struct intel_pinctrl_soc_data *soc_data)
+int intel_pinctrl_probe(struct platform_device *pdev,
+			const struct intel_pinctrl_soc_data *soc_data)
 {
 	struct device *dev = &pdev->dev;
 	struct intel_pinctrl *pctrl;
@@ -1625,6 +1625,7 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(intel_pinctrl_probe, PINCTRL_INTEL);
 
 int intel_pinctrl_probe_by_hid(struct platform_device *pdev)
 {
diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index e7d911a65584..fde65e18cd14 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -252,6 +252,9 @@ struct intel_pinctrl {
 	int irq;
 };
 
+int intel_pinctrl_probe(struct platform_device *pdev,
+			const struct intel_pinctrl_soc_data *soc_data);
+
 int intel_pinctrl_probe_by_hid(struct platform_device *pdev);
 int intel_pinctrl_probe_by_uid(struct platform_device *pdev);
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/3] pinctrl: intel: Add a generic Intel pin control platform driver

[Andy Shevchenko]

New generations of Intel platforms will provide better description
of the pin control devices in the ACPI tables. Hence, we may provide
a generic pin control platform driver to cover all of them.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/Kconfig                 |   8 +
 drivers/pinctrl/intel/Makefile                |   1 +
 .../pinctrl/intel/pinctrl-intel-platform.c    | 223 ++++++++++++++++++
 3 files changed, 232 insertions(+)
 create mode 100644 drivers/pinctrl/intel/pinctrl-intel-platform.c

diff --git a/drivers/pinctrl/intel/Kconfig b/drivers/pinctrl/intel/Kconfig
index d66f4f6932d8..42a6bc8b7a45 100644
--- a/drivers/pinctrl/intel/Kconfig
+++ b/drivers/pinctrl/intel/Kconfig
@@ -37,6 +37,14 @@ config PINCTRL_INTEL
 	select GPIOLIB
 	select GPIOLIB_IRQCHIP
 
+config PINCTRL_INTEL_PLATFORM
+	tristate "Intel pinctrl and GPIO platform driver"
+	depends on ACPI
+	select PINCTRL_INTEL
+	help
+	  This pinctrl driver provides an interface that allows configuring
+	  of Intel PCH pins and using them as GPIOs.
+
 config PINCTRL_ALDERLAKE
 	tristate "Intel Alder Lake pinctrl and GPIO driver"
 	select PINCTRL_INTEL
diff --git a/drivers/pinctrl/intel/Makefile b/drivers/pinctrl/intel/Makefile
index f6d30f2d973a..96c93ed4bd58 100644
--- a/drivers/pinctrl/intel/Makefile
+++ b/drivers/pinctrl/intel/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINCTRL_TANGIER)		+= pinctrl-tangier.o
 obj-$(CONFIG_PINCTRL_MERRIFIELD)	+= pinctrl-merrifield.o
 obj-$(CONFIG_PINCTRL_MOOREFIELD)	+= pinctrl-moorefield.o
 obj-$(CONFIG_PINCTRL_INTEL)		+= pinctrl-intel.o
+obj-$(CONFIG_PINCTRL_INTEL_PLATFORM)	+= pinctrl-intel-platform.o
 obj-$(CONFIG_PINCTRL_ALDERLAKE)		+= pinctrl-alderlake.o
 obj-$(CONFIG_PINCTRL_BROXTON)		+= pinctrl-broxton.o
 obj-$(CONFIG_PINCTRL_CANNONLAKE)	+= pinctrl-cannonlake.o
diff --git a/drivers/pinctrl/intel/pinctrl-intel-platform.c b/drivers/pinctrl/intel/pinctrl-intel-platform.c
new file mode 100644
index 000000000000..2305d8befdd3
--- /dev/null
+++ b/drivers/pinctrl/intel/pinctrl-intel-platform.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel PCH pinctrl/GPIO driver
+ *
+ * Copyright (C) 2021-2023, Intel Corporation
+ * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/property.h>
+#include <linux/string_helpers.h>
+
+#include <linux/pinctrl/pinctrl.h>
+
+#include "pinctrl-intel.h"
+
+struct intel_platform_pins {
+	struct pinctrl_pin_desc *pins;
+	size_t npins;
+};
+
+static int intel_platform_pinctrl_prepare_pins(struct device *dev, size_t base,
+					       const char *name, u32 size,
+					       struct intel_platform_pins *pins)
+{
+	struct pinctrl_pin_desc *descs;
+	char **pin_names;
+	unsigned int i;
+
+	pin_names = devm_kasprintf_strarray(dev, name, size);
+	if (IS_ERR(pin_names))
+		return PTR_ERR(pin_names);
+
+	descs = devm_krealloc_array(dev, pins->pins, base + size, sizeof(*descs), GFP_KERNEL);
+	if (!descs)
+		return -ENOMEM;
+
+	for (i = 0; i < size; i++) {
+		unsigned int pin_number = base + i;
+		char *pin_name = pin_names[i];
+		struct pinctrl_pin_desc *desc;
+
+		/* Unify delimiter for pin name */
+		strreplace(pin_name, '-', '_');
+
+		desc = &descs[pin_number];
+		desc->number = pin_number;
+		desc->name = pin_name;
+	}
+
+	pins->pins = descs;
+	pins->npins = base + size;
+
+	return 0;
+}
+
+static int intel_platform_pinctrl_prepare_group(struct device *dev,
+						struct fwnode_handle *child,
+						struct intel_padgroup *gpp,
+						struct intel_platform_pins *pins)
+{
+	size_t base = pins->npins;
+	const char *name;
+	u32 size;
+	int ret;
+
+	ret = fwnode_property_read_string(child, "intc-gpio-group-name", &name);
+	if (ret)
+		return ret;
+
+	ret = fwnode_property_read_u32(child, "intc-gpio-pad-count", &size);
+	if (ret)
+		return ret;
+
+	ret = intel_platform_pinctrl_prepare_pins(dev, base, name, size, pins);
+	if (ret)
+		return ret;
+
+	gpp->base = base;
+	gpp->size = size;
+	gpp->gpio_base = INTEL_GPIO_BASE_MATCH;
+
+	return 0;
+}
+
+static int intel_platform_pinctrl_prepare_community(struct device *dev,
+						    struct intel_community *community,
+						    struct intel_platform_pins *pins)
+{
+	struct fwnode_handle *child;
+	struct intel_padgroup *gpps;
+	unsigned int group;
+	size_t ngpps;
+	u32 offset;
+	int ret;
+
+	ret = device_property_read_u32(dev, "intc-gpio-pad-ownership-offset", &offset);
+	if (ret)
+		return ret;
+	community->padown_offset = offset;
+
+	ret = device_property_read_u32(dev, "intc-gpio-pad-configuration-lock-offset", &offset);
+	if (ret)
+		return ret;
+	community->padcfglock_offset = offset;
+
+	ret = device_property_read_u32(dev, "intc-gpio-host-software-pad-ownership-offset", &offset);
+	if (ret)
+		return ret;
+	community->hostown_offset = offset;
+
+	ret = device_property_read_u32(dev, "intc-gpio-gpi-interrupt-status-offset", &offset);
+	if (ret)
+		return ret;
+	community->is_offset = offset;
+
+	ret = device_property_read_u32(dev, "intc-gpio-gpi-interrupt-enable-offset", &offset);
+	if (ret)
+		return ret;
+	community->ie_offset = offset;
+
+	ngpps = device_get_child_node_count(dev);
+	if (ngpps == 0)
+		return -ENODEV;
+
+	gpps = devm_kcalloc(dev, ngpps, sizeof(*gpps), GFP_KERNEL);
+	if (!gpps)
+		return -ENOMEM;
+
+	group = 0;
+	device_for_each_child_node(dev, child) {
+		struct intel_padgroup *gpp = &gpps[group];
+
+		gpp->reg_num = group;
+
+		ret = intel_platform_pinctrl_prepare_group(dev, child, gpp, pins);
+		if (ret)
+			return ret;
+
+		group++;
+	}
+
+	community->ngpps = ngpps;
+	community->gpps = gpps;
+
+	return 0;
+}
+
+static int intel_platform_pinctrl_prepare_soc_data(struct device *dev,
+						   struct intel_pinctrl_soc_data *data)
+{
+	struct intel_platform_pins pins = {};
+	struct intel_community *communities;
+	size_t ncommunities;
+	unsigned int i;
+	int ret;
+
+	ncommunities = 1,
+	communities = devm_kcalloc(dev, ncommunities, sizeof(*communities), GFP_KERNEL);
+	if (!communities)
+		return -ENOMEM;
+
+	for (i = 0; i < ncommunities; i++) {
+		struct intel_community *community = &communities[i];
+
+		community->barno = i;
+		community->pin_base = pins.npins;
+
+		ret = intel_platform_pinctrl_prepare_community(dev, community, &pins);
+		if (ret)
+			return ret;
+
+		community->npins = pins.npins - community->pin_base;
+	}
+
+	data->ncommunities = ncommunities;
+	data->communities = communities;
+
+	data->npins = pins.npins;
+	data->pins = pins.pins;
+
+	return 0;
+}
+
+static int intel_platform_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_pinctrl_soc_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = intel_platform_pinctrl_prepare_soc_data(dev, data);
+	if (ret)
+		return ret;
+
+	return intel_pinctrl_probe(pdev, data);
+}
+
+static const struct acpi_device_id intel_platform_pinctrl_acpi_match[] = {
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, intel_platform_pinctrl_acpi_match);
+
+static struct platform_driver intel_platform_pinctrl_driver = {
+	.probe = intel_platform_pinctrl_probe,
+	.driver = {
+		.name = "intel-pinctrl",
+		.acpi_match_table = intel_platform_pinctrl_acpi_match,
+		.pm = pm_sleep_ptr(&intel_pinctrl_pm_ops),
+	},
+};
+module_platform_driver(intel_platform_pinctrl_driver);
+
+MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
+MODULE_DESCRIPTION("Intel PCH pinctrl/GPIO driver");
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(PINCTRL_INTEL);
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 3/3] pinctrl: intel: Add ACPI ID to the generic platform driver

[Andy Shevchenko]

The INTC105F is an ACPI _CID that reflects compatibility with
the _DSD shema supported by the generic Intel pin control
platform driver. Add it there.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel-platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel-platform.c b/drivers/pinctrl/intel/pinctrl-intel-platform.c
index 2305d8befdd3..5605583b1a34 100644
--- a/drivers/pinctrl/intel/pinctrl-intel-platform.c
+++ b/drivers/pinctrl/intel/pinctrl-intel-platform.c
@@ -203,6 +203,7 @@ static int intel_platform_pinctrl_probe(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id intel_platform_pinctrl_acpi_match[] = {
+	{ "INTC105F" },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, intel_platform_pinctrl_acpi_match);
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 3/3] pinctrl: intel: Add ACPI ID to the generic platform driver

[Mika Westerberg]

On Mon, Oct 30, 2023 at 04:10:34PM +0200, Andy Shevchenko wrote:
> The INTC105F is an ACPI _CID that reflects compatibility with
> the _DSD shema supported by the generic Intel pin control
> platform driver. Add it there.

You can add this in the patch introducing the driver.

Re: [PATCH v1 2/3] pinctrl: intel: Add a generic Intel pin control platform driver

[Mika Westerberg]

On Mon, Oct 30, 2023 at 04:10:33PM +0200, Andy Shevchenko wrote:
> New generations of Intel platforms will provide better description
> of the pin control devices in the ACPI tables. Hence, we may provide
> a generic pin control platform driver to cover all of them.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/Kconfig                 |   8 +
>  drivers/pinctrl/intel/Makefile                |   1 +
>  .../pinctrl/intel/pinctrl-intel-platform.c    | 223 ++++++++++++++++++
>  3 files changed, 232 insertions(+)
>  create mode 100644 drivers/pinctrl/intel/pinctrl-intel-platform.c
> 
> diff --git a/drivers/pinctrl/intel/Kconfig b/drivers/pinctrl/intel/Kconfig
> index d66f4f6932d8..42a6bc8b7a45 100644
> --- a/drivers/pinctrl/intel/Kconfig
> +++ b/drivers/pinctrl/intel/Kconfig
> @@ -37,6 +37,14 @@ config PINCTRL_INTEL
>  	select GPIOLIB
>  	select GPIOLIB_IRQCHIP
>  
> +config PINCTRL_INTEL_PLATFORM
> +	tristate "Intel pinctrl and GPIO platform driver"
> +	depends on ACPI
> +	select PINCTRL_INTEL
> +	help
> +	  This pinctrl driver provides an interface that allows configuring
> +	  of Intel PCH pins and using them as GPIOs.

Add here some description that explains why this needs to be enabled,
for example for Lunar Lake. Now it is all too generic for distro folks
to understand if this is needed or not.

> +
>  config PINCTRL_ALDERLAKE
>  	tristate "Intel Alder Lake pinctrl and GPIO driver"
>  	select PINCTRL_INTEL
> diff --git a/drivers/pinctrl/intel/Makefile b/drivers/pinctrl/intel/Makefile
> index f6d30f2d973a..96c93ed4bd58 100644
> --- a/drivers/pinctrl/intel/Makefile
> +++ b/drivers/pinctrl/intel/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PINCTRL_TANGIER)		+= pinctrl-tangier.o
>  obj-$(CONFIG_PINCTRL_MERRIFIELD)	+= pinctrl-merrifield.o
>  obj-$(CONFIG_PINCTRL_MOOREFIELD)	+= pinctrl-moorefield.o
>  obj-$(CONFIG_PINCTRL_INTEL)		+= pinctrl-intel.o
> +obj-$(CONFIG_PINCTRL_INTEL_PLATFORM)	+= pinctrl-intel-platform.o
>  obj-$(CONFIG_PINCTRL_ALDERLAKE)		+= pinctrl-alderlake.o
>  obj-$(CONFIG_PINCTRL_BROXTON)		+= pinctrl-broxton.o
>  obj-$(CONFIG_PINCTRL_CANNONLAKE)	+= pinctrl-cannonlake.o
> diff --git a/drivers/pinctrl/intel/pinctrl-intel-platform.c b/drivers/pinctrl/intel/pinctrl-intel-platform.c
> new file mode 100644
> index 000000000000..2305d8befdd3
> --- /dev/null
> +++ b/drivers/pinctrl/intel/pinctrl-intel-platform.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel PCH pinctrl/GPIO driver
> + *
> + * Copyright (C) 2021-2023, Intel Corporation

That's 2023

> + * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/string_helpers.h>
> +
> +#include <linux/pinctrl/pinctrl.h>
> +
> +#include "pinctrl-intel.h"
> +
> +struct intel_platform_pins {
> +	struct pinctrl_pin_desc *pins;
> +	size_t npins;
> +};
> +
> +static int intel_platform_pinctrl_prepare_pins(struct device *dev, size_t base,
> +					       const char *name, u32 size,
> +					       struct intel_platform_pins *pins)
> +{
> +	struct pinctrl_pin_desc *descs;
> +	char **pin_names;
> +	unsigned int i;
> +
> +	pin_names = devm_kasprintf_strarray(dev, name, size);
> +	if (IS_ERR(pin_names))
> +		return PTR_ERR(pin_names);
> +
> +	descs = devm_krealloc_array(dev, pins->pins, base + size, sizeof(*descs), GFP_KERNEL);
> +	if (!descs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < size; i++) {
> +		unsigned int pin_number = base + i;
> +		char *pin_name = pin_names[i];
> +		struct pinctrl_pin_desc *desc;
> +
> +		/* Unify delimiter for pin name */
> +		strreplace(pin_name, '-', '_');
> +
> +		desc = &descs[pin_number];
> +		desc->number = pin_number;
> +		desc->name = pin_name;
> +	}
> +
> +	pins->pins = descs;
> +	pins->npins = base + size;
> +
> +	return 0;
> +}
> +
> +static int intel_platform_pinctrl_prepare_group(struct device *dev,
> +						struct fwnode_handle *child,
> +						struct intel_padgroup *gpp,
> +						struct intel_platform_pins *pins)
> +{
> +	size_t base = pins->npins;
> +	const char *name;
> +	u32 size;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(child, "intc-gpio-group-name", &name);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_property_read_u32(child, "intc-gpio-pad-count", &size);
> +	if (ret)
> +		return ret;
> +
> +	ret = intel_platform_pinctrl_prepare_pins(dev, base, name, size, pins);
> +	if (ret)
> +		return ret;
> +
> +	gpp->base = base;
> +	gpp->size = size;
> +	gpp->gpio_base = INTEL_GPIO_BASE_MATCH;
> +
> +	return 0;
> +}
> +
> +static int intel_platform_pinctrl_prepare_community(struct device *dev,
> +						    struct intel_community *community,
> +						    struct intel_platform_pins *pins)
> +{
> +	struct fwnode_handle *child;
> +	struct intel_padgroup *gpps;
> +	unsigned int group;
> +	size_t ngpps;
> +	u32 offset;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev, "intc-gpio-pad-ownership-offset", &offset);
> +	if (ret)
> +		return ret;
> +	community->padown_offset = offset;
> +
> +	ret = device_property_read_u32(dev, "intc-gpio-pad-configuration-lock-offset", &offset);
> +	if (ret)
> +		return ret;
> +	community->padcfglock_offset = offset;
> +
> +	ret = device_property_read_u32(dev, "intc-gpio-host-software-pad-ownership-offset", &offset);
> +	if (ret)
> +		return ret;
> +	community->hostown_offset = offset;
> +
> +	ret = device_property_read_u32(dev, "intc-gpio-gpi-interrupt-status-offset", &offset);
> +	if (ret)
> +		return ret;
> +	community->is_offset = offset;
> +
> +	ret = device_property_read_u32(dev, "intc-gpio-gpi-interrupt-enable-offset", &offset);
> +	if (ret)
> +		return ret;
> +	community->ie_offset = offset;
> +
> +	ngpps = device_get_child_node_count(dev);
> +	if (ngpps == 0)

if (!nggps)

> +		return -ENODEV;
> +
> +	gpps = devm_kcalloc(dev, ngpps, sizeof(*gpps), GFP_KERNEL);
> +	if (!gpps)
> +		return -ENOMEM;
> +
> +	group = 0;
> +	device_for_each_child_node(dev, child) {
> +		struct intel_padgroup *gpp = &gpps[group];
> +
> +		gpp->reg_num = group;
> +
> +		ret = intel_platform_pinctrl_prepare_group(dev, child, gpp, pins);
> +		if (ret)
> +			return ret;
> +
> +		group++;
> +	}
> +
> +	community->ngpps = ngpps;
> +	community->gpps = gpps;
> +
> +	return 0;
> +}
> +
> +static int intel_platform_pinctrl_prepare_soc_data(struct device *dev,
> +						   struct intel_pinctrl_soc_data *data)
> +{
> +	struct intel_platform_pins pins = {};
> +	struct intel_community *communities;
> +	size_t ncommunities;
> +	unsigned int i;
> +	int ret;
> +
> +	ncommunities = 1,

Why this is 1? Can't we have more communities?

> +	communities = devm_kcalloc(dev, ncommunities, sizeof(*communities), GFP_KERNEL);
> +	if (!communities)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ncommunities; i++) {
> +		struct intel_community *community = &communities[i];
> +
> +		community->barno = i;
> +		community->pin_base = pins.npins;
> +
> +		ret = intel_platform_pinctrl_prepare_community(dev, community, &pins);
> +		if (ret)
> +			return ret;
> +
> +		community->npins = pins.npins - community->pin_base;
> +	}
> +
> +	data->ncommunities = ncommunities;
> +	data->communities = communities;
> +
> +	data->npins = pins.npins;
> +	data->pins = pins.pins;
> +
> +	return 0;
> +}
> +
> +static int intel_platform_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct intel_pinctrl_soc_data *data;


Change the ordering of the above:

	struct intel_pinctrl_soc_data *data;
	struct device *dev = &pdev->dev;

> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	ret = intel_platform_pinctrl_prepare_soc_data(dev, data);
> +	if (ret)
> +		return ret;
> +
> +	return intel_pinctrl_probe(pdev, data);
> +}
> +
> +static const struct acpi_device_id intel_platform_pinctrl_acpi_match[] = {
> +	{ }

And add the _CID here in this patch as I commented in the last patch.

> +};
> +MODULE_DEVICE_TABLE(acpi, intel_platform_pinctrl_acpi_match);
> +
> +static struct platform_driver intel_platform_pinctrl_driver = {
> +	.probe = intel_platform_pinctrl_probe,
> +	.driver = {
> +		.name = "intel-pinctrl",
> +		.acpi_match_table = intel_platform_pinctrl_acpi_match,
> +		.pm = pm_sleep_ptr(&intel_pinctrl_pm_ops),
> +	},
> +};
> +module_platform_driver(intel_platform_pinctrl_driver);
> +
> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
> +MODULE_DESCRIPTION("Intel PCH pinctrl/GPIO driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(PINCTRL_INTEL);
> -- 
> 2.40.0.1.gaa8946217a0b

Re: [PATCH v1 2/3] pinctrl: intel: Add a generic Intel pin control platform driver

[Andy Shevchenko]

On Fri, Nov 03, 2023 at 07:57:38AM +0200, Mika Westerberg wrote:
> On Mon, Oct 30, 2023 at 04:10:33PM +0200, Andy Shevchenko wrote:

...

> > +config PINCTRL_INTEL_PLATFORM
> > +	tristate "Intel pinctrl and GPIO platform driver"
> > +	depends on ACPI
> > +	select PINCTRL_INTEL
> > +	help
> > +	  This pinctrl driver provides an interface that allows configuring
> > +	  of Intel PCH pins and using them as GPIOs.
> 
> Add here some description that explains why this needs to be enabled,
> for example for Lunar Lake. Now it is all too generic for distro folks
> to understand if this is needed or not.

OK!

...

> > + * Copyright (C) 2021-2023, Intel Corporation
> 
> That's 2023

As-is it is still valid and reflects the history.

...

> > +	ngpps = device_get_child_node_count(dev);
> > +	if (ngpps == 0)
> 
> if (!nggps)

0 is a plain number here (as count) and explicit comparison makes sense.
But I'm okay with another form.


> > +		return -ENODEV;

...

> > +	ncommunities = 1,
> 
> Why this is 1? Can't we have more communities?

As for now (version 1.0 of the specification) it's assumed that it's one
community per device node in the ACPI, so I would leave this as is (we have
also drivers with single community per device node, hence this is kinda
pattern. Should I add a comment?

...

> > +	struct device *dev = &pdev->dev;
> > +	struct intel_pinctrl_soc_data *data;
> 
> 
> Change the ordering of the above:
> 
> 	struct intel_pinctrl_soc_data *data;
> 	struct device *dev = &pdev->dev;

Sure.

...

> > +static const struct acpi_device_id intel_platform_pinctrl_acpi_match[] = {
> > +	{ }
> 
> And add the _CID here in this patch as I commented in the last patch.

OK! I'll squash the next patch into this one.

> > +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] pinctrl: intel: Add a generic Intel pin control platform driver

[Mika Westerberg]

On Mon, Nov 13, 2023 at 02:12:48PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 03, 2023 at 07:57:38AM +0200, Mika Westerberg wrote:
> > On Mon, Oct 30, 2023 at 04:10:33PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > +config PINCTRL_INTEL_PLATFORM
> > > +	tristate "Intel pinctrl and GPIO platform driver"
> > > +	depends on ACPI
> > > +	select PINCTRL_INTEL
> > > +	help
> > > +	  This pinctrl driver provides an interface that allows configuring
> > > +	  of Intel PCH pins and using them as GPIOs.
> > 
> > Add here some description that explains why this needs to be enabled,
> > for example for Lunar Lake. Now it is all too generic for distro folks
> > to understand if this is needed or not.
> 
> OK!
> 
> ...
> 
> > > + * Copyright (C) 2021-2023, Intel Corporation
> > 
> > That's 2023
> 
> As-is it is still valid and reflects the history.
> 
> ...
> 
> > > +	ngpps = device_get_child_node_count(dev);
> > > +	if (ngpps == 0)
> > 
> > if (!nggps)
> 
> 0 is a plain number here (as count) and explicit comparison makes sense.
> But I'm okay with another form.
> 
> 
> > > +		return -ENODEV;
> 
> ...
> 
> > > +	ncommunities = 1,
> > 
> > Why this is 1? Can't we have more communities?
> 
> As for now (version 1.0 of the specification) it's assumed that it's one
> community per device node in the ACPI, so I would leave this as is (we have
> also drivers with single community per device node, hence this is kinda
> pattern. Should I add a comment?
> 

Yes, I think it warrants a comment.

> ...
> 
> > > +	struct device *dev = &pdev->dev;
> > > +	struct intel_pinctrl_soc_data *data;
> > 
> > 
> > Change the ordering of the above:
> > 
> > 	struct intel_pinctrl_soc_data *data;
> > 	struct device *dev = &pdev->dev;
> 
> Sure.
> 
> ...
> 
> > > +static const struct acpi_device_id intel_platform_pinctrl_acpi_match[] = {
> > > +	{ }
> > 
> > And add the _CID here in this patch as I commented in the last patch.
> 
> OK! I'll squash the next patch into this one.
> 
> > > +};
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>