DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: devel@driverdev.osuosl.org, Bingbu Cao <bingbu.cao@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Tian Shu Qiu <tian.shu.qiu@intel.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>
Subject: Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
Date: Wed, 20 May 2020 09:44:00 +0200
Message-ID: <20200520094400.5137e7f2@coco.lan> (raw)
In-Reply-To: <20200517103659.GS17578@paasikivi.fi.intel.com>

Hi Sakari,

Em Sun, 17 May 2020 13:36:59 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Thanks for the patch.

Thanks for reviewing it.

> 
> On Sat, May 16, 2020 at 12:43:39PM +0200, Mauro Carvalho Chehab wrote:
> > On devices without ACPI, or which ACPI is not prepared to
> > export sensor data via DT, we need a different probing
> > method.
> > 
> > This little driver adds initial support to probe the
> > sensors found on a Dell Latitude 7285.
> > 
> > For now, it just detects the hardware and use request_module()
> > to load a sensor driver.
> > 
> > In the specific case of this device, the ACPI DTST dable
> > describes 2 camera sensors for this module, but the
> > current upstream doesn't have yet drivers for such
> > sensors. So, this patch just detects the PMIC used on
> > this device and tries to load a sensor.
> > 
> > Once the sensor gets added, some additional code will
> > be needed to pass via platform_data other details, like
> > callbacks for PMIC's command to turn the sensor on/off
> > and other sensor-specific settings.
> > 
> > The idea of this patch was inspired on how the sensors
> > are probed by the staging atomisp driver.
> > 
> > The current result of this driver with the Dell
> > Latitude 7285 is:
> > 
> > 	ipu3_acpi i2c-INT3477:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK1, HID=INT3477
> > 	ipu3_acpi i2c-INT3477:00: Found DMI entry for 'Latitude 7285' with sensor INT3477
> > 	ipu3_acpi i2c-INT3477:00: Loading sensor module ov8858
> > 	ipu3_acpi i2c-OVTI9234:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK2, HID=OVTI9234
> > 	ipu3_acpi i2c-OVTI9234:00: Found DMI entry for 'Latitude 7285' with sensor OVTI9234
> > 	ipu3_acpi i2c-OVTI9234:00: Loading sensor module ov9234
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  

With regards to the approach this patch took: it is currently
incomplete: the I2C core doesn't currently allow to have two 
drivers for the same I2C address at the same bus. So, if we end by having
some ancillary drivers to help the I2C core to work with media devs,
we may need some changes at I2C core (or to use an I2C virtual mux).

> ...
> 
> > +/*
> > + * Should list known sensor devices found at DSDT table as "CAM0", "CAM1", ...
> > + *
> > + * The table below is probably incomplete. It came from the DSDT table found
> > + * at a Dell Latitude 7285 (Method HCID).
> > + */
> > +static const struct acpi_device_id ipu3_acpi_acpi_match[] = {
> > +	{"INT3471"},
> > +	{"INT33BE"},
> > +	{"INT3476"},
> > +	{"INT3477"},
> > +	{"INT3474"},
> > +	{"INT3473"},
> > +	{"INT3475"},
> > +	{"INT3478"},
> > +	{"INT3479"},
> > +	{"INT347A"},
> > +	{"INT347B"},
> > +	{"OVTI9234"},
> > +	{"OVTI9734"},
> > +	{"OVTI8856"},
> > +	{"OVTIF860"},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, ipu3_acpi_acpi_match);  
> 
> Instead of creating a new way to probe drivers on ACPI systems, please add
> the appropriate ACPI device IDs to the respective drivers. E.g.
> drivers/media/i2c/imx319.c implements this.

The ACPI code at imx319 is incomplete. I mean, it will only tell the I2C
core that the driver should be probed via ACPI, but it tells nothing
how to power up the device. It just assumes that the driver will work
using pm_runtime support.

It also doesn't tell how to get device-specific platform data from
the BIOS (with is machine-specific).

Also, at least in the case of the atomisp approach, a single code
to parse BIOS for devices with ISP2300/ISP2400/ISP2401/ISP2500 should
work with all sensors supported by those models.

Copying those inside all sensor drivers is probably a bad idea.
I mean, we should likely need a core support for parsing it, as
the code is the same for a given set of PCI IDs.

-

As the atomisp driver is now minimally working, my plan is to merge
it upstream, under staging.

Before going ahead and fixing other troubles there at atomisp,
I may try to port the needed ACPI bits from the atomisp-ov2880 
staging driver into the mainline one. This should be an interesting 
exercise to check what's missing there.

Even if the atomisp never gets out of staging, doing that will help
to identify what it would take for a sensor to be able to work with
more than one different ISP. As as result, we may design something
that will properly support ACPI at the media subsystem.

Thanks,
Mauro
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-16 10:43 Mauro Carvalho Chehab
2020-05-17 10:36 ` Sakari Ailus
2020-05-20  7:44   ` Mauro Carvalho Chehab [this message]
2020-05-20  8:26     ` Sakari Ailus
2020-05-20 11:18       ` Mauro Carvalho Chehab
2020-05-21  8:00         ` Andy Shevchenko
2020-05-22  9:57           ` Mauro Carvalho Chehab
2020-05-26 14:31             ` Heikki Krogerus
2020-07-01  1:16               ` Jordan Hand
2020-05-25  7:59           ` Heikki Krogerus

Reply instructions:

You may reply publicly 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=20200520094400.5137e7f2@coco.lan \
    --to=mchehab+huawei@kernel.org \
    --cc=bingbu.cao@intel.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-media@vger.kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=tian.shu.qiu@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

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/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 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


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