Re: [PATCH 2/2] vfio: platform: Add generic DT reset support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

On Thu, 2018-02-22 at 09:50 +0100, Geert Uytterhoeven wrote:
[...]
> > > @@ -127,8 +134,15 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
> > >               vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
> > >                                                       &vdev->reset_module);
> > >       }
> > > +     if (vdev->of_reset)
> > > +             return 0;
> > > +
> > > +     vdev->reset_control = __of_reset_control_get(vdev->device->of_node,
> > > +                                                  NULL, 0, false, false);
> > > +     if (!IS_ERR(vdev->reset_control))
> > > +             return 0;
> > 
> > if you assign to a local variable first here:
> > 
> >         struct reset_control *rstc;
> > 
> >          ...
> > 
> >         rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
> >         if (!IS_ERR(rstc)) {
> >                 vdev->reset_control = rstc;
> >                 return 0;
> >         }
> > 
> > Also, please don't use __of_reset_control_get directly.
> 
> OK, apparently I didn't read <linux/reset.h> beyond the first #else...

I don't blame you, this is missing some documentation.

> > > @@ -217,6 +233,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
> > >       } else if (vdev->of_reset) {
> > >               dev_info(vdev->device, "reset\n");
> > >               return vdev->of_reset(vdev);
> > > +     } else if (!IS_ERR_OR_NULL(vdev->reset_control)) {
> > > +             dev_info(vdev->device, "reset\n");
> > > +             return reset_control_reset(vdev->reset_control);
> > 
> >         } else {
> >                 if (vdev->reset_control)
> >                         dev_info(vdev->device, "reset\n");
> >                 return reset_control_reset(vdev->reset_control);
> > 
> > >       }
> 
> I'd like to keep the "else if", as that's the pattern used by the blocks above.

my bad, there's more code after this.

	} else if (vdev->reset_control) {
		dev_info(vdev->device, "reset\n");
		return reset_control_reset(vdev->reset_control);
	}

is better as it doesn't lose the warning if vdev->reset_control == NULL.
It seems I've focused too much on getting rid of the IS_ERR_OR_NULL
macro.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux