On Fri, May 12, 2023 at 10:19:14PM +0300, andy.shevchenko@gmail.com 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