[PATCH v1] i2c: icy: Don't use software node when it's an overkill

[PATCH v1] i2c: icy: Don't use software node when it's an overkill

[Andy Shevchenko]

There is no evidence, including commit message of the original change,
that software node should be used in the simple case where we would like
to instantiate an I²C client from board info. Board info contains a member
to pass device properties for long time. Let's use a simple way over
the complicated software node approach.

Fixes: 724041ae15ed ("i2c: icy: Add LTC2990 present on 2019 board revision")
Cc: Max Staudt <max@enpas.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-icy.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-icy.c b/drivers/i2c/busses/i2c-icy.c
index 271470f4d8a9..ce68c436f06e 100644
--- a/drivers/i2c/busses/i2c-icy.c
+++ b/drivers/i2c/busses/i2c-icy.c
@@ -53,7 +53,7 @@ struct icy_i2c {
 
 	void __iomem *reg_s0;
 	void __iomem *reg_s1;
-	struct fwnode_handle *ltc2990_fwnode;
+
 	struct i2c_client *ltc2990_client;
 };
 
@@ -109,7 +109,7 @@ static unsigned short const icy_ltc2990_addresses[] = {
  */
 static const u32 icy_ltc2990_meas_mode[] = {0, 3};
 
-static const struct property_entry icy_ltc2990_props[] = {
+static const struct property_entry icy_ltc2990_properties[] = {
 	PROPERTY_ENTRY_U32_ARRAY("lltc,meas-mode", icy_ltc2990_meas_mode),
 	{ }
 };
@@ -122,6 +122,7 @@ static int icy_probe(struct zorro_dev *z,
 	struct fwnode_handle *new_fwnode;
 	struct i2c_board_info ltc2990_info = {
 		.type		= "ltc2990",
+		.properties	= icy_ltc2990_properties,
 	};
 
 	i2c = devm_kzalloc(&z->dev, sizeof(*i2c), GFP_KERNEL);
@@ -173,25 +174,10 @@ static int icy_probe(struct zorro_dev *z,
 	 *
 	 * See property_entry above for in1, in2, temp3.
 	 */
-	new_fwnode = fwnode_create_software_node(icy_ltc2990_props, NULL);
-	if (IS_ERR(new_fwnode)) {
-		dev_info(&z->dev, "Failed to create fwnode for LTC2990, error: %ld\n",
-			 PTR_ERR(new_fwnode));
-	} else {
-		/*
-		 * Store the fwnode so we can destroy it on .remove().
-		 * Only store it on success, as fwnode_remove_software_node()
-		 * is NULL safe, but not PTR_ERR safe.
-		 */
-		i2c->ltc2990_fwnode = new_fwnode;
-		ltc2990_info.fwnode = new_fwnode;
-
-		i2c->ltc2990_client =
-			i2c_new_scanned_device(&i2c->adapter,
+	i2c->ltc2990_client = i2c_new_scanned_device(&i2c->adapter,
 					       &ltc2990_info,
 					       icy_ltc2990_addresses,
 					       NULL);
-	}
 
 	return 0;
 }
@@ -201,7 +187,6 @@ static void icy_remove(struct zorro_dev *z)
 	struct icy_i2c *i2c = dev_get_drvdata(&z->dev);
 
 	i2c_unregister_device(i2c->ltc2990_client);
-	fwnode_remove_software_node(i2c->ltc2990_fwnode);
 
 	i2c_del_adapter(&i2c->adapter);
 }
-- 
2.25.1

Re: [PATCH v1] i2c: icy: Don't use software node when it's an overkill

[Max Staudt]

Thank you, that looks much nicer indeed. I have also tested it on my Amiga just to be sure - it works.


However, no signature yet because the call stack is giving me a headache:

icy_probe()
 -> i2c_new_scanned_device()
 -> i2c_new_client_device()
 -> device_add_properties()

And here we are in drivers/base/property.c looking at device_add_properties():

 * WARNING: The callers should not use this function if it is known that there
 * is no real firmware node associated with @dev! In that case the callers
 * should create a software node and assign it to @dev directly.


Why is this warning there? It flies right in the face of what we're trying to achieve here.

It was introduced in 2018 with commit caf35cd52242 .


So either the warning is superfluous, or i2c_new_client_device() should be creating a software fwnode, I guess?


Thanks
Max

Re: [PATCH v1] i2c: icy: Don't use software node when it's an overkill

[Andy Shevchenko]

On Wed, Apr 08, 2020 at 11:24:20PM +0200, Max Staudt wrote:
> Thank you, that looks much nicer indeed. I have also tested it on my Amiga just to be sure - it works.

Thank you.

> However, no signature yet because the call stack is giving me a headache:
> 
> icy_probe()
>  -> i2c_new_scanned_device()
>  -> i2c_new_client_device()
>  -> device_add_properties()
> 
> And here we are in drivers/base/property.c looking at device_add_properties():
> 
>  * WARNING: The callers should not use this function if it is known that there
>  * is no real firmware node associated with @dev! In that case the callers
>  * should create a software node and assign it to @dev directly.
> 
> 
> Why is this warning there? It flies right in the face of what we're trying to achieve here.
> 
> It was introduced in 2018 with commit caf35cd52242 .
> 
> 
> So either the warning is superfluous, or i2c_new_client_device() should be creating a software fwnode, I guess?

No and no.

First one because the mechanism is added to have quirks, it must not replace
the actual possibility to provide this via firmware (DT / ACPI).

Second one, because software node API should have (has?) the same warning.

+Cc Heikki.

Heikki, am I correct?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] i2c: icy: Don't use software node when it's an overkill

[Heikki Krogerus]

On Thu, Apr 09, 2020 at 01:37:35PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 08, 2020 at 11:24:20PM +0200, Max Staudt wrote:
> > Thank you, that looks much nicer indeed. I have also tested it on my Amiga just to be sure - it works.
> 
> Thank you.
> 
> > However, no signature yet because the call stack is giving me a headache:
> > 
> > icy_probe()
> >  -> i2c_new_scanned_device()
> >  -> i2c_new_client_device()
> >  -> device_add_properties()
> > 
> > And here we are in drivers/base/property.c looking at device_add_properties():
> > 
> >  * WARNING: The callers should not use this function if it is known that there
> >  * is no real firmware node associated with @dev! In that case the callers
> >  * should create a software node and assign it to @dev directly.
> > 
> > 
> > Why is this warning there? It flies right in the face of what we're trying to achieve here.
> > 
> > It was introduced in 2018 with commit caf35cd52242 .
> > 
> > 
> > So either the warning is superfluous, or i2c_new_client_device() should be creating a software fwnode, I guess?
> 
> No and no.
> 
> First one because the mechanism is added to have quirks, it must not replace
> the actual possibility to provide this via firmware (DT / ACPI).
> 
> Second one, because software node API should have (has?) the same warning.
> 
> +Cc Heikki.
> 
> Heikki, am I correct?

In this case it should be possible supply a handle to a software node
with the board info. That should then later replace the fwnode and
properties members once the existing code is converted:

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f6b942150631..648ea384d480 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -409,8 +409,7 @@ struct i2c_board_info {
        const char      *dev_name;
        void            *platform_data;
        struct device_node *of_node;
-       struct fwnode_handle *fwnode;
-       const struct property_entry *properties;
+       const struct software_node *swnode
        const struct resource *resources;
        unsigned int    num_resources;
        int             irq;

I2C core would then need to take care of registering that swnode of
course.

thanks,

-- 
heikki

Re: [PATCH v1] i2c: icy: Don't use software node when it's an overkill

[Max Staudt]

On 4/9/20 2:16 PM, Heikki Krogerus wrote:
> On Thu, Apr 09, 2020 at 01:37:35PM +0300, Andy Shevchenko wrote:
>> Heikki, am I correct?
> 
> In this case it should be possible supply a handle to a software node
> with the board info. That should then later replace the fwnode and
> properties members once the existing code is converted:
> 
> [... snip sample patch ...]
> 
> I2C core would then need to take care of registering that swnode of
> course.

Are you saying that the comment in property.c is correct, and i2c_new_client_device() shall *not* call device_add_properties() ?

I mean, the code works and stuff, except that the swnode that device_add_properties() created won't be freed as far as I can see.


In other words, should the current properties code in i2c_new_client_device() be replaced by something that creates a swnode, just like the i2c-icy driver currently does manually when it instantiates the ltc2990 I2C client?


Max

Re: [PATCH v1] i2c: icy: Don't use software node when it's an overkill

[Heikki Krogerus]

On Fri, Apr 10, 2020 at 01:51:58AM +0200, Max Staudt wrote:
> On 4/9/20 2:16 PM, Heikki Krogerus wrote:
> > On Thu, Apr 09, 2020 at 01:37:35PM +0300, Andy Shevchenko wrote:
> >> Heikki, am I correct?
> > 
> > In this case it should be possible supply a handle to a software node
> > with the board info. That should then later replace the fwnode and
> > properties members once the existing code is converted:
> > 
> > [... snip sample patch ...]
> > 
> > I2C core would then need to take care of registering that swnode of
> > course.
> 
> Are you saying that the comment in property.c is correct, and
> i2c_new_client_device() shall *not* call device_add_properties() ?
> 
> I mean, the code works and stuff, except that the swnode that
> device_add_properties() created won't be freed as far as I can see.

They actually are freed when device_del() is finally called, which is
pretty damn confusing. That is actually one of the reasons why we
should avoid the old device_add/del_properties() API.

> In other words, should the current properties code in i2c_new_client_device()
> be replaced by something that creates a swnode, just like the i2c-icy driver
> currently does manually when it instantiates the ltc2990 I2C client?

Yes. The subsystem needs to take care of that, not the drivers.

thanks,

-- 
heikki