Hi Dan,
On 5/31/23 17:18, Dan Scally wrote:
> Hi both
>
> On 25/05/2023 19:40, Hans de Goede wrote:
>> Hi all,
>>
>> On 5/24/23 05:51, bingbu.cao@xxxxxxxxx wrote:
>>> From: Hao Yao <hao.yao@xxxxxxxxx>
>>>
>>> Add a new sensor support in INT3472 driver which module name is '09B13'.
>>>
>>> Signed-off-by: Hao Yao <hao.yao@xxxxxxxxx>
>>> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>> ---
>>> drivers/platform/x86/intel/int3472/discrete.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>>> index d5d5c650d6d2..63acb48bf8df 100644
>>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>>> @@ -60,6 +60,8 @@ static const struct int3472_sensor_config int3472_sensor_configs[] = {
>>> { "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL },
>>> /* Surface Go 1&2 - OV5693, Front */
>>> { "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL },
>>> + /* OV13B10 */
>>> + { "09B13", REGULATOR_SUPPLY("vcc", NULL), NULL },
>> "vcc" is not really a useful supply name. All the existing sensor drivers in drivers/media/i2c typically check for both "avdd" and "dvdd". Can you verify if this is supplying digital or analog power using the schematics of the laptop ?
>>
>> And then use one of the standard "avdd" or "dvdd" supply names ?
>>
>> I would like to try and get rid of this table and sofar all the sensors which have a regulator GPIO are listed as sing it for "avdd" so I was hoping to be able to just always use "avdd".
>
>
> I agree this is quite unsatisfactory in its current form, but I'm hoping to add the ov7251 in the near future; the driver for which uses "vdda" instead, so unfortunately not in line with that.
>
>>
>> At least I would like us to come up with some default fallback for the supply-name in case a sensor-module is not listed in this table and "avdd" seems to be the best fallback.
>
>
> I wonder if it'd be better to simply support regulator_get() calls without a supply name in the event that the device only has a single supply associated with it? Then we needn't pick a default at all.
I do not believe that the regulator subsystem maintainers will accept such a change. They really only want to touch regulators with full constraints to avoid frying things and this would very much go against that.
I think what we need is for the sensor drivers to use standardized supply-names, so pick one of "avdd" or "vdda" and use that everywhere.
This will require some compat code in some of the sensor drivers to try the old supply name if the new standardized supply name fails (easy when using the bulk regulator API, one of the 2 will just become a dummy regulator), but I believe that in the long run this is the best solution.
Like how we also have all the sensor driver standardized on using "powerdown" and "reset" as GPIO con-ids.
Regards,
Hans
p.s.
Talking about this I just (minutes ago) finished writing a patch for the mainline ov2680 driver which allows dropping the sensor specific GPIO remapping in the int3472 driver for the ov2680, see the attached patch (compile tested only so far). The problem is not with the int3472 code, but that the original ov2680 driver is asking for a "reset" GPIO while the pin is named "XSHUTDN" in the datasheet.
Dan, the reason why I'm poking at the mainline ov2680 driver now is because I have the atomisp code at a point where it can work with standard v4l2 sensor drivers without any atomisp specific API between the atomisp code and the sensor driver. So I want to get rid of the atomisp-ov2680.ko private driver. This involves porting recent improvements like selection API (cropping) support from
atomisp-ov2680.c to the standard driver.
I was sorta hoping to also test this on a miix510, but upon checking I saw that the mainline ov2680.c does not yet work on the miix510. Porting my atomisp-ov2680.c changes should get us close to having the standard ov2680.c work on the miix510 (ACPI enumeration, runtime-pm and selection API support will all be added).
I have recently bought a second hand miix 510. Long story short: Can you give me some quick instructions, or a docs pointer for testing a new sensor with libcamera ? Or preferably I guess first outside of libcamera just grabbing raw-bayer + some userspace debayer tool for testing and then later on test under libcamera ?
More p.s. :
Dan what about: https://patchwork.kernel.org/project/platform-driver-x86/patch/20230311223019.14123-1-dan.scally@xxxxxxxxxxxxxxxx/ and my remarks on that patch from you ?
From f7a05e2fb19c3bb3bb704d5540746465cc53125d Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Wed, 31 May 2023 16:53:36 +0200
Subject: [PATCH] media: ov2680: Check for "powerdown" GPIO con-id before
checking for "reset" GPIO con-id
The datasheet of the OV2680 labels the single GPIO to put the sensor in
powersaving mode as XSHUTDN aka shutdown, _not_ reset.
This is important because some boards have standardized sensor connectors
which allow connecting various sensor modules. These connectors have both
reset and powerdown signals and the powerdown signal is routed to
the OV2680's XSHUTDN pin.
On x86/ACPI multiple Bay Trail, Cherry Trail, Sky Lake and Kaby Lake models
have an OV2680 connected to the ISP2 / IPU3. On these devices the GPIOS are
not described in DT instead the GPIOs are described with an Intel specific
DSM which labels them as either powerdown or reset. Often this DSM returns
both reset and powerdown pins even though the OV2680 has only 1 such pin.
For the ov2680 driver to work on these devices it must use the GPIO with
"powerdown" as con-id, matching the XSHUTDN name from the datasheet.
As for why "powerdown" vs say "shutdown" the ACPI DSM -> con-id mapping
code is shared, so we must use standardized names and currently all of
the following sensor drivers already use "powerdown":
adv7180, gc0310, isl7998x, ov02a10, ov2659, ov5640, ov5648, ov5670,
ov5693, ov7670, ov772x, ov7740, ov8858, ov8865 and ov9650 .
Where as the hi846 driver is the lonely standout using "shutdown".
Try the "powerdown" con-id first to make things work, falling back to
"reset" to keep existing DT setups working.
Cc: Dan Scally <dan.scally@xxxxxxxxxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/media/i2c/ov2680.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index b5283082f96d..e5564ef8f443 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -104,7 +104,7 @@ struct ov2680_dev {
u32 xvclk_freq;
struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES];
- struct gpio_desc *reset_gpio;
+ struct gpio_desc *pwdn_gpio;
struct mutex lock; /* protect members */
bool mode_pending_changes;
@@ -192,19 +192,19 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
static void ov2680_power_up(struct ov2680_dev *sensor)
{
- if (!sensor->reset_gpio)
+ if (!sensor->pwdn_gpio)
return;
- gpiod_set_value(sensor->reset_gpio, 0);
+ gpiod_set_value(sensor->pwdn_gpio, 0);
usleep_range(5000, 10000);
}
static void ov2680_power_down(struct ov2680_dev *sensor)
{
- if (!sensor->reset_gpio)
+ if (!sensor->pwdn_gpio)
return;
- gpiod_set_value(sensor->reset_gpio, 1);
+ gpiod_set_value(sensor->pwdn_gpio, 1);
usleep_range(5000, 10000);
}
@@ -441,7 +441,7 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
return ret;
}
- if (!sensor->reset_gpio) {
+ if (!sensor->pwdn_gpio) {
ret = cci_write(sensor->regmap, OV2680_REG_SOFT_RESET, 0x01, NULL);
if (ret != 0) {
dev_err(dev, "sensor soft reset failed\n");
@@ -917,9 +917,17 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
struct device *dev = ov2680_to_dev(sensor);
int ret;
- sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
- GPIOD_OUT_HIGH);
- ret = PTR_ERR_OR_ZERO(sensor->reset_gpio);
+ /*
+ * The pin we want is named XSHUTDN in the datasheet. Linux sensor
+ * drivers have standardized on using "powerdown" as con-id name
+ * for powerdown or shutdown pins. Older DTB files use "reset",
+ * so fallback to that if there is no "powerdown" pin.
+ */
+ sensor->pwdn_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH);
+ if (!sensor->pwdn_gpio)
+ sensor->pwdn_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+
+ ret = PTR_ERR_OR_ZERO(sensor->pwdn_gpio);
if (ret < 0) {
dev_dbg(dev, "error while getting reset gpio: %d\n", ret);
return ret;
--
2.40.1
