From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Petazzoni Date: Wed, 17 Jun 2020 22:38:24 +0200 Subject: [Buildroot] [PATCH 1/1] package/gpiod: add gpiod hardware handling daemon In-Reply-To: <20200616164200.6399-1-marc.chalain@gmail.com> References: <20200616164200.6399-1-marc.chalain@gmail.com> Message-ID: <20200617223824.0ab717d2@windsurf.home> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: buildroot@busybox.net On Tue, 16 Jun 2020 18:42:00 +0200 mchalain wrote: > Gpiod is a little daemon to trig gpio event and launch scripts on level > changing events. > As udev or mdev, it reads rules files to attach scripts on events. and > launch the scripts with environment variables to describe the event. > It uses libgpiod to monitor the gpio and libconfig to read the rules. > It is tested on Raspberry Pi (0,3,4) with success, during few months. > > Signed-off-by: mchalain Thanks a lot for your contribution. I have not reviewed it carefully. However, while it looks potentially interesting, I am personally always a bit concerned in packaging software components that look like "personal projects". This project has only been written by you, there are no contributions from others, no stars, no fork on Github, nothing indicates that anyone but you is using gpiod. So I am wondering if it is not too early to have that in Buildroot. On the other hand, you could certainly say that having it in Buildroot would give it some exposure that might encourage some users to try it out. Anyway, we will need your Signed-off-by to use your full first name and last name, properly capitalized. > diff --git a/package/gpiod/Config.in b/package/gpiod/Config.in > new file mode 100644 > index 0000000000..e9d5dc47f9 > --- /dev/null > +++ b/package/gpiod/Config.in > @@ -0,0 +1,14 @@ > +config BR2_PACKAGE_GPIOD > + bool "gpiod: gpio monitor daemon" > + depends on BR2_USE_MMU > + depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8 This needs a comment: depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8 # libgpiod > + select BR2_PACKAGE_LIBGPIOD > + select BR2_PACKAGE_LIBCONFIG > + help > + GPIOD monitors gpio events and start scripts. > + The daemon loads rules defining a gpio and > + the scripts to launch when the level of gpio changes. > + > +comment "gpiod: needs a toolchain w/ support of MMU and headers > 4.8" > + depends on !BR2_USE_MMU > + depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8 No, this should be: comment "gpiod needs a toolchain w/ kernel headers >= 4.8" depends on BR2_USE_MMU depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_8 > +RUNDIR=/var/run > +SBINDIR=/usr/sbin > +BINDIR=/usr/bin > + > +start() { > + printf "Starting gpiod: " > + start-stop-daemon -S -q --exec ${SBINDIR}/gpiod -- -D -p ${RUNDIR}/gpiod.pid > + [ $? == 0 ] && echo "OK" || echo "FAILED" > +} > +stop() { > + printf "Stopping gpiod: " > + ${SBINDIR}/gpiod -K -p ${RUNDIR}/gpiod.pid > + echo "OK" > +} Please follow the template package/busybox/S01syslogd for your init script. > diff --git a/package/gpiod/gpiod.hash b/package/gpiod/gpiod.hash > new file mode 100644 > index 0000000000..4a9c10297c > --- /dev/null > +++ b/package/gpiod/gpiod.hash > @@ -0,0 +1 @@ > +sha256 f7c12fafcfb02515ae34d9502b4121d7980606fb53b57bee35143bd985bfdddc gpiod-1.0.tar.gz We need a hash for the license file. > diff --git a/package/gpiod/gpiod.mk b/package/gpiod/gpiod.mk > new file mode 100644 > index 0000000000..ed99f1d3be > --- /dev/null > +++ b/package/gpiod/gpiod.mk > @@ -0,0 +1,52 @@ > +################################################################################ > +# > +# gpiod > +# > +################################################################################ > + > +GPIOD_VERSION = 1.0 > +GPIOD_SITE = $(call github,mchalain,gpiod,$(GPIOD_VERSION)) We need GPIOD_LICENSE and GPIOD_LICENSE_FILES. > +GPIOD_MAKE_OPTS+=prefix=/usr > +GPIOD_MAKE_OPTS+=sysconfdir=/etc/gpiod Just one assignment, and unconditional: GPIOD_MAKE_OPTS = \ prefix=/usr \ sysconfdir=/etc/gpiod > +#GPIOD_MAKE_OPTS+=DEBUG=y Drop comments please. > + > +GPIOD_KCONFIG_FILE=$(GPIOD_PKGDIR)/gpiod_defconfig > +GPIOD_KCONFIG_EDITORS = config > +GPIOD_KCONFIG_OPTS = $(GPIOD_MAKE_OPTS) > + > +GPIOD_DEPENDENCIES += libgpiod > +GPIOD_DEPENDENCIES += libconfig One line for dependencies, and unconditional: GPIOD_DEPENDENCIES = libgpiod libconfig > +define GPIOD_LIBCONFIG_OPTS > + $(call KCONFIG_ENABLE_OPT,LIBCONFIG,$(@D)/.config) > +endef > + > +define GPIOD_KCONFIG_FIXUP_CMDS > + $(GPIOD_LIBCONFIG_OPTS) > +endef > + > +define GPIOD_BUILD_CMDS > + $(TARGET_CONFIGURE_OPTS) $(TARGET_MAKE_ENV) \ > + $(MAKE1) -C $(@D) $(GPIOD_MAKE_OPTS) > +endef > + > +define GPIOD_INSTALL_TARGET_CMDS > + $(INSTALL) -d -m 755 $(TARGET_DIR)/etc/gpiod/rules.d Why do you need this ? > + $(MAKE) -C $(@D) $(GPIOD_MAKE_OPTS) \ > + DESTDIR="$(TARGET_DIR)" DEVINSTALL=n install I don't think you need DEVINSTALL=n here > +endef > + > +define GPIOD_INSTALL_INIT_SYSTEMD > + $(INSTALL) -D -m 644 $(GPIOD_PKGDIR)/gpiod.service \ > + $(TARGET_DIR)/usr/lib/systemd/system/gpiod.service This is OK. Perhaps use $(@D) instead of $(GPIOD_PKGDIR) However, the service file was missing in your patch. > + mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants > + ln -fs ../../../../usr/lib/systemd/system/gpiod.service \ > + $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/gpiod.service These two commands are not needed, systemd preset-all is going to take care of this. > +endef > +define GPIOD_INSTALL_INIT_SYSV > + $(INSTALL) -D -m 755 $(GPIOD_PKGDIR)/S20gpiod \ > + $(TARGET_DIR)/etc/init.d/S20gpiod > +endef > + > +$(eval $(kconfig-package)) I'm a bit skeptical on the usage of kconfig-package here. You are *not* using kconfig at all. You have a makefile that emulates some of the kconfig behavior, but you're not using kconfig. So if we later change how kconfig-package behave, to use some kconfig functionality that you do not emulate, things will no longer work for the gpiod package. > diff --git a/package/gpiod/gpiod_defconfig b/package/gpiod/gpiod_defconfig > new file mode 100644 > index 0000000000..e69de29bb2 So it's just empty ? Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com