diff mbox series

[libgpiod,v2,3/7] treewide: rename BIAS_AS_IS to BIAS_UNKNOWN

Message ID 20210115103018.27704-4-brgl@bgdev.pl
State New
Headers show
Series treewide: remove more cruft and add some improvements | expand

Commit Message

Bartosz Golaszewski Jan. 15, 2021, 10:30 a.m. UTC 
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

When inspecting the current bias setting of a GPIO line, the AS_IS name
of one of the possible values really means that the kernel uAPI can't
determine the bias setting because it didn't set it itself. In this case
it's better to change the name to BIAS_UNKNOWN to reflect that.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 bindings/cxx/gpiod.hpp                 |  2 +-
 bindings/cxx/line.cpp                  |  2 +-
 bindings/cxx/tests/tests-line.cpp      |  8 ++++----
 bindings/python/gpiodmodule.c          | 10 +++++-----
 bindings/python/tests/gpiod_py_test.py |  6 +++---
 include/gpiod.h                        |  4 ++--
 lib/core.c                             |  2 +-
 tests/tests-line.c                     | 12 ++++++------
 8 files changed, 23 insertions(+), 23 deletions(-)

Comments

Kent Gibson Jan. 18, 2021, 12:07 a.m. UTC | #1 
On Fri, Jan 15, 2021 at 11:30:14AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> When inspecting the current bias setting of a GPIO line, the AS_IS name
> of one of the possible values really means that the kernel uAPI can't
> determine the bias setting because it didn't set it itself. In this case
> it's better to change the name to BIAS_UNKNOWN to reflect that.
> 

Your checkin comment incorporates some of my review comments, which were
actually a bit sloppy.  While I didn't bother to correct myself for that
email, I'd rather the checkin comment be more precise.

Specifically, I was conflating gpiolib and the cdev uAPI.  If the bias
is set via gpiolib then the uAPI will report it correctly.  If it is set
otherwise then the setting is unknown to gpiolib and therefore the uAPI.

And I'm not sure if the DT example that I used in that email was a good
one. But say the hardware initialises with pull-up enabled.  If it hasn't
also been set via gpiolib then it will be reported as unknown.

Cheers,
Kent.
Bartosz Golaszewski Jan. 18, 2021, 11:40 a.m. UTC | #2 
On Mon, Jan 18, 2021 at 1:07 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jan 15, 2021 at 11:30:14AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > When inspecting the current bias setting of a GPIO line, the AS_IS name
> > of one of the possible values really means that the kernel uAPI can't
> > determine the bias setting because it didn't set it itself. In this case
> > it's better to change the name to BIAS_UNKNOWN to reflect that.
> >
>
> Your checkin comment incorporates some of my review comments, which were
> actually a bit sloppy.  While I didn't bother to correct myself for that
> email, I'd rather the checkin comment be more precise.
>
> Specifically, I was conflating gpiolib and the cdev uAPI.  If the bias
> is set via gpiolib then the uAPI will report it correctly.  If it is set
> otherwise then the setting is unknown to gpiolib and therefore the uAPI.
>
> And I'm not sure if the DT example that I used in that email was a good
> one. But say the hardware initialises with pull-up enabled.  If it hasn't
> also been set via gpiolib then it will be reported as unknown.
>

Which makes me wonder whether we shouldn't add a get_config() callback
to drivers in gpiolib for that because some controllers allow you to
query their current settings.

---
When inspecting the current bias setting of a GPIO line, the AS_IS name
of one of the possible values really means that the kernel GPIO subsystem
can't determine the bias setting because it didn't set it itself (e.g. the
hardware may have internally initialized pull-up or pull-down resistors).
In this case it's better to change the name to BIAS_UNKNOWN to reflect that.
---

Does this sound good?

Bartosz
Kent Gibson Jan. 19, 2021, 12:27 a.m. UTC | #3 
On Mon, Jan 18, 2021 at 12:40:11PM +0100, Bartosz Golaszewski wrote:
> On Mon, Jan 18, 2021 at 1:07 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Jan 15, 2021 at 11:30:14AM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > When inspecting the current bias setting of a GPIO line, the AS_IS name
> > > of one of the possible values really means that the kernel uAPI can't
> > > determine the bias setting because it didn't set it itself. In this case
> > > it's better to change the name to BIAS_UNKNOWN to reflect that.
> > >
> >
> > Your checkin comment incorporates some of my review comments, which were
> > actually a bit sloppy.  While I didn't bother to correct myself for that
> > email, I'd rather the checkin comment be more precise.
> >
> > Specifically, I was conflating gpiolib and the cdev uAPI.  If the bias
> > is set via gpiolib then the uAPI will report it correctly.  If it is set
> > otherwise then the setting is unknown to gpiolib and therefore the uAPI.
> >
> > And I'm not sure if the DT example that I used in that email was a good
> > one. But say the hardware initialises with pull-up enabled.  If it hasn't
> > also been set via gpiolib then it will be reported as unknown.
> >
> 
> Which makes me wonder whether we shouldn't add a get_config() callback
> to drivers in gpiolib for that because some controllers allow you to
> query their current settings.
> 

