| Message ID | 20170717220116.17886-5-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Jul 18, 2017 at 01:01:13AM +0300, Sakari Ailus wrote: > From: Pavel Machek <pavel@ucw.cz> > > If regulator returns -EPROBE_DEFER, we need to return it too, so that > omap3isp will be re-probed when regulator is ready. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/platform/omap3isp/isp.c | 3 ++- > drivers/media/platform/omap3isp/ispccp2.c | 5 +++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c > index 80ed5a5f862a..4e6ba7f90e35 100644 > --- a/drivers/media/platform/omap3isp/isp.c > +++ b/drivers/media/platform/omap3isp/isp.c > @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device *isp) > > ret = omap3isp_ccp2_init(isp); > if (ret < 0) { > - dev_err(isp->dev, "CCP2 initialization failed\n"); > + if (ret != -EPROBE_DEFER) > + dev_err(isp->dev, "CCP2 initialization failed\n"); > goto error_ccp2; > } > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c > index 4f8fd0c00748..47210b102bcb 100644 > --- a/drivers/media/platform/omap3isp/ispccp2.c > +++ b/drivers/media/platform/omap3isp/ispccp2.c > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) > if (isp->revision == ISP_REVISION_2_0) { > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > if (IS_ERR(ccp2->vdds_csib)) { > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { > + dev_dbg(isp->dev, > + "Can't get regulator vdds_csib, deferring probing\n"); > + return -EPROBE_DEFER; > + } I wonder if the right approach wouldn't be to always bail out for errors. devm_regulator_get should provide a dummy regulator if none is specified. If we get an error here it means something is configured incorrectly or we have serious problems. > dev_dbg(isp->dev, > "Could not get regulator vdds_csib\n"); > ccp2->vdds_csib = NULL; -- Sebastian
Hi Sakari, Thank you for the patch. On Tuesday 18 Jul 2017 01:01:13 Sakari Ailus wrote: > From: Pavel Machek <pavel@ucw.cz> > > If regulator returns -EPROBE_DEFER, we need to return it too, so that > omap3isp will be re-probed when regulator is ready. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/platform/omap3isp/isp.c | 3 ++- > drivers/media/platform/omap3isp/ispccp2.c | 5 +++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/omap3isp/isp.c > b/drivers/media/platform/omap3isp/isp.c index 80ed5a5f862a..4e6ba7f90e35 > 100644 > --- a/drivers/media/platform/omap3isp/isp.c > +++ b/drivers/media/platform/omap3isp/isp.c > @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device > *isp) > > ret = omap3isp_ccp2_init(isp); > if (ret < 0) { > - dev_err(isp->dev, "CCP2 initialization failed\n"); > + if (ret != -EPROBE_DEFER) > + dev_err(isp->dev, "CCP2 initialization failed\n"); > goto error_ccp2; > } > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c > b/drivers/media/platform/omap3isp/ispccp2.c index > 4f8fd0c00748..47210b102bcb 100644 > --- a/drivers/media/platform/omap3isp/ispccp2.c > +++ b/drivers/media/platform/omap3isp/ispccp2.c > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) > if (isp->revision == ISP_REVISION_2_0) { > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > if (IS_ERR(ccp2->vdds_csib)) { > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { > + dev_dbg(isp->dev, > + "Can't get regulator vdds_csib, deferring probing\n"); > + return -EPROBE_DEFER; > + } > dev_dbg(isp->dev, > "Could not get regulator vdds_csib\n"); I would just move this message above the -EPROBE_DEFER check and remove the one inside the check. Probe deferral debug information can be obtained by enabling the debug messages in the driver core. > ccp2->vdds_csib = NULL;
Hi! > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c > > b/drivers/media/platform/omap3isp/ispccp2.c index > > 4f8fd0c00748..47210b102bcb 100644 > > --- a/drivers/media/platform/omap3isp/ispccp2.c > > +++ b/drivers/media/platform/omap3isp/ispccp2.c > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) > > if (isp->revision == ISP_REVISION_2_0) { > > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > > if (IS_ERR(ccp2->vdds_csib)) { > > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { > > + dev_dbg(isp->dev, > > + "Can't get regulator vdds_csib, > deferring probing\n"); > > + return -EPROBE_DEFER; > > + } > > dev_dbg(isp->dev, > > "Could not get regulator vdds_csib\n"); > > I would just move this message above the -EPROBE_DEFER check and remove the > one inside the check. Probe deferral debug information can be obtained by > enabling the debug messages in the driver core. Actually, in such case perhaps the message in -EPROBE_DEFER could be removed. Deferred probing happens all the time. OTOH "Could not get regulator" probably should be dev_err(), as it will make device unusable? Thanks, Pavel
Hi Pavel, On Tuesday 18 Jul 2017 12:03:52 Pavel Machek wrote: > Hi! > > >> diff --git a/drivers/media/platform/omap3isp/ispccp2.c > >> b/drivers/media/platform/omap3isp/ispccp2.c index > >> 4f8fd0c00748..47210b102bcb 100644 > >> --- a/drivers/media/platform/omap3isp/ispccp2.c > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c > >> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) > >> if (isp->revision == ISP_REVISION_2_0) { > >> ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > >> if (IS_ERR(ccp2->vdds_csib)) { > >> + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { > >> + dev_dbg(isp->dev, > >> + "Can't get regulator vdds_csib, > >> deferring probing\n"); > >> + return -EPROBE_DEFER; > >> + } > >> > >> dev_dbg(isp->dev, > >> "Could not get regulator vdds_csib\n"); > > > > I would just move this message above the -EPROBE_DEFER check and remove > > the one inside the check. Probe deferral debug information can be obtained > > by enabling the debug messages in the driver core. > > Actually, in such case perhaps the message in -EPROBE_DEFER could be > removed. Deferred probing happens all the time. OTOH "Could not get > regulator" probably should be dev_err(), as it will make device > unusable? Messages along the lines of "I'm deferring probe" are in my opinion not valuable, as we can get that from the driver core. Debug messages that tell why probe is being deferred can however be useful for debugging. That's why I proposed to move the regulator get error debug message above the probe deferral check, as it would then always be printed. Turning it into an error makes sense, but only when not deferring probe then.
Hi Pavel, On Tue, Jul 18, 2017 at 12:03:52PM +0200, Pavel Machek wrote: > Hi! > > > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c > > > b/drivers/media/platform/omap3isp/ispccp2.c index > > > 4f8fd0c00748..47210b102bcb 100644 > > > --- a/drivers/media/platform/omap3isp/ispccp2.c > > > +++ b/drivers/media/platform/omap3isp/ispccp2.c > > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) > > > if (isp->revision == ISP_REVISION_2_0) { > > > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > > > if (IS_ERR(ccp2->vdds_csib)) { > > > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { > > > + dev_dbg(isp->dev, > > > + "Can't get regulator vdds_csib, > > deferring probing\n"); > > > + return -EPROBE_DEFER; > > > + } > > > dev_dbg(isp->dev, > > > "Could not get regulator vdds_csib\n"); > > > > I would just move this message above the -EPROBE_DEFER check and remove the > > one inside the check. Probe deferral debug information can be obtained by > > enabling the debug messages in the driver core. > > Actually, in such case perhaps the message in -EPROBE_DEFER could be > removed. Deferred probing happens all the time. OTOH "Could not get > regulator" probably should be dev_err(), as it will make device > unusable? Isn't this only if you need ccp2?
Hi! > > > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c > > > > b/drivers/media/platform/omap3isp/ispccp2.c index > > > > 4f8fd0c00748..47210b102bcb 100644 > > > > --- a/drivers/media/platform/omap3isp/ispccp2.c > > > > +++ b/drivers/media/platform/omap3isp/ispccp2.c > > > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) > > > > if (isp->revision == ISP_REVISION_2_0) { > > > > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > > > > if (IS_ERR(ccp2->vdds_csib)) { > > > > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { > > > > + dev_dbg(isp->dev, > > > > + "Can't get regulator vdds_csib, > > > deferring probing\n"); > > > > + return -EPROBE_DEFER; > > > > + } > > > > dev_dbg(isp->dev, > > > > "Could not get regulator vdds_csib\n"); > > > > > > I would just move this message above the -EPROBE_DEFER check and remove the > > > one inside the check. Probe deferral debug information can be obtained by > > > enabling the debug messages in the driver core. > > > > Actually, in such case perhaps the message in -EPROBE_DEFER could be > > removed. Deferred probing happens all the time. OTOH "Could not get > > regulator" probably should be dev_err(), as it will make device > > unusable? > > Isn't this only if you need ccp2? No idea really. I only have N900 working with linux at the moment. I'm trying to get N9 and N950 working, but no luck so far. Pavel
On Tue, Jul 18, 2017 at 11:02:28PM +0200, Pavel Machek wrote: > Hi! > > > > > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c > > > > > b/drivers/media/platform/omap3isp/ispccp2.c index > > > > > 4f8fd0c00748..47210b102bcb 100644 > > > > > --- a/drivers/media/platform/omap3isp/ispccp2.c > > > > > +++ b/drivers/media/platform/omap3isp/ispccp2.c > > > > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) > > > > > if (isp->revision == ISP_REVISION_2_0) { > > > > > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > > > > > if (IS_ERR(ccp2->vdds_csib)) { > > > > > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { > > > > > + dev_dbg(isp->dev, > > > > > + "Can't get regulator vdds_csib, > > > > deferring probing\n"); > > > > > + return -EPROBE_DEFER; > > > > > + } > > > > > dev_dbg(isp->dev, > > > > > "Could not get regulator vdds_csib\n"); > > > > > > > > I would just move this message above the -EPROBE_DEFER check and remove the > > > > one inside the check. Probe deferral debug information can be obtained by > > > > enabling the debug messages in the driver core. > > > > > > Actually, in such case perhaps the message in -EPROBE_DEFER could be > > > removed. Deferred probing happens all the time. OTOH "Could not get > > > regulator" probably should be dev_err(), as it will make device > > > unusable? > > > > Isn't this only if you need ccp2? > > No idea really. I only have N900 working with linux at the moment. I'm > trying to get N9 and N950 working, but no luck so far. Still no? :-( Do you know if you get the kernel booting? Do you have access to the serial console? I might have seen the e-mail chain but I lost the track. What happens after the flasher has pushed the kernel to RAM and the boot starts? It's wonderful for debugging if something's wrong...
Hi! > > No idea really. I only have N900 working with linux at the moment. I'm > > trying to get N9 and N950 working, but no luck so far. > > Still no? :-( > > Do you know if you get the kernel booting? Do you have access to the serial > console? I might have seen the e-mail chain but I lost the track. What > happens after the flasher has pushed the kernel to RAM and the boot starts? > It's wonderful for debugging if something's wrong... Still no. No serial cable, unfortunately. Flasher seems to run the kernel, but I see no evidence new kernel started successfully. I was told display is not expected to work, and on USB I see bootloader disconnecting and that's it. If you had a kernel binary that works for you, and does something I can observe, that would be welcome :-). Best regards, Pavel
On Tue, Jul 18, 2017 at 11:27:12PM +0200, Pavel Machek wrote: > Hi! EHLO > > > > No idea really. I only have N900 working with linux at the moment. I'm > > > trying to get N9 and N950 working, but no luck so far. > > > > Still no? :-( > > > > Do you know if you get the kernel booting? Do you have access to the serial > > console? I might have seen the e-mail chain but I lost the track. What > > happens after the flasher has pushed the kernel to RAM and the boot starts? > > It's wonderful for debugging if something's wrong... > > Still no. No serial cable, unfortunately. Flasher seems to run the > kernel, but I see no evidence new kernel started successfully. I was > told display is not expected to work, and on USB I see bootloader > disconnecting and that's it. > > If you had a kernel binary that works for you, and does something I > can observe, that would be welcome :-). I put my .config I use for N9 here: <URL:http://www.retiisi.org.uk/v4l2/tmp/config.n9> The root filesystem is over NFS root with usbnet. You should see something like this in dmesg: [35792.056138] usb 2-2: new high-speed USB device number 58 using ehci-pci [35792.206238] usb 2-2: New USB device found, idVendor=0525, idProduct=a4a1 [35792.206247] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [35792.206252] usb 2-2: Product: Ethernet Gadget [35792.206257] usb 2-2: Manufacturer: Linux 4.13.0-rc1-00089-g4c341695f3b6 with musb-hdrc
Hi! > > > > No idea really. I only have N900 working with linux at the moment. I'm > > > > trying to get N9 and N950 working, but no luck so far. > > > > > > Still no? :-( > > > > > > Do you know if you get the kernel booting? Do you have access to the serial > > > console? I might have seen the e-mail chain but I lost the track. What > > > happens after the flasher has pushed the kernel to RAM and the boot starts? > > > It's wonderful for debugging if something's wrong... > > > > Still no. No serial cable, unfortunately. Flasher seems to run the > > kernel, but I see no evidence new kernel started successfully. I was > > told display is not expected to work, and on USB I see bootloader > > disconnecting and that's it. > > > > If you had a kernel binary that works for you, and does something I > > can observe, that would be welcome :-). > > I put my .config I use for N9 here: > > <URL:http://www.retiisi.org.uk/v4l2/tmp/config.n9> > > The root filesystem is over NFS root with usbnet. You should see something > like this in dmesg: > > [35792.056138] usb 2-2: new high-speed USB device number 58 using ehci-pci > [35792.206238] usb 2-2: New USB device found, idVendor=0525, idProduct=a4a1 > [35792.206247] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0 > [35792.206252] usb 2-2: Product: Ethernet Gadget > [35792.206257] usb 2-2: Manufacturer: Linux 4.13.0-rc1-00089-g4c341695f3b6 with musb-hdrc Could not get it to work, same result as usual: no response on the device, disconnect on USB and then quiet. Could I get actual zImage-dtb from you? What development options are enabled in your case? I was mostly using none -- sudo ../maemo/0xffff/src/0xFFFF -F "" -R 0 . Thanks, Pavel
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index 80ed5a5f862a..4e6ba7f90e35 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device *isp) ret = omap3isp_ccp2_init(isp); if (ret < 0) { - dev_err(isp->dev, "CCP2 initialization failed\n"); + if (ret != -EPROBE_DEFER) + dev_err(isp->dev, "CCP2 initialization failed\n"); goto error_ccp2; } diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c index 4f8fd0c00748..47210b102bcb 100644 --- a/drivers/media/platform/omap3isp/ispccp2.c +++ b/drivers/media/platform/omap3isp/ispccp2.c @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp) if (isp->revision == ISP_REVISION_2_0) { ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); if (IS_ERR(ccp2->vdds_csib)) { + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) { + dev_dbg(isp->dev, + "Can't get regulator vdds_csib, deferring probing\n"); + return -EPROBE_DEFER; + } dev_dbg(isp->dev, "Could not get regulator vdds_csib\n"); ccp2->vdds_csib = NULL;