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