I've thought the same, but until all the pinctrl drivers support it you
are still going to be returning unknown.  So you still can't provide any
guarantee that the information is available.  And as such is it any more
use than just requiring the user set it explicitly? And if they really
care about the bias they will probably set it anyway.

> ---
> When inspecting the current bias setting of a GPIO line, the AS_IS name
> of one of the possible values really means that the kernel GPIO subsystem
> can't determine the bias setting because it didn't set it itself (e.g. the
> hardware may have internally initialized pull-up or pull-down resistors).
> In this case it's better to change the name to BIAS_UNKNOWN to reflect that.
> ---
> 
> Does this sound good?
> 

That works for me.

Cheers,
Kent.
diff mbox series

Patch

diff --git a/bindings/cxx/gpiod.hpp b/bindings/cxx/gpiod.hpp
index 8b4a8f9..fb7f1fa 100644
--- a/bindings/cxx/gpiod.hpp
+++ b/bindings/cxx/gpiod.hpp
@@ -486,7 +486,7 @@  public:
 	 * @brief Possible bias settings.
 	 */
 	enum : int {
-		BIAS_AS_IS = 1,
+		BIAS_UNKNOWN = 1,
 		/**< Line's bias state is unknown. */
 		BIAS_DISABLE,
 		/**< Line's internal bias is disabled. */
diff --git a/bindings/cxx/line.cpp b/bindings/cxx/line.cpp
index 5a907db..c605790 100644
--- a/bindings/cxx/line.cpp
+++ b/bindings/cxx/line.cpp
@@ -17,7 +17,7 @@  const ::std::map<int, int> bias_mapping = {
 	{ GPIOD_LINE_BIAS_PULL_UP,	line::BIAS_PULL_UP, },
 	{ GPIOD_LINE_BIAS_PULL_DOWN,	line::BIAS_PULL_DOWN, },
 	{ GPIOD_LINE_BIAS_DISABLE,	line::BIAS_DISABLE, },
-	{ GPIOD_LINE_BIAS_AS_IS,	line::BIAS_AS_IS, },
+	{ GPIOD_LINE_BIAS_UNKNOWN,	line::BIAS_UNKNOWN, },
 };
 
 } /* namespace */
diff --git a/bindings/cxx/tests/tests-line.cpp b/bindings/cxx/tests/tests-line.cpp
index 3c7ea39..7ea9b3a 100644
--- a/bindings/cxx/tests/tests-line.cpp
+++ b/bindings/cxx/tests/tests-line.cpp
@@ -35,7 +35,7 @@  TEST_CASE("Line information can be correctly retrieved", "[line]")
 		REQUIRE_FALSE(line.is_used());
 		REQUIRE_FALSE(line.is_open_drain());
 		REQUIRE_FALSE(line.is_open_source());
-		REQUIRE(line.bias() == ::gpiod::line::BIAS_AS_IS);
+		REQUIRE(line.bias() == ::gpiod::line::BIAS_UNKNOWN);
 	}
 
 	SECTION("exported line")
@@ -54,7 +54,7 @@  TEST_CASE("Line information can be correctly retrieved", "[line]")
 		REQUIRE(line.is_used());
 		REQUIRE_FALSE(line.is_open_drain());
 		REQUIRE_FALSE(line.is_open_source());
-		REQUIRE(line.bias() == ::gpiod::line::BIAS_AS_IS);
+		REQUIRE(line.bias() == ::gpiod::line::BIAS_UNKNOWN);
 	}
 
 	SECTION("exported line with flags")
@@ -75,7 +75,7 @@  TEST_CASE("Line information can be correctly retrieved", "[line]")
 		REQUIRE(line.is_used());
 		REQUIRE(line.is_open_drain());
 		REQUIRE_FALSE(line.is_open_source());
-		REQUIRE(line.bias() == ::gpiod::line::BIAS_AS_IS);
+		REQUIRE(line.bias() == ::gpiod::line::BIAS_UNKNOWN);
 	}
 
 	SECTION("exported open source line")
