| Message ID | 20200526032635.13599-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Tue, 2020-05-26 at 06:26 +0300, Laurent Pinchart wrote: > The preferred coding style in libcamera is to declare private > functions > before private variables in class definitions. This rule isn't > followed > by some of the internal classes. Update them accordingly. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../internal/device_enumerator_udev.h | 16 ++++++------ > .../internal/event_dispatcher_poll.h | 12 ++++----- > include/libcamera/internal/ipa_manager.h | 4 +-- > include/libcamera/internal/ipa_module.h | 4 +-- > include/libcamera/internal/media_device.h | 26 +++++++++------ > ---- > include/libcamera/internal/media_object.h | 4 +-- > 6 files changed, 33 insertions(+), 33 deletions(-) > I spotted one more place that is not included in this patch, which is : include/libcamera/ipa_context_wrapper.h Apart from that: Reviewed-by: Umang Jain <email@uajain.com> > diff --git a/include/libcamera/internal/device_enumerator_udev.h > b/include/libcamera/internal/device_enumerator_udev.h > index fdaa20968ef0..10d17ed2abac 100644 > --- a/include/libcamera/internal/device_enumerator_udev.h > +++ b/include/libcamera/internal/device_enumerator_udev.h > @@ -36,10 +36,6 @@ public: > int enumerate() final; > > private: > - struct udev *udev_; > - struct udev_monitor *monitor_; > - EventNotifier *notifier_; > - > using DependencyMap = std::map<dev_t, std::list<MediaEntity > *>>; > > struct MediaDeviceDeps { > @@ -58,16 +54,20 @@ private: > DependencyMap deps_; > }; > > - std::set<dev_t> orphans_; > - std::list<MediaDeviceDeps> pending_; > - std::map<dev_t, MediaDeviceDeps *> devMap_; > - > int addUdevDevice(struct udev_device *dev); > int populateMediaDevice(MediaDevice *media, DependencyMap > *deps); > std::string lookupDeviceNode(dev_t devnum); > > int addV4L2Device(dev_t devnum); > void udevNotify(EventNotifier *notifier); > + > + struct udev *udev_; > + struct udev_monitor *monitor_; > + EventNotifier *notifier_; > + > + std::set<dev_t> orphans_; > + std::list<MediaDeviceDeps> pending_; > + std::map<dev_t, MediaDeviceDeps *> devMap_; > }; > > } /* namespace libcamera */ > diff --git a/include/libcamera/internal/event_dispatcher_poll.h > b/include/libcamera/internal/event_dispatcher_poll.h > index 1f0738617425..3c9099660c4d 100644 > --- a/include/libcamera/internal/event_dispatcher_poll.h > +++ b/include/libcamera/internal/event_dispatcher_poll.h > @@ -41,16 +41,16 @@ private: > EventNotifier *notifiers[3]; > }; > > - std::map<int, EventNotifierSetPoll> notifiers_; > - std::list<Timer *> timers_; > - int eventfd_; > - > - bool processingEvents_; > - > int poll(std::vector<struct pollfd> *pollfds); > void processInterrupt(const struct pollfd &pfd); > void processNotifiers(const std::vector<struct pollfd> > &pollfds); > void processTimers(); > + > + std::map<int, EventNotifierSetPoll> notifiers_; > + std::list<Timer *> timers_; > + int eventfd_; > + > + bool processingEvents_; > }; > > } /* namespace libcamera */ > diff --git a/include/libcamera/internal/ipa_manager.h > b/include/libcamera/internal/ipa_manager.h > index 2412d75746ac..16d742918cf2 100644 > --- a/include/libcamera/internal/ipa_manager.h > +++ b/include/libcamera/internal/ipa_manager.h > @@ -29,8 +29,6 @@ public: > uint32_t minVersion); > > private: > - std::vector<IPAModule *> modules_; > - > IPAManager(); > ~IPAManager(); > > @@ -40,6 +38,8 @@ private: > > bool isSignatureValid(IPAModule *ipa) const; > > + std::vector<IPAModule *> modules_; > + > #if HAVE_IPA_PUBKEY > static const uint8_t publicKeyData_[]; > static const PubKey pubKey_; > diff --git a/include/libcamera/internal/ipa_module.h > b/include/libcamera/internal/ipa_module.h > index 5b54cb31a48a..788e31d8bf03 100644 > --- a/include/libcamera/internal/ipa_module.h > +++ b/include/libcamera/internal/ipa_module.h > @@ -42,6 +42,8 @@ protected: > std::string logPrefix() const override; > > private: > + int loadIPAModuleInfo(); > + > struct IPAModuleInfo info_; > std::vector<uint8_t> signature_; > > @@ -52,8 +54,6 @@ private: > void *dlHandle_; > typedef struct ipa_context *(*IPAIntfFactory)(void); > IPAIntfFactory ipaCreate_; > - > - int loadIPAModuleInfo(); > }; > > } /* namespace libcamera */ > diff --git a/include/libcamera/internal/media_device.h > b/include/libcamera/internal/media_device.h > index 9fe76c514b17..19af059d9291 100644 > --- a/include/libcamera/internal/media_device.h > +++ b/include/libcamera/internal/media_device.h > @@ -58,26 +58,13 @@ protected: > std::string logPrefix() const; > > private: > - std::string driver_; > - std::string deviceNode_; > - std::string model_; > - unsigned int version_; > - > - int fd_; > - bool valid_; > - bool acquired_; > - bool lockOwner_; > - > int open(); > void close(); > > - std::map<unsigned int, MediaObject *> objects_; > MediaObject *object(unsigned int id); > bool addObject(MediaObject *object); > void clear(); > > - std::vector<MediaEntity *> entities_; > - > struct media_v2_interface *findInterface(const struct > media_v2_topology &topology, > unsigned int > entityId); > bool populateEntities(const struct media_v2_topology > &topology); > @@ -87,6 +74,19 @@ private: > > friend int MediaLink::setEnabled(bool enable); > int setupLink(const MediaLink *link, unsigned int flags); > + > + std::string driver_; > + std::string deviceNode_; > + std::string model_; > + unsigned int version_; > + > + int fd_; > + bool valid_; > + bool acquired_; > + bool lockOwner_; > + > + std::map<unsigned int, MediaObject *> objects_; > + std::vector<MediaEntity *> entities_; > }; > > } /* namespace libcamera */ > diff --git a/include/libcamera/internal/media_object.h > b/include/libcamera/internal/media_object.h > index 748eafdc880b..e8f2f27ce99e 100644 > --- a/include/libcamera/internal/media_object.h > +++ b/include/libcamera/internal/media_object.h > @@ -107,6 +107,8 @@ private: > MediaEntity(const MediaEntity &) = delete; > ~MediaEntity(); > > + void addPad(MediaPad *pad); > + > std::string name_; > unsigned int function_; > unsigned int flags_; > @@ -115,8 +117,6 @@ private: > unsigned int minor_; > > std::vector<MediaPad *> pads_; > - > - void addPad(MediaPad *pad); > }; > > } /* namespace libcamera */
On Tue, 2020-05-26 at 05:24 +0000, Umang Jain wrote: > Hi Laurent, > > On Tue, 2020-05-26 at 06:26 +0300, Laurent Pinchart wrote: > > The preferred coding style in libcamera is to declare private > > functions > > before private variables in class definitions. This rule isn't > > followed > > by some of the internal classes. Update them accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../internal/device_enumerator_udev.h | 16 ++++++------ > > .../internal/event_dispatcher_poll.h | 12 ++++----- > > include/libcamera/internal/ipa_manager.h | 4 +-- > > include/libcamera/internal/ipa_module.h | 4 +-- > > include/libcamera/internal/media_device.h | 26 +++++++++------ > > ---- > > include/libcamera/internal/media_object.h | 4 +-- > > 6 files changed, 33 insertions(+), 33 deletions(-) > > > I spotted one more place that is not included in this patch, which > is : include/libcamera/ipa_context_wrapper.h Apologies, the correct path is include/libcamera/internal/ipa_context_wrapper.h > > Apart from that: > Reviewed-by: Umang Jain <email@uajain.com> > > diff --git a/include/libcamera/internal/device_enumerator_udev.h > > b/include/libcamera/internal/device_enumerator_udev.h > > index fdaa20968ef0..10d17ed2abac 100644 > > --- a/include/libcamera/internal/device_enumerator_udev.h > > +++ b/include/libcamera/internal/device_enumerator_udev.h > > @@ -36,10 +36,6 @@ public: > > int enumerate() final; > > > > private: > > - struct udev *udev_; > > - struct udev_monitor *monitor_; > > - EventNotifier *notifier_; > > - > > using DependencyMap = std::map<dev_t, std::list<MediaEntity > > *>>; > > > > struct MediaDeviceDeps { > > @@ -58,16 +54,20 @@ private: > > DependencyMap deps_; > > }; > > > > - std::set<dev_t> orphans_; > > - std::list<MediaDeviceDeps> pending_; > > - std::map<dev_t, MediaDeviceDeps *> devMap_; > > - > > int addUdevDevice(struct udev_device *dev); > > int populateMediaDevice(MediaDevice *media, DependencyMap > > *deps); > > std::string lookupDeviceNode(dev_t devnum); > > > > int addV4L2Device(dev_t devnum); > > void udevNotify(EventNotifier *notifier); > > + > > + struct udev *udev_; > > + struct udev_monitor *monitor_; > > + EventNotifier *notifier_; > > + > > + std::set<dev_t> orphans_; > > + std::list<MediaDeviceDeps> pending_; > > + std::map<dev_t, MediaDeviceDeps *> devMap_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/event_dispatcher_poll.h > > b/include/libcamera/internal/event_dispatcher_poll.h > > index 1f0738617425..3c9099660c4d 100644 > > --- a/include/libcamera/internal/event_dispatcher_poll.h > > +++ b/include/libcamera/internal/event_dispatcher_poll.h > > @@ -41,16 +41,16 @@ private: > > EventNotifier *notifiers[3]; > > }; > > > > - std::map<int, EventNotifierSetPoll> notifiers_; > > - std::list<Timer *> timers_; > > - int eventfd_; > > - > > - bool processingEvents_; > > - > > int poll(std::vector<struct pollfd> *pollfds); > > void processInterrupt(const struct pollfd &pfd); > > void processNotifiers(const std::vector<struct pollfd> > > &pollfds); > > void processTimers(); > > + > > + std::map<int, EventNotifierSetPoll> notifiers_; > > + std::list<Timer *> timers_; > > + int eventfd_; > > + > > + bool processingEvents_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/ipa_manager.h > > b/include/libcamera/internal/ipa_manager.h > > index 2412d75746ac..16d742918cf2 100644 > > --- a/include/libcamera/internal/ipa_manager.h > > +++ b/include/libcamera/internal/ipa_manager.h > > @@ -29,8 +29,6 @@ public: > > uint32_t minVersion); > > > > private: > > - std::vector<IPAModule *> modules_; > > - > > IPAManager(); > > ~IPAManager(); > > > > @@ -40,6 +38,8 @@ private: > > > > bool isSignatureValid(IPAModule *ipa) const; > > > > + std::vector<IPAModule *> modules_; > > + > > #if HAVE_IPA_PUBKEY > > static const uint8_t publicKeyData_[]; > > static const PubKey pubKey_; > > diff --git a/include/libcamera/internal/ipa_module.h > > b/include/libcamera/internal/ipa_module.h > > index 5b54cb31a48a..788e31d8bf03 100644 > > --- a/include/libcamera/internal/ipa_module.h > > +++ b/include/libcamera/internal/ipa_module.h > > @@ -42,6 +42,8 @@ protected: > > std::string logPrefix() const override; > > > > private: > > + int loadIPAModuleInfo(); > > + > > struct IPAModuleInfo info_; > > std::vector<uint8_t> signature_; > > > > @@ -52,8 +54,6 @@ private: > > void *dlHandle_; > > typedef struct ipa_context *(*IPAIntfFactory)(void); > > IPAIntfFactory ipaCreate_; > > - > > - int loadIPAModuleInfo(); > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/media_device.h > > b/include/libcamera/internal/media_device.h > > index 9fe76c514b17..19af059d9291 100644 > > --- a/include/libcamera/internal/media_device.h > > +++ b/include/libcamera/internal/media_device.h > > @@ -58,26 +58,13 @@ protected: > > std::string logPrefix() const; > > > > private: > > - std::string driver_; > > - std::string deviceNode_; > > - std::string model_; > > - unsigned int version_; > > - > > - int fd_; > > - bool valid_; > > - bool acquired_; > > - bool lockOwner_; > > - > > int open(); > > void close(); > > > > - std::map<unsigned int, MediaObject *> objects_; > > MediaObject *object(unsigned int id); > > bool addObject(MediaObject *object); > > void clear(); > > > > - std::vector<MediaEntity *> entities_; > > - > > struct media_v2_interface *findInterface(const struct > > media_v2_topology &topology, > > unsigned int > > entityId); > > bool populateEntities(const struct media_v2_topology > > &topology); > > @@ -87,6 +74,19 @@ private: > > > > friend int MediaLink::setEnabled(bool enable); > > int setupLink(const MediaLink *link, unsigned int flags); > > + > > + std::string driver_; > > + std::string deviceNode_; > > + std::string model_; > > + unsigned int version_; > > + > > + int fd_; > > + bool valid_; > > + bool acquired_; > > + bool lockOwner_; > > + > > + std::map<unsigned int, MediaObject *> objects_; > > + std::vector<MediaEntity *> entities_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/media_object.h > > b/include/libcamera/internal/media_object.h > > index 748eafdc880b..e8f2f27ce99e 100644 > > --- a/include/libcamera/internal/media_object.h > > +++ b/include/libcamera/internal/media_object.h > > @@ -107,6 +107,8 @@ private: > > MediaEntity(const MediaEntity &) = delete; > > ~MediaEntity(); > > > > + void addPad(MediaPad *pad); > > + > > std::string name_; > > unsigned int function_; > > unsigned int flags_; > > @@ -115,8 +117,6 @@ private: > > unsigned int minor_; > > > > std::vector<MediaPad *> pads_; > > - > > - void addPad(MediaPad *pad); > > }; > > > > } /* namespace libcamera */ > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://u15657259.ct.sendgrid.net/ls/click?upn=8H1KCc2bev8KdIveckpOEBeWjI3THEr-2F8W-2FrEpvXj1fUaD8nZSgfyCwFn-2BKX4QPmXiU9H2IGCLkMp0Cw1NTkpQ-3D-3DzQdG_C3wFy2Q4UgRsRLDAYieRZ5Z3EhAWyy0-2FkOzyYc6FPc1dn6ROcAJqKXb9hjP566uP6jUnxKU2JYw41zoRJ3n6FfFynnIkRxAddEjNjj165XJ-2FRNlSgfWT9Aqv6PykMK3xWdzLEAb3cLCNkDxVPVRb0Ac3agobniytGjxx3v5SKbA2Q1FuCZUndHBCF41S29TO8ogj61uOtLN-2FRYoijKcI7vM7Hbyls6PmzrlTekav1UNBi6yovb12Oa-2Fluzl0tiq9
Hi Umang, On Tue, May 26, 2020 at 05:24:52AM +0000, Umang Jain wrote: > On Tue, 2020-05-26 at 06:26 +0300, Laurent Pinchart wrote: > > The preferred coding style in libcamera is to declare private functions > > before private variables in class definitions. This rule isn't followed > > by some of the internal classes. Update them accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../internal/device_enumerator_udev.h | 16 ++++++------ > > .../internal/event_dispatcher_poll.h | 12 ++++----- > > include/libcamera/internal/ipa_manager.h | 4 +-- > > include/libcamera/internal/ipa_module.h | 4 +-- > > include/libcamera/internal/media_device.h | 26 +++++++++------ > > ---- > > include/libcamera/internal/media_object.h | 4 +-- > > 6 files changed, 33 insertions(+), 33 deletions(-) > > I spotted one more place that is not included in this patch, which > is : include/libcamera/ipa_context_wrapper.h In that file, we have private: static void queue_frame_action(void *ctx, unsigned int frame, struct ipa_operation_data &data); static const struct ipa_callback_ops callbacks_; void doQueueFrameAction(unsigned int frame, const IPAOperationData &data); struct ipa_context *ctx_; IPAInterface *intf_; ControlSerializer serializer_; I'm not sure if we should keep the two static members together, or strictly group all data after the functions. I went for the former as it's a grey area, but if you think the latter is better, I'll change it. > Apart from that: > Reviewed-by: Umang Jain <email@uajain.com> > > > diff --git a/include/libcamera/internal/device_enumerator_udev.h > > b/include/libcamera/internal/device_enumerator_udev.h > > index fdaa20968ef0..10d17ed2abac 100644 > > --- a/include/libcamera/internal/device_enumerator_udev.h > > +++ b/include/libcamera/internal/device_enumerator_udev.h > > @@ -36,10 +36,6 @@ public: > > int enumerate() final; > > > > private: > > - struct udev *udev_; > > - struct udev_monitor *monitor_; > > - EventNotifier *notifier_; > > - > > using DependencyMap = std::map<dev_t, std::list<MediaEntity > > *>>; > > > > struct MediaDeviceDeps { > > @@ -58,16 +54,20 @@ private: > > DependencyMap deps_; > > }; > > > > - std::set<dev_t> orphans_; > > - std::list<MediaDeviceDeps> pending_; > > - std::map<dev_t, MediaDeviceDeps *> devMap_; > > - > > int addUdevDevice(struct udev_device *dev); > > int populateMediaDevice(MediaDevice *media, DependencyMap > > *deps); > > std::string lookupDeviceNode(dev_t devnum); > > > > int addV4L2Device(dev_t devnum); > > void udevNotify(EventNotifier *notifier); > > + > > + struct udev *udev_; > > + struct udev_monitor *monitor_; > > + EventNotifier *notifier_; > > + > > + std::set<dev_t> orphans_; > > + std::list<MediaDeviceDeps> pending_; > > + std::map<dev_t, MediaDeviceDeps *> devMap_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/event_dispatcher_poll.h > > b/include/libcamera/internal/event_dispatcher_poll.h > > index 1f0738617425..3c9099660c4d 100644 > > --- a/include/libcamera/internal/event_dispatcher_poll.h > > +++ b/include/libcamera/internal/event_dispatcher_poll.h > > @@ -41,16 +41,16 @@ private: > > EventNotifier *notifiers[3]; > > }; > > > > - std::map<int, EventNotifierSetPoll> notifiers_; > > - std::list<Timer *> timers_; > > - int eventfd_; > > - > > - bool processingEvents_; > > - > > int poll(std::vector<struct pollfd> *pollfds); > > void processInterrupt(const struct pollfd &pfd); > > void processNotifiers(const std::vector<struct pollfd> > > &pollfds); > > void processTimers(); > > + > > + std::map<int, EventNotifierSetPoll> notifiers_; > > + std::list<Timer *> timers_; > > + int eventfd_; > > + > > + bool processingEvents_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/ipa_manager.h > > b/include/libcamera/internal/ipa_manager.h > > index 2412d75746ac..16d742918cf2 100644 > > --- a/include/libcamera/internal/ipa_manager.h > > +++ b/include/libcamera/internal/ipa_manager.h > > @@ -29,8 +29,6 @@ public: > > uint32_t minVersion); > > > > private: > > - std::vector<IPAModule *> modules_; > > - > > IPAManager(); > > ~IPAManager(); > > > > @@ -40,6 +38,8 @@ private: > > > > bool isSignatureValid(IPAModule *ipa) const; > > > > + std::vector<IPAModule *> modules_; > > + > > #if HAVE_IPA_PUBKEY > > static const uint8_t publicKeyData_[]; > > static const PubKey pubKey_; > > diff --git a/include/libcamera/internal/ipa_module.h > > b/include/libcamera/internal/ipa_module.h > > index 5b54cb31a48a..788e31d8bf03 100644 > > --- a/include/libcamera/internal/ipa_module.h > > +++ b/include/libcamera/internal/ipa_module.h > > @@ -42,6 +42,8 @@ protected: > > std::string logPrefix() const override; > > > > private: > > + int loadIPAModuleInfo(); > > + > > struct IPAModuleInfo info_; > > std::vector<uint8_t> signature_; > > > > @@ -52,8 +54,6 @@ private: > > void *dlHandle_; > > typedef struct ipa_context *(*IPAIntfFactory)(void); > > IPAIntfFactory ipaCreate_; > > - > > - int loadIPAModuleInfo(); > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/media_device.h > > b/include/libcamera/internal/media_device.h > > index 9fe76c514b17..19af059d9291 100644 > > --- a/include/libcamera/internal/media_device.h > > +++ b/include/libcamera/internal/media_device.h > > @@ -58,26 +58,13 @@ protected: > > std::string logPrefix() const; > > > > private: > > - std::string driver_; > > - std::string deviceNode_; > > - std::string model_; > > - unsigned int version_; > > - > > - int fd_; > > - bool valid_; > > - bool acquired_; > > - bool lockOwner_; > > - > > int open(); > > void close(); > > > > - std::map<unsigned int, MediaObject *> objects_; > > MediaObject *object(unsigned int id); > > bool addObject(MediaObject *object); > > void clear(); > > > > - std::vector<MediaEntity *> entities_; > > - > > struct media_v2_interface *findInterface(const struct > > media_v2_topology &topology, > > unsigned int > > entityId); > > bool populateEntities(const struct media_v2_topology > > &topology); > > @@ -87,6 +74,19 @@ private: > > > > friend int MediaLink::setEnabled(bool enable); > > int setupLink(const MediaLink *link, unsigned int flags); > > + > > + std::string driver_; > > + std::string deviceNode_; > > + std::string model_; > > + unsigned int version_; > > + > > + int fd_; > > + bool valid_; > > + bool acquired_; > > + bool lockOwner_; > > + > > + std::map<unsigned int, MediaObject *> objects_; > > + std::vector<MediaEntity *> entities_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/internal/media_object.h > > b/include/libcamera/internal/media_object.h > > index 748eafdc880b..e8f2f27ce99e 100644 > > --- a/include/libcamera/internal/media_object.h > > +++ b/include/libcamera/internal/media_object.h > > @@ -107,6 +107,8 @@ private: > > MediaEntity(const MediaEntity &) = delete; > > ~MediaEntity(); > > > > + void addPad(MediaPad *pad); > > + > > std::string name_; > > unsigned int function_; > > unsigned int flags_; > > @@ -115,8 +117,6 @@ private: > > unsigned int minor_; > > > > std::vector<MediaPad *> pads_; > > - > > - void addPad(MediaPad *pad); > > }; > > > > } /* namespace libcamera */
Hi Laurent, On 6/2/20 4:59 PM, Laurent Pinchart wrote: > Hi Umang, > > On Tue, May 26, 2020 at 05:24:52AM +0000, Umang Jain wrote: >> On Tue, 2020-05-26 at 06:26 +0300, Laurent Pinchart wrote: >>> The preferred coding style in libcamera is to declare private functions >>> before private variables in class definitions. This rule isn't followed >>> by some of the internal classes. Update them accordingly. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> .../internal/device_enumerator_udev.h | 16 ++++++------ >>> .../internal/event_dispatcher_poll.h | 12 ++++----- >>> include/libcamera/internal/ipa_manager.h | 4 +-- >>> include/libcamera/internal/ipa_module.h | 4 +-- >>> include/libcamera/internal/media_device.h | 26 +++++++++------ >>> ---- >>> include/libcamera/internal/media_object.h | 4 +-- >>> 6 files changed, 33 insertions(+), 33 deletions(-) >> I spotted one more place that is not included in this patch, which >> is : include/libcamera/ipa_context_wrapper.h > In that file, we have > > private: > static void queue_frame_action(void *ctx, unsigned int frame, > struct ipa_operation_data &data); > static const struct ipa_callback_ops callbacks_; > > void doQueueFrameAction(unsigned int frame, > const IPAOperationData &data); > > struct ipa_context *ctx_; > IPAInterface *intf_; > > ControlSerializer serializer_; > > I'm not sure if we should keep the two static members together, or > strictly group all data after the functions. I went for the former as > it's a grey area, but if you think the latter is better, I'll change it. Indeed a grey area and after some thinking I agree that static members should be kept together, so this change can be merged as is. Thanks. >> Apart from that: >> Reviewed-by: Umang Jain <email@uajain.com> >> >>> diff --git a/include/libcamera/internal/device_enumerator_udev.h >>> b/include/libcamera/internal/device_enumerator_udev.h >>> index fdaa20968ef0..10d17ed2abac 100644 >>> --- a/include/libcamera/internal/device_enumerator_udev.h >>> +++ b/include/libcamera/internal/device_enumerator_udev.h >>> @@ -36,10 +36,6 @@ public: >>> int enumerate() final; >>> >>> private: >>> - struct udev *udev_; >>> - struct udev_monitor *monitor_; >>> - EventNotifier *notifier_; >>> - >>> using DependencyMap = std::map<dev_t, std::list<MediaEntity >>> *>>; >>> >>> struct MediaDeviceDeps { >>> @@ -58,16 +54,20 @@ private: >>> DependencyMap deps_; >>> }; >>> >>> - std::set<dev_t> orphans_; >>> - std::list<MediaDeviceDeps> pending_; >>> - std::map<dev_t, MediaDeviceDeps *> devMap_; >>> - >>> int addUdevDevice(struct udev_device *dev); >>> int populateMediaDevice(MediaDevice *media, DependencyMap >>> *deps); >>> std::string lookupDeviceNode(dev_t devnum); >>> >>> int addV4L2Device(dev_t devnum); >>> void udevNotify(EventNotifier *notifier); >>> + >>> + struct udev *udev_; >>> + struct udev_monitor *monitor_; >>> + EventNotifier *notifier_; >>> + >>> + std::set<dev_t> orphans_; >>> + std::list<MediaDeviceDeps> pending_; >>> + std::map<dev_t, MediaDeviceDeps *> devMap_; >>> }; >>> >>> } /* namespace libcamera */ >>> diff --git a/include/libcamera/internal/event_dispatcher_poll.h >>> b/include/libcamera/internal/event_dispatcher_poll.h >>> index 1f0738617425..3c9099660c4d 100644 >>> --- a/include/libcamera/internal/event_dispatcher_poll.h >>> +++ b/include/libcamera/internal/event_dispatcher_poll.h >>> @@ -41,16 +41,16 @@ private: >>> EventNotifier *notifiers[3]; >>> }; >>> >>> - std::map<int, EventNotifierSetPoll> notifiers_; >>> - std::list<Timer *> timers_; >>> - int eventfd_; >>> - >>> - bool processingEvents_; >>> - >>> int poll(std::vector<struct pollfd> *pollfds); >>> void processInterrupt(const struct pollfd &pfd); >>> void processNotifiers(const std::vector<struct pollfd> >>> &pollfds); >>> void processTimers(); >>> + >>> + std::map<int, EventNotifierSetPoll> notifiers_; >>> + std::list<Timer *> timers_; >>> + int eventfd_; >>> + >>> + bool processingEvents_; >>> }; >>> >>> } /* namespace libcamera */ >>> diff --git a/include/libcamera/internal/ipa_manager.h >>> b/include/libcamera/internal/ipa_manager.h >>> index 2412d75746ac..16d742918cf2 100644 >>> --- a/include/libcamera/internal/ipa_manager.h >>> +++ b/include/libcamera/internal/ipa_manager.h >>> @@ -29,8 +29,6 @@ public: >>> uint32_t minVersion); >>> >>> private: >>> - std::vector<IPAModule *> modules_; >>> - >>> IPAManager(); >>> ~IPAManager(); >>> >>> @@ -40,6 +38,8 @@ private: >>> >>> bool isSignatureValid(IPAModule *ipa) const; >>> >>> + std::vector<IPAModule *> modules_; >>> + >>> #if HAVE_IPA_PUBKEY >>> static const uint8_t publicKeyData_[]; >>> static const PubKey pubKey_; >>> diff --git a/include/libcamera/internal/ipa_module.h >>> b/include/libcamera/internal/ipa_module.h >>> index 5b54cb31a48a..788e31d8bf03 100644 >>> --- a/include/libcamera/internal/ipa_module.h >>> +++ b/include/libcamera/internal/ipa_module.h >>> @@ -42,6 +42,8 @@ protected: >>> std::string logPrefix() const override; >>> >>> private: >>> + int loadIPAModuleInfo(); >>> + >>> struct IPAModuleInfo info_; >>> std::vector<uint8_t> signature_; >>> >>> @@ -52,8 +54,6 @@ private: >>> void *dlHandle_; >>> typedef struct ipa_context *(*IPAIntfFactory)(void); >>> IPAIntfFactory ipaCreate_; >>> - >>> - int loadIPAModuleInfo(); >>> }; >>> >>> } /* namespace libcamera */ >>> diff --git a/include/libcamera/internal/media_device.h >>> b/include/libcamera/internal/media_device.h >>> index 9fe76c514b17..19af059d9291 100644 >>> --- a/include/libcamera/internal/media_device.h >>> +++ b/include/libcamera/internal/media_device.h >>> @@ -58,26 +58,13 @@ protected: >>> std::string logPrefix() const; >>> >>> private: >>> - std::string driver_; >>> - std::string deviceNode_; >>> - std::string model_; >>> - unsigned int version_; >>> - >>> - int fd_; >>> - bool valid_; >>> - bool acquired_; >>> - bool lockOwner_; >>> - >>> int open(); >>> void close(); >>> >>> - std::map<unsigned int, MediaObject *> objects_; >>> MediaObject *object(unsigned int id); >>> bool addObject(MediaObject *object); >>> void clear(); >>> >>> - std::vector<MediaEntity *> entities_; >>> - >>> struct media_v2_interface *findInterface(const struct >>> media_v2_topology &topology, >>> unsigned int >>> entityId); >>> bool populateEntities(const struct media_v2_topology >>> &topology); >>> @@ -87,6 +74,19 @@ private: >>> >>> friend int MediaLink::setEnabled(bool enable); >>> int setupLink(const MediaLink *link, unsigned int flags); >>> + >>> + std::string driver_; >>> + std::string deviceNode_; >>> + std::string model_; >>> + unsigned int version_; >>> + >>> + int fd_; >>> + bool valid_; >>> + bool acquired_; >>> + bool lockOwner_; >>> + >>> + std::map<unsigned int, MediaObject *> objects_; >>> + std::vector<MediaEntity *> entities_; >>> }; >>> >>> } /* namespace libcamera */ >>> diff --git a/include/libcamera/internal/media_object.h >>> b/include/libcamera/internal/media_object.h >>> index 748eafdc880b..e8f2f27ce99e 100644 >>> --- a/include/libcamera/internal/media_object.h >>> +++ b/include/libcamera/internal/media_object.h >>> @@ -107,6 +107,8 @@ private: >>> MediaEntity(const MediaEntity &) = delete; >>> ~MediaEntity(); >>> >>> + void addPad(MediaPad *pad); >>> + >>> std::string name_; >>> unsigned int function_; >>> unsigned int flags_; >>> @@ -115,8 +117,6 @@ private: >>> unsigned int minor_; >>> >>> std::vector<MediaPad *> pads_; >>> - >>> - void addPad(MediaPad *pad); >>> }; >>> >>> } /* namespace libcamera */
diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h index fdaa20968ef0..10d17ed2abac 100644 --- a/include/libcamera/internal/device_enumerator_udev.h +++ b/include/libcamera/internal/device_enumerator_udev.h @@ -36,10 +36,6 @@ public: int enumerate() final; private: - struct udev *udev_; - struct udev_monitor *monitor_; - EventNotifier *notifier_; - using DependencyMap = std::map<dev_t, std::list<MediaEntity *>>; struct MediaDeviceDeps { @@ -58,16 +54,20 @@ private: DependencyMap deps_; }; - std::set<dev_t> orphans_; - std::list<MediaDeviceDeps> pending_; - std::map<dev_t, MediaDeviceDeps *> devMap_; - int addUdevDevice(struct udev_device *dev); int populateMediaDevice(MediaDevice *media, DependencyMap *deps); std::string lookupDeviceNode(dev_t devnum); int addV4L2Device(dev_t devnum); void udevNotify(EventNotifier *notifier); + + struct udev *udev_; + struct udev_monitor *monitor_; + EventNotifier *notifier_; + + std::set<dev_t> orphans_; + std::list<MediaDeviceDeps> pending_; + std::map<dev_t, MediaDeviceDeps *> devMap_; }; } /* namespace libcamera */ diff --git a/include/libcamera/internal/event_dispatcher_poll.h b/include/libcamera/internal/event_dispatcher_poll.h index 1f0738617425..3c9099660c4d 100644 --- a/include/libcamera/internal/event_dispatcher_poll.h +++ b/include/libcamera/internal/event_dispatcher_poll.h @@ -41,16 +41,16 @@ private: EventNotifier *notifiers[3]; }; - std::map<int, EventNotifierSetPoll> notifiers_; - std::list<Timer *> timers_; - int eventfd_; - - bool processingEvents_; - int poll(std::vector<struct pollfd> *pollfds); void processInterrupt(const struct pollfd &pfd); void processNotifiers(const std::vector<struct pollfd> &pollfds); void processTimers(); + + std::map<int, EventNotifierSetPoll> notifiers_; + std::list<Timer *> timers_; + int eventfd_; + + bool processingEvents_; }; } /* namespace libcamera */ diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h index 2412d75746ac..16d742918cf2 100644 --- a/include/libcamera/internal/ipa_manager.h +++ b/include/libcamera/internal/ipa_manager.h @@ -29,8 +29,6 @@ public: uint32_t minVersion); private: - std::vector<IPAModule *> modules_; - IPAManager(); ~IPAManager(); @@ -40,6 +38,8 @@ private: bool isSignatureValid(IPAModule *ipa) const; + std::vector<IPAModule *> modules_; + #if HAVE_IPA_PUBKEY static const uint8_t publicKeyData_[]; static const PubKey pubKey_; diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h index 5b54cb31a48a..788e31d8bf03 100644 --- a/include/libcamera/internal/ipa_module.h +++ b/include/libcamera/internal/ipa_module.h @@ -42,6 +42,8 @@ protected: std::string logPrefix() const override; private: + int loadIPAModuleInfo(); + struct IPAModuleInfo info_; std::vector<uint8_t> signature_; @@ -52,8 +54,6 @@ private: void *dlHandle_; typedef struct ipa_context *(*IPAIntfFactory)(void); IPAIntfFactory ipaCreate_; - - int loadIPAModuleInfo(); }; } /* namespace libcamera */ diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h index 9fe76c514b17..19af059d9291 100644 --- a/include/libcamera/internal/media_device.h +++ b/include/libcamera/internal/media_device.h @@ -58,26 +58,13 @@ protected: std::string logPrefix() const; private: - std::string driver_; - std::string deviceNode_; - std::string model_; - unsigned int version_; - - int fd_; - bool valid_; - bool acquired_; - bool lockOwner_; - int open(); void close(); - std::map<unsigned int, MediaObject *> objects_; MediaObject *object(unsigned int id); bool addObject(MediaObject *object); void clear(); - std::vector<MediaEntity *> entities_; - struct media_v2_interface *findInterface(const struct media_v2_topology &topology, unsigned int entityId); bool populateEntities(const struct media_v2_topology &topology); @@ -87,6 +74,19 @@ private: friend int MediaLink::setEnabled(bool enable); int setupLink(const MediaLink *link, unsigned int flags); + + std::string driver_; + std::string deviceNode_; + std::string model_; + unsigned int version_; + + int fd_; + bool valid_; + bool acquired_; + bool lockOwner_; + + std::map<unsigned int, MediaObject *> objects_; + std::vector<MediaEntity *> entities_; }; } /* namespace libcamera */ diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h index 748eafdc880b..e8f2f27ce99e 100644 --- a/include/libcamera/internal/media_object.h +++ b/include/libcamera/internal/media_object.h @@ -107,6 +107,8 @@ private: MediaEntity(const MediaEntity &) = delete; ~MediaEntity(); + void addPad(MediaPad *pad); + std::string name_; unsigned int function_; unsigned int flags_; @@ -115,8 +117,6 @@ private: unsigned int minor_; std::vector<MediaPad *> pads_; - - void addPad(MediaPad *pad); }; } /* namespace libcamera */
The preferred coding style in libcamera is to declare private functions before private variables in class definitions. This rule isn't followed by some of the internal classes. Update them accordingly. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../internal/device_enumerator_udev.h | 16 ++++++------ .../internal/event_dispatcher_poll.h | 12 ++++----- include/libcamera/internal/ipa_manager.h | 4 +-- include/libcamera/internal/ipa_module.h | 4 +-- include/libcamera/internal/media_device.h | 26 +++++++++---------- include/libcamera/internal/media_object.h | 4 +-- 6 files changed, 33 insertions(+), 33 deletions(-)