diff mbox series

[v5,09/15] i2c: acpi: Introduce i2c_acpi_get_i2c_resource() helper

Message ID 20181128114535.80223-10-andriy.shevchenko@linux.intel.com (mailing list archive)
State Accepted, archived
Headers show
Series i2c-multi-instantiate: Adapt for INT3515 and alike | expand

Commit Message

Andy Shevchenko Nov. 28, 2018, 11:45 a.m. UTC 
Besides current two users one more is coming. Definitely makes sense to
introduce a helper.

No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 41 ++++++++++++++++++++++++++-----------
 include/linux/acpi.h        | 11 ++++++++++
 2 files changed, 40 insertions(+), 12 deletions(-)

Comments

Mika Westerberg Nov. 28, 2018, 2:47 p.m. UTC | #1 
On Wed, Nov 28, 2018 at 01:45:29PM +0200, Andy Shevchenko wrote:
> Besides current two users one more is coming. Definitely makes sense to
> introduce a helper.
> 
> No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Wolfram Sang Nov. 30, 2018, 9:55 a.m. UTC | #2 
On Wed, Nov 28, 2018 at 01:45:29PM +0200, Andy Shevchenko wrote:
> Besides current two users one more is coming. Definitely makes sense to
> introduce a helper.
> 
> No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/i2c/i2c-core-acpi.c | 41 ++++++++++++++++++++++++++-----------
>  include/linux/acpi.h        | 11 ++++++++++

Any reason this is not in i2c.h?
Andy Shevchenko Nov. 30, 2018, 10:57 a.m. UTC | #3 
On Fri, Nov 30, 2018 at 10:55:33AM +0100, Wolfram Sang wrote:
> On Wed, Nov 28, 2018 at 01:45:29PM +0200, Andy Shevchenko wrote:
> > Besides current two users one more is coming. Definitely makes sense to
> > introduce a helper.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/i2c/i2c-core-acpi.c | 41 ++++++++++++++++++++++++++-----------
> >  include/linux/acpi.h        | 11 ++++++++++
> 
> Any reason this is not in i2c.h?

Yes. As I explained earlier to some people there are facts affecting this:
- the function is operate on top of solely ACPI structures
- there is already similar function for GPIO and it stays like that

On top of this recently I've discovered, that i2c.h has separate #ifdef for I2C
and ACPI which theoretically might produce a linker error in some cases.
Wolfram Sang Nov. 30, 2018, 11:06 a.m. UTC | #4 
> > >  drivers/i2c/i2c-core-acpi.c | 41 ++++++++++++++++++++++++++-----------
> > >  include/linux/acpi.h        | 11 ++++++++++
> > 
> > Any reason this is not in i2c.h?
> 
> Yes. As I explained earlier to some people there are facts affecting this:
> - the function is operate on top of solely ACPI structures

And moving the function itself to the ACPI realm then?

I don't say this is a show-stopper for this series, but I just wonder...

> - there is already similar function for GPIO and it stays like that
> 
> On top of this recently I've discovered, that i2c.h has separate #ifdef for I2C
> and ACPI which theoretically might produce a linker error in some cases.

Is this something we have to live with or which can be cleaned up
somewhen?
Andy Shevchenko Nov. 30, 2018, 11:45 a.m. UTC | #5 
On Fri, Nov 30, 2018 at 12:06:52PM +0100, Wolfram Sang wrote:
> > > >  drivers/i2c/i2c-core-acpi.c | 41 ++++++++++++++++++++++++++-----------
> > > >  include/linux/acpi.h        | 11 ++++++++++
> > > 
> > > Any reason this is not in i2c.h?
> > 
> > Yes. As I explained earlier to some people there are facts affecting this:
> > - the function is operate on top of solely ACPI structures
> 
> And moving the function itself to the ACPI realm then?

Unfortunately I don't see the place where it fits good. See below.

> I don't say this is a show-stopper for this series, but I just wonder...
> 
> > - there is already similar function for GPIO and it stays like that
> > 
> > On top of this recently I've discovered, that i2c.h has separate #ifdef for I2C
> > and ACPI which theoretically might produce a linker error in some cases.
> 
> Is this something we have to live with or which can be cleaned up
> somewhen?

My opinion that we might need something like
drivers/acpi/acpi_i2c_lib.c
drivers/acpi/acpi_gpio_lib.c
etc.

But better to ask Rafael and Mika what they think about this.
Wolfram Sang Nov. 30, 2018, 11:49 a.m. UTC | #6 
> > Is this something we have to live with or which can be cleaned up
> > somewhen?
> 
> My opinion that we might need something like
> drivers/acpi/acpi_i2c_lib.c
> drivers/acpi/acpi_gpio_lib.c
> etc.
>