@@ -95,7 +95,7 @@  TEST_CASE("Line information can be correctly retrieved", "[line]")
 		REQUIRE(line.is_used());
 		REQUIRE_FALSE(line.is_open_drain());
 		REQUIRE(line.is_open_source());
-		REQUIRE(line.bias() == ::gpiod::line::BIAS_AS_IS);
+		REQUIRE(line.bias() == ::gpiod::line::BIAS_UNKNOWN);
 	}
 
 	SECTION("exported bias disable line")
diff --git a/bindings/python/gpiodmodule.c b/bindings/python/gpiodmodule.c
index b48a83a..e0cfab3 100644
--- a/bindings/python/gpiodmodule.c
+++ b/bindings/python/gpiodmodule.c
@@ -68,7 +68,7 @@  enum {
 };
 
 enum {
-	gpiod_BIAS_AS_IS = 1,
+	gpiod_BIAS_UNKNOWN = 1,
 	gpiod_BIAS_DISABLE,
 	gpiod_BIAS_PULL_UP,
 	gpiod_BIAS_PULL_DOWN,
@@ -374,9 +374,9 @@  static PyObject *gpiod_Line_bias(gpiod_LineObject *self,
 		return Py_BuildValue("I", gpiod_BIAS_PULL_DOWN);
 	case GPIOD_LINE_BIAS_DISABLE:
 		return Py_BuildValue("I", gpiod_BIAS_DISABLE);
-	case GPIOD_LINE_BIAS_AS_IS:
+	case GPIOD_LINE_BIAS_UNKNOWN:
 	default:
-		return Py_BuildValue("I", gpiod_BIAS_AS_IS);
+		return Py_BuildValue("I", gpiod_BIAS_UNKNOWN);
 	}
 }
 
@@ -2527,8 +2527,8 @@  static gpiod_ConstDescr gpiod_ConstList[] = {
 	},
 	{
 		.typeobj = &gpiod_LineType,
-		.name = "BIAS_AS_IS",
-		.val = gpiod_BIAS_AS_IS,
+		.name = "BIAS_UNKNOWN",
+		.val = gpiod_BIAS_UNKNOWN,
 	},
 	{
 		.typeobj = &gpiod_LineType,
diff --git a/bindings/python/tests/gpiod_py_test.py b/bindings/python/tests/gpiod_py_test.py
index 3093a1c..7d790f3 100755
--- a/bindings/python/tests/gpiod_py_test.py
+++ b/bindings/python/tests/gpiod_py_test.py
@@ -241,7 +241,7 @@  class LineInfo(MockupTestCase):
             self.assertTrue(line.is_requested())
             self.assertTrue(line.is_open_drain())
             self.assertFalse(line.is_open_source())
-            self.assertEqual(line.bias(), gpiod.Line.BIAS_AS_IS)
+            self.assertEqual(line.bias(), gpiod.Line.BIAS_UNKNOWN)
 
     def test_exported_open_drain_line(self):
         with gpiod.Chip(mockup.chip_path(0)) as chip:
@@ -259,7 +259,7 @@  class LineInfo(MockupTestCase):
             self.assertTrue(line.is_requested())
             self.assertTrue(line.is_open_drain())
             self.assertFalse(line.is_open_source())
-            self.assertEqual(line.bias(), gpiod.Line.BIAS_AS_IS)
+            self.assertEqual(line.bias(), gpiod.Line.BIAS_UNKNOWN)
 
     def test_exported_open_source_line(self):
         with gpiod.Chip(mockup.chip_path(0)) as chip:
@@ -277,7 +277,7 @@  class LineInfo(MockupTestCase):
             self.assertTrue(line.is_requested())
             self.assertFalse(line.is_open_drain())
             self.assertTrue(line.is_open_source())
-            self.assertEqual(line.bias(), gpiod.Line.BIAS_AS_IS)
+            self.assertEqual(line.bias(), gpiod.Line.BIAS_UNKNOWN)
 
     def test_exported_bias_disable_line(self):
         with gpiod.Chip(mockup.chip_path(0)) as chip:
diff --git a/include/gpiod.h b/include/gpiod.h
index 75da84c..1aaa6cd 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -284,7 +284,7 @@  enum {
  * @brief Possible internal bias settings.
  */
 enum {
-	GPIOD_LINE_BIAS_AS_IS = 1,
+	GPIOD_LINE_BIAS_UNKNOWN = 1,
 	/**< The internal bias state is unknown. */
 	GPIOD_LINE_BIAS_DISABLE,
 	/**< The internal bias is disabled. */
@@ -337,7 +337,7 @@  bool gpiod_line_is_active_low(struct gpiod_line *line) GPIOD_API;
  * @brief Read the GPIO line bias setting.
  * @param line GPIO line object.
  * @return Returns GPIOD_LINE_BIAS_PULL_UP, GPIOD_LINE_BIAS_PULL_DOWN,
- *         GPIOD_LINE_BIAS_DISABLE or GPIOD_LINE_BIAS_AS_IS.
+ *         GPIOD_LINE_BIAS_DISABLE or GPIOD_LINE_BIAS_UNKNOWN.
  */
 int gpiod_line_bias(struct gpiod_line *line) GPIOD_API;
 
diff --git a/lib/core.c b/lib/core.c
index c6fb474..9ee84cf 100644
--- a/lib/core.c
+++ b/lib/core.c
@@ -485,7 +485,7 @@  int gpiod_line_bias(struct gpiod_line *line)
 	if (line->info_flags & GPIOLINE_FLAG_BIAS_PULL_DOWN)
 		return GPIOD_LINE_BIAS_PULL_DOWN;
 
-	return GPIOD_LINE_BIAS_AS_IS;
+	return GPIOD_LINE_BIAS_UNKNOWN;
 }
 
 bool gpiod_line_is_used(struct gpiod_line *line)
diff --git a/tests/tests-line.c b/tests/tests-line.c
index d6264af..88857a0 100644
--- a/tests/tests-line.c
+++ b/tests/tests-line.c
@@ -418,7 +418,7 @@  GPIOD_TEST_CASE(set_flags_bias, 0, { 8 })
 	ret = gpiod_line_request_input(line, GPIOD_TEST_CONSUMER);
 	g_assert_cmpint(ret, ==, 0);
 	gpiod_test_return_if_failed();
-	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
+	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_UNKNOWN);
 
 	ret = gpiod_line_set_flags(line,
 		GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE);
@@ -762,7 +762,7 @@  GPIOD_TEST_CASE(misc_flags, 0, { 8 })
 	g_assert_false(gpiod_line_is_used(line));
 	g_assert_false(gpiod_line_is_open_drain(line));
 	g_assert_false(gpiod_line_is_open_source(line));
-	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
+	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_UNKNOWN);
 
 	config.request_type = GPIOD_LINE_REQUEST_DIRECTION_OUTPUT;
 	config.consumer = GPIOD_TEST_CONSUMER;
