The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
(Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
for portable applications. It provides a high dynamic range, stereo
DAC for headphone output, two integrated Class D amplifiers for
loudspeakers, and two ADCs for wired headset microphone input or
stereo line input. PDM inputs are provided for digital microphones.
Add a basic pinctrl driver which supports driver strength for the
various pins, gpios, and pinmux for the 2 multi-function pins.
Signed-off-by: Charles Keepax <[email protected]>
---
MAINTAINERS | 1 +
drivers/pinctrl/cirrus/Kconfig | 11 +
drivers/pinctrl/cirrus/Makefile | 2 +
drivers/pinctrl/cirrus/pinctrl-cs42l43.c | 614 +++++++++++++++++++++++
4 files changed, 628 insertions(+)
create mode 100644 drivers/pinctrl/cirrus/pinctrl-cs42l43.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 13945ee6cdcfe..2890f54f70afc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4930,6 +4930,7 @@ F: Documentation/devicetree/bindings/mfd/cirrus,cs*
F: Documentation/devicetree/bindings/sound/cirrus,cs*
F: drivers/irqchip/irq-cs42l43*
F: drivers/mfd/cs42l43*
+F: drivers/pinctrl/cirrus/pinctrl-cs42l43*
F: include/dt-bindings/sound/cs*
F: include/linux/irqchip/cs42l43*
F: include/linux/mfd/cs42l43*
diff --git a/drivers/pinctrl/cirrus/Kconfig b/drivers/pinctrl/cirrus/Kconfig
index 530426a74f751..d6318cb57aff2 100644
--- a/drivers/pinctrl/cirrus/Kconfig
+++ b/drivers/pinctrl/cirrus/Kconfig
@@ -1,4 +1,15 @@
# SPDX-License-Identifier: GPL-2.0-only
+config PINCTRL_CS42L43
+ tristate "Cirrus Logic CS42L43 Pinctrl Driver"
+ depends on MFD_CS42L43
+ select GPIOLIB
+ select PINMUX
+ select PINCONF
+ select GENERIC_PINCONF
+ help
+ Select this to support the GPIO/Pinctrl functions of the Cirrus
+ Logic CS42L43 PC CODEC.
+
config PINCTRL_LOCHNAGAR
tristate "Cirrus Logic Lochnagar pinctrl driver"
depends on MFD_LOCHNAGAR
diff --git a/drivers/pinctrl/cirrus/Makefile b/drivers/pinctrl/cirrus/Makefile
index a484518c840e3..9b618d7669071 100644
--- a/drivers/pinctrl/cirrus/Makefile
+++ b/drivers/pinctrl/cirrus/Makefile
@@ -1,5 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
# Cirrus Logic pinctrl drivers
+obj-$(CONFIG_PINCTRL_CS42L43) += pinctrl-cs42l43.o
+
obj-$(CONFIG_PINCTRL_LOCHNAGAR) += pinctrl-lochnagar.o
pinctrl-madera-objs := pinctrl-madera-core.o
diff --git a/drivers/pinctrl/cirrus/pinctrl-cs42l43.c b/drivers/pinctrl/cirrus/pinctrl-cs42l43.c
new file mode 100644
index 0000000000000..df5bae1cefcd7
--- /dev/null
+++ b/drivers/pinctrl/cirrus/pinctrl-cs42l43.c
@@ -0,0 +1,614 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// CS42L43 Pinctrl and GPIO driver
+//
+// Copyright (c) 2023 Cirrus Logic, Inc. and
+// Cirrus Logic International Semiconductor Ltd.
+
+#include <linux/bits.h>
+#include <linux/build_bug.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/mfd/cs42l43.h>
+#include <linux/mfd/cs42l43-regs.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include "../pinctrl-utils.h"
+
+#define CS42L43_NUM_GPIOS 3
+
+struct cs42l43_pin {
+ struct device *dev;
+ struct regmap *regmap;
+ bool shutters_locked;
+
+ struct gpio_chip gpio_chip;
+ struct pinctrl_gpio_range range;
+};
+
+struct cs42l43_pin_data {
+ unsigned int reg;
+ unsigned int shift;
+ unsigned int mask;
+};
+
+#define CS42L43_PIN(_number, _name, _reg, _field) { \
+ .number = _number, .name = _name, \
+ .drv_data = &((struct cs42l43_pin_data){ \
+ .reg = CS42L43_##_reg, \
+ .shift = CS42L43_##_field##_DRV_SHIFT, \
+ .mask = CS42L43_##_field##_DRV_MASK, \
+ }), \
+}
+
+static const struct pinctrl_pin_desc cs42l43_pin_pins[] = {
+ CS42L43_PIN(0, "gpio1", DRV_CTRL4, GPIO1),
+ CS42L43_PIN(1, "gpio2", DRV_CTRL4, GPIO2),
+ CS42L43_PIN(2, "gpio3", DRV_CTRL4, GPIO3),
+ CS42L43_PIN(3, "asp_dout", DRV_CTRL1, ASP_DOUT),
+ CS42L43_PIN(4, "asp_fsync", DRV_CTRL1, ASP_FSYNC),
+ CS42L43_PIN(5, "asp_bclk", DRV_CTRL1, ASP_BCLK),
+ CS42L43_PIN(6, "pdmout2_clk", DRV_CTRL3, PDMOUT2_CLK),
+ CS42L43_PIN(7, "pdmout2_data", DRV_CTRL3, PDMOUT2_DATA),
+ CS42L43_PIN(8, "pdmout1_clk", DRV_CTRL3, PDMOUT1_CLK),
+ CS42L43_PIN(9, "pdmout1_data", DRV_CTRL3, PDMOUT1_DATA),
+ CS42L43_PIN(10, "i2c_sda", DRV_CTRL3, I2C_SDA),
+ CS42L43_PIN(11, "i2c_scl", DRV_CTRL_5, I2C_SCL),
+ CS42L43_PIN(12, "spi_miso", DRV_CTRL3, SPI_MISO),
+ CS42L43_PIN(13, "spi_sck", DRV_CTRL_5, SPI_SCK),
+ CS42L43_PIN(14, "spi_ssb", DRV_CTRL_5, SPI_SSB),
+};
+
+static const unsigned int cs42l43_pin_gpio1_pins[] = { 0 };
+static const unsigned int cs42l43_pin_gpio2_pins[] = { 1 };
+static const unsigned int cs42l43_pin_gpio3_pins[] = { 2 };
+static const unsigned int cs42l43_pin_asp_pins[] = { 3, 4, 5 };
+static const unsigned int cs42l43_pin_pdmout2_pins[] = { 6, 7 };
+static const unsigned int cs42l43_pin_pdmout1_pins[] = { 8, 9 };
+static const unsigned int cs42l43_pin_i2c_pins[] = { 10, 11 };
+static const unsigned int cs42l43_pin_spi_pins[] = { 12, 13, 14 };
+
+#define CS42L43_PINGROUP(_name) \
+(struct pingroup){ \
+ .name = #_name, \
+ .pins = cs42l43_pin_##_name##_pins, \
+ .npins = ARRAY_SIZE(cs42l43_pin_##_name##_pins) \
+}
+
+static const struct pingroup cs42l43_pin_groups[] = {
+ CS42L43_PINGROUP(gpio1),
+ CS42L43_PINGROUP(gpio2),
+ CS42L43_PINGROUP(gpio3),
+ CS42L43_PINGROUP(asp),
+ CS42L43_PINGROUP(pdmout2),
+ CS42L43_PINGROUP(pdmout1),
+ CS42L43_PINGROUP(i2c),
+ CS42L43_PINGROUP(spi),
+};
+
+static int cs42l43_pin_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ return ARRAY_SIZE(cs42l43_pin_groups);
+}
+
+static const char *cs42l43_pin_get_group_name(struct pinctrl_dev *pctldev,
+ unsigned int group_idx)
+{
+ return cs42l43_pin_groups[group_idx].name;
+}
+
+static int cs42l43_pin_get_group_pins(struct pinctrl_dev *pctldev,
+ unsigned int group_idx,
+ const unsigned int **pins,
+ unsigned int *num_pins)
+{
+ *pins = cs42l43_pin_groups[group_idx].pins;
+ *num_pins = cs42l43_pin_groups[group_idx].npins;
+
+ return 0;
+}
+
+static const struct pinctrl_ops cs42l43_pin_group_ops = {
+ .get_groups_count = cs42l43_pin_get_groups_count,
+ .get_group_name = cs42l43_pin_get_group_name,
+ .get_group_pins = cs42l43_pin_get_group_pins,
+#if IS_ENABLED(CONFIG_OF)
+ .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+ .dt_free_map = pinconf_generic_dt_free_map,
+#endif
+};
+
+enum cs42l43_pin_funcs {
+ CS42L43_FUNC_GPIO,
+ CS42L43_FUNC_SPDIF,
+ CS42L43_FUNC_IRQ,
+ CS42L43_FUNC_MIC_SHT,
+ CS42L43_FUNC_SPK_SHT,
+ CS42L43_FUNC_MAX,
+};
+
+static const char * const cs42l43_pin_funcs[] = {
+ "gpio", "spdif", "irq", "mic-shutter", "spk-shutter"
+};
+
+static const char * const cs42l43_pin_gpio_groups[] = { "gpio1", "gpio3" };
+static const char * const cs42l43_pin_spdif_groups[] = { "gpio3" };
+static const char * const cs42l43_pin_irq_groups[] = { "gpio1" };
+static const char * const cs42l43_pin_shutter_groups[] = { "gpio1", "gpio2", "gpio3" };
+
+struct cs42l43_pin_func_group {
+ const char * const *groups;
+ unsigned int ngroups;
+};
+
+static const struct cs42l43_pin_func_group cs42l43_pin_func_groups[] = {
+ { cs42l43_pin_gpio_groups, ARRAY_SIZE(cs42l43_pin_gpio_groups) },
+ { cs42l43_pin_spdif_groups, ARRAY_SIZE(cs42l43_pin_spdif_groups) },
+ { cs42l43_pin_irq_groups, ARRAY_SIZE(cs42l43_pin_irq_groups) },
+ { cs42l43_pin_shutter_groups, ARRAY_SIZE(cs42l43_pin_shutter_groups) },
+ { cs42l43_pin_shutter_groups, ARRAY_SIZE(cs42l43_pin_shutter_groups) },
+};
+
+static int cs42l43_pin_get_func_count(struct pinctrl_dev *pctldev)
+{
+ BUILD_BUG_ON(ARRAY_SIZE(cs42l43_pin_funcs) != CS42L43_FUNC_MAX);
+ BUILD_BUG_ON(ARRAY_SIZE(cs42l43_pin_func_groups) != CS42L43_FUNC_MAX);
+
+ return ARRAY_SIZE(cs42l43_pin_funcs);
+}
+
+static const char *cs42l43_pin_get_func_name(struct pinctrl_dev *pctldev,
+ unsigned int func_idx)
+{
+ return cs42l43_pin_funcs[func_idx];
+}
+
+static int cs42l43_pin_get_func_groups(struct pinctrl_dev *pctldev,
+ unsigned int func_idx,
+ const char * const **groups,
+ unsigned int * const num_groups)
+{
+ *groups = cs42l43_pin_func_groups[func_idx].groups;
+ *num_groups = cs42l43_pin_func_groups[func_idx].ngroups;
+
+ return 0;
+}
+
+static int cs42l43_pin_set_mux(struct pinctrl_dev *pctldev,
+ unsigned int func_idx, unsigned int group_idx)
+{
+ struct cs42l43_pin *priv = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int reg, mask, val;
+
+ dev_dbg(priv->dev, "Setting %s to %s\n",
+ cs42l43_pin_groups[group_idx].name, cs42l43_pin_funcs[func_idx]);
+
+ switch (func_idx) {
+ case CS42L43_FUNC_MIC_SHT:
+ reg = CS42L43_SHUTTER_CONTROL;
+ mask = CS42L43_MIC_SHUTTER_CFG_MASK;
+ val = 0x2 << (group_idx + CS42L43_MIC_SHUTTER_CFG_SHIFT);
+ break;
+ case CS42L43_FUNC_SPK_SHT:
+ reg = CS42L43_SHUTTER_CONTROL;
+ mask = CS42L43_SPK_SHUTTER_CFG_MASK;
+ val = 0x2 << (group_idx + CS42L43_SPK_SHUTTER_CFG_SHIFT);
+ break;
+ default:
+ reg = CS42L43_GPIO_FN_SEL;
+ mask = BIT(group_idx + CS42L43_GPIO1_FN_SEL_SHIFT);
+ val = (func_idx == CS42L43_FUNC_GPIO) <<
+ (group_idx + CS42L43_GPIO1_FN_SEL_SHIFT);
+ break;
+ }
+
+ if (priv->shutters_locked && reg == CS42L43_SHUTTER_CONTROL) {
+ dev_err(priv->dev, "Shutter configuration not available\n");
+ return -EPERM;
+ }
+
+ return regmap_update_bits(priv->regmap, reg, mask, val);
+}
+
+static int cs42l43_gpio_set_direction(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range,
+ unsigned int offset, bool input)
+{
+ struct cs42l43_pin *priv = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int shift = offset + CS42L43_GPIO1_DIR_SHIFT;
+ int ret;
+
+ dev_dbg(priv->dev, "Setting gpio%d to %s\n",
+ offset + 1, input ? "input" : "output");
+
+ ret = pm_runtime_resume_and_get(priv->dev);
+ if (ret) {
+ dev_err(priv->dev, "Failed to resume for direction: %d\n", ret);
+ return ret;
+ }
+
+ ret = regmap_update_bits(priv->regmap, CS42L43_GPIO_CTRL1,
+ BIT(shift), !!input << shift);
+ if (ret)
+ dev_err(priv->dev, "Failed to set gpio%d direction: %d\n",
+ offset + 1, ret);
+
+ pm_runtime_put(priv->dev);
+
+ return ret;
+}
+
+static int cs42l43_gpio_request_enable(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range,
+ unsigned int offset)
+{
+ return cs42l43_pin_set_mux(pctldev, 0, offset);
+}
+
+static void cs42l43_gpio_disable_free(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range,
+ unsigned int offset)
+{
+ cs42l43_gpio_set_direction(pctldev, range, offset, true);
+}
+
+static const struct pinmux_ops cs42l43_pin_mux_ops = {
+ .get_functions_count = cs42l43_pin_get_func_count,
+ .get_function_name = cs42l43_pin_get_func_name,
+ .get_function_groups = cs42l43_pin_get_func_groups,
+
+ .set_mux = cs42l43_pin_set_mux,
+
+ .gpio_request_enable = cs42l43_gpio_request_enable,
+ .gpio_disable_free = cs42l43_gpio_disable_free,
+ .gpio_set_direction = cs42l43_gpio_set_direction,
+
+ .strict = true,
+};
+
+static const unsigned int cs42l43_pin_drv_str_ma[] = { 1, 2, 4, 8, 9, 10, 12, 16 };
+
+static inline int cs42l43_pin_get_drv_str(struct cs42l43_pin *priv, unsigned int pin)
+{
+ const struct cs42l43_pin_data *pdat = cs42l43_pin_pins[pin].drv_data;
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(priv->regmap, pdat->reg, &val);
+ if (ret)
+ return ret;
+
+ return cs42l43_pin_drv_str_ma[(val & pdat->mask) >> pdat->shift];
+}
+
+static inline int cs42l43_pin_set_drv_str(struct cs42l43_pin *priv, unsigned int pin,
+ unsigned int ma)
+{
+ const struct cs42l43_pin_data *pdat = cs42l43_pin_pins[pin].drv_data;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(cs42l43_pin_drv_str_ma); i++) {
+ if (ma == cs42l43_pin_drv_str_ma[i]) {
+ if ((i << pdat->shift) > pdat->mask)
+ goto err;
+
+ dev_dbg(priv->dev, "Set drive strength for %s to %d mA\n",
+ cs42l43_pin_pins[pin].name, ma);
+
+ return regmap_update_bits(priv->regmap, pdat->reg,
+ pdat->mask, i << pdat->shift);
+ }
+ }
+
+err:
+ dev_err(priv->dev, "Invalid drive strength for %s: %d mA\n",
+ cs42l43_pin_pins[pin].name, ma);
+ return -EINVAL;
+}
+
+static inline int cs42l43_pin_get_db(struct cs42l43_pin *priv, unsigned int pin)
+{
+ unsigned int val;
+ int ret;
+
+ if (pin >= CS42L43_NUM_GPIOS)
+ return -ENOTSUPP;
+
+ ret = regmap_read(priv->regmap, CS42L43_GPIO_CTRL2, &val);
+ if (ret)
+ return ret;
+
+ if (val & (CS42L43_GPIO1_DEGLITCH_BYP_MASK << pin))
+ return 0;
+ else
+ return 85; // Debounce is roughly 85uS
+}
+
+static inline int cs42l43_pin_set_db(struct cs42l43_pin *priv, unsigned int pin,
+ unsigned int us)
+{
+ if (pin >= CS42L43_NUM_GPIOS)
+ return -ENOTSUPP;
+
+ dev_dbg(priv->dev, "Set debounce %s for %s\n",
+ us ? "on" : "off", cs42l43_pin_pins[pin].name);
+
+ return regmap_update_bits(priv->regmap, CS42L43_GPIO_CTRL2,
+ CS42L43_GPIO1_DEGLITCH_BYP_MASK << pin,
+ !!us << pin);
+}
+
+static int cs42l43_pin_config_get(struct pinctrl_dev *pctldev,
+ unsigned int pin, unsigned long *config)
+{
+ struct cs42l43_pin *priv = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int param = pinconf_to_config_param(*config);
+ int ret;
+
+ switch (param) {
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ ret = cs42l43_pin_get_drv_str(priv, pin);
+ if (ret < 0)
+ return ret;
+ break;
+ case PIN_CONFIG_INPUT_DEBOUNCE:
+ ret = cs42l43_pin_get_db(priv, pin);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ *config = pinconf_to_config_packed(param, ret);
+
+ return 0;
+}
+
+static int cs42l43_pin_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
+ unsigned long *configs, unsigned int num_configs)
+{
+ struct cs42l43_pin *priv = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int val;
+ int ret;
+
+ while (num_configs) {
+ val = pinconf_to_config_argument(*configs);
+
+ switch (pinconf_to_config_param(*configs)) {
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ ret = cs42l43_pin_set_drv_str(priv, pin, val);
+ if (ret)
+ return ret;
+ break;
+ case PIN_CONFIG_INPUT_DEBOUNCE:
+ ret = cs42l43_pin_set_db(priv, pin, val);
+ if (ret)
+ return ret;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ ++configs;
+ --num_configs;
+ }
+
+ return 0;
+}
+
+static int cs42l43_pin_config_group_get(struct pinctrl_dev *pctldev,
+ unsigned int selector, unsigned long *config)
+{
+ int i, ret;
+
+ for (i = 0; i < cs42l43_pin_groups[selector].npins; ++i) {
+ ret = cs42l43_pin_config_get(pctldev,
+ cs42l43_pin_groups[selector].pins[i],
+ config);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int cs42l43_pin_config_group_set(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ unsigned long *configs,
+ unsigned int num_configs)
+{
+ int i, ret;
+
+ for (i = 0; i < cs42l43_pin_groups[selector].npins; ++i) {
+ ret = cs42l43_pin_config_set(pctldev,
+ cs42l43_pin_groups[selector].pins[i],
+ configs, num_configs);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct pinconf_ops cs42l43_pin_conf_ops = {
+ .is_generic = true,
+
+ .pin_config_get = cs42l43_pin_config_get,
+ .pin_config_set = cs42l43_pin_config_set,
+ .pin_config_group_get = cs42l43_pin_config_group_get,
+ .pin_config_group_set = cs42l43_pin_config_group_set,
+};
+
+static struct pinctrl_desc cs42l43_pin_desc = {
+ .name = "cs42l43-pinctrl",
+ .owner = THIS_MODULE,
+
+ .pins = cs42l43_pin_pins,
+ .npins = ARRAY_SIZE(cs42l43_pin_pins),
+
+ .pctlops = &cs42l43_pin_group_ops,
+ .pmxops = &cs42l43_pin_mux_ops,
+ .confops = &cs42l43_pin_conf_ops,
+};
+
+static int cs42l43_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct cs42l43_pin *priv = gpiochip_get_data(chip);
+ unsigned int val;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(priv->dev);
+ if (ret) {
+ dev_err(priv->dev, "Failed to resume for get: %d\n", ret);
+ return ret;
+ }
+
+ ret = regmap_read(priv->regmap, CS42L43_GPIO_STS, &val);
+ if (ret)
+ dev_err(priv->dev, "Failed to get gpio%d: %d\n", offset + 1, ret);
+ else
+ ret = !!(val & BIT(offset + CS42L43_GPIO1_STS_SHIFT));
+
+ pm_runtime_put(priv->dev);
+
+ return ret;
+}
+
+static void cs42l43_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+ struct cs42l43_pin *priv = gpiochip_get_data(chip);
+ unsigned int shift = offset + CS42L43_GPIO1_LVL_SHIFT;
+ int ret;
+
+ dev_dbg(priv->dev, "Setting gpio%d to %s\n",
+ offset + 1, value ? "high" : "low");
+
+ ret = pm_runtime_resume_and_get(priv->dev);
+ if (ret) {
+ dev_err(priv->dev, "Failed to resume for set: %d\n", ret);
+ return;
+ }
+
+ ret = regmap_update_bits(priv->regmap, CS42L43_GPIO_CTRL1,
+ BIT(shift), value << shift);
+ if (ret)
+ dev_err(priv->dev, "Failed to set gpio%d: %d\n", offset + 1, ret);
+
+ pm_runtime_put(priv->dev);
+}
+
+static int cs42l43_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
+{
+ return pinctrl_gpio_direction_input(chip->base + offset);
+}
+
+static int cs42l43_gpio_direction_out(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ cs42l43_gpio_set(chip, offset, value);
+
+ return pinctrl_gpio_direction_output(chip->base + offset);
+}
+
+static int cs42l43_pin_probe(struct platform_device *pdev)
+{
+ struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
+ struct cs42l43_pin *priv;
+ struct pinctrl_dev *pctldev;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+ priv->regmap = cs42l43->regmap;
+
+ priv->shutters_locked = cs42l43->hw_lock;
+
+ priv->gpio_chip.request = gpiochip_generic_request;
+ priv->gpio_chip.free = gpiochip_generic_free;
+ priv->gpio_chip.direction_input = cs42l43_gpio_direction_in;
+ priv->gpio_chip.direction_output = cs42l43_gpio_direction_out;
+ priv->gpio_chip.get = cs42l43_gpio_get;
+ priv->gpio_chip.set = cs42l43_gpio_set;
+ priv->gpio_chip.label = dev_name(priv->dev);
+ priv->gpio_chip.parent = priv->dev;
+ priv->gpio_chip.can_sleep = true;
+ priv->gpio_chip.base = -1;
+ priv->gpio_chip.ngpio = CS42L43_NUM_GPIOS;
+ priv->gpio_chip.fwnode = dev_fwnode(cs42l43->dev);
+
+ if (is_of_node(dev_fwnode(cs42l43->dev))) {
+ device_set_node(priv->dev,
+ fwnode_get_named_child_node(dev_fwnode(cs42l43->dev),
+ "pinctrl"));
+ } else {
+ device_set_node(priv->dev, dev_fwnode(cs42l43->dev));
+ }
+
+ pm_runtime_enable(priv->dev);
+ pm_runtime_idle(priv->dev);
+
+ pctldev = devm_pinctrl_register(priv->dev, &cs42l43_pin_desc, priv);
+ if (IS_ERR(pctldev)) {
+ ret = PTR_ERR(pctldev);
+ dev_err(priv->dev, "Failed to register pinctrl: %d\n", ret);
+ goto err_pm;
+ }
+
+ ret = devm_gpiochip_add_data(priv->dev, &priv->gpio_chip, priv);
+ if (ret) {
+ dev_err(priv->dev, "Failed to register gpiochip: %d\n", ret);
+ goto err_pm;
+ }
+
+ if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
+ ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
+ 0, 0, CS42L43_NUM_GPIOS);
+ if (ret) {
+ dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
+ goto err_pm;
+ }
+ }
+
+ return 0;
+
+err_pm:
+ pm_runtime_disable(priv->dev);
+
+ return ret;
+}
+
+static int cs42l43_pin_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(&pdev->dev);
+
+ return 0;
+}
+
+static struct platform_driver cs42l43_pin_driver = {
+ .driver = {
+ .name = "cs42l43-pinctrl",
+ },
+
+ .probe = cs42l43_pin_probe,
+ .remove = cs42l43_pin_remove,
+};
+module_platform_driver(cs42l43_pin_driver);
+
+MODULE_DESCRIPTION("CS42L43 Pinctrl Driver");
+MODULE_AUTHOR("Charles Keepax <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:cs42l43-pinctrl");
--
2.30.2
On 12/05/2023 14:28, Charles Keepax wrote:
> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> for portable applications. It provides a high dynamic range, stereo
> DAC for headphone output, two integrated Class D amplifiers for
> loudspeakers, and two ADCs for wired headset microphone input or
> stereo line input. PDM inputs are provided for digital microphones.
>
> Add a basic pinctrl driver which supports driver strength for the
> various pins, gpios, and pinmux for the 2 multi-function pins.
>
> Signed-off-by: Charles Keepax <[email protected]>
...
> +{
> + struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
> + struct cs42l43_pin *priv;
> + struct pinctrl_dev *pctldev;
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = &pdev->dev;
> + priv->regmap = cs42l43->regmap;
> +
> + priv->shutters_locked = cs42l43->hw_lock;
> +
> + priv->gpio_chip.request = gpiochip_generic_request;
> + priv->gpio_chip.free = gpiochip_generic_free;
> + priv->gpio_chip.direction_input = cs42l43_gpio_direction_in;
> + priv->gpio_chip.direction_output = cs42l43_gpio_direction_out;
> + priv->gpio_chip.get = cs42l43_gpio_get;
> + priv->gpio_chip.set = cs42l43_gpio_set;
> + priv->gpio_chip.label = dev_name(priv->dev);
> + priv->gpio_chip.parent = priv->dev;
> + priv->gpio_chip.can_sleep = true;
> + priv->gpio_chip.base = -1;
> + priv->gpio_chip.ngpio = CS42L43_NUM_GPIOS;
> + priv->gpio_chip.fwnode = dev_fwnode(cs42l43->dev);
> +
> + if (is_of_node(dev_fwnode(cs42l43->dev))) {
> + device_set_node(priv->dev,
> + fwnode_get_named_child_node(dev_fwnode(cs42l43->dev),
> + "pinctrl"));
That's something unusual. It seems you want to bind to a DT node because
you miss compatible in DT node?
> + } else {
> + device_set_node(priv->dev, dev_fwnode(cs42l43->dev));
> + }
> +
> + pm_runtime_enable(priv->dev);
> + pm_runtime_idle(priv->dev);
> +
....
> +
> +static struct platform_driver cs42l43_pin_driver = {
> + .driver = {
> + .name = "cs42l43-pinctrl",
> + },
> +
> + .probe = cs42l43_pin_probe,
> + .remove = cs42l43_pin_remove,
> +};
> +module_platform_driver(cs42l43_pin_driver);
> +
> +MODULE_DESCRIPTION("CS42L43 Pinctrl Driver");
> +MODULE_AUTHOR("Charles Keepax <[email protected]>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:cs42l43-pinctrl");
Same comment, so I guess you have this pattern everywhere.
Best regards,
Krzysztof
On Fri, May 12, 2023 at 05:30:37PM +0200, Krzysztof Kozlowski wrote:
> On 12/05/2023 14:28, Charles Keepax wrote:
> > + priv->gpio_chip.fwnode = dev_fwnode(cs42l43->dev);
> > +
> > + if (is_of_node(dev_fwnode(cs42l43->dev))) {
> > + device_set_node(priv->dev,
> > + fwnode_get_named_child_node(dev_fwnode(cs42l43->dev),
> > + "pinctrl"));
>
> That's something unusual. It seems you want to bind to a DT node because
> you miss compatible in DT node?
>
Kinda, I don't really want to add multiple compatibles for the
device. This is just a CODEC device, even in device tree it
seems a little weird to have multiple compatibles for a single
I2C device. On ACPI I am pretty sure it would be considered flat
out right wrong. The fact Linux supports the device using multiple
drivers is seemed to be a Linux implementation detail, rather than
describing the hardware.
The original (internal) version of the patches just had a single
firmware node, but the DT schema would not verify because the
node is both a pinctrl node and a spi node. And the pinctrl
schema requires the node to be called "pinctrl" and the spi
requires it to be called "spi", it is impossible to satisfy both.
Any advice/guidance you had on this one would be greatly
appreciated?
> > + } else {
> > + device_set_node(priv->dev, dev_fwnode(cs42l43->dev));
> > + }
> > +
> > + pm_runtime_enable(priv->dev);
> > + pm_runtime_idle(priv->dev);
> > +
>
> > +MODULE_DESCRIPTION("CS42L43 Pinctrl Driver");
> > +MODULE_AUTHOR("Charles Keepax <[email protected]>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:cs42l43-pinctrl");
>
> Same comment, so I guess you have this pattern everywhere.
Yeah this is not problem to fix up, I was just unaware using the
id_table was preferrable for MFD components, there are a lot of
devices doing it both ways.
Thanks,
Charles
Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> for portable applications. It provides a high dynamic range, stereo
> DAC for headphone output, two integrated Class D amplifiers for
> loudspeakers, and two ADCs for wired headset microphone input or
> stereo line input. PDM inputs are provided for digital microphones.
>
> Add a basic pinctrl driver which supports driver strength for the
> various pins, gpios, and pinmux for the 2 multi-function pins.
...
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
Can you order them and split into a separate group that goes...
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
...here?
> +#include "../pinctrl-utils.h"
...
> +struct cs42l43_pin {
> + struct device *dev;
> + struct regmap *regmap;
> + bool shutters_locked;
> + struct gpio_chip gpio_chip;
If you move this to be the first member you might save a few bytes of code.
> + struct pinctrl_gpio_range range;
Is it really needed here?
> +};
...
> +#define CS42L43_PIN(_number, _name, _reg, _field) { \
> + .number = _number, .name = _name, \
> + .drv_data = &((struct cs42l43_pin_data){ \
> + .reg = CS42L43_##_reg, \
> + .shift = CS42L43_##_field##_DRV_SHIFT, \
> + .mask = CS42L43_##_field##_DRV_MASK, \
> + }), \
Do you need this to be GCC extention for the value evaluation?
I mean the compound literal, IIRC, can be used directly as
.foo = &(struct foo){ ... },
Am I mistaken?
> +}
...
> +#define CS42L43_PINGROUP(_name) \
Use PINCTRL_PINGROUP() instead of open coded.
> +(struct pingroup){ \
> + .name = #_name, \
> + .pins = cs42l43_pin_##_name##_pins, \
> + .npins = ARRAY_SIZE(cs42l43_pin_##_name##_pins) \
> +}
...
> +enum cs42l43_pin_funcs {
> + CS42L43_FUNC_GPIO,
> + CS42L43_FUNC_SPDIF,
> + CS42L43_FUNC_IRQ,
> + CS42L43_FUNC_MIC_SHT,
> + CS42L43_FUNC_SPK_SHT,
> + CS42L43_FUNC_MAX,
No comma for the terminator entry
> +};
...
> +static const char * const cs42l43_pin_funcs[] = {
> + "gpio", "spdif", "irq", "mic-shutter", "spk-shutter"
I would keep trailing comma.
> +};
...
> +struct cs42l43_pin_func_group {
> + const char * const *groups;
> + unsigned int ngroups;
> +};
We have struct pinfunction.
> +static const struct cs42l43_pin_func_group cs42l43_pin_func_groups[] = {
> + { cs42l43_pin_gpio_groups, ARRAY_SIZE(cs42l43_pin_gpio_groups) },
> + { cs42l43_pin_spdif_groups, ARRAY_SIZE(cs42l43_pin_spdif_groups) },
> + { cs42l43_pin_irq_groups, ARRAY_SIZE(cs42l43_pin_irq_groups) },
> + { cs42l43_pin_shutter_groups, ARRAY_SIZE(cs42l43_pin_shutter_groups) },
> + { cs42l43_pin_shutter_groups, ARRAY_SIZE(cs42l43_pin_shutter_groups) },
We have PINCTRL_PINFUNCTION().
> +};
...
> +static int cs42l43_pin_get_func_count(struct pinctrl_dev *pctldev)
> +{
> + BUILD_BUG_ON(ARRAY_SIZE(cs42l43_pin_funcs) != CS42L43_FUNC_MAX);
> + BUILD_BUG_ON(ARRAY_SIZE(cs42l43_pin_func_groups) != CS42L43_FUNC_MAX);
Use static_assert() in the global scope instead.
> +
> + return ARRAY_SIZE(cs42l43_pin_funcs);
> +}
...
> + default:
> + reg = CS42L43_GPIO_FN_SEL;
> + mask = BIT(group_idx + CS42L43_GPIO1_FN_SEL_SHIFT);
> + val = (func_idx == CS42L43_FUNC_GPIO) <<
> + (group_idx + CS42L43_GPIO1_FN_SEL_SHIFT);
This would be better as ternary.
> + break;
> + }
...
> + dev_dbg(priv->dev, "Setting gpio%d to %s\n",
> + offset + 1, input ? "input" : "output");
How ' + 1' part won't be confusing?
...
> +static inline int cs42l43_pin_get_db(struct cs42l43_pin *priv, unsigned int pin)
> +{
> + unsigned int val;
> + int ret;
> +
> + if (pin >= CS42L43_NUM_GPIOS)
> + return -ENOTSUPP;
> +
> + ret = regmap_read(priv->regmap, CS42L43_GPIO_CTRL2, &val);
> + if (ret)
> + return ret;
> +
> + if (val & (CS42L43_GPIO1_DEGLITCH_BYP_MASK << pin))
> + return 0;
> + else
Redundant.
> + return 85; // Debounce is roughly 85uS
// Debounce is roughly 85uS
return 85;
> +}
...
> + dev_dbg(priv->dev, "Set debounce %s for %s\n",
> + us ? "on" : "off", cs42l43_pin_pins[pin].name);
str_on_off()
...
> + ++configs;
> + --num_configs;
Why preincrements?
...
> + if (is_of_node(dev_fwnode(cs42l43->dev))) {
> + device_set_node(priv->dev,
> + fwnode_get_named_child_node(dev_fwnode(cs42l43->dev),
> + "pinctrl"));
> + } else {
> + device_set_node(priv->dev, dev_fwnode(cs42l43->dev));
> + }
This can be called once after if.
...
> + pctldev = devm_pinctrl_register(priv->dev, &cs42l43_pin_desc, priv);
> + if (IS_ERR(pctldev)) {
> + ret = PTR_ERR(pctldev);
> + dev_err(priv->dev, "Failed to register pinctrl: %d\n", ret);
ret = dev_err_probe();
Same for other similar cases.
> + goto err_pm;
> + }
> + if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
> + ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
> + 0, 0, CS42L43_NUM_GPIOS);
> + if (ret) {
> + dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
> + goto err_pm;
> + }
> + }
Besides the fact that we have a callback for this, why GPIO library can't
handle this for you already?
...
> +static int cs42l43_pin_remove(struct platform_device *pdev)
> +{
> + pm_runtime_disable(&pdev->dev);
This is simply wrong order because it's a mix of non-devm_*() followed by
devm_*() calls in the probe.
> + return 0;
> +}
...
> +static struct platform_driver cs42l43_pin_driver = {
> + .driver = {
> + .name = "cs42l43-pinctrl",
> + },
> +
Redundant blank line.
> + .probe = cs42l43_pin_probe,
> + .remove = cs42l43_pin_remove,
> +};
--
With Best Regards,
Andy Shevchenko
On 12/05/2023 17:54, Charles Keepax wrote:
> On Fri, May 12, 2023 at 05:30:37PM +0200, Krzysztof Kozlowski wrote:
>> On 12/05/2023 14:28, Charles Keepax wrote:
>>> + priv->gpio_chip.fwnode = dev_fwnode(cs42l43->dev);
What's also a bit confusing is that gpio_chip is the parent's node, but
pinctrl is not...
>>> +
>>> + if (is_of_node(dev_fwnode(cs42l43->dev))) {
>>> + device_set_node(priv->dev,
>>> + fwnode_get_named_child_node(dev_fwnode(cs42l43->dev),
>>> + "pinctrl"));
>>
>> That's something unusual. It seems you want to bind to a DT node because
>> you miss compatible in DT node?
>>
>
> Kinda, I don't really want to add multiple compatibles for the
> device. This is just a CODEC device, even in device tree it
> seems a little weird to have multiple compatibles for a single
> I2C device. On ACPI I am pretty sure it would be considered flat
> out right wrong. The fact Linux supports the device using multiple
> drivers is seemed to be a Linux implementation detail, rather than
> describing the hardware.
>
I think if you do not have compatible, then the device node should be
rather the parent (so the main node with compatible), not the child.
Child is just a wrapper for pinctrls, but not something representing a
device.
Best regards,
Krzysztof
On Fri, May 12, 2023 at 10:19:14PM +0300, [email protected] wrote:
> Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
> > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> > for portable applications. It provides a high dynamic range, stereo
> > DAC for headphone output, two integrated Class D amplifiers for
> > loudspeakers, and two ADCs for wired headset microphone input or
> > stereo line input. PDM inputs are provided for digital microphones.
> >
> > Add a basic pinctrl driver which supports driver strength for the
> > various pins, gpios, and pinmux for the 2 multi-function pins.
>
Thanks for the review, will fix up most of the comments.
> > +#define CS42L43_PIN(_number, _name, _reg, _field) { \
> > + .number = _number, .name = _name, \
> > + .drv_data = &((struct cs42l43_pin_data){ \
> > + .reg = CS42L43_##_reg, \
> > + .shift = CS42L43_##_field##_DRV_SHIFT, \
> > + .mask = CS42L43_##_field##_DRV_MASK, \
> > + }), \
>
> Do you need this to be GCC extention for the value evaluation?
> I mean the compound literal, IIRC, can be used directly as
>
> .foo = &(struct foo){ ... },
>
> Am I mistaken?
I will double check this, I had a feeling it needed the GCC
extension.
> > + dev_dbg(priv->dev, "Setting gpio%d to %s\n",
> > + offset + 1, input ? "input" : "output");
>
> How ' + 1' part won't be confusing?
Kinda an un-avoidable confusion somewhere, the GPIOs in the datasheet are
numbered from one. So this makes the debug print match the
datasheet name for the pin, which is used in the pinctrl strings
as well.
> > + if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
> > + ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
> > + 0, 0, CS42L43_NUM_GPIOS);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
> > + goto err_pm;
> > + }
> > + }
>
> Besides the fact that we have a callback for this, why GPIO library can't
> handle this for you already?
>
Apologies but I am not quite sure I follow you, in the device
tree case this will be handled by the GPIO library. But for ACPI
this information does not exist so has to be called manually, the
library does not necessarily know which values to call with,
although admittedly our case is trivial but not all are.
> ...
>
> > +static int cs42l43_pin_remove(struct platform_device *pdev)
> > +{
> > + pm_runtime_disable(&pdev->dev);
>
> This is simply wrong order because it's a mix of non-devm_*() followed by
> devm_*() calls in the probe.
>
I had missed there are now devm_pm_runtime calls, I will switch
to that. But I would like to understand the wrong order, remove
will be called before the devm bits are destroyed and it seems
reasonable to disable the pm_runtime before destroying the
pinctrl device. What exactly would run in the wrong order here?
Thanks,
Charles
On Mon, May 15, 2023 at 1:13 PM Charles Keepax
<[email protected]> wrote:
> On Fri, May 12, 2023 at 10:19:14PM +0300, [email protected] wrote:
> > Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
...
> > > + dev_dbg(priv->dev, "Setting gpio%d to %s\n",
> > > + offset + 1, input ? "input" : "output");
> >
> > How ' + 1' part won't be confusing?
>
> Kinda an un-avoidable confusion somewhere, the GPIOs in the datasheet are
> numbered from one. So this makes the debug print match the
> datasheet name for the pin, which is used in the pinctrl strings
> as well.
The problem here is that the entire Linux pin control and GPIO cores
in their debug/info/error messages will use offset + 0. With the above
invention it will well make users confused a lot. I think you need a
Linus W blessing for this.
...
> > > + if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
> > > + ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
> > > + 0, 0, CS42L43_NUM_GPIOS);
> > > + if (ret) {
> > > + dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
> > > + goto err_pm;
> > > + }
> > > + }
> >
> > Besides the fact that we have a callback for this, why GPIO library can't
> > handle this for you already?
>
> Apologies but I am not quite sure I follow you, in the device
> tree case this will be handled by the GPIO library. But for ACPI
> this information does not exist so has to be called manually, the
> library does not necessarily know which values to call with,
> although admittedly our case is trivial but not all are.
Why can't the firmware provide this information? _DSD() is a part of
ACPI v5.1 IIRC.
Although it might require moving some code from gpiolib-of.c to
gpiolib.c with replacing OF APIs with agnostic ones.
...
> > > +static int cs42l43_pin_remove(struct platform_device *pdev)
> > > +{
> > > + pm_runtime_disable(&pdev->dev);
> >
> > This is simply wrong order because it's a mix of non-devm_*() followed by
> > devm_*() calls in the probe.
> >
>
> I had missed there are now devm_pm_runtime calls, I will switch
> to that. But I would like to understand the wrong order, remove
> will be called before the devm bits are destroyed and it seems
> reasonable to disable the pm_runtime before destroying the
> pinctrl device. What exactly would run in the wrong order here?
At the ->remove() stage after this call an IRQ can be fired (or on SMP
systems any other APIs can be called), for example. So, would it be a
problem to service it with PM disabled?
But in any case the shuffling ordering like this is prone to subtle
bugs. I prefer to have strict ordering if there is nothing preventing
from doing that way.
--
With Best Regards,
Andy Shevchenko
On Tue, May 16, 2023 at 10:03:45PM +0300, Andy Shevchenko wrote:
> On Mon, May 15, 2023 at 1:13 PM Charles Keepax
> <[email protected]> wrote:
> > On Fri, May 12, 2023 at 10:19:14PM +0300, [email protected] wrote:
> > > Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
> > > > + if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
> > > > + ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
> > > > + 0, 0, CS42L43_NUM_GPIOS);
> > > > + if (ret) {
> > > > + dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
> > > > + goto err_pm;
> > > > + }
> > > > + }
> > >
> > > Besides the fact that we have a callback for this, why GPIO library can't
> > > handle this for you already?
> >
> > Apologies but I am not quite sure I follow you, in the device
> > tree case this will be handled by the GPIO library. But for ACPI
> > this information does not exist so has to be called manually, the
> > library does not necessarily know which values to call with,
> > although admittedly our case is trivial but not all are.
>
> Why can't the firmware provide this information? _DSD() is a part of
> ACPI v5.1 IIRC.
>
I am very very far from confident we can guarantee that will be
present in the ACPI. The ACPI is typically made for and by the
Windows side.
> Although it might require moving some code from gpiolib-of.c to
> gpiolib.c with replacing OF APIs with agnostic ones.
>
I really think if we want to start doing things that way on ACPI
platforms someone with a little more clout than us needs to start
doing it first. If Intel or someone was doing it that way it
might give us a little more levelage to push it as being the
"correct" way to do it.
I will switch to the callback, but really don't think we can rely
on this being in DSD yet.
>
> > > > +static int cs42l43_pin_remove(struct platform_device *pdev)
> > > > +{
> > > > + pm_runtime_disable(&pdev->dev);
> > >
> > > This is simply wrong order because it's a mix of non-devm_*() followed by
> > > devm_*() calls in the probe.
> > >
> >
> > I had missed there are now devm_pm_runtime calls, I will switch
> > to that. But I would like to understand the wrong order, remove
> > will be called before the devm bits are destroyed and it seems
> > reasonable to disable the pm_runtime before destroying the
> > pinctrl device. What exactly would run in the wrong order here?
>
> At the ->remove() stage after this call an IRQ can be fired (or on SMP
> systems any other APIs can be called), for example. So, would it be a
> problem to service it with PM disabled?
>
> But in any case the shuffling ordering like this is prone to subtle
> bugs. I prefer to have strict ordering if there is nothing preventing
> from doing that way.
Yeah happy enough to use devm_ here, just didn't know it existed
and wanted to better understand your concerns as I was having
difficulty seeing the issue.
Thanks,
Charles
On Wed, May 17, 2023 at 1:13 PM Charles Keepax
<[email protected]> wrote:
> On Tue, May 16, 2023 at 10:03:45PM +0300, Andy Shevchenko wrote:
> > On Mon, May 15, 2023 at 1:13 PM Charles Keepax
> > <[email protected]> wrote:
> > > On Fri, May 12, 2023 at 10:19:14PM +0300, [email protected] wrote:
> > > > Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
> > > > > + if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
> > > > > + ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
> > > > > + 0, 0, CS42L43_NUM_GPIOS);
> > > > > + if (ret) {
> > > > > + dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
> > > > > + goto err_pm;
> > > > > + }
> > > > > + }
> > > >
> > > > Besides the fact that we have a callback for this, why GPIO library can't
> > > > handle this for you already?
> > >
> > > Apologies but I am not quite sure I follow you, in the device
> > > tree case this will be handled by the GPIO library. But for ACPI
> > > this information does not exist so has to be called manually, the
> > > library does not necessarily know which values to call with,
> > > although admittedly our case is trivial but not all are.
> >
> > Why can't the firmware provide this information? _DSD() is a part of
> > ACPI v5.1 IIRC.
>
> I am very very far from confident we can guarantee that will be
> present in the ACPI. The ACPI is typically made for and by the
> Windows side.
Why? You may insist firmware vendors / OEMs to use that as a
requirement to the platforms that would like to use your chip. The
_DSD() is part of the specification, I don't see how the above can be
an argument.
The times when ACPI == Windows are quite behind.
> > Although it might require moving some code from gpiolib-of.c to
> > gpiolib.c with replacing OF APIs with agnostic ones.
>
> I really think if we want to start doing things that way on ACPI
> platforms someone with a little more clout than us needs to start
> doing it first. If Intel or someone was doing it that way it
> might give us a little more levelage to push it as being the
> "correct" way to do it.
So, we have the meta-acpi [1] project which contains dozens of
examples on how ACPI DSD is being used for real devices, besides some
documentation in the Linux kernel.
> I will switch to the callback, but really don't think we can rely
> on this being in DSD yet.
Why not?
...
> > > I had missed there are now devm_pm_runtime calls,
Btw, even if there is no such API one can always call
devm_add_action() / devm_add_action_or_reset() to open code such a
call.
> > > I will switch
> > > to that. But I would like to understand the wrong order, remove
> > > will be called before the devm bits are destroyed and it seems
> > > reasonable to disable the pm_runtime before destroying the
> > > pinctrl device. What exactly would run in the wrong order here?
> >
> > At the ->remove() stage after this call an IRQ can be fired (or on SMP
> > systems any other APIs can be called), for example. So, would it be a
> > problem to service it with PM disabled?
> >
> > But in any case the shuffling ordering like this is prone to subtle
> > bugs. I prefer to have strict ordering if there is nothing preventing
> > from doing that way.
>
> Yeah happy enough to use devm_ here, just didn't know it existed
> and wanted to better understand your concerns as I was having
> difficulty seeing the issue.
Ah, you are welcome!
...
[1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples
(mostly under edison/ folder)
--
With Best Regards,
Andy Shevchenko
On 5/17/23 08:59, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 1:13 PM Charles Keepax
> <[email protected]> wrote:
>> On Tue, May 16, 2023 at 10:03:45PM +0300, Andy Shevchenko wrote:
>>> On Mon, May 15, 2023 at 1:13 PM Charles Keepax
>>> <[email protected]> wrote:
>>>> On Fri, May 12, 2023 at 10:19:14PM +0300, [email protected] wrote:
>>>>> Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
>>>>>> + if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
>>>>>> + ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
>>>>>> + 0, 0, CS42L43_NUM_GPIOS);
>>>>>> + if (ret) {
>>>>>> + dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
>>>>>> + goto err_pm;
>>>>>> + }
>>>>>> + }
>>>>>
>>>>> Besides the fact that we have a callback for this, why GPIO library can't
>>>>> handle this for you already?
>>>>
>>>> Apologies but I am not quite sure I follow you, in the device
>>>> tree case this will be handled by the GPIO library. But for ACPI
>>>> this information does not exist so has to be called manually, the
>>>> library does not necessarily know which values to call with,
>>>> although admittedly our case is trivial but not all are.
>>>
>>> Why can't the firmware provide this information? _DSD() is a part of
>>> ACPI v5.1 IIRC.
>>
>> I am very very far from confident we can guarantee that will be
>> present in the ACPI. The ACPI is typically made for and by the
>> Windows side.
>
> Why? You may insist firmware vendors / OEMs to use that as a
> requirement to the platforms that would like to use your chip. The
> _DSD() is part of the specification, I don't see how the above can be
> an argument.
>
> The times when ACPI == Windows are quite behind.
This is one of those Yogi Berra-isms: In theory, there is no difference
between theory and practice. In practice there is.
DSD is not really used indeed for audio devices. Even for SoundWire
where we inked the requirement to use DSD in a MIPI standardization
document, the only _DSD properties are for the manager side, the
peripheral side information is not populated or mostly
useless/incorrect. Most codec drivers hard-code the properties that were
intended to be set in the DSDT.
Unless there is firm evidence that the firmware does provide the
required DSD properties we can assume it does not. We can't force the
ecosystem to use DSD, even if it makes sense. it's frustrating but it is
what it is.
On Wed, May 17, 2023 at 04:59:50PM +0300, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 1:13 PM Charles Keepax
> > I am very very far from confident we can guarantee that will be
> > present in the ACPI. The ACPI is typically made for and by the
> > Windows side.
> Why? You may insist firmware vendors / OEMs to use that as a
> requirement to the platforms that would like to use your chip. The
> _DSD() is part of the specification, I don't see how the above can be
> an argument.
> The times when ACPI == Windows are quite behind.
Nobody is going to loose a sale over something like that, especially
when it's just not idiomatic. It's very unlikely to even be worth the
effort of educating customers who don't care what DSD is when there's no
ecosystem push for it, it'd just make you look difficult and weird.