[PATCH] i2c: core: helper function to detect slave mode

[Luis Oliveira]

This function has the purpose of mode detection by checking the
device nodes for a reg matching with the I2C_OWN_SLAVE_ADDREESS flag.
Currently only checks using OF functions (ACPI slave not supported yet).

Signed-off-by: Luis Oliveira <***@synopsys.com>
---
Due to the need of checking if the I2C slave address is our own (in
other words: if we are the I2C slave) I created a helper function
(proposed to me by @Andy) to enable that check.
Currently (because I am not able to test it using ACPI) it only
supports devicetree declarations.

drivers/i2c/i2c-core.c | 19 +++++++++++++++++++
include/linux/i2c.h | 1 +
2 files changed, 20 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3de95a29024c..48e705b23c59 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -3691,6 +3691,25 @@ int i2c_slave_unregister(struct i2c_client *client)
return ret;
}
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+
+int i2c_slave_mode_detect(struct device *dev)
+{
+ struct device_node *child;
+ u32 reg;
+
+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
+ for_each_child_of_node(dev->of_node, child) {
+ of_property_read_u32(child, "reg", &reg);
+ if (reg & I2C_OWN_SLAVE_ADDRESS)
+ return 1;
+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_mode_detect);
+
#endif

MODULE_AUTHOR("Simon G. Vogl <***@tk.uni-linz.ac.at>");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b2109c522dec..53cf99569af5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -282,6 +282,7 @@ enum i2c_slave_event {

extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+extern int i2c_slave_mode_detect(struct device *dev);

static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)

[Andy Shevchenko]

On Thu, Jan 5, 2017 at 7:24 PM, Luis Oliveira

Post by Luis Oliveira
This function has the purpose of mode detection by checking the
device nodes for a reg matching with the I2C_OWN_SLAVE_ADDREESS flag.
Currently only checks using OF functions (ACPI slave not supported yet).

The code looks good, one important comment below, after addressing it:

Reviewed-by: Andy Shevchenko <***@gmail.com>

P.S. Btw, we have Suggested-by tag for giving credit for suggestion.
Please use it.

Post by Luis Oliveira
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -3691,6 +3691,25 @@ int i2c_slave_unregister(struct i2c_client *client)

Please, add kernel doc description here, important thing is to explain
return codes in Return: section of it.

Post by Luis Oliveira
+int i2c_slave_mode_detect(struct device *dev)
+{
+ struct device_node *child;
+ u32 reg;

I would consider them as private to the OF branch. But it's really
minor and up to you (I don't remember if we have such style examples
in i2c core code).

Post by Luis Oliveira
+
+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
+ for_each_child_of_node(dev->of_node, child) {
+ of_property_read_u32(child, "reg", &reg);
+ if (reg & I2C_OWN_SLAVE_ADDRESS)
+ return 1;
+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_mode_detect);

[Luis Oliveira]

Post by Andy Shevchenko
On Thu, Jan 5, 2017 at 7:24 PM, Luis Oliveira

Post by Luis Oliveira
This function has the purpose of mode detection by checking the
device nodes for a reg matching with the I2C_OWN_SLAVE_ADDREESS flag.
Currently only checks using OF functions (ACPI slave not supported yet).

P.S. Btw, we have Suggested-by tag for giving credit for suggestion.
Please use it.

I will do it in the V2, thank you.

Post by Andy Shevchenko

Post by Luis Oliveira
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -3691,6 +3691,25 @@ int i2c_slave_unregister(struct i2c_client *client)

Please, add kernel doc description here, important thing is to explain
return codes in Return: section of it.

Post by Luis Oliveira
+int i2c_slave_mode_detect(struct device *dev)
+{
+ struct device_node *child;
+ u32 reg;

I would consider them as private to the OF branch. But it's really
minor and up to you (I don't remember if we have such style examples
in i2c core code).

I can change that in the V2 too.

Post by Andy Shevchenko

Post by Luis Oliveira
+
+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
+ for_each_child_of_node(dev->of_node, child) {
+ of_property_read_u32(child, "reg", &reg);
+ if (reg & I2C_OWN_SLAVE_ADDRESS)
+ return 1;
+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_mode_detect);

[Andy Shevchenko]

On Fri, Jan 6, 2017 at 7:15 PM, Luis Oliveira

Post by Andy Shevchenko
On Thu, Jan 5, 2017 at 7:24 PM, Luis Oliveira
Please, add kernel doc description here, important thing is to explain
return codes in Return: section of it.

Post by Luis Oliveira
+int i2c_slave_mode_detect(struct device *dev)

Just to make sure you didn't miss this one.

[Luis Oliveira]

Post by Andy Shevchenko
On Fri, Jan 6, 2017 at 7:15 PM, Luis Oliveira

Post by Andy Shevchenko
On Thu, Jan 5, 2017 at 7:24 PM, Luis Oliveira
Please, add kernel doc description here, important thing is to explain
return codes in Return: section of it.

Post by Luis Oliveira
+int i2c_slave_mode_detect(struct device *dev)

Just to make sure you didn't miss this one.

Yes, I saw it. You were talking of something like this, right?

/**
* i2c_slave_mode_detect - detect operation mode
* @dev: The device owning the bus
*
* This checks the device nodes for a I2C slave by checking the address
* used.
*
* Returns true if a I2C slave is detected, otherwise returns false.
*/

[Andy Shevchenko]

On Fri, Jan 6, 2017 at 7:46 PM, Luis Oliveira

Post by Luis Oliveira

Post by Andy Shevchenko
On Fri, Jan 6, 2017 at 7:15 PM, Luis Oliveira

Post by Andy Shevchenko
On Thu, Jan 5, 2017 at 7:24 PM, Luis Oliveira
Please, add kernel doc description here, important thing is to explain
return codes in Return: section of it.

Post by Luis Oliveira
+int i2c_slave_mode_detect(struct device *dev)

Just to make sure you didn't miss this one.

Yes, I saw it. You were talking of something like this, right?
/**
* i2c_slave_mode_detect - detect operation mode
*
* This checks the device nodes for a I2C slave by checking the address

a -> an

Post by Luis Oliveira
* used.
*
* Returns true if a I2C slave is detected, otherwise returns false.

It has int type and I would reword this accordingly.

Something like

* Return:
* 1 when an I2C slave is detected, 0 for I2C master mode,
* and negative error otherwise.

Post by Luis Oliveira
*/

[Rob Herring]

On Thu, Jan 5, 2017 at 11:24 AM, Luis Oliveira

Post by Luis Oliveira
This function has the purpose of mode detection by checking the
device nodes for a reg matching with the I2C_OWN_SLAVE_ADDREESS flag.
Currently only checks using OF functions (ACPI slave not supported yet).
---
Due to the need of checking if the I2C slave address is our own (in
other words: if we are the I2C slave) I created a helper function
Currently (because I am not able to test it using ACPI) it only
supports devicetree declarations.
drivers/i2c/i2c-core.c | 19 +++++++++++++++++++
include/linux/i2c.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3de95a29024c..48e705b23c59 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -3691,6 +3691,25 @@ int i2c_slave_unregister(struct i2c_client *client)
return ret;
}
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+
+int i2c_slave_mode_detect(struct device *dev)

This can be bool instead. Otherwise, looks good to me.

Post by Luis Oliveira
+{
+ struct device_node *child;
+ u32 reg;
+
+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
+ for_each_child_of_node(dev->of_node, child) {
+ of_property_read_u32(child, "reg", &reg);
+ if (reg & I2C_OWN_SLAVE_ADDRESS)
+ return 1;
+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_mode_detect);
+
#endif
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b2109c522dec..53cf99569af5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -282,6 +282,7 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+extern int i2c_slave_mode_detect(struct device *dev);
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
--
2.11.0

[Luis Oliveira]

Post by Rob Herring
On Thu, Jan 5, 2017 at 11:24 AM, Luis Oliveira

Post by Luis Oliveira
This function has the purpose of mode detection by checking the
device nodes for a reg matching with the I2C_OWN_SLAVE_ADDREESS flag.
Currently only checks using OF functions (ACPI slave not supported yet).
---
Due to the need of checking if the I2C slave address is our own (in
other words: if we are the I2C slave) I created a helper function
Currently (because I am not able to test it using ACPI) it only
supports devicetree declarations.
drivers/i2c/i2c-core.c | 19 +++++++++++++++++++
include/linux/i2c.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3de95a29024c..48e705b23c59 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -3691,6 +3691,25 @@ int i2c_slave_unregister(struct i2c_client *client)
return ret;
}
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+
+int i2c_slave_mode_detect(struct device *dev)

This can be bool instead. Otherwise, looks good to me.

Ok great, thank you.

Post by Rob Herring

Post by Luis Oliveira
+{
+ struct device_node *child;
+ u32 reg;
+
+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
+ for_each_child_of_node(dev->of_node, child) {
+ of_property_read_u32(child, "reg", &reg);
+ if (reg & I2C_OWN_SLAVE_ADDRESS)
+ return 1;
+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_mode_detect);
+
#endif
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b2109c522dec..53cf99569af5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -282,6 +282,7 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+extern int i2c_slave_mode_detect(struct device *dev);
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)
--
2.11.0

[Vladimir Zapolskiy]

Hello Luis,

Post by Luis Oliveira
This function has the purpose of mode detection by checking the
device nodes for a reg matching with the I2C_OWN_SLAVE_ADDREESS flag.
Currently only checks using OF functions (ACPI slave not supported yet).
---
Due to the need of checking if the I2C slave address is our own (in
other words: if we are the I2C slave) I created a helper function
Currently (because I am not able to test it using ACPI) it only
supports devicetree declarations.
drivers/i2c/i2c-core.c | 19 +++++++++++++++++++
include/linux/i2c.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3de95a29024c..48e705b23c59 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -3691,6 +3691,25 @@ int i2c_slave_unregister(struct i2c_client *client)
return ret;
}
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+
+int i2c_slave_mode_detect(struct device *dev)
+{
+ struct device_node *child;
+ u32 reg;
+
+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {

IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
should be sufficient.

But then you may do for_each_child_of_node() loop totally unconditionally,
because of_get_next_child() correctly handle NULL as the first argument.

Post by Luis Oliveira
+ for_each_child_of_node(dev->of_node, child) {
+ of_property_read_u32(child, "reg", &reg);
+ if (reg & I2C_OWN_SLAVE_ADDRESS)
+ return 1;

Leaked incremented reference to a child device node, please add
of_node_put(child) before return in the loop body.

Also reg variable may be uninitialized here, if child device node
does not have "reg" property.

Post by Luis Oliveira
+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }

If so, then it might be better to drop else-if stub for now.

Post by Luis Oliveira
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_mode_detect);
+
#endif
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b2109c522dec..53cf99569af5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -282,6 +282,7 @@ enum i2c_slave_event {
extern int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
extern int i2c_slave_unregister(struct i2c_client *client);
+extern int i2c_slave_mode_detect(struct device *dev);
static inline int i2c_slave_event(struct i2c_client *client,
enum i2c_slave_event event, u8 *val)

--
With best wishes,
Vladimir

[Andy Shevchenko]

Post by Vladimir Zapolskiy

+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {

IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
should be sufficient.

Sorry, but you missed the point.
This will enable compile time optimization and basically be collapsed to no-op.

Post by Vladimir Zapolskiy

+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }

If so, then it might be better to drop else-if stub for now.

Please, don't.

[Vladimir Zapolskiy]

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {

IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
should be sufficient.

Sorry, but you missed the point.
This will enable compile time optimization and basically be collapsed to no-op.

Good point, do you plan to add more "IS_BUILTIN(CONFIG_OF)" checks all
over the code to reduce the size of the built image?

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }

If so, then it might be better to drop else-if stub for now.

Please, don't.

Why do you ask for this stub to be added?

--
With best wishes,
Vladimir

[Andy Shevchenko]

Post by Vladimir Zapolskiy

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {

IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
should be sufficient.

Sorry, but you missed the point.
This will enable compile time optimization and basically be collapsed to no-op.

Good point, do you plan to add more "IS_BUILTIN(CONFIG_OF)" checks all
over the code to reduce the size of the built image?

There is no black and white, don't be silly.

Post by Vladimir Zapolskiy

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }

If so, then it might be better to drop else-if stub for now.

Please, don't.

Why do you ask for this stub to be added?

1. Exactly the reason you asked above. Here is the code which has
built differently on different platforms. x86 usually is not using
CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
existing examples.
2. We might add that support later, but here is again, just no-op.

So, what is your strong argument here against that?

[Vladimir Zapolskiy]

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {

IS_BUILTIN(CONFIG_OF) looks excessive, check for non-NULL dev->of_node
should be sufficient.

Sorry, but you missed the point.
This will enable compile time optimization and basically be collapsed to no-op.

Good point, do you plan to add more "IS_BUILTIN(CONFIG_OF)" checks all
over the code to reduce the size of the built image?

There is no black and white, don't be silly.

Post by Vladimir Zapolskiy

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ }
+ } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported yet\n");
+ }

If so, then it might be better to drop else-if stub for now.

Please, don't.

Why do you ask for this stub to be added?

1. Exactly the reason you asked above. Here is the code which has
built differently on different platforms. x86 usually is not using
CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
existing examples.

From the context by the stub I mean dev_dbg() in i2c_slave_mode_detect()
function, I don't see a connection to GPIO library, please clarify.

Post by Andy Shevchenko
2. We might add that support later, but here is again, just no-op.
So, what is your strong argument here against that?

When the support is ready for ACPI case, you'll remove the added
dev_dbg(), and I don't see a good point by adding it temporarily.

What is wrong with the approach of adding the ACPI case handling
branch when it is ready and remove any kind of stubs right now?

On ACPI platforms the function returns 'false' always, will the
function work correctly (= corresponding to its description) as is?

PS, if it is possible, please give up on arrogance in discussion.

--
With best wishes,
Vladimir

[Andy Shevchenko]

Post by Vladimir Zapolskiy

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+             }

