Re: [PATCH v3 2/3] phy: ti: gmii-sel: Update methods for fetching and using qsgmii main port

[Roger Quadros]

Hi Siddharth,

On 26/10/2022 10:45, Siddharth Vadapalli wrote:
> The number of QSGMII main ports are specific to the device. TI's J7200 for
> which the QSGMII main port property is fetched from the device-tree has
> only one QSGMII main port. However, devices like TI's J721e support up to
> two QSGMII main ports. Thus, the existing methods for fetching and using
> the QSGMII main port are not scalable.
> 
> Update the existing methods for handling the QSGMII main ports and its
> associated requirements to make it scalable for future devices.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> ---
>  drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index 0bcfd6d96b4d..c8f30d2e1f46 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>  	bool use_of_data;
>  	u64 extra_modes;
> +	u32 num_qsgmii_main_ports;
>  };
>  
>  struct phy_gmii_sel_priv {
> @@ -213,6 +214,8 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>  	.use_of_data = true,
>  	.regfields = phy_gmii_sel_fields_am654,
>  	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
> +	.num_ports = 4,
> +	.num_qsgmii_main_ports = 1,
>  };
>  
>  static const struct of_device_id phy_gmii_sel_id_table[] = {
> @@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
>  static int phy_gmii_sel_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	const struct phy_gmii_sel_soc_data *soc_data;
>  	struct device_node *node = dev->of_node;
>  	const struct of_device_id *of_id;
>  	struct phy_gmii_sel_priv *priv;
>  	u32 main_ports = 1;
>  	int ret;
> +	u32 i;
>  
>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>  	if (!of_id)
> @@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>  
>  	priv->dev = &pdev->dev;
>  	priv->soc_data = of_id->data;
> +	soc_data = priv->soc_data;
>  	priv->num_ports = priv->soc_data->num_ports;
> -	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
> +	priv->qsgmii_main_ports = 0;
> +
>  	/*
> -	 * Ensure that main_ports is within bounds. If the property
> -	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
> -	 * is out of bounds, default to 1.
> +	 * Based on the compatible, try to read the appropriate number of
> +	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
> +	 * the device-tree node.
>  	 */
> -	if (main_ports < 1 || main_ports > 4)
> -		main_ports = 1;
> -	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
> +	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
> +		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
> +		/*
> +		 * Ensure that main_ports is within bounds.
> +		 */
> +		if (main_ports < 1 || main_ports > soc_data->num_ports) {
> +			dev_err(dev, "Invalid qsgmii main port provided\n");

nit: This message is a bit misleading if the property does not exist in DT.

How about just "Invalid ti,qsgmii-main-ports"

> +			return -EINVAL;
> +		}
> +		priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports);
> +	}
>  
>  	priv->regmap = syscon_node_to_regmap(node->parent);
>  	if (IS_ERR(priv->regmap)) {

Reviewed-by: Roger Quadros <rogerq@xxxxxxxxxx>

cheers,
-roger