Re: [PATCH 2/2] dsa: mv88e6xxx: make serdes SGMII/Fiber output amplitude configurable

["Marek Behún" <kabel@kernel.org>]

On Fri, 26 Nov 2021 16:42:49 +0100
Holger Brunck <holger.brunck@hitachienergy.com> wrote:

> 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@lunn.ch>
> CC: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.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

>  
>  	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() ?

Marek