Post by Vladimir Zapolskiy

+     } else if (IS_BUILTIN(CONFIG_ACPI) &&
ACPI_HANDLE(dev)) {
+             dev_dbg(dev, "ACPI slave is not supported
yet\n");
+     }

If so, then it might be better to drop else-if stub for now.

Please, don't.

Why do you ask for this stub to be added?

1. Exactly the reason you asked above. Here is the code which has
built differently on different platforms. x86 usually is not using
CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
existing examples.

From the context by the stub I mean dev_dbg() in
i2c_slave_mode_detect()
function, I don't see a connection to GPIO library, please clarify.

I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro.

Post by Vladimir Zapolskiy

Post by Andy Shevchenko
2. We might add that support later, but here is again, just no-op.
So, what is your strong argument here against that?

When the support is ready for ACPI case, you'll remove the added
dev_dbg(), and I don't see a good point by adding it temporarily.

It would remind me to look at it at some point.

Post by Vladimir Zapolskiy
What is wrong with the approach of adding the ACPI case handling
branch when it is ready and remove any kind of stubs right now?

I will not object. Here is maintainer, let him speak.

Post by Vladimir Zapolskiy
On ACPI platforms the function returns 'false' always, will the
function work correctly (= corresponding to its description) as is?

Yes.

[Luis Oliveira]

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ }

