Hi,
On 5/26/23 22:30, Andy Shevchenko wrote:
> On Thu, May 25, 2023 at 10:01 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Add support for using v4l2-async sensor registration.
>>
>> This has been tested with both the gc0310 and the ov2680 sensor drivers.
>>
>> Drivers must add the ACPI HIDs they match on to the supported_sensors[]
>> array in the same commit as that they are converted to
>> v4l2_async_register_subdev_sensor().
>>
>> Sensor drivers also must check they have a fwnode graph endpoint and return
>> -EPROBE_DEFER from probe() if there is no endpoint yet. This guarantees
>> that the GPIO mappingss are in place before the driver tries to get GPIOs.
>
> mappings
>
>> For now it also is still possible to use the old atomisp_gmin_platform
>> based sensor drivers. This is mainly intended for testing while moving
>> other sensor drivers over to runtime-pm + v4l2-async.
>
> ...
>
>> +struct acpi_device;
>> struct atomisp_device;
>> -struct v4l2_device;
>> struct atomisp_sub_device;
>> +struct v4l2_device;
>
> I would group atomisp* separately
>
> struct acpi_device;
> struct v4l2_device;
>
> struct atomisp_device;
> struct atomisp_sub_device;
>
> ...
>
>> +struct atomisp_csi2_bridge {
>> + char csi2_node_name[14];
>> + struct software_node csi2_node;
>
> Wondering if swapping these two saves some code (due to potential use
> of container_of() for the node member).
>
>> + u32 data_lanes[CSI2_MAX_LANES];
>> + unsigned int n_sensors;
>> + struct atomisp_csi2_sensor sensors[ATOMISP_CAMERA_NR_PORTS];
>> +};
>
> ...
>
>> +static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key)
>> +{
>> + union acpi_object *obj, *key_el, *val_el;
>> + char *val = NULL;
>> + int i;
>> +
>> + obj = acpi_evaluate_dsm_typed(adev->handle, &atomisp_dsm_guid, 0, 0,
>> + NULL, ACPI_TYPE_PACKAGE);
>> + if (!obj)
>> + return NULL;
>> +
>> + for (i = 0; i < obj->package.count - 1; i += 2) {
>> + key_el = &obj->package.elements[i + 0];
>> + val_el = &obj->package.elements[i + 1];
>> +
>> + if (key_el->type != ACPI_TYPE_STRING || val_el->type != ACPI_TYPE_STRING)
>> + break;
>> +
>> + if (!strcmp(key_el->string.pointer, key)) {
>> + val = kstrdup(val_el->string.pointer, GFP_KERNEL);
>
>> + dev_info(&adev->dev, "Using DSM entry %s=%s\n", key, val);
>
> Do we really want to have "(null)" to be printed in case of the
> kstrdup() failure?
> Also this code may become a honeypot for all possible static analyzers
> and even if we don't care about NULL it may become noisy activity.
Ok, I'll change this to:
val = kstrdup(val_el->string.pointer, GFP_KERNEL);
if (!val)
break;
> Besides that since we have a handle, wouldn't it be better to use
> acpi_handle_info() here?
Yes since we are purely dealing with ACPI / fwnode stuff here using
that would make more sense. I'll switch to that.
I also agree with all your other remarks and I'll fix them all up
(and test things again) before merging.
Andy, may I add your Reviewed-by to this patch too after fixing
up all your remarks ?
Regards,
Hans
