netdev - RE: [PATCH 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <AM0PR0602MB3666A56A4BB143F3BB45346FF7669@AM0PR0602MB3666.eurprd06.prod.outlook.com>
Date:   Mon, 29 Nov 2021 11:17:48 +0000
From:   Holger Brunck <holger.brunck@...achienergy.com>
To:     Marek Behún <kabel@...nel.org>,
        Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: RE: [PATCH 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output
 amplitude configurable

> > The mv88e6352, mv88e6240 and mv88e6176  have a serdes interface. This
> > patch allows to configure the output swing to a desired value in the
> > devicetree node of the switch.
> >
> > CC: Andrew Lunn <andrew@...n.ch>
> > CC: Jakub Kicinski <kuba@...nel.org>
> > Signed-off-by: Holger Brunck <holger.brunck@...achienergy.com>
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c   | 14 ++++++++++++++
> >  drivers/net/dsa/mv88e6xxx/chip.h   |  3 +++
> >  drivers/net/dsa/mv88e6xxx/serdes.c | 14 ++++++++++++++
> > drivers/net/dsa/mv88e6xxx/serdes.h |  4 ++++
> >  4 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > b/drivers/net/dsa/mv88e6xxx/chip.c
> > index f00cbf5753b9..5182128959a0 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -3173,9 +3173,11 @@ static void mv88e6xxx_teardown(struct
> > dsa_switch *ds)  static int mv88e6xxx_setup(struct dsa_switch *ds)  {
> >       struct mv88e6xxx_chip *chip = ds->priv;
> > +     struct device_node *np = chip->dev->of_node;
> >       u8 cmode;
> >       int err;
> >       int i;
> > +     int out_amp;
> 
> Reverse christmas tree please, where possible.
> 
>         struct mv88e6xxx_chip *chip = ds->priv;
> +       struct device_node *np = chip->dev->of_node;
> +       int out_amp;
>         u8 cmode;
>         int err;
>         int
> 

ok.

> >
> >       chip->ds = ds;
> >       ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip); @@ -3292,6
> > +3294,15 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> >       if (err)
> >               goto unlock;
> >
> > +     if (chip->info->ops->serdes_set_out_amplitude && np) {
> > +             if (!of_property_read_u32(np, "serdes-output-amplitude",
> 
> Hmm. Andrew, why don't we use <linux/property.h> instead of <linux/of*.h>
> stuff in this dirver? Is there a reason or is this just because it wasn't converted
> yet?
> 
> A simple device_property_read_u32() would be better here and we wouldn't
> need the np pointer.
> 
> ...
> 
> > +int mv88e6352_serdes_set_out_amplitude(struct mv88e6xxx_chip *chip,
> > +int val) {
> > +     u16 reg;
> > +     int err;
> > +
> > +     err = mv88e6352_serdes_read(chip, MV88E6352_SERDES_SPEC_CTRL2,
> &reg);
> > +     if (err)
> > +             return err;
> > +
> > +     reg = (reg & MV88E6352_SERDES_OUT_AMP_MASK) | val;
> > +
> > +     return mv88e6352_serdes_write(chip, MV88E6352_SERDES_SPEC_CTRL2,
> > +reg); }
> 
> Is there a reason why we call this from driver probe instead of 6352's
> serdes_power() ?
> 

serdes_power is always called for enable and disable. It should be better to configure
it only once. But I agree that it should be moved from chip_probe to port_setup.

Best regards
Holger