[libcamera-devel] [PATCH v3 4/5] libcamera: properties: Provide a Devices camera property

[Kieran Bingham]

Hi Barnabás

Thank you for weighing in!

Quoting Barnabás Pőcze (2023-06-14 22:50:57)
> Hi
> 
> 
> 2023. június 13., kedd 19:10 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:
> 
> > Quoting Barnabás Pőcze (2023-06-13 16:48:39)
> > > Hi
> > >
> > >
> > > 2023. június 12., hétfő 18:28 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:
> > >
> > > > [...]
> > > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > > > > index cb55e0ed2283..6141942969f9 100644
> > > > > > --- a/src/libcamera/property_ids.yaml
> > > > > > +++ b/src/libcamera/property_ids.yaml
> > > > > > @@ -690,6 +690,13 @@ controls:
> > > > > >          that is twice that of the full resolution mode. This value will be valid
> > > > > >          after the configure method has returned successfully.
> > > > > >
> > > > > > +  - Devices:
> > > > > > +      type: int64_t
> > > > >
> > > > > I am wondering why the type is `int64_t`. POSIX only says `dev_t` is an integer type[0].
> > > > > Both glibc[1] and musl[2] define it as an unsigned (64-bit) integer (at least as far as I checked).
> > > >
> > > > Indeed every implementation I can see define it as a 64-bit, but the
> > > > kernel uses only 32 bits.
> > > >
> > > > This is a libcamera control limitation ultimately, as we don't have
> > > > unsigned integer controls. I'm not sure yet if adding would be helpful
> > > > or difficult. It might be worth trying out.
> > > > [...]
> > >
> > > Ahh, I wasn't aware that libcamera does not have uint controls. Thanks for the clarification.
> > 
> > Have you reviewed the patches enough to consider providing a set of
> > Reviewed-by tags perhaps?

How about this point. Do you conisder that you've looked through this
code enough to say you have reviewed it?

> > 
> > Do you have any thoughts or preferences on the control name? Laurent has
> > discussed the option between "Devices" and "SystemDevices" - I'm really
> > hoping someone will weigh in on which they prefer to be able to move
> > forwards and merge this series.
> > 
> > (I prefer Devices, so that makes it 1 vs 1)
> > [...]
> 
> Well, I can certainly weigh in, but I don't think you will be pleased, because
> my preference leans towards "SystemDevices". But I would even go as far as to

I'm afraid to say I am pleased! I don't mind favouring the least popular
option. That's just my personal preference, but if more people voice
their opinions it breaks an otherwise tie point ;-)

> suggest "{,Underlying}{Kernel,System}DeviceIDs" as potential candidates.
> Admittedly some of those are long, but consider that
> 
>  - most programs won't need this, and
>  - those that need will very likely only query it in a single place.
> 
> So I think even something like "UnderlyingKernelDeviceIDs" could work,
> and I think that name is quite descriptive.
> 
> But, of course, since this is likely a very rarely used control, I think both
> "Devices" and "SystemDevices" would work just fine.

I don't think I want to make the name overly long, but you did throw in
one option that I like more than SystemDevices

 KernelDevices

But now I have a third option to pick from - so I need more voters and
people to weigh in on their opinion ...

Maybe I won't be pleased after all :-) (Joking of course, I'm very happy
for the input - thanks!)

Anyway, as it stands, if no one else comments I'll update this with
SystemDevices to prepare for merge (Unless we raise the votes on
KernelDevices).

Regards
--
Kieran