Post by Vladimir Zapolskiy

+ } else if (IS_BUILTIN(CONFIG_ACPI) &&
ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported
yet\n");
+ }

If so, then it might be better to drop else-if stub for now.

Please, don't.

Why do you ask for this stub to be added?

1. Exactly the reason you asked above. Here is the code which has
built differently on different platforms. x86 usually is not using
CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
existing examples.

From the context by the stub I mean dev_dbg() in
i2c_slave_mode_detect()
function, I don't see a connection to GPIO library, please clarify.

I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro.

I can prepare a V3 and remove it if that's the decision.

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

Post by Andy Shevchenko
2. We might add that support later, but here is again, just no-op.
So, what is your strong argument here against that?

When the support is ready for ACPI case, you'll remove the added
dev_dbg(), and I don't see a good point by adding it temporarily.

It would remind me to look at it at some point.

Post by Vladimir Zapolskiy
What is wrong with the approach of adding the ACPI case handling
branch when it is ready and remove any kind of stubs right now?

I will not object. Here is maintainer, let him speak.

Post by Vladimir Zapolskiy
On ACPI platforms the function returns 'false' always, will the
function work correctly (= corresponding to its description) as is?

Yes.

[Vladimir Zapolskiy]

Hello Luis,

Post by Luis Oliveira

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

