Re: [PATCH v8 11/17] power: pwrseq: add a driver for the PMU module on the QCom WCN chipsets

[Bjorn Helgaas]

On Tue, May 28, 2024 at 09:03:19PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> 
> This adds the power sequencing driver for the PMU modules present on the
> Qualcomm WCN Bluetooth and Wifi chipsets. It uses the pwrseq subsystem
> and knows how to match the sequencer to the consumer device by verifying
> the relevant properties and DT layout.

> +config POWER_SEQUENCING_QCOM_WCN
> +	tristate "Qualcomm WCN family PMU driver"
> +	default m if ARCH_QCOM
> +	help
> +	  Say Y here to enable the power sequencing driver for Qualcomm
> +	  WCN Bluetooth/WLAN chipsets.
> +
> +	  Typically, a package from the Qualcomm WCN family contains the BT
> +	  and WLAN modules whose power is controlled by the PMU module. As the
> +	  former two share the power-up sequence which is executed by the PMU,
> +	  this driver is needed for correct power control.

"needed for correct power control" suggests that this fixes an
existing problem, and I assume everybody with this kind of device
wants this, and they will see some benefit from enabling it.  But it's
not clear what that user-visible benefit is.  Could be useful both
here and in commit log.

> +struct pwrseq_qcom_wcn_pdata {
> +	const char *const *vregs;
> +	size_t num_vregs;
> +	unsigned int pwup_delay_msec;
> +	unsigned int gpio_enable_delay;

Seems like it'd be nice to have a hint about the units of
gpio_enable_delay (apparently ms) and last_gpio_enable (apparently
jiffies)?  Maybe even use the same units for both, but I'm sure you
have a reason for this.

> +static int pwrseq_qcom_wcn_match(struct pwrseq_device *pwrseq,
> +				 struct device *dev)
> +{
> +	struct pwrseq_qcom_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +	struct device_node *dev_node = dev->of_node;
> +
> +	/*
> +	 * The PMU supplies power to the Bluetooth and WLAN modules. both

s/both/Both/