[PATCH v3 14/16] phy: Add an USB PHY driver for the Lantiq SoCs using the RCU module

[Hauke Mehrtens]

On 05/30/2017 08:31 PM, Andy Shevchenko wrote:
> On Sun, May 28, 2017 at 9:40 PM, Hauke Mehrtens <hauke at hauke-m.de> wrote:
>> This driver starts the DWC2 core(s) built into the XWAY SoCs and provides
>> the PHY interfaces for each core. The phy instances can be passed to the
>> dwc2 driver, which already supports the generic phy interface.
> 
>> +static void ltq_rcu_usb2_start_cores(struct platform_device *pdev)
> 
> It should return int. See below.
> 
>> +{
> 
>> +       /* Power on the USB core. */
>> +       if (clk_prepare_enable(priv->ctrl_gate_clk)) {
> 
> You basically shadow the error, why?

Thanks for the hint I changed it.

>> +               dev_err(dev, "failed to enable CTRL gate\n");
>> +               return;
>> +       }
> 
>> +       if (clk_prepare_enable(priv->phy_gate_clk)) {
>> +               dev_err(dev, "failed to enable PHY gate\n");
>> +               return;
>> +       }
> 
> Ditto.

same here.

>> +static int ltq_rcu_usb2_of_probe(struct device_node *phynode,
>> +                                   struct ltq_rcu_usb2_priv *priv)
>> +{
>> +       struct device *dev = priv->dev;
>> +       const struct of_device_id *match =
>> +               of_match_node(ltq_rcu_usb2_phy_of_match, phynode);
>> +       int ret;
>> +
> 
>> +       if (!match) {
>> +               dev_err(dev, "Not a compatible Lantiq RCU USB PHY\n");
>> +               return -EINVAL;
>> +       }
> 
> Can it ever happen?

As long as this device gets probed by device tree this can not happen,
so I will remove it.

>> +
>> +       priv->reg_bits = match->data;
> 
> I think there is a helper to get driver data directly from node.

Thanks for the hint, there is of_device_get_match_data() which does this.

>> +       if (priv->reg_bits->have_ana_cfg) {
>> +               ret = device_property_read_u32(dev, "offset-ana",
>> +                                              &priv->ana_cfg1_reg_offset);
>> +               if (ret) {
>> +                       dev_dbg(dev, "Failed to get RCU ANA CFG1 reg offset\n");
>> +                       return ret;
>> +               }
>> +       }
> 
> ret = device_property_...(...);
> if (ret && priv->reg_bits->have_ana_cfg) {
>  ...
>  return ret;
> }
> 
> ?

Yes, that should look better, I will change it.

> 
> 
>> +       priv->dev = &pdev->dev;
> 
>> +       dev_set_drvdata(priv->dev, priv);
> 
> Move this to the end of function. Ideally it should be run if and only
> if the function returns 0.

Done

>> +       provider = devm_of_phy_provider_register(&pdev->dev,
>> +                                                of_phy_simple_xlate);
>> +
>> +       return PTR_ERR_OR_ZERO(provider);
> 
> I would do explicitly, though it's up to you and maintainer.
> 
> if (IS_ERR(provider))
>  return PTR_ERR();
> 
> return 0;

I do not care, but this was I can call dev_set_drvdata() when this is
really returning 0.

Hauke