Collect all of them in drivers/acpi/acpi_libs.c?

> But better to ask Rafael and Mika what they think about this.

Sure!
Mika Westerberg Nov. 30, 2018, 12:12 p.m. UTC | #7 
On Fri, Nov 30, 2018 at 12:49:50PM +0100, Wolfram Sang wrote:
> 
> > > Is this something we have to live with or which can be cleaned up
> > > somewhen?
> > 
> > My opinion that we might need something like
> > drivers/acpi/acpi_i2c_lib.c
> > drivers/acpi/acpi_gpio_lib.c
> > etc.
> >
> 
> Collect all of them in drivers/acpi/acpi_libs.c?
> 
> > But better to ask Rafael and Mika what they think about this.

IMHO all the bus specific ACPI things should go under those buses (so
the opposite what is proposed here) but I also don't think any of this
is show stopper for the patch series under discussion ;-)
Wolfram Sang Nov. 30, 2018, 12:16 p.m. UTC | #8 
On Wed, Nov 28, 2018 at 01:45:29PM +0200, Andy Shevchenko wrote:
> Besides current two users one more is coming. Definitely makes sense to
> introduce a helper.
> 
> No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>
Andy Shevchenko Nov. 30, 2018, 12:17 p.m. UTC | #9 
On Fri, Nov 30, 2018 at 02:12:31PM +0200, Mika Westerberg wrote:
> On Fri, Nov 30, 2018 at 12:49:50PM +0100, Wolfram Sang wrote:
> > 
> > > > Is this something we have to live with or which can be cleaned up
> > > > somewhen?
> > > 
> > > My opinion that we might need something like
> > > drivers/acpi/acpi_i2c_lib.c
> > > drivers/acpi/acpi_gpio_lib.c
> > > etc.
> > >
> > 
> > Collect all of them in drivers/acpi/acpi_libs.c?
> > 
> > > But better to ask Rafael and Mika what they think about this.
> 
> IMHO all the bus specific ACPI things should go under those buses (so
> the opposite what is proposed here) but I also don't think any of this
> is show stopper for the patch series under discussion ;-)

Thank you guys for your input, let me fix this later, out of this series.
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8a88586e0902..272800692088 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -45,6 +45,33 @@  struct i2c_acpi_lookup {
 	u32 min_speed;
 };
 
+/**
+ * i2c_acpi_get_i2c_resource - Gets I2cSerialBus resource if type matches
+ * @ares:	ACPI resource
+ * @i2c:	Pointer to I2cSerialBus resource will be returned here
+ *
+ * Checks if the given ACPI resource is of type I2cSerialBus.
+ * In this case, returns a pointer to it to the caller.
+ *
+ * Returns true if resource type is of I2cSerialBus, otherwise false.
+ */
+bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+			       struct acpi_resource_i2c_serialbus **i2c)
+{
+	struct acpi_resource_i2c_serialbus *sb;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return false;
+
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return false;
+
+	*i2c = sb;
+	return true;
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_get_i2c_resource);
+
 static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 {
 	struct i2c_acpi_lookup *lookup = data;
@@ -52,11 +79,7 @@  static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 	struct acpi_resource_i2c_serialbus *sb;
 	acpi_status status;
 
-	if (info->addr || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
-		return 1;
-
-	sb = &ares->data.i2c_serial_bus;
-	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+	if (info->addr || !i2c_acpi_get_i2c_resource(ares, &sb))
 		return 1;
 
 	if (lookup->index != -1 && lookup->n++ != lookup->index)
@@ -534,13 +557,7 @@  i2c_acpi_space_handler(u32 function, acpi_physical_address command,
 		goto err;
 	}
 
-	if (!value64 || ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-		ret = AE_BAD_PARAMETER;
-		goto err;
-	}
-
-	sb = &ares->data.i2c_serial_bus;
-	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+	if (!value64 || !i2c_acpi_get_i2c_resource(ares, &sb)) {
 		ret = AE_BAD_PARAMETER;
 		goto err;
 	}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ed80f147bd50..6afc6e3c4c5c 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1054,6 +1054,17 @@  static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 }
 #endif
 
+#if defined(CONFIG_ACPI) && IS_ENABLED(CONFIG_I2C)
+bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+			       struct acpi_resource_i2c_serialbus **i2c);
+#else
+static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
+					     struct acpi_resource_i2c_serialbus **i2c)
+{
+	return false;
+}
+#endif
+
 /* Device properties */
 
 #ifdef CONFIG_ACPI