@@ -775,7 +775,7 @@  GPIOD_TEST_CASE(misc_flags, 0, { 8 })
 	g_assert_true(gpiod_line_is_used(line));
 	g_assert_true(gpiod_line_is_open_drain(line));
 	g_assert_false(gpiod_line_is_open_source(line));
-	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
+	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_UNKNOWN);
 	g_assert_cmpint(gpiod_line_direction(line), ==,
 			GPIOD_LINE_DIRECTION_OUTPUT);
 
@@ -790,7 +790,7 @@  GPIOD_TEST_CASE(misc_flags, 0, { 8 })
 	g_assert_true(gpiod_line_is_used(line));
 	g_assert_false(gpiod_line_is_open_drain(line));
 	g_assert_true(gpiod_line_is_open_source(line));
-	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
+	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_UNKNOWN);
 	g_assert_cmpint(gpiod_line_direction(line), ==,
 			GPIOD_LINE_DIRECTION_OUTPUT);
 
@@ -829,7 +829,7 @@  GPIOD_TEST_CASE(misc_flags_work_together, 0, { 8 })
 	g_assert_true(gpiod_line_is_used(line));
 	g_assert_true(gpiod_line_is_open_drain(line));
 	g_assert_false(gpiod_line_is_open_source(line));
-	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
+	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_UNKNOWN);
 	g_assert_true(gpiod_line_is_active_low(line));
 	g_assert_cmpint(gpiod_line_direction(line), ==,
 			GPIOD_LINE_DIRECTION_OUTPUT);
@@ -846,7 +846,7 @@  GPIOD_TEST_CASE(misc_flags_work_together, 0, { 8 })
 	g_assert_true(gpiod_line_is_used(line));
 	g_assert_false(gpiod_line_is_open_drain(line));
 	g_assert_true(gpiod_line_is_open_source(line));
-	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_AS_IS);
+	g_assert_cmpint(gpiod_line_bias(line), ==, GPIOD_LINE_BIAS_UNKNOWN);
 	g_assert_true(gpiod_line_is_active_low(line));
 
 	gpiod_line_release(line);