Hi Laurent,
Thank you for reviewing the patches.
> 2022/08/01 1:18、Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>のメール:
>
> One more comment.
>
> On Sun, Jul 31, 2022 at 07:01:00PM +0300, Laurent Pinchart wrote:
>> Hi Hayama-san,
>>
>> Thank you for the patch.
>>
>> On Mon, Jul 04, 2022 at 11:56:30AM +0900, Takanari Hayama wrote:
>>> Add a support for enum property type to AtomicRequest.
>>>
>>> Signed-off-by: Takanari Hayama <taki@xxxxxxxxxx>
>>> ---
>>> tests/kmstest.py | 12 +++++++++++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/kmstest.py b/tests/kmstest.py
>>> index 11cc328b5b32..224c160e32fa 100755
>>> --- a/tests/kmstest.py
>>> +++ b/tests/kmstest.py
>>> @@ -269,8 +269,18 @@ class AtomicRequest(pykms.AtomicReq):
>>>
>>> min, max = prop.values
>>> v = min + int((max - min) * int(v[:-1]) / 100)
>>> - else:
>>> + elif v.isnumeric():
>>> v = int(v)
>>> + else:
>>> + prop = obj.get_prop(k)
>
> I've run this test on a kernel that doesn't support the blend mode
> property, and the prop.type access below raised an exception that isn't
> very nice to read. If that's fine with you, I'll add
>
> if not prop:
> raise RuntimeError(f'Property {k} not supported by object {obj}')
>
> here to make error messages more readable.
Right. That makes more sense.
>
>>> + if prop.type != pykms.PropertyType.Enum:
>>> + raise RuntimeError(f'Unsupported property type {prop.type} for value {v}')
>>> + for value, mode in prop.enums.items():
>>
>> I'd replace "mode" with "name" here. Apart from that,
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>
>> I'll change this when applying the patch.
Thank you.
>>
>>> + if mode == v:
>>> + v = value
>>> + break
>>> + else:
>>> + raise RuntimeError(f'Enum value with name "{v}" not found in property {k}')
>>>
>>> if not isinstance(v, int):
>>> raise RuntimeError(f'Unsupported value type {type(v)} for property {k}')
>
> --
> Regards,
>
> Laurent Pinchart
Cheers,
Takanari Hayama, Ph.D. <taki@xxxxxxxxxx>
IGEL Co., Ltd.
https://www.igel.co.jp/
