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,
+};