linux-kernel - Re: [PATCH] dp83640: Get gpio and master/slave configuration from DT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140210134237.GF29080@e106331-lin.cambridge.arm.com>
Date:	Mon, 10 Feb 2014 13:42:38 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Stefan Sørensen <stefan.sorensen@...ctralink.com>
Cc:	"richardcochran@...il.com" <richardcochran@...il.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH] dp83640: Get gpio and master/slave configuration from
 DT

On Mon, Feb 10, 2014 at 01:00:40PM +0000, Stefan Sørensen wrote:
> This patch removes the module parameters gpio_tab and chosen_phy in favour of
> retrieving the configuration from DT through the properties
> 	dp83640,slave
> 	dp83640,calibrate-gpio
> 	dp83640,perout-gpios
> 	dp83640,extts-gpios
> The configuration is now stored for each master clock device, allowing different 
> gpio setups for each master.
> 
> Furthermore the code is enhanced to handle more than one periodic output.

Binding document please.

I have some basic comments below, but without a description of what the
properties mean, it's difficult to provide any meaningful review.

[...]

> +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;
> +
> +	if (!node)
> +		return 0;
> +
> +	if (of_find_property(node, "dp83640,slave", NULL))
> +		dp83640->slave = true;

Use of_property_read_bool.

> +	if (!dp83640->slave && clock->chosen) {
> +		pr_err("dp83640,slave must be set if more than one device on the same bus");
> +		return -EINVAL;
> +	}
> +
> +	prop = of_find_property(node, "dp83640,perout-gpios", &proplen);
> +	if (prop) {
> +		if (dp83640->slave) {
> +			pr_err("dp83640,perout-gpios property can not be set together with dp83640,slave");
> +			return -EINVAL;
> +		}
> +
> +		clock->caps.n_per_out = proplen / sizeof(u32);
> +		if (clock->caps.n_per_out > N_EXT) {
> +			pr_err("dp83640,perout-gpios may not have more than %d entries",
> +			       N_EXT);
> +			return -EINVAL;
> +		}
> +		err = of_property_read_u32_array(node, "dp83640,perout-gpios",
> +						 clock->perout_gpios,
> +						 clock->caps.n_per_out);
> +		if (err < 0)
> +			return err;
> +	}

This looks nothing like the standard gpio bindings. What _exactly_ is
this property describing?

If this is not using the standard gpio bindings then this should be
renamed.

Either way this must be documented.

> +
> +	prop = of_find_property(node, "dp83640,extts-gpios", &proplen);
> +	if (prop) {
> +		if (dp83640->slave) {
> +			pr_err("dp83640,extts-gpios property can not be set together with dp83640,slave");
> +			return -EINVAL;
> +		}
> +
> +		clock->caps.n_ext_ts = proplen / sizeof(u32);
> +		if (clock->caps.n_ext_ts > N_EXT) {
> +			pr_err("dp83640,extts-gpios may not have more than %d entries",
> +			       N_EXT);
> +			return -EINVAL;
> +		}
> +		err = of_property_read_u32_array(node, "dp83640,extts-gpios",
> +						 clock->extts_gpios,
> +						 clock->caps.n_ext_ts);
> +		if (err < 0)
> +			return err;
> +	}

Similarly this does not look right for parsing a standard -gpios property.

> +
> +	prop = of_find_property(node, "dp83640,calibrate-gpio", &proplen);
> +	if (prop) {
> +		if (dp83640->slave) {
> +			pr_err("dp83640,calibrate-gpio property can not be set together with dp83640,slave");
> +			return -EINVAL;
> +		}
> +		clock->calibrate_gpio = -1;
> +		of_property_read_u32(node, "dp83640,calibrate-gpio",
> +				     &clock->calibrate_gpio);
> +	}

And again, this doesn't look right.

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