Re: [PATCH 08/10] pinctrl: cs42l43: Add support for the cs42l43

[Charles Keepax]

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