On Wed, Mar 26, 2014 at 12:05 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: > On 25 March 2014 22:22, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: >> On Fri, Mar 21, 2014 at 1:14 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> >>> These bits is currently used from platform data, but will be needed >>> from DT as well, so let's make them available. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >> (...) >>> +#define MCI_ST_DATA31DIREN (1 << 5) >>> +#define MCI_ST_FBCLKEN (1 << 7) >>> +#define MCI_ST_DATA74DIREN (1 << 8) >> >> Isn't MCI_ST_FBCLKEN = feedback clock enable and nothing >> to do with directions really? > > You do have a point here, but then we are about to go into the details. :-) > > What MCI_ST_FBCLKEN means, is that the internal logic in the primecell > can decide whether to use the signal from clockout pin or the > feebackclock pin, when its latching incoming signals on the data bus. > > That said, we for sure need a dt binding for bit as well, since this > is not possible to figure out how pins are routed in runtime. > > Also, I do believe it is very closely related to the other sigdir > pins, since it's all about how pins is being routed, even if it's as > you say - in this case a matter of signal directions. > > So, I guess we have two options here; Either move the MCI_ST_FBCLKEN > out of the signal-direction binding and invent a separate binding for > it, or just keep them as is. If they are very closely related I guess that will become very clear when you document it in the requested explicit bindings, so no strong opinion here. I'd probably ACK either version. >> Maybe we should just have single strings for these >> things in the binding instead, like: >> >> @@ -118,6 +119,10 @@ >> bus-width = <4>; >> mmc-cap-sd-highspeed; >> mmc-cap-mmc-highspeed; >> + st,signal-direction-data2; >> + st,signal-direction-cmd; >> + st,signal-direction-data0; >> + st,feedback-clock-enable; >> vmmc-supply = <&ab8500_ldo_aux3_reg>; >> vqmmc-supply = <&vmmci>; >> pinctrl-names = "default", "sleep"; > > I guess this is an option. To me it just seems a bit silly to have one > separate binding for each single bit, but maybe it becomes clearer? The idea with device trees is to be very human-readable, so I guess if you have preprocessor macro things |:ing the arguments together to a singular 32bit value it serves the same purpose. Arguably this version is even easier for a human to read though. What we should avoid is using magic number assignments to save space (e.g. have magic numbers in the tree rather than readable strings) so I lean toward this, but it's admittedly in a grey area, so again no strong opinion. Yours, Linus Walleij -- 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
