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