[libcamera-devel] [PATCH v4 1/7] libcamera: properties: Add model property

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Aug 14, 2020 at 12:37:16AM +0200, Niklas Söderlund wrote:
> The model property describes the camera model name.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> * Changes since v3
> - s/as ASCII/in ASCII/
> ---
>  src/libcamera/property_ids.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 74ad0195d6310367..d3b2c3fb7f540012 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -387,6 +387,13 @@ controls:
>                                |                    |
>                                +--------------------+
>  
> +  - Model:
> +      type: string
> +      description: |
> +        Model name of the camera sensor. The format of the string is free-form
> +        and should be encoded in ASCII. Example model names are 'ov5670',

"should" is only a recommendation, this needs to be a requirement.

> +        'imx219' or 'Logitech Webcam C930e'.

I'm sorry, but this is still inconsistent. The commit message says
"camera model name", here the documentation states "model name of the
camera sensor". "Logitech Webcam C930e" is not a camera sensor model.

This needs more documentation. Saying "it's free-form" and giving a few
examples isn't enough, we need text that clearly documents the property.
Examples are allowed, but may not replace the definition, only
complement it.

> +
>    - UnitCellSize:
>        type: Size
>        description: |

-- 
Regards,

Laurent Pinchart