Re: [PATCH net-next 2/5] net: lan966x: add the basic lan966x driver

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 11/17/2021 10:52, Philipp Zabel wrote:
> 
> Hi Horatio,

Hi Phillip,

> 
> On Wed, 2021-11-17 at 10:18 +0100, Horatiu Vultur wrote:
> > +static int lan966x_reset_switch(struct lan966x *lan966x)
> > +{
> > +     struct reset_control *reset;
> > +     int val = 0;
> > +     int ret;
> > +
> > +     reset = devm_reset_control_get_shared(lan966x->dev, "switch");
> > +     if (IS_ERR(reset))
> > +             dev_warn(lan966x->dev, "Could not obtain switch reset: %ld\n",
> > +                      PTR_ERR(reset));
> > +     else
> > +             reset_control_reset(reset);
> 
> According to the device tree bindings, both resets are required.
> I'd expect this to return on error.
> Is there any chance of the device working with out the switch reset
> being triggered?

The only case that I see is if the bootloader triggers this switch
reset and then when bootloader starts the kernel and doesn't set back
the switch in reset. Is this a valid scenario or is a bug in the
bootloader?

> 
> > +
> > +     reset = devm_reset_control_get_shared(lan966x->dev, "phy");
> > +     if (IS_ERR(reset)) {
> > +             dev_warn(lan966x->dev, "Could not obtain phy reset: %ld\n",
> > +                      PTR_ERR(reset));
> > +     } else {
> > +             reset_control_reset(reset);
> > +     }
> 
> Same as above.
> Consider printing errors with %pe or dev_err_probe().
> 
> > +     lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan966x, SYS_RESET_CFG);
> > +     lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan966x, SYS_RAM_INIT);
> > +     ret = readx_poll_timeout(lan966x_ram_init, lan966x,
> > +                              val, (val & BIT(1)) == 0, READL_SLEEP_US,
> > +                              READL_TIMEOUT_US);
> > +     if (ret)
> > +             return ret;
> > +
> > +     lan_wr(SYS_RESET_CFG_CORE_ENA_SET(1), lan966x, SYS_RESET_CFG);
> > +
> > +     return 0;
> > +}
> > +
> > +static int lan966x_probe(struct platform_device *pdev)
> > +{
> > +     struct fwnode_handle *ports, *portnp;
> > +     struct lan966x *lan966x;
> > +     int err, i;
> > +
> > +     lan966x = devm_kzalloc(&pdev->dev, sizeof(*lan966x), GFP_KERNEL);
> > +     if (!lan966x)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, lan966x);
> > +     lan966x->dev = &pdev->dev;
> > +
> > +     ports = device_get_named_child_node(&pdev->dev, "ethernet-ports");
> > +     if (!ports) {
> > +             dev_err(&pdev->dev, "no ethernet-ports child not found\n");
> > +             err = -ENODEV;
> > +             goto out;
> 
> No need to goto as long as there's just a "return err;" after the out:
> label.

True, I will udate this.

> 
> > +     }
> > +
> > +     err = lan966x_create_targets(pdev, lan966x);
> > +     if (err)
> > +             goto out;
> > +
> > +     if (lan966x_reset_switch(lan966x)) {
> > +             err = -EINVAL;
> 
> This should propagate the error returned from lan966x_reset_switch()
> instead.

I will fix it in the next version.

> 
> > +             goto out;
> > +     }
> > +
> > +     i = 0;
> > +     fwnode_for_each_available_child_node(ports, portnp)
> > +             ++i;
> > +
> > +     lan966x->num_phys_ports = i;
> > +     lan966x->ports = devm_kcalloc(&pdev->dev, lan966x->num_phys_ports,
> > +                                   sizeof(struct lan966x_port *),
> > +                                   GFP_KERNEL);
> 
>         if (!lan966x->ports)
>                 return -ENOMEM;

Good catch.

> 
> regards
> Philipp

-- 
/Horatiu