Hi Andy,
As always thank you for the review. I know you spend a lot of time
on reviews and it is much appreciated!
I agree with all your comments and I'll address them as suggested
for the next version.
Regards,
Hans
On 5/19/23 14:45, Andy Shevchenko wrote:
> 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.
>
>> + };
>
