Messages in this thread | | | Date | Fri, 10 Mar 2023 12:53:46 +0100 | From | Lukasz Majewski <> | Subject | Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation |
| |
Hi Andrew,
> > > If I understand this correctly, in patch 4, you add a call to the > > > 6250 family to call mv88e6185_g1_set_max_frame_size(), which sets > > > a bit called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size > > > is larger than 1518. > > > > Yes, correct. > > > > > > > > However, you're saying that 6250 has a frame size of 2048. That's > > > fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather > > > misleading as a definition. While the bit may increase the frame > > > size, I think if we're going to do this, then this definition > > > ought to be renamed. > > > > I thought about rename, but then I've double checked; register > > offset and exact bit definition is the same as for 6185, so to avoid > > unnecessary code duplication - I've reused the existing function. > > > > Maybe comment would be just enough? > > The driver takes care with its namespace in order to add per switch > family defines. So you can add MV88E6250_G1_CTL1_MAX_FRAME_2048. It > does not matter if it is the same bit. You can also add a > mv88e6250_g1_set_max_frame_size() and it also does not matter if it is > in effect the same as mv88e6185_g1_set_max_frame_size(). > > We should always make the driver understandably first, compact and > without redundancy second. We are then less likely to get into > situations like this again where it is not clear what MTU a device > actually supports because the code is cryptic.
Ok, I will add new function.
Thanks for hints.
> > Andrew
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de [unhandled content-type:application/pgp-signature] | |