diff mbox series

[3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support

Message ID 20230524035135.90315-3-bingbu.cao@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio | expand

Commit Message

Bingbu Cao May 24, 2023, 3:51 a.m. UTC
From: Hao Yao <hao.yao@intel.com>

Add a new sensor support in INT3472 driver which module name is '09B13'.

Signed-off-by: Hao Yao <hao.yao@intel.com>
Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Hans de Goede May 25, 2023, 6:40 p.m. UTC | #1
Hi all,

On 5/24/23 05:51, bingbu.cao@intel.com wrote:
> From: Hao Yao <hao.yao@intel.com>
> 
> Add a new sensor support in INT3472 driver which module name is '09B13'.
> 
> Signed-off-by: Hao Yao <hao.yao@intel.com>
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> ---
>  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".

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.

Note that if your current sensor driver expects "vcc" that that is not a good reason to go with "vcc" here. It would be better to adjust the sensor driver to use the standard "avdd" and "dvdd" supply names (using the bulk regulator api), like all the other sensor drivers do.

Regards,

Hans
Daniel Scally May 31, 2023, 3:18 p.m. UTC | #2
Hi both

On 25/05/2023 19:40, Hans de Goede wrote:
> Hi all,
>
> On 5/24/23 05:51, bingbu.cao@intel.com wrote:
>> From: Hao Yao <hao.yao@intel.com>
>>
>> Add a new sensor support in INT3472 driver which module name is '09B13'.
>>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> ---
>>   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.


>
> Note that if your current sensor driver expects "vcc" that that is not a good reason to go with "vcc" here. It would be better to adjust the sensor driver to use the standard "avdd" and "dvdd" supply names (using the bulk regulator api), like all the other sensor drivers do.
>
> Regards,
>
> Hans
>
Hans de Goede May 31, 2023, 3:34 p.m. UTC | #3
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@intel.com wrote:
>>> From: Hao Yao <hao.yao@intel.com>
>>>
>>> Add a new sensor support in INT3472 driver which module name is '09B13'.
>>>
>>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>>> ---
>>>   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@ideasonboard.com/ and my remarks on that patch from you ?
Hans de Goede June 1, 2023, 8:33 a.m. UTC | #4
Hi Dan,

On 5/31/23 21:34, Dan Scally wrote:
> Hi Hans
> 
> On 31/05/2023 16:34, Hans de Goede wrote:

<snip>

>> 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.
> 
> I agree that this is ideal long term yes - I'll change the ov7251 to that effect.

Ack.

<snip>

>> 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).
> 
> 
> That would be nice. The ov2680 on a Miix510 is the sensor I was originally trying to get working, a very long time ago. I had a tree that "worked" in that you could stream with it but it was very ugly, as I had no idea what I was doing at the time and swiftly got the Surface instead and so moved on to that. The references to OV2680 in the CIO2/INT3472 code are hangovers from that early support really.
> 
>>
>> 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 ?
> 
> 
> Outside of libcamera is a case of configuring the formats and then grabbing frames from the CIO2's devnode, then queuing those frames to the IPU3's input devnode. The easiest way to do it is using the ipu3 utils within the libcamera project, which don't actually use the library itself but rather just configure with media-ctl and capture with yavta. See [1] and [2] for example. Testing within libcamera requires that the driver meet libcamera's requirements [3] which mostly means ensuring it has the 5 mandatory controls, plus some targets for the .get_selection() callback - I can't remember exactly which ones but I think it's CROP, NATIVE_SIZE, CROP_BOUNDS and CROP_DEFAULT. And I think it would also need an entry adding right at [4] to describe the sensor for libcamera. After that the cam tool from libcamera should list it with cam -l and capture from it with cam -cN --capture, or alternatively qcam would display the stream.
> 
> 
> [1] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-capture.sh
> 
> [2] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-process.sh
> 
> [3] https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/sensor_driver_requirements.rst
> 
> [4] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera_sensor_properties.cpp#n140

Thank you for the testing instructions. I will give this a try once I have all the recent atomisp
improvements ported to the main ov2680 driver and I have the main ov2680 driver working with the atomisp.

>> More p.s. :
>>
>> Dan what about: https://patchwork.kernel.org/project/platform-driver-x86/patch/20230311223019.14123-1-dan.scally@ideasonboard.com/ and my remarks on that patch from you ?
> 
> 
> That one completely fell off my radar - sorry about that. I can pick it up again in the morning.

Note I send a another reply in that thread yesterday with an alternative suggestion for fixing this (I would like to not have to add any GPIO remapping stuff in INT3472 when possible, IMHO the sensor drivers should be patched to use the right con-ids where possible).

Regards,

Hans
Daniel Scally June 1, 2023, 8:41 a.m. UTC | #5
Morning Hans

On 01/06/2023 09:33, Hans de Goede wrote:
> Hi Dan,
>
> On 5/31/23 21:34, Dan Scally wrote:
>> Hi Hans
>>
>> On 31/05/2023 16:34, Hans de Goede wrote:
> <snip>
>
>>> 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.
>> I agree that this is ideal long term yes - I'll change the ov7251 to that effect.
> Ack.
>
> <snip>
>
>>> 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).
>>
>> That would be nice. The ov2680 on a Miix510 is the sensor I was originally trying to get working, a very long time ago. I had a tree that "worked" in that you could stream with it but it was very ugly, as I had no idea what I was doing at the time and swiftly got the Surface instead and so moved on to that. The references to OV2680 in the CIO2/INT3472 code are hangovers from that early support really.
>>
>>> 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 ?
>>
>> Outside of libcamera is a case of configuring the formats and then grabbing frames from the CIO2's devnode, then queuing those frames to the IPU3's input devnode. The easiest way to do it is using the ipu3 utils within the libcamera project, which don't actually use the library itself but rather just configure with media-ctl and capture with yavta. See [1] and [2] for example. Testing within libcamera requires that the driver meet libcamera's requirements [3] which mostly means ensuring it has the 5 mandatory controls, plus some targets for the .get_selection() callback - I can't remember exactly which ones but I think it's CROP, NATIVE_SIZE, CROP_BOUNDS and CROP_DEFAULT. And I think it would also need an entry adding right at [4] to describe the sensor for libcamera. After that the cam tool from libcamera should list it with cam -l and capture from it with cam -cN --capture, or alternatively qcam would display the stream.
>>
>>
>> [1] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-capture.sh
>>
>> [2] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-process.sh
>>
>> [3] https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/sensor_driver_requirements.rst
>>
>> [4] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera_sensor_properties.cpp#n140
> Thank you for the testing instructions. I will give this a try once I have all the recent atomisp
> improvements ported to the main ov2680 driver and I have the main ov2680 driver working with the atomisp.
>
>>> More p.s. :
>>>
>>> Dan what about: https://patchwork.kernel.org/project/platform-driver-x86/patch/20230311223019.14123-1-dan.scally@ideasonboard.com/ and my remarks on that patch from you ?
>>
>> That one completely fell off my radar - sorry about that. I can pick it up again in the morning.
> Note I send a another reply in that thread yesterday with an alternative suggestion for fixing this (I would like to not have to add any GPIO remapping stuff in INT3472 when possible, IMHO the sensor drivers should be patched to use the right con-ids where possible).


Yep, I saw that and agree with it - I'll drop that patch and raise another for the sensor driver.

> Regards,
>
> Hans
>
>
diff mbox series

Patch

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 },
 };
 
 static const struct int3472_sensor_config *