> +static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
> + phy_interface_t interface)
> +{
> + unsigned int val;
> + int ret;
> +
> + /* Configure PCS interface mode */
> + switch (interface) {
> + case PHY_INTERFACE_MODE_SGMII:
> + /* Select Qualcomm SGMII AN mode */
> + ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> + PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> + PCS_MODE_SGMII);
How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
> +static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
> + int index,
> + unsigned int neg_mode,
> + phy_interface_t interface)
> +{
> + int ret;
> +
> + /* Access to PCS registers such as PCS_MODE_CTRL which are
> + * common to all MIIs, is lock protected and configured
> + * only once. This is required only for interface modes
> + * such as QSGMII.
> + */
> + if (interface == PHY_INTERFACE_MODE_QSGMII)
> + mutex_lock(&qpcs->config_lock);
Is there a lot of contention on this lock? Why not take it for every
interface mode? It would make the code simpler.
> +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
> +{
> + struct platform_device *pdev;
> + struct ipq_pcs_mii *qpcs_mii;
> + struct device_node *pcs_np;
> + struct ipq_pcs *qpcs;
> + int i, ret;
> + u32 index;
> +
> + if (!of_device_is_available(np))
> + return ERR_PTR(-ENODEV);
> +
> + if (of_property_read_u32(np, "reg", &index))
> + return ERR_PTR(-EINVAL);
> +
> + if (index >= PCS_MAX_MII_NRS)
> + return ERR_PTR(-EINVAL);
> +
> + pcs_np = of_get_parent(np);
> + if (!pcs_np)
> + return ERR_PTR(-ENODEV);
> +
> + if (!of_device_is_available(pcs_np)) {
> + of_node_put(pcs_np);
> + return ERR_PTR(-ENODEV);
> + }
How have you got this far if the parent is not available?
> + for (i = 0; i < PCS_MII_CLK_MAX; i++) {
> + qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
> + if (IS_ERR(qpcs_mii->clk[i])) {
> + dev_err(qpcs->dev,
> + "Failed to get MII %d interface clock %s\n",
> + index, pcs_mii_clk_name[i]);
> + goto err_clk_get;
> + }
> +
> + ret = clk_prepare_enable(qpcs_mii->clk[i]);
> + if (ret) {
> + dev_err(qpcs->dev,
> + "Failed to enable MII %d interface clock %s\n",
> + index, pcs_mii_clk_name[i]);
> + goto err_clk_en;
> + }
> + }
Maybe devm_clk_bulk_get() etc will help you here? I've never actually
used them, so i don't know for sure.
Andrew
