Re: [PATCH v2 2/2] dp83640: Get pin and master/slave configuration from DT

From: Richard Cochran
Date: Tue Feb 11 2014 - 15:19:44 EST


On Tue, Feb 11, 2014 at 04:29:22PM +0100, Stefan Sørensen wrote:

> diff --git a/Documentation/devicetree/bindings/net/dp83640.txt b/Documentation/devicetree/bindings/net/dp83640.txt
> new file mode 100644
> index 0000000..b9a57c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dp83640.txt
> @@ -0,0 +1,29 @@
> +Required properties for the National DP83640 ethernet phy:
> +
> +- compatible : Must contain "national,dp83640"
> +
> +Optional properties:
> +
> +- dp83640,slave: If present, this phy will be slave to another dp83640
> + on the same mdio bus.

Wouldn't it be more natural to have one "dp83640,master" property
rather than multiple slave properties?

> @@ -949,6 +940,95 @@ static void dp83640_clock_put(struct dp83640_clock *clock)
> mutex_unlock(&clock->clock_lock);
> }
>
> +#ifdef CONFIG_OF
> +static int dp83640_probe_dt(struct device_node *node,
> + struct dp83640_private *dp83640)
> +{
> + struct dp83640_clock *clock = dp83640->clock;
> + struct property *prop;
> + int err, proplen;
> +
> + dp83640->slave = of_property_read_bool(node, "dp83640,slave");
> + if (!dp83640->slave && clock->chosen) {
> + pr_err("dp83640,slave must be set if more than one device on the same bus");

Most of these pr_err lines are a bit _way_ too long for coding style.

> + return -EINVAL;
> + }
> +
> + prop = of_find_property(node, "dp83640,perout-pins", &proplen);
> + if (prop) {
> + if (dp83640->slave) {
> + pr_err("dp83640,perout-pins property can not be set together with dp83640,slave");

(Here especially and in the code that followed.)

Overall the series is looking better. I will try to test the non-DT
case later on this week.

Thanks,
Richard


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/