Re: [net-next PATCH 05/19] net: dsa: qca8k: move read switch id function in qca8k_setup

[Ansuel Smith]

On Fri, Nov 19, 2021 at 03:03:05AM +0200, Vladimir Oltean wrote:
> On Wed, Nov 17, 2021 at 10:04:37PM +0100, Ansuel Smith wrote:
> > Move read_switch_id function in qca8k_setup in preparation for regmap
> > conversion. Sw probe should NOT contain function that depends on reading
> > from switch regs.
> 
> It shouldn't? Why? We have plenty of switch drivers that use regmap in
> the probe function.
>

The initial idea was to make a shared probe function. (when the ipq40xx
code will be proposed)
Currently the regmap is init in the setup function so we can both 
move the switch id to setup or move regmap to probe.
What should be better in your opinion?

> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > ---
> >  drivers/net/dsa/qca8k.c | 71 ++++++++++++++++++++---------------------
> >  1 file changed, 35 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 19331edf1fd4..be98d11b17ec 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -1073,6 +1073,36 @@ qca8k_parse_port_config(struct qca8k_priv *priv)
> >  	return 0;
> >  }
> >  
> > +static int qca8k_read_switch_id(struct qca8k_priv *priv)
> > +{
> > +	const struct qca8k_match_data *data;
> > +	u32 val;
> > +	u8 id;
> > +	int ret;
> > +
> > +	/* get the switches ID from the compatible */
> > +	data = of_device_get_match_data(priv->dev);
> > +	if (!data)
> > +		return -ENODEV;
> > +
> > +	ret = qca8k_read(priv, QCA8K_REG_MASK_CTRL, &val);
> > +	if (ret < 0)
> > +		return -ENODEV;
> > +
> > +	id = QCA8K_MASK_CTRL_DEVICE_ID(val);
> > +	if (id != data->id) {
> > +		dev_err(priv->dev, "Switch id detected %x but expected %x", id, data->id);
> > +		return -ENODEV;
> > +	}
> > +
> > +	priv->switch_id = id;
> > +
> > +	/* Save revision to communicate to the internal PHY driver */
> > +	priv->switch_revision = QCA8K_MASK_CTRL_REV_ID(val);
> > +
> > +	return 0;
> > +}
> > +
> >  static int
> >  qca8k_setup(struct dsa_switch *ds)
> >  {
> > @@ -1080,6 +1110,11 @@ qca8k_setup(struct dsa_switch *ds)
> >  	int cpu_port, ret, i;
> >  	u32 mask;
> >  
> > +	/* Check the detected switch id */
> > +	ret = qca8k_read_switch_id(priv);
> > +	if (ret)
> > +		return ret;
> > +
> >  	cpu_port = qca8k_find_cpu_port(ds);
> >  	if (cpu_port < 0) {
> >  		dev_err(priv->dev, "No cpu port configured in both cpu port0 and port6");
> > @@ -2023,41 +2058,10 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.get_phy_flags		= qca8k_get_phy_flags,
> >  };
> >  
> > -static int qca8k_read_switch_id(struct qca8k_priv *priv)
> > -{
> > -	const struct qca8k_match_data *data;
> > -	u32 val;
> > -	u8 id;
> > -	int ret;
> > -
> > -	/* get the switches ID from the compatible */
> > -	data = of_device_get_match_data(priv->dev);
> > -	if (!data)
> > -		return -ENODEV;
> > -
> > -	ret = qca8k_read(priv, QCA8K_REG_MASK_CTRL, &val);
> > -	if (ret < 0)
> > -		return -ENODEV;
> > -
> > -	id = QCA8K_MASK_CTRL_DEVICE_ID(val);
> > -	if (id != data->id) {
> > -		dev_err(priv->dev, "Switch id detected %x but expected %x", id, data->id);
> > -		return -ENODEV;
> > -	}
> > -
> > -	priv->switch_id = id;
> > -
> > -	/* Save revision to communicate to the internal PHY driver */
> > -	priv->switch_revision = QCA8K_MASK_CTRL_REV_ID(val);
> > -
> > -	return 0;
> > -}
> > -
> >  static int
> >  qca8k_sw_probe(struct mdio_device *mdiodev)
> >  {
> >  	struct qca8k_priv *priv;
> > -	int ret;
> >  
> >  	/* allocate the private data struct so that we can probe the switches
> >  	 * ID register
> > @@ -2083,11 +2087,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> >  		gpiod_set_value_cansleep(priv->reset_gpio, 0);
> >  	}
> >  
> > -	/* Check the detected switch id */
> > -	ret = qca8k_read_switch_id(priv);
> > -	if (ret)
> > -		return ret;
> > -
> >  	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
> >  	if (!priv->ds)
> >  		return -ENOMEM;
> > -- 
> > 2.32.0
> > 
> 

-- 
	Ansuel