lkml.org 
[lkml]   [2014]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2014-02-10 15:41    [W:0.046 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site