Post by Andy Shevchenko

Post by Vladimir Zapolskiy

+ }

Post by Vladimir Zapolskiy

+ } else if (IS_BUILTIN(CONFIG_ACPI) &&
ACPI_HANDLE(dev)) {
+ dev_dbg(dev, "ACPI slave is not supported
yet\n");
+ }

If so, then it might be better to drop else-if stub for now.

Please, don't.

Why do you ask for this stub to be added?

1. Exactly the reason you asked above. Here is the code which has
built differently on different platforms. x86 usually is not using
CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
existing examples.

From the context by the stub I mean dev_dbg() in
i2c_slave_mode_detect()
function, I don't see a connection to GPIO library, please clarify.

I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro.

I can prepare a V3 and remove it if that's the decision.

from my review comments you may find that your v2 change contains a bug
plus it misses a minor but desirable code optimization. You may get more
review comments then, for example it is not obvious that on a platform
with both CONFIG_ACPI and CONFIG_OF enabled there should be an exclusive
selection of only one of two possible branches as in your code etc.

The discussed IS_BUILTIN() and dev_dbg() code chunks are other ones, for
both of them you may find my review comments and Andy's responses, it's
up to you as the author to make any updates or keep the code as is,
taking into account any expressed concerns and concerns about concerns
the maintainer will make a decision.

--
With best wishes,
Vladimir

[Andy Shevchenko]

Post by Vladimir Zapolskiy
review comments then, for example it is not obvious that on a platform
with both CONFIG_ACPI and CONFIG_OF enabled there should be an
exclusive
selection of only one of two possible branches as in your code etc.

ACPI and DT approach differently to this property. Like I already said
to you check GPIO library where we have similarities.