On Thu, 2015-04-02 at 10:22 +0200, Rafał Miłecki wrote: > Hi Ian, > > On 10 March 2015 at 04:30, Ian Kent <[email protected]> wrote: > > Add new sprom revision 11 variables to the nvram -> sprom reader. > > > > Signed-off-by: Ian Kent <[email protected]> > > There are few problems with bcm53xx's SPROM and I've few comments to your > patch.
As I expected, ;) > > bcm53xx's SPROM driver is modified version of mainline > arch/mips/bcm74xx/sprom.c. It already differs a bit, but we can still > see the differences and try to mainline them. By adding more changes > to it we'll get lost and upsteaming changes will become more complex. Yes, that's not good. > > Other than that, current way of handling revisions is quite messy, I > guess you noticed it by yourself. I started reworking, see: > http://patchwork.linux-mips.org/patch/9659/ > Again, my change if for upstream driver. I've noticed a couple of changes you've submitted. I must admit they are interesting but not what I would have come up with. I guess it's my lack of experience with the code, but that'll come in time. > > Also your changes to struct ssb_sprom may get complex to maintain, I > believe you should try to upstream them. Sure, we needed a place to start and something to work with. At least the patch captures the names that need to be catered for and should at least save a bit of leg work. > > So my idea to resolve this situation is to: > 1) sync bcm53xx SPROM driver with mainline one, let it differ only > with DT specific code > 2) keep submitting SPROM changes to the mainline driver and just > backport them to bcm53xx > 3) clear bcm47xx's sprom.c and work on moving it to the > drivers/firmware/broadcom/ OK, just to clarify, your recommending the canonical source is the current bcm47xx and the goal is to make that generic and move it to a common location in the source tree. So I should familiarize myself with that source too. > > I'm really happy you worked on SPROM rev 11 properties, it would be > great to get them as a patch for the bcm47xx's driver. LOL, I need to start somewhere, thanks. > > > > ++ entry_count = ARRAY_SIZE(gains_info->rxgains5gmelnagaina); > > ++ for (j = 0; j < entry_count; j++) { > > ++ snprintf(postfix, sizeof(postfix), "%i", j); > > ++ snprintf(tmp, sizeof(tmp), "%i:%s", i, > > "rxgains5gmelnagaina"); > > ++ nvram_read_u8(fill, postfix, tmp, > > ++ &gains_info->rxgains5gmelnagaina[j], > > 0); > > ++ } > > You shouldn't let unexpected NVRAM content crash your driver. Don't > trust the entry_count, verify it with your array size. Umm ... don't have my patch handy and don't quite follow. Since I set the array size in the sprom structure I've assumed I could use it. I'll check since I suspect I've used local working storage and perhaps your saying I shouldn't trust the structure. So yes, I should check entry_count in case something has changed in the sprom structure. I'll need to check what happens if a value isn't present, IIRC the last parameter was used in that case, setting the gains_info entry to NULL. Ian _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
