On Wed, Nov 06, 2024 at 05:19:03PM +0100, Andrew Lunn wrote:
> > +static const u8 dsa_r50ohm_table[] = {
> > + 127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
> > + 127, 127, 127, 127, 127, 127, 127, 126, 122, 117,
> > + 112, 109, 104, 101, 97, 94, 90, 88, 84, 80,
> > + 78, 74, 72, 68, 66, 64, 61, 58, 56, 53,
> > + 51, 48, 47, 44, 42, 40, 38, 36, 34, 32,
> > + 31, 28, 27, 24, 24, 22, 20, 18, 16, 16,
> > + 14, 12, 11, 9
> > +};
> > +
> > +static int en8855_get_r50ohm_val(struct device *dev, const char *calib_name,
> > + u8 *dest)
> > +{
> > + u32 shift_sel, val;
> > + int ret;
> > + int i;
> > +
> > + ret = nvmem_cell_read_u32(dev, calib_name, &val);
> > + if (ret)
> > + return ret;
> > +
> > + shift_sel = FIELD_GET(AN8855_SWITCH_EFUSE_R50O, val);
> > + for (i = 0; i < ARRAY_SIZE(dsa_r50ohm_table); i++)
> > + if (dsa_r50ohm_table[i] == shift_sel)
> > + break;
>
> Is an exact match expected? Should this be >= so the nearest match is
> found?
>
As strange as this is, yes this is what the original code does.
> > +
> > + if (i < 8 || i >= ARRAY_SIZE(dsa_r50ohm_table))
> > + *dest = dsa_r50ohm_table[25];
> > + else
> > + *dest = dsa_r50ohm_table[i - 8];
> > +
> > + return 0;
> > +}
> > +
> > +static int an8855_probe(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + struct device_node *node = dev->of_node;
> > + struct air_an8855_priv *priv;
> > + int ret;
> > +
> > + /* If we don't have a node, skip get calib */
> > + if (!node)
> > + return 0;
>
> phydev->priv will be a NULL pointer, causing problems in
> an8855_config_init()
>
Quite unlikely scenario since for the switch, defining the internal PHY
in an MDIO node is mandatory but yes it's a fragility.
2 solution:
- I check priv in config_init and skip that section
- I always set phydev->priv
Solution 1 is safer (handle case where for some reason
en8855_get_r50ohm_val fails (it's really almost impossible)) but error
prone if the PHY gets extended with other parts and priv starts to gets
used for other thing.
Solution 2 require an extra bool to signal full calibrarion read and is
waste more resource (in case calib is not needed...)
Anyway thanks for the review!
--
Ansuel
