Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-acpi@vger.kernel.org, "Mani,
	Rajmohan" <rajmohan.mani@intel.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>
Subject: Re: [PATCH 3/5] ov5670: Support probe whilst the device is off
Date: Mon, 26 Aug 2019 18:21:21 +0900
Message-ID: <CAAFQd5CL9asYPguLpP-sQJeuvzwMjjfED7z37OcYXqmyGZMyTg@mail.gmail.com> (raw)
In-Reply-To: <20190826083813.GK31967@paasikivi.fi.intel.com>

On Mon, Aug 26, 2019 at 5:38 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tomasz,
>
> On Fri, Jun 07, 2019 at 04:54:06PM +0900, Tomasz Figa wrote:
> > On Wed, Jun 5, 2019 at 7:15 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote:
> > > > Hi Sakari,
> > > >
> > > > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote:
> > > > > Tell ACPI device PM code that the driver supports the device being powered
> > > > > off when the driver's probe function is entered.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
> > > > >  1 file changed, 14 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > > > index 041fcbb4eebdf..57e8b92f90e09 100644
> > > > > --- a/drivers/media/i2c/ov5670.c
> > > > > +++ b/drivers/media/i2c/ov5670.c
> > > > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
> > > > >     struct ov5670 *ov5670;
> > > > >     const char *err_msg;
> > > > >     u32 input_clk = 0;
> > > > > +   bool powered_off;
> > > > >     int ret;
> > > > >
> > > > >     device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > > > > @@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
> > > > >     /* Initialize subdev */
> > > > >     v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> > > > >
> > > > > -   /* Check module identity */
> > > > > -   ret = ov5670_identify_module(ov5670);
> > > > > -   if (ret) {
> > > > > -           err_msg = "ov5670_identify_module() error";
> > > > > -           goto error_print;
> > > > > +   powered_off = acpi_dev_powered_off_for_probe(&client->dev);
> > > > > +   if (!powered_off) {
> > > > > +           /* Check module identity */
> > > > > +           ret = ov5670_identify_module(ov5670);
> > > > > +           if (ret) {
> > > > > +                   err_msg = "ov5670_identify_module() error";
> > > > > +                   goto error_print;
> > > > > +           }
> > > > >     }
> > > >
> > > > I don't like the fact that we can't detect any hardware connection issue
> > > > here anymore and we would actually get some obscure failure when we
> > > > actually start streaming.
> > > >
> > > > Wouldn't it be possible to still keep this behavior of not powering on
> > > > the device at boot-up if no driver is bound and then have this driver
> > > > built as a module and loaded later when the camera is to be used for the
> > > > first time after the system boots?
> > >
> > > That'd be a way to work around this, but the downside would be that the
> > > user space would need to know not only which drivers to load, but also
> > > which drivers _not_ to load. The user space could obtain the former from
> > > the kernel but not the latter, it'd be system specific configuration.
> > >
> > > Moving the responsibility of loading the driver to user space would also
> > > not address figuring out whether the sensor is accessible through its
> > > control bus: you have to power it on to do that. In fact, if you want to be
> > > sure that the hardware is all right, you have to start streaming on the
> > > device first and that is not a part of a typical driver initialisation
> > > sequence. Just checking the sensor is accessible over I涎 is not enough.
> > >
> > > The proposed solution addresses the problem without user space changes.
> >
> > I guess that makes sense indeed. If going this way, why not just move
> > all the hardware access from probe to streamon and avoid any
> > conditional checks at all?
>
> My apologies for the late answer.
>
> In that case there would be no way to verify the hardware actually is
> there, even on systems where there is no adverse effect from doing that.
> For a sensor driver this could be just fine, but I have doubts it'd be
> appropriate for e.g. the at24 driver.

For an eeprom, the first read could fail. That said, I agree that for
systems on which there is no such concern it would still be desirable
to check if the device is accessible in probe.

Anyway, I think you convinced me. :)

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-10 10:09 [PATCH 0/5] Support running driver's probe for a device powered off Sakari Ailus
2019-05-10 10:09 ` [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time Sakari Ailus
2019-05-31  9:17   ` Rafael J. Wysocki
2019-06-05 12:07     ` Sakari Ailus
2019-05-10 10:09 ` [PATCH 2/5] ACPI: Add a convenience function to tell a device is suspended in probe Sakari Ailus
2019-05-10 10:09 ` [PATCH 3/5] ov5670: Support probe whilst the device is off Sakari Ailus
2019-06-05  7:07   ` Tomasz Figa
2019-06-05 10:15     ` Sakari Ailus
2019-06-07  7:54       ` Tomasz Figa
2019-08-26  8:38         ` Sakari Ailus
2019-08-26  9:21           ` Tomasz Figa [this message]
2019-05-10 10:09 ` [PATCH 4/5] media: i2c: imx319: Support probe while " Sakari Ailus
2019-05-10 10:09 ` [PATCH 5/5] at24: Support probing while off Sakari Ailus

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAAFQd5CL9asYPguLpP-sQJeuvzwMjjfED7z37OcYXqmyGZMyTg@mail.gmail.com \
    --to=tfiga@chromium.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=rajmohan.mani@intel.com \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git