[OpenWrt-Devel] [PATCH 3/6] bcm53xx: update sprom from nvram to handle rev 11

[Ian Kent]

On Thu, 2015-04-02 at 10:22 +0200, Rafał Miłecki wrote:
> Hi Ian,
> 
> On 10 March 2015 at 04:30, Ian Kent <raven at themaw.net> wrote:
> > Add new sprom revision 11 variables to the nvram -> sprom reader.
> >
> > Signed-off-by: Ian Kent <raven at themaw.net>
> 
> 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
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel