On 05/23/2017 09:52 AM, Andy Shevchenko wrote:
> 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.
I will remove this.
>
>> + 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.
Ok
>> + */
>
>> +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.
The OR_NULL part is just wrong I will remove it.
>> + 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.
I think this was a workaround for an already fixed bug in the reset
controller framework, it will be removed.
>> + }
>> +
>> + 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 {}.
OK
>> + 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.
This clock is optional, I will check the error before.
> Thus, complete line is redundant.
>
>> + clk_prepare_enable(priv->gphy_clk_gate);
>
> You need to check return value.
>
