[Buildroot] [PATCH 1/1] package/gpiod: add gpiod hardware handling daemon

Thomas Petazzoni thomas.petazzoni at bootlin.com
Wed Jun 17 20:38:24 UTC 2020


On Tue, 16 Jun 2020 18:42:00 +0200
mchalain <marc.chalain at gmail.com> 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 <marc.chalain at gmail.com>

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


More information about the buildroot mailing list