[LEDE-DEV] [PATCH] ramips: Add support for the HNET C108

[Kristian Evensen]

Hi,

Thanks for the feedback.

On Wed, Sep 6, 2017 at 9:03 AM, Mathias Kresin <dev at kresin.me> wrote:
>> Not working:
>> * USB port.
>
>
> What does not working means? Not detected? USB devices not powered (but are
> working with an external powered hub)? Any error messages?

Not detected and no error messages, so it seems to be completely dead.
I see the same with the default firmware. Will update comment. If you
have any hints or tips on how to enable it, then that would be great.

>
>> * Wifi LED. It is always switched on.
>
>
> Always switched on in terms of no relation to the up/down state of the
> wireless interface or it doesn't blink on activity?
>
> Is the LED connected to the SoC? Have you tried to set the "wled" group to
> the gpio function? The wled group is only GPIO#72 (&gpio3 0).

No relation to the state. I tried to set the wled group + set gpio 72
to high/low, but it had no effect. Will update comment.

>> diff --git a/target/linux/ramips/base-files/etc/diag.sh
>> b/target/linux/ramips/base-files/etc/diag.sh
>> index 960e189283..7b267a6854 100644
>> --- a/target/linux/ramips/base-files/etc/diag.sh
>> +++ b/target/linux/ramips/base-files/etc/diag.sh
>> @@ -296,6 +296,9 @@ get_status_led() {
>>         zbt-wg3526-32M)
>>                 status_led="zbt-wg3526:green:status"
>>                 ;;
>> +       c108)
>> +               status_len="$board:green:lan"
>> +               ;;
>
>
> Please keep alphabetical order!

I was trying to figure out the lexicographical order in this file :)
The different cases seems to group together devices with different
names, so I thought it safest to place my entry at the end since I did
not find another device with same LED name. Any suggestions for a
different location (or LED name) are more than welcome.

I missed the typo ("status_len"), so I will fix that for a v3.

>> +               power_modem {
>> +                       gpio-export,name = "power_modem";
>> +                       gpio-export,output = <GPIO_ACTIVE_LOW>;
>
>
> You set the output value to 1 here. Please use gpio-export,output = 1.

Based on feedback I have got on earlier patches, it is preferred to
use the _LOW/_HIGH constants than 0/1 directly.

>> +&gpio3 {
>> +       status = "okay";
>> +};
>
>
> gpio3 is enabled but not used. either use the only available GPIO on this
> gpio bank or drop it.

Thanks, will remove it.

>> +&ethernet {
>> +       mtd-mac-address = <&factory 0x4>;
>> +       mediatek,portmap = "l";
>
>
> Beside that fact that I'm not sure if the property is supported by the
> driver, it looks wrong. The mediatek,portmap property can be only "llllw" or
> "wllll".

Yes, that is correct, but I saw that for another device (MR200) a
non-supported portmap is used ("llll"). So I thought it was ok to add,
in case the portmap will in the future support more values (the driver
anyway ignores maps different than the two you mention). But I will
remove it for v3.

>> diff --git a/target/linux/ramips/image/mt7620.mk
>> b/target/linux/ramips/image/mt7620.mk
>> index f9a9fdb84c..0061a018b9 100644
>> --- a/target/linux/ramips/image/mt7620.mk
>> +++ b/target/linux/ramips/image/mt7620.mk
>> @@ -62,6 +62,14 @@ define Device/ArcherMR200
>>   endef
>>   TARGET_DEVICES += ArcherMR200
>>   +define Device/c108
>> +  DTS := C108
>> +  IMAGE_SIZE := $(ralink_default_fw_size_16M)
>
>
> Doesn't match the size of your firmware partition.
>
> ralink_default_fw_size_16M == 16121856 == 0xF60000

Good catch, thanks. I based the dts on the dts of another 16MB mt7620
device. I will take a look at the bootlog of the default firmware and
correct firmware size.

Thanks again for all the feedback.

-Kristian