<p dir="ltr"><br>
Am 07.12.2015 22:12 schrieb "Chris Blake" <<a href="mailto:chrisrblake93@gmail.com">chrisrblake93@gmail.com</a>>:<br>
><br>
> Hey Yousong,<br>
><br>
> looking at the hexdump comment, this function was actually based off<br>
> of the function mtd_get_mac_binary() which uses the same dd offset<br>
> logic as you can see in mtd_get_mac_binary_ubi(). If this were to be<br>
> changed, shouldn't the same change apply to mtd_get_mac_binary()?<br>
></p>
<p dir="ltr">Yes, it should</p>
<p dir="ltr">> I know this would require a new patch, but just want to make sure we<br>
> are on the same page.</p>
<p dir="ltr">Previously, I thought mtd_get_mac_binary_ubi was a new function and not aware of its resemblence to mtd_get_mac_binary. A new patch is good if we think it can help improve the code even though it is mostly unrelated in the whole series. </p>
<p dir="ltr">And a new nitpick crawls out below...</p>
<p dir="ltr">><br>
> Regards,<br>
> Chris Blake<br>
><br>
> On Sun, Dec 6, 2015 at 10:05 PM, Yousong Zhou <<a href="mailto:yszhou4tech@gmail.com">yszhou4tech@gmail.com</a>> wrote:<br>
> > Hello, Christian, a few nitpicks inline :)<br>
> ><br>
> > On 6 December 2015 at 06:48, Christian Lamparter<br>
> > <<a href="mailto:chunkeey@googlemail.com">chunkeey@googlemail.com</a>> wrote:<br>
> >> From: Chris R Blake <<a href="mailto:chrisrblake93@gmail.com">chrisrblake93@gmail.com</a>><br>
> >><br>
> >> The MR18 stores the ath9k eeprom values on the NAND.<br>
> >> This patch makes it possible to retrieve the images<br>
> >> from there.<br>
> >><br>
> >> Signed-off-by: Chris R Blake <<a href="mailto:chrisrblake93@gmail.com">chrisrblake93@gmail.com</a>><br>
> >> ---<br>
> >> package/base-files/files/lib/functions/system.sh | 17 +++++++++++++++++<br>
> >> .../base-files/etc/hotplug.d/firmware/10-ath9k-eeprom | 6 +++++-<br>
> >> 2 files changed, 22 insertions(+), 1 deletion(-)<br>
> >><br>
> >> diff --git a/package/base-files/files/lib/functions/system.sh b/package/base-files/files/lib/functions/system.sh<br>
> >> index 8d75a5a..928a429 100644<br>
> >> --- a/package/base-files/files/lib/functions/system.sh<br>
> >> +++ b/package/base-files/files/lib/functions/system.sh<br>
> >> @@ -41,6 +41,23 @@ mtd_get_mac_binary() {<br>
> >> dd bs=1 skip=$offset count=6 if=$part 2>/dev/null | hexdump -v -n 6 -e '5/1 "%02x:" 1/1 "%02x"'<br>
> >> }<br>
> >><br>
> >> +mtd_get_mac_binary_ubi() {<br>
> >> + local mtdname="$1"<br>
> >> + local offset="$2"<br>
> >> +<br>
> >> + . /lib/upgrade/nand.sh<br>
> >> +<br>
> >> + local ubidev=$( nand_find_ubi $CI_UBIPART )<br>
> >> + local part="$( nand_find_volume $ubidev $1 )"<br>
> ><br>
> > Quotes and padding whitespaces are not need here for consistency of code style.<br>
> ><br>
> >> +<br>
> >> + if [ -z "$part" ]; then<br>
> >> + echo "mtd_get_mac_binary: ubi partition $mtdname not found!" >&2</p>
<p dir="ltr">Here the error message should be reworded.</p>
<p dir="ltr"> yousong</p>
<p dir="ltr">> >> + return<br>
> >> + fi<br>
> >> +<br>
> >> + dd bs=1 skip=$offset count=6 if=/dev/$part 2>/dev/null | hexdump -v -n 6 -e '5/1 "%02x:" 1/1 "%02x"'<br>
> ><br>
> > hexdump accepts an argument for offset with -s option<br>
> ><br>
> >> +}<br>
> >> +<br>
> >> mtd_get_part_size() {<br>
> >> local part_name=$1<br>
> >> local first dev size erasesize name<br>
> >> diff --git a/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom b/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom<br>
> >> index b5f0588..7287809 100644<br>
> >> --- a/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom<br>
> >> +++ b/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom<br>
> >> @@ -9,11 +9,14 @@ ath9k_eeprom_extract() {<br>
> >> local part=$1<br>
> >> local offset=$2<br>
> >> local count=$3<br>
> >> + local ubidev=$( nand_find_ubi $CI_UBIPART )<br>
> >> local mtd<br>
> >><br>
> >> mtd=$(find_mtd_chardev $part)<br>
> >> [ -n "$mtd" ] || \<br>
> >> - ath9k_eeprom_die "no mtd device found for partition $part"<br>
> >> + mtd="/dev/$(nand_find_volume $ubidev $part)"<br>
> >> + [ -n "$mtd" ] || \<br>
> >> + ath9k_eeprom_die "no mtd device found for partition $part"<br>
> >><br>
> ><br>
> > The indentation here can be misleading<br>
> ><br>
> >> dd if=$mtd of=/lib/firmware/$FIRMWARE bs=1 skip=$offset count=$count 2>/dev/null || \<br>
> >> ath9k_eeprom_die "failed to extract from $mtd"<br>
> >> @@ -32,6 +35,7 @@ ath9k_patch_firmware_mac() {<br>
> >> . /lib/ar71xx.sh<br>
> >> . /lib/functions.sh<br>
> >> . /lib/functions/system.sh<br>
> >> +. /lib/upgrade/nand.sh<br>
> >><br>
> ><br>
> > I suggest we move this part including the line "[ -e<br>
> > /lib/firmware/$FIRMWARE ] && exit 0" to top of this file.<br>
> ><br>
> > yousong<br>
> ><br>
> ><br>
> >> board=$(ar71xx_board_name)<br>
> >><br>
> >> --<br>
> >> 2.6.2<br>
> >> _______________________________________________<br>
> >> openwrt-devel mailing list<br>
> >><a href="mailto:openwrt-devel@lists.openwrt.org"> openwrt-devel@lists.openwrt.org</a><br>
> >><a href="https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel"> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel</a><br>
> > _______________________________________________<br>
> > openwrt-devel mailing list<br>
> ><a href="mailto:openwrt-devel@lists.openwrt.org"> openwrt-devel@lists.openwrt.org</a><br>
> ><a href="https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel"> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel</a><br>
</p>