Re: [PATCH v3 4/5] PCI/pwrctrl: tc9563: Switch per-port reset to GPIO descriptor API

[Alex Elder <elder@riscstar.com>]

On 9/4/26 12:15 PM, Lorenzo Bianconi wrote:
> Remove the local TC9563_GPIO_MASK and TC9563_GPIO_DEASSERT_BITS
> definitions, which are no longer used after switching to the GPIO
> descriptor API. Move TC9563_GPIO_CONFIG and TC9563_RESET_GPIO
> definitions in tc9563.h header file.
> 
> Replace the direct regmap-based per-port reset logic in
> assert_deassert_reset() with gpiod_direction_output() calls, falling
> back to the legacy regmap approach only when no reset-gpios DT
> property is present for a given port.
> 
> Add the reset GPIO pointer to struct tc9563_pwrctrl_cfg and introduce
> tc9563_pwrctrl_parse_reset_line() to look up reset-gpios from each
> PCI downstream port child node. The lookup is done lazily at the
> beginning of power_on(), returning -EPROBE_DEFER until the GPIO chip
> is registered.
> 
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Looks good.

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
>   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 85 ++++++++++++++++++++++++++------
>   include/linux/soc/qcom/tc9563.h          |  3 ++
>   2 files changed, 73 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 2230765950b8..d9b69c3aeb7e 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -25,9 +25,6 @@
>   
>   #include "../pci.h"
>   
> -#define TC9563_GPIO_CONFIG		0x801208
> -#define TC9563_RESET_GPIO		0x801210
> -
>   #define TC9563_PORT_L0S_DELAY		0x82496c
>   #define TC9563_PORT_L1_DELAY		0x824970
>   
> @@ -59,9 +56,6 @@
>   #define TC9563_POWER_CONTROL		0x82b09c
>   #define TC9563_POWER_CONTROL_OVREN	0x82b2c8
>   
> -#define TC9563_GPIO_MASK		0xfffffff3
> -#define TC9563_GPIO_DEASSERT_BITS	0xc  /* Clear to deassert GPIO */
> -

I very much like getting rid of both of those masks...

>   #define TC9563_TX_MARGIN_MIN_UA		400000
>   
>   /*
> @@ -87,6 +81,7 @@ struct tc9563_pwrctrl_cfg {
>   	u8 nfts[2]; /* GEN1 & GEN2 */
>   	bool disable_dfe;
>   	bool disable_port;
> +	struct gpio_desc *reset;
>   };
>   
>   #define TC9563_PWRCTL_MAX_SUPPLY	6
> @@ -351,16 +346,40 @@ static int tc9563_pwrctrl_set_nfts(struct tc9563_pwrctrl *tc9563,
>   static int tc9563_pwrctrl_assert_deassert_reset(struct tc9563_pwrctrl *tc9563,
>   						bool deassert)
>   {
> -	int ret, val;
> -
> -	ret = regmap_write(tc9563->regmap, TC9563_GPIO_CONFIG,
> -			   TC9563_GPIO_MASK);
> -	if (ret)
> -		return ret;
> -
> -	val = deassert ? TC9563_GPIO_DEASSERT_BITS : 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tc9563->cfg); i++) {
> +		int err;
> +
> +		if (tc9563->cfg[i].reset) {
> +			err = gpiod_direction_output(tc9563->cfg[i].reset,
> +						     !deassert);
> +			if (err)
> +				return err;
> +		} else {
> +			/* Fallback: legacy DTS without reset-gpios */
> +			switch (i) {
> +			case TC9563_DSP1:
> +			case TC9563_DSP2:
> +				err = regmap_clear_bits(tc9563->regmap,
> +							TC9563_GPIO_CONFIG,
> +							BIT(i + 1));
> +				if (err)
> +					return err;
> +
> +				err = regmap_assign_bits(tc9563->regmap,
> +							 TC9563_RESET_GPIO,
> +							 BIT(i + 1), deassert);
> +				if (err)
> +					return err;
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	}
>   
> -	return regmap_write(tc9563->regmap, TC9563_RESET_GPIO, val);
> +	return 0;
>   }
>   
>   static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
> @@ -395,6 +414,38 @@ static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
>   	return 0;
>   }
>   
> +static int tc9563_pwrctrl_parse_reset_line(struct tc9563_pwrctrl *tc9563)
> +{
> +	enum tc9563_pwrctrl_ports port = TC9563_USP;
> +	struct device *dev = tc9563->pwrctrl.dev;
> +	struct device_node *node = dev->of_node;
> +
> +	for_each_child_of_node_scoped(node, child) {
> +		struct tc9563_pwrctrl_cfg *cfg;
> +
> +		if (++port >= TC9563_MAX)
> +			break;
> +
> +		cfg = &tc9563->cfg[port];
> +		if (cfg->reset) /* Already discovered */
> +			continue;
> +
> +		cfg->reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> +						   "reset", GPIOD_ASIS,
> +						   NULL);
> +		if (IS_ERR(cfg->reset)) {
> +			int err = PTR_ERR(cfg->reset);
> +
> +			cfg->reset = NULL;
> +			if (err != -ENOENT)
> +				return dev_err_probe(dev, err,
> +						     "failed to get reset\n");
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static void tc9563_pwrctrl_adev_release(struct device *dev)
>   {
>   	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> @@ -478,6 +529,10 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
>   	struct tc9563_pwrctrl_cfg *cfg;
>   	int ret, i;
>   
> +	ret = tc9563_pwrctrl_parse_reset_line(tc9563);
> +	if (ret)
> +		return ret;
> +
>   	ret = regulator_bulk_enable(ARRAY_SIZE(tc9563->supplies),
>   				    tc9563->supplies);
>   	if (ret < 0)
> diff --git a/include/linux/soc/qcom/tc9563.h b/include/linux/soc/qcom/tc9563.h
> index 0dfd25747b9a..086f37a40d80 100644
> --- a/include/linux/soc/qcom/tc9563.h
> +++ b/include/linux/soc/qcom/tc9563.h
> @@ -13,4 +13,7 @@
>   #define TC9563_GPIO_EN0_OFFSET		0x801208
>   #define TC9563_GPIO_OUT0_OFFSET		0x801210
>   
> +#define TC9563_GPIO_CONFIG		TC9563_GPIO_EN0_OFFSET
> +#define TC9563_RESET_GPIO		TC9563_GPIO_OUT0_OFFSET
> +
>   #endif /* __QCOM_TC9563_H */
>