Re: [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration

Andy Shevchenko
2023-05-19T12:46:32Z

On Thu, May 18, 2023 at 6:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> +static const guid_t atomisp_dsm_guid = GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
> + 0x97, 0xb9, 0x88, 0x2a,
> + 0x68, 0x60, 0xa4, 0xbe);

Can we use the de facto pattern for this kind of assignments?

... guid_t foo =
<TAB>GUID_INIT(...first 3 parameters...
<TAB><TAB>[spaces if needed)...last 8 parameters...);
?

Also would be nice to have a comment where the GUID is represented in
text format so it can be easily googled/searched for in
internet/documentation.

...

> + for (i = 0; i < obj->package.count - 1; i += 2) {
> + /* The package should only contain strings */

> + if (obj->package.elements[i].type != ACPI_TYPE_STRING ||

i + 0 ?

> + obj->package.elements[i + 1].type != ACPI_TYPE_STRING)
> + break;
> +
> + if (!strcmp(obj->package.elements[i].string.pointer, key)) {

Ditto?

> + val = kstrdup(obj->package.elements[i + 1].string.pointer, GFP_KERNEL);
> + dev_info(&adev->dev, "Using DSM entry %s=%s\n", key, val);
> + break;
> + }

I would even go for temporary for element pointer

... *elem0 = &[i + 0];
... *elem1 = &[i + 1];

> + }

...

> +use_default:

out_use_default:

...

> + status = acpi_evaluate_object(adev->handle, "_PR0", NULL, &buffer);

acpi_evaluate_object_typed()

> + if (!ACPI_SUCCESS(status))
> + return -ENOENT;

...

> + if (!buffer.length || !package || package->type != ACPI_TYPE_PACKAGE)

See above.

> + goto fail;

...

> + if (strlen(name) == 4 && !strncmp(name, "CLK", 3) &&

strlen() assumes that name is NUL-terminated, hence it can be simply
replaced with name[5] == '\0' check which can go at the end of
conditional, so that it's also implied in strncmp() for the start of
the string, but why not using str_has_prefix()?

> + name[3] >= '0' && name[3] <= '4') {

It's also possible to have it done via kstrtou8() that does almost all
checks along with conversion. You will only need to check for > 4.

> + clock_num = name[3] - '0';
> + break;
> + }
> + }

Altogether

if (str_has_prefix()) {
ret = kstrto...(&clock_num);
if (ret)
...
check for clock_num range if needed.
}

Yes it's longer in code.

...

> +fail:

err_free_pointer:
(It will be also in align with the rest of the code AFAICS)

> + ACPI_FREE(buffer.pointer);
> +
> + return clock_num;

...

> + /* Intel DSM or DMI quirk overrides PR0 derived default */
> + port = gmin_cfg_get_int(adev, "CsiPort", port);
> +
> + return port;

return gmin_...;

...

> + if (dev->fwnode && dev->fwnode->secondary)

Please, use dev_fwnode() instead of direct access to the fwnode in
struct device.

> + return 0;

...

> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY

I would add a trailing comma here.

> + };

--
With Best Regards,
Andy Shevchenko