On Sun, May 21, 2017 at 4:09 PM, Hauke Mehrtens <hauke@xxxxxxxxxx> wrote:
> Compared to the old xrx200_phy_fw driver the new version has multiple
> enhancements. The name of the firmware files does not have to be added
> to all .dts files anymore - one now configures the GPHY mode (FE or GE)
> instead. Each GPHY can now also boot separate firmware (thus mixing of
> GE and FE GPHYs is now possible).
> The new implementation is based on the RCU syscon-mfd and uses the
> reeset_controller framework instead of raw RCU register reads/writes.
> +static int xway_gphy_load(struct platform_device *pdev,
> + const char *fw_name, dma_addr_t *dev_addr)
> +{
> + const struct firmware *fw;
> + void *fw_addr;
> + size_t size;
> + int ret;
> +
> + dev_info(&pdev->dev, "requesting %s\n", fw_name);
Noise.
> + ret = request_firmware(&fw, fw_name, &pdev->dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to load firmware: %s, error: %i\n",
> + fw_name, ret);
> + return ret;
> + }
> +
> + /*
> + * GPHY cores need the firmware code in a persistent and contiguous
> + * memory area with a 16 kB boundary aligned start address
Add period to the end.
> + */
> +static int xway_gphy_of_probe(struct platform_device *pdev,
> + struct xway_gphy_priv *priv)
> +{
> + priv->gphy_reset = devm_reset_control_get(&pdev->dev, "gphy");
> + if (IS_ERR_OR_NULL(priv->gphy_reset)) {
_OR_NULL part looks suspicious.
There is _optional() variant of reset API, AFAIR.
> + if (PTR_ERR(priv->gphy_reset) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to lookup gphy reset\n");
> + return PTR_ERR(priv->gphy_reset);
> + }
> + priv->gphy_reset2 = devm_reset_control_get_optional(&pdev->dev, "gphy2");
> + if (IS_ERR(priv->gphy_reset2)) {
> + if (PTR_ERR(priv->gphy_reset2) == -EPROBE_DEFER)
> + return PTR_ERR(priv->gphy_reset2);
> + priv->gphy_reset2 = NULL;
Why?
If there is a problem in reset framework it should be fixed there, not here.
> + }
> +
> + if (of_property_read_u32(np, "lantiq,gphy-mode", &gphy_mode))
> + /* Default to GE mode */
If you put more lines in the branch you perhaps need {}.
> + gphy_mode = GPHY_MODE_GE;
> +static int xway_gphy_probe(struct platform_device *pdev)
> +{
> + struct xway_gphy_priv *priv;
> + dma_addr_t fw_addr = 0;
> + int ret;
> + if (!IS_ERR_OR_NULL(priv->gphy_clk_gate))
_OR_NULL is redundant.
IS_ERR should not happen here if clock is mandatory.
Thus, complete line is redundant.
> + clk_prepare_enable(priv->gphy_clk_gate);
You need to check return value.
--
With Best Regards,
Andy Shevchenko
