On 28 March 2014 22:03, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > 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. Thanks for your advise. I seems like a good idea to move to use readable strings instead. In that way, we also get the clean cut between signal directions and the feedback clock pin. Kind regards Ulf Hansson > > 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
