[v3,18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation
Commit Message
Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from
the v4l2-async bound op so that an I2C-client will be instatiated for
the VCM.
Note unfortunately on atomisp the _DSM to get the VCM type sometimes
returns a VCM even though there is none. Since VCMs are typically only
used together with certain sensors, work around this by adding a vcm
field to atomisp_sensor_config and only check for a VCM when that is set.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
.../media/atomisp/pci/atomisp_csi2_bridge.c | 44 ++++++++++++++++++-
1 file changed, 42 insertions(+), 2 deletions(-)
Comments
On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
> Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from
> the v4l2-async bound op so that an I2C-client will be instatiated for
> the VCM.
>
> Note unfortunately on atomisp the _DSM to get the VCM type sometimes
> returns a VCM even though there is none. Since VCMs are typically only
> used together with certain sensors, work around this by adding a vcm
> field to atomisp_sensor_config and only check for a VCM when that is set.
...
> +static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev)
> +{
> + union acpi_object *obj;
> + char *vcm_type;
> +
> + obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0,
> + NULL, ACPI_TYPE_STRING);
> + if (!obj)
> + return NULL;
> +
> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
Where is the counterpart kfree()?
> + ACPI_FREE(obj);
> +
> + if (!vcm_type)
> + return NULL;
> +
> + string_lower(vcm_type, vcm_type);
> + return vcm_type;
> +}
Hi,
On 7/6/23 12:15, Andy Shevchenko wrote:
> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
>> Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from
>> the v4l2-async bound op so that an I2C-client will be instatiated for
>> the VCM.
>>
>> Note unfortunately on atomisp the _DSM to get the VCM type sometimes
>> returns a VCM even though there is none. Since VCMs are typically only
>> used together with certain sensors, work around this by adding a vcm
>> field to atomisp_sensor_config and only check for a VCM when that is set.
>
> ...
>
>> +static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev)
>> +{
>> + union acpi_object *obj;
>> + char *vcm_type;
>> +
>> + obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0,
>> + NULL, ACPI_TYPE_STRING);
>> + if (!obj)
>> + return NULL;
>> +
>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
>
> Where is the counterpart kfree()?
The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
code only creates those once and them leaves them in memory, even on
a rmmod. So this is deliberately leaked just like that the ipu_bridge
struct which contains all the swnode-s is deliberately leaked by
ipu-bridge.c
Regards,
Hans
>
>> + ACPI_FREE(obj);
>> +
>> + if (!vcm_type)
>> + return NULL;
>> +
>> + string_lower(vcm_type, vcm_type);
>> + return vcm_type;
>> +}
>
On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
> On 7/6/23 12:15, Andy Shevchenko wrote:
> > On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
> >> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
> >
> > Where is the counterpart kfree()?
>
> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
> code only creates those once and them leaves them in memory, even on
> a rmmod. So this is deliberately leaked just like that the ipu_bridge
> struct which contains all the swnode-s is deliberately leaked by
> ipu-bridge.c
Should we worry about those leakages?
Hi,
On 7/6/23 14:42, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
>> On 7/6/23 12:15, Andy Shevchenko wrote:
>>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
>
>>>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
>>>
>>> Where is the counterpart kfree()?
>>
>> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
>> code only creates those once and them leaves them in memory, even on
>> a rmmod. So this is deliberately leaked just like that the ipu_bridge
>> struct which contains all the swnode-s is deliberately leaked by
>> ipu-bridge.c
>
> Should we worry about those leakages?
No this is by design because removing the swnodes while e.g. a sensor
driver might still be bound to the i2c-client is trouble-some and
the callers of ipu_bridge_init check if it has already run and then
skip calling it.
So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver
ipu_bridge_init() will not get called a second time. Instead
the old swnodes (1) which are already set as secondary fwnodes for
the sensor and bridge devices are re-used.
Regards,
Hans
1) + the properties they contain
On Thu, Jul 06, 2023 at 02:47:36PM +0200, Hans de Goede wrote:
> On 7/6/23 14:42, Andy Shevchenko wrote:
> > On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
> >> On 7/6/23 12:15, Andy Shevchenko wrote:
> >>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
> >
> >>>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
> >>>
> >>> Where is the counterpart kfree()?
> >>
> >> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
> >> code only creates those once and them leaves them in memory, even on
> >> a rmmod. So this is deliberately leaked just like that the ipu_bridge
> >> struct which contains all the swnode-s is deliberately leaked by
> >> ipu-bridge.c
> >
> > Should we worry about those leakages?
>
> No this is by design because removing the swnodes while e.g. a sensor
> driver might still be bound to the i2c-client is trouble-some and
> the callers of ipu_bridge_init check if it has already run and then
> skip calling it.
>
> So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver
> ipu_bridge_init() will not get called a second time. Instead
> the old swnodes (1) which are already set as secondary fwnodes for
> the sensor and bridge devices are re-used.
But this will be actual leak if we hot unplug/plug back the device, right?
(I think we can do that in some [debug?] cases).
Whatever, it's out of scope of this series...
> 1) + the properties they contain
Hi,
On 7/6/23 14:56, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 02:47:36PM +0200, Hans de Goede wrote:
>> On 7/6/23 14:42, Andy Shevchenko wrote:
>>> On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
>>>> On 7/6/23 12:15, Andy Shevchenko wrote:
>>>>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
>>>
>>>>>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
>>>>>
>>>>> Where is the counterpart kfree()?
>>>>
>>>> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
>>>> code only creates those once and them leaves them in memory, even on
>>>> a rmmod. So this is deliberately leaked just like that the ipu_bridge
>>>> struct which contains all the swnode-s is deliberately leaked by
>>>> ipu-bridge.c
>>>
>>> Should we worry about those leakages?
>>
>> No this is by design because removing the swnodes while e.g. a sensor
>> driver might still be bound to the i2c-client is trouble-some and
>> the callers of ipu_bridge_init check if it has already run and then
>> skip calling it.
>>
>> So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver
>> ipu_bridge_init() will not get called a second time. Instead
>> the old swnodes (1) which are already set as secondary fwnodes for
>> the sensor and bridge devices are re-used.
>
> But this will be actual leak if we hot unplug/plug back the device, right?
> (I think we can do that in some [debug?] cases).
No, the kstrdup() is called from the parse_sensor_fwnode() callback passed
to ipu_bridge_init(), so it will only happen once even on unbind + re-bind
or rmmod + modprobe.
Regards,
Hans
@@ -66,15 +66,25 @@ static const guid_t atomisp_dsm_guid =
GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
0x97, 0xb9, 0x88, 0x2a, 0x68, 0x60, 0xa4, 0xbe);
+/*
+ * 75c9a639-5c8a-4a00-9f48-a9c3b5da789f
+ * This _DSM GUID returns a string giving the VCM type e.g. "AD5823".
+ */
+static const guid_t vcm_dsm_guid =
+ GUID_INIT(0x75c9a639, 0x5c8a, 0x4a00,
+ 0x9f, 0x48, 0xa9, 0xc3, 0xb5, 0xda, 0x78, 0x9f);
+
struct atomisp_sensor_config {
int lanes;
+ bool vcm;
};
-#define ATOMISP_SENSOR_CONFIG(_HID, _LANES) \
+#define ATOMISP_SENSOR_CONFIG(_HID, _LANES, _VCM) \
{ \
.id = _HID, \
.driver_data = (long)&((const struct atomisp_sensor_config) { \
.lanes = _LANES, \
+ .vcm = _VCM, \
}) \
}
@@ -490,8 +500,28 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
return ret;
}
+static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev)
+{
+ union acpi_object *obj;
+ char *vcm_type;
+
+ obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0,
+ NULL, ACPI_TYPE_STRING);
+ if (!obj)
+ return NULL;
+
+ vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
+ ACPI_FREE(obj);
+
+ if (!vcm_type)
+ return NULL;
+
+ string_lower(vcm_type, vcm_type);
+ return vcm_type;
+}
+
static const struct acpi_device_id atomisp_sensor_configs[] = {
- ATOMISP_SENSOR_CONFIG("INT33BE", 2), /* OV5693 */
+ ATOMISP_SENSOR_CONFIG("INT33BE", 2, true), /* OV5693 */
{}
};
@@ -500,6 +530,7 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
{
const struct acpi_device_id *id;
int ret, clock_num;
+ bool vcm = false;
int lanes = 1;
id = acpi_match_acpi_device(atomisp_sensor_configs, adev);
@@ -508,6 +539,7 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
(struct atomisp_sensor_config *)id->driver_data;
lanes = cfg->lanes;
+ vcm = cfg->vcm;
}
/*
@@ -545,6 +577,9 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
sensor->orientation = (sensor->link == 1) ?
V4L2_FWNODE_ORIENTATION_BACK : V4L2_FWNODE_ORIENTATION_FRONT;
+ if (vcm)
+ sensor->vcm_type = atomisp_csi2_get_vcm_type(adev);
+
return 0;
}
@@ -583,6 +618,7 @@ static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier,
{
struct atomisp_device *isp = notifier_to_atomisp(notifier);
struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
+ int ret;
if (s_asd->port >= ATOMISP_CAMERA_NR_PORTS) {
dev_err(isp->dev, "port %d not supported\n", s_asd->port);
@@ -594,6 +630,10 @@ static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier,
return -EBUSY;
}
+ ret = ipu_bridge_instantiate_vcm(sd->dev);
+ if (ret)
+ return ret;
+
isp->sensor_subdevs[s_asd->port] = sd;
return 0;
}