On 05/30/2017 08:31 PM, Andy Shevchenko wrote:
> On Sun, May 28, 2017 at 9:40 PM, Hauke Mehrtens <hauke@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
