Date: Tue, 23 Apr 2024 21:23:36 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Kent Gibson <warthog618@gmail.com>,
	Erik Schilling <erik.schilling@linaro.org>,
	Phil Howard <phil@gadgetoid.com>,
	Viresh Kumar <viresh.kumar@linaro.org>, linux-gpio@vger.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [libgpiod][RFC/RFT 06/18] bindings: glib: add examples
Message-ID: <Zif8qBoZq7I3Xrbb@smile.fi.intel.com>
References: <20240412122804.109323-1-brgl@bgdev.pl>
 <20240412122804.109323-7-brgl@bgdev.pl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240412122804.109323-7-brgl@bgdev.pl>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Fri, Apr 12, 2024 at 02:27:52PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add example programs showcasing the usage of GLib bindings to libgpiod.

> +		ret = g_gpiod_chip_get_line_offset_from_name(chip, line_name,
> +							     &offset, &err);
> +		if (ret) {
> +			info = g_gpiod_chip_get_info(chip, &err);
> +			if (!info) {
> +				g_printerr("Failed to get chip info: %s\n",
> +					   err->message);
> +				return EXIT_FAILURE;
> +			}
> +
> +			g_print("%s %u\n",
> +				g_gpiod_chip_info_get_name(info),
> +				offset);
> +
> +			return EXIT_SUCCESS;
> +		} else if (!ret && err) {

Besides redundant 'else' the !ret is also redundant.

> +			g_printerr("Failed to map the line name '%s' to offset: %s\n",
> +				   line_name, err->message);
> +			return EXIT_FAILURE;
> +		}
> +	}

...

> +		direction == G_GPIOD_LINE_DIRECTION_INPUT ?
> +					"input" : "output",

One line?

...

> +	settings = g_gpiod_line_settings_new(
> +			"direction", G_GPIOD_LINE_DIRECTION_INPUT,
> +			NULL);

Personally I do not like the open parenthesis style...

I don't even know why you have done this way with having a plenty of room in
the previous line at least for the first parameter.

...

> +	req_cfg = g_gpiod_request_config_new(
> +			"consumer", "get-multiple-line-values", NULL);

Ditto. And so on across the code of the entire series...

...

> +	ret = g_gpiod_line_request_set_value(data->request, data->line_offset,
> +					     data->value, &err);
> +	if (!ret) {

ret == 0 equals error?!

> +		g_printerr("failed to set line value: %s\n", err->message);
> +		exit(EXIT_FAILURE);

Don't you have something like err->code to propagate?

> +	}


-- 
With Best Regards,
Andy Shevchenko