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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
