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

[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 <lolivei@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 <simon@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)
-- 
2.11.0

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

[Andy Shevchenko]

On Thu, Jan 5, 2017 at 7:24 PM, Luis Oliveira
<Luis.Oliveira@synopsys.com> wrote:
> 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 <andy.shevchenko@gmail.com>

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

> --- 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.

> +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).

> +
> +       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);

-- 
With Best Regards,
Andy Shevchenko

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

[Luis Oliveira]

On 06-Jan-17 16:29, Andy Shevchenko wrote:
> On Thu, Jan 5, 2017 at 7:24 PM, Luis Oliveira
> <Luis.Oliveira@synopsys.com> wrote:
>> 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 <andy.shevchenko@gmail.com>
> 
> 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.

>> --- 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.
> 
>> +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.

> 
>> +
>> +       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);
> 

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

[Andy Shevchenko]

On Fri, Jan 6, 2017 at 7:15 PM, Luis Oliveira
<Luis.Oliveira@synopsys.com> wrote:
> On 06-Jan-17 16:29, Andy Shevchenko wrote:
>> 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.
>>
>>> +int i2c_slave_mode_detect(struct device *dev)

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


-- 
With Best Regards,
Andy Shevchenko

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

[Luis Oliveira]

On 06-Jan-17 17:17, Andy Shevchenko wrote:
> On Fri, Jan 6, 2017 at 7:15 PM, Luis Oliveira
> <Luis.Oliveira@synopsys.com> wrote:
>> On 06-Jan-17 16:29, Andy Shevchenko wrote:
>>> 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.
>>>
>>>> +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.
 */

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

[Andy Shevchenko]

On Fri, Jan 6, 2017 at 7:46 PM, Luis Oliveira
<Luis.Oliveira@synopsys.com> wrote:
> On 06-Jan-17 17:17, Andy Shevchenko wrote:
>> On Fri, Jan 6, 2017 at 7:15 PM, Luis Oliveira
>> <Luis.Oliveira@synopsys.com> wrote:
>>> On 06-Jan-17 16:29, Andy Shevchenko wrote:
>>>> 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.
>>>>
>>>>> +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

a -> an

>  * 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.


>  */



-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Thu, Jan 5, 2017 at 11:24 AM, Luis Oliveira
<Luis.Oliveira@synopsys.com> wrote:
> 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 <lolivei@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)

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

> +{
> +       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 <simon@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)
> --
> 2.11.0
>
>

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

[Luis Oliveira]

On 06-Jan-17 16:35, Rob Herring wrote:
> On Thu, Jan 5, 2017 at 11:24 AM, Luis Oliveira
> <Luis.Oliveira@synopsys.com> wrote:
>> 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 <lolivei@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)
> 
> This can be bool instead. Otherwise, looks good to me.
> 

Ok great, thank you.

>> +{
>> +       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 <simon@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)
>> --
>> 2.11.0
>>
>>

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

[Vladimir Zapolskiy]

Hello Luis,

On 01/05/2017 07:24 PM, Luis Oliveira wrote:
> 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 <lolivei@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) {

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.

> +		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.

> +		}
> +	} 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.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_slave_mode_detect);
> +
>  #endif
>  
>  MODULE_AUTHOR("Simon G. Vogl <simon@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)
> 

--
With best wishes,
Vladimir

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

[Andy Shevchenko]

On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>> +     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.

>> +             }
>> +     } 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Vladimir Zapolskiy]

On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
> On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>>> +     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? 

>>> +             }
>>> +     } 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

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

[Andy Shevchenko]

On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
>> On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>>>> +     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.

>
>>>> +             }
>>>> +     } 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?

-- 
With Best Regards,
Andy Shevchenko

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

[Vladimir Zapolskiy]

On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
>>> On Fri, Jan 6, 2017 at 11:46 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>>>>> +     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.
> 
>>
>>>>> +             }
>>>>> +     } 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.

> 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

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

[Andy Shevchenko]

On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote:
> On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
> > On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com>
> > wrote:
> > > On 01/07/2017 12:45 AM, Andy Shevchenko wrote:

> > > > +             }
> > > > > > +     } 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.

> > 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.

> 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.

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

Yes.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Luis Oliveira]

On 12-Jan-17 17:01, Andy Shevchenko wrote:
> On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote:
>> On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
>>> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com>
>>> wrote:
>>>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
> 
>>>>> +             }
>>>>>>> +     } 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.

> 
>>> 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.
> 
>> 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.
> 
>> On ACPI platforms the function returns 'false' always, will the
>> function work correctly (= corresponding to its description) as is?
> 
> Yes.
> 

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

[Vladimir Zapolskiy]

Hello Luis,

On 01/16/2017 12:32 PM, Luis Oliveira wrote:
> On 12-Jan-17 17:01, Andy Shevchenko wrote:
>> On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote:
>>> On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
>>>> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com>
>>>> wrote:
>>>>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
>>
>>>>>> +             }
>>>>>>>> +     } 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

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

[Andy Shevchenko]

On Tue, 2017-01-17 at 01:14 +0200, Vladimir Zapolskiy wrote:


> 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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy