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

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

The 11/18/2021 11:19, Philipp Zabel wrote:
> 
> Hi Horatiu,
> 
> On Wed, 2021-11-17 at 22:42 +0100, Horatiu Vultur wrote:
> > > 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?
> 
> I'm not sure. In general, the kernel shouldn't rely on the bootloader to
> have put the devices into a certain working state. If the driver will
> not work or worse, if register access could hang the system if the
> bootloader has passed control to the kernel with the switch held in
> reset and no reset control is available to the driver, it should not
> continue after failure to get the reset handle.
> 
> I'd suggest to just use:
> 
>         reset = devm_reset_control_get_shared(lan966x->dev, "switch");
>         if (IS_ERR(reset))
>                 return dev_err_probe(lan966x->dev, PTR_ERR(reset),
>                                      "Could not obtain switch reset");
>         reset_control_reset(reset);
> 
> unless you have a good reason to do otherwise.

I agree with you. I will do like you suggested in the next version.

> 
> regards
> Philipp

-- 
/Horatiu