Hi Lukasz,
please find some comments below as usual.
On 01/02/2017 04:44 PM, Lukasz Majewski wrote:
> Hi Vladimir,
>
> Thank you for review. Comments without my remarks have been applied
> already.
>
>> Hello Lukasz,
>>
>> On 12/27/2016 01:19 AM, Lukasz Majewski wrote:
>>> Signed-off-by: Lukasz Majewski <l.majewski@xxxxxxxxx>
>>
>> please add a commit message with a short description of the change.
>>
>> Also change subject line to "ARM: dts: imx6q: Add mccmon6 board
>> support".
>>
>>> ---
[snip]
>>> +/ {
>>> + model = "Monitor6 i.MX6 Quad Board";
>>
>> Missing hardware vendor name.
>>
>>> + compatible = "mccmon6", "fsl,imx6q";
>>
>> Missing hardware vendor prefix before "mccmon6".
>
> "lwn,mccmon6" ?
>
Something like that, but please ensure that you add "lwn" vendor in a separate
preceding change to Documentation/devicetree/bindings/vendor-prefixes.txt
>>
>>> +
>>> + memory {
>>> + reg = <0x10000000 0x80000000>;
>>> + };
>>> +
>>> + ethernet0 {
>>> + status = "okay";
>>> + };
>>
>> It looks like a useless device node, you have a description of &fec
>> already.
>>
>>> +
>>> + backlight_lvds: backlight {
>>> + compatible = "pwm-backlight";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pinctrl_display>;
>>
>> I would recommend to rename "pinctrl_display" to "pinctrl_backlight".
>>
>>> + pwms = <&pwm2 0 5000000 PWM_POLARITY_INVERTED>;
>>
>> This should work when extension to the i.MX PWM driver is merged.
>
> Yes. The PWM -> apply is an ongoing work. But without the PMW patch the
> board is also fully operational (with reversed PWM :-) )
>
Right, I believe that the current PWM driver igonores the value passed
in the third cell, so it should be okay.
>>
>>> + brightness-levels = < 0 1 2 3 4 5 6
>>> 7 8 9
>>> + 10 11 12 13 14 15 16
>>> 17 18 19
>>> + 20 21 22 23 24 25 26
>>> 27 28 29
>>> + 30 31 32 33 34 35 36
>>> 37 38 39
>>> + 40 41 42 43 44 45 46
>>> 47 48 49
>>> + 50 51 52 53 54 55 56
>>> 57 58 59
>>> + 60 61 62 63 64 65 66
>>> 67 68 69
>>> + 70 71 72 73 74 75 76
>>> 77 78 79
>>> + 80 81 82 83 84 85 86
>>> 87 88 89
>>> + 90 91 92 93 94 95 96
>>> 97 98 99
>>> + 100 101 102 103 104 105 106
>>> 107 108 109
>>> + 110 111 112 113 114 115 116
>>> 117 118 119
>>> + 120 121 122 123 124 125 126
>>> 127 128 129
>>> + 130 131 132 133 134 135 136
>>> 137 138 139
>>> + 140 141 142 143 144 145 146
>>> 147 148 149
>>> + 150 151 152 153 154 155 156
>>> 157 158 159
>>> + 160 161 162 163 164 165 166
>>> 167 168 169
>>> + 170 171 172 173 174 175 176
>>> 177 178 179
>>> + 180 181 182 183 184 185 186
>>> 187 188 189
>>> + 190 191 192 193 194 195 196
>>> 197 198 199
>>> + 200 201 202 203 204 205 206
>>> 207 208 209
>>> + 210 211 212 213 214 215 216
>>> 217 218 219
>>> + 220 221 222 223 224 225 226
>>> 227 228 229
>>> + 230 231 232 233 234 235 236
>>> 237 238 239
>>> + 240 241 242 243 244 245 246
>>> 247 248 249
>>> + 250 251 252 253 254 255>;
>>
>> I'm not sure that actually need such a long list of brightness levels.
>
> Such brightness-level property is so verbose on purpose - in this board
> we need fine brightness adjustment (harsh environment operation).
Okay.
>>
>>> + default-brightness-level = <50>;
>>> + enable-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;
>>> + };
>>> +
[snip]
>>> + pinctrl_display: dispgrp {
>>> + fsl,pins = <
>>> + /* BLEN_OUT */
>>> + MX6QDL_PAD_GPIO_2__GPIO1_IO02
>>> 0x1b0b0
>>> + /* LVDS_PPEN_OUT */
>>> + MX6QDL_PAD_SD1_DAT2__GPIO1_IO19
>>> 0x1b0b0
>>
>> This GPIO should be moved to a pinctrl group of regulator-lvds device
>> node.
>
> You mean to provide separate:
>
> pinctrl_reg_lvds: req_lvds_grp {
> fsl,pins = <
> /* LVDS_PPEN_OUT */
> MX6QDL_PAD_SD1_DAT2__GPIO1_IO19
> >;
>
> and then
>
> reg_lvds: regulator-lvds {
> compatible = "regulator-fixed";
> regulator-name = "lvds_ppen";
> regulator-min-microvolt = <3300000>;
> regulator-max-microvolt = <3300000>;
> regulator-boot-on;
>
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_reg_lvds>;
>
> gpio = <&gpio1 19 GPIO_ACTIVE_HIGH>;
> enable-active-high;
> };
>
This looks correct.
[snip]
>>> +
>>> +&uart1 {
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pinctrl_uart1>;
>>
>> Should you add "uart-has-rtscts" property?
>
> This is a simple "console" uart without rts/cts, so this property is
> not needed.
>
You are right, my review comment is valid for UART4 only.
[snip]
--
With best wishes,
Vladimir
--
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
