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

[Hans de Goede <hdegoede@redhat.com>]

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@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.
> 
>> +               };
>