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

[Richard Cochran]

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/