Re: [PATCH v3 2/2] arm64: dts: rockchip: Add basic support for Kobol's Helios64

[Johan Jonker]

Hi Uwe,

On 10/13/20 10:22 PM, Uwe Kleine-König wrote:
> Hello Johan,
>
> On 10/13/20 7:34 PM, Johan Jonker wrote:
>> Part 1 of 2 missing here.
>
> Please complain to gmail then, given that patch 1 can be found on
>
https://lore.kernel.org/linux-arm-kernel/20201013161340.720403-2-uwe@xxxxxxxxxxxxxxxxx/.

Will check my spam.

>
>> Submit all patches to all maintainers and mail lists.
>> Don't forget robh+dt !
>
> I'm really surprised you insist here. In my experience Rob (with his
> dt-hat on) is not interested in specifics of the machine files and he
> already acked patch 1.

As rule of the dumb. Keep everybody in that list informed till the last
version. What they do with is not our problem or concern.

>
>> Add a little change log here.
>
> I assume you also didn't get the cover letter?
>
>>> +		fault-led {
>> fault_led: led-0 {}
>>
>> My fault.
>> Change ones more...
>>   # The first form is preferred, but fall back to just 'led' anywhere
in the
>>   # node name to at least catch some child nodes.
>>   "(^led-[0-9a-f]$|led)":
>
> ok, the label isn't necessary, is it?

Not really, for legacy and readability reasons keep it.
Without label both in nodename and as property one has to compare pin
numbers in pinctrl to see which node belong to each other.
Some users may want to add there own functions. With already available
label they can do so.

>
>>> +			label = "helios64:green:status";
>>> +			gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
>>
>>> +			linux,default-trigger = "none";
>>
>> Don't use 'none' for mainline.
>
> Oh, I missed that. Thanks for your persistence.

No thanks.

>
>>> +			default-state = "on";
>>> +		};
>>> +	};
>>> +
>>> +	vcc1v8_sys_s0: vcc1v8-sys-s0 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "vcc1v8_sys_s0";
>>> +		regulator-always-on;
>>> +		regulator-boot-on;
>>> +		regulator-min-microvolt = <1800000>;
>>> +		regulator-max-microvolt = <1800000>;
>>> +		vin-supply = <&vcc1v8_sys_s3>;
>>> +	};
>>> +
>>> +	vcc3v0_sd: vcc3v0-sd {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "vcc3v0_sd";
>>
>> Doesn't a sd card need a on/off gpio?
>> Could you check the schematics?
>
> Hmm, there is a GPIO. I didn't consider a problem there given that the
> machine logs
>
> 	[   31.708706] vcc3v0_sd: disabling
>
> when there is no SD-card in the slot. Will investigate further.

Let us know...

>
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&pmic_int_l>;
>>
>> sort
>
> I would expect this is an exception from the sorting rule.
>
> $ git grep pinctrl-names linus/master:arch/arm64/boot/dts/ | wc -l
> 1905
>
> $ git grep -A1 pinctrl-names linus/master:arch/arm64/boot/dts/ | \
>   grep pinctrl-0 | wc -l
> 1412
>
> When grepping over arch/arm64/boot/dts/rockchip the numbers are
> 453 and 445 respectively.
>
> Will use pinctrl-names above pinctrl-0 consistently.

That people in the past blindly followed the manufacturer dts doesn't
make it an exception to the sort rule. Whatever you do, do it consistent.

>
>>> +				regulator-max-microvolt = <1350000>;
>>> +				regulator-min-microvolt = <750000>;
>>
>>
>> The rest has min above max.
>> Exception to the sort rule, not sure what Heiko wants, but keep it every
>> where the same.
>
> OK, most rockchip dts have min before max, will fix up.
>
>>> +	i2c-scl-rising-time-ns = <160>;
>>> +	i2c-scl-falling-time-ns = <30>;
>>
>> sort
>
> I consider it logical to have rise before fall. In 43 of 59 cases in
> arch/arm64/boot/dts/rockchip/ it is done this way.

That people in the past blindly followed the manufacturer dts doesn't
make it an exception to the sort rule. Whatever you do, do it consistent.

>
>>> +	vqmmc-supply = <&vcc1v8_sys_s0>;
>>> +	status = "okay";
>>> +};
>>> +
>>> +&sdmmc {
>>> +	bus-width = <4>;
>>> +	cap-sd-highspeed;
>>
>>> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
>>
>> see regulator?

Little longer version:
See my question at the sd regulator.
We have a pin to detect, but not to switch the regulator.
Haven't looked at the schematics. Just a reminder to check if it's
correct. Let us know..

>
> GPIO0_A7 is the card detect line. I don't understand your question. Is
> it the same as above, i.e. that it should be possible that the SD
> regulator can be disabled?
>
>>> +	disable-wp;
>>> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
>>> +	pinctrl-names = "default";
>>> +	vmmc-supply = <&vcc3v0_sd>;
>>> +	vqmmc-supply = <&vcc_sdio_s0>;
>>> +	status = "okay";
>>> +};
>
> Best regards
> Uwe
>