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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux