On Thu, May 18, 2023 at 6:38 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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
