[RFC PATCH 0/2] i2c: core: Introduce i2c_client_get_device_id helper

[RFC PATCH 0/2] i2c: core: Introduce i2c_client_get_device_id helper

[Angel Iglesias]

Hello,

I don't want to step anyone's work here, so I'm sending this RFC to the
devs involved in the original discussion. I read on Uwe Kleine-König's
patchset submission thread the necessity for an i2c helper to aid with the
migration to the new i2c_driver .probe_new callback. Following the
suggestions made there, I wrote this small patchset implementing the
suggested helper function and ported the bmp280 IIO i2c probe to the new
probe using that helper.

Thanks for your time!
Angel

Original discussion thread for additional context:
https://lore.kernel.org/all/20221023132302.911644-11-u.kleine-koenig@pengutronix.de/

Angel Iglesias (2):
  i2c: core: Introduce i2c_client_get_device_id helper function
  iio: pressure: bmp280: convert to i2c's .probe_new()

 drivers/i2c/i2c-core-base.c       | 15 +++++++++++++++
 drivers/iio/pressure/bmp280-i2c.c |  8 ++++----
 include/linux/i2c.h               |  1 +
 3 files changed, 20 insertions(+), 4 deletions(-)


base-commit: c32793afc6976e170f6ab11ca3750fe94fb3454d
-- 
2.38.1

[RFC PATCH 1/2] i2c: core: Introduce i2c_client_get_device_id helper function

[Angel Iglesias]

Introduces new helper function to aid in .probe_new() refactors. In order
to use existing i2c_get_device_id() on the probe callback, the device
match table needs to be accessible in that function, which would require
bigger refactors in some drivers using the deprecated .probe callback.

This issue was discussed in more detail in the IIO mailing list.

Link: https://lore.kernel.org/all/20221023132302.911644-11-u.kleine-koenig@pengutronix.de/
Suggested-by: Nuno Sá <noname.nuno@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
---
 drivers/i2c/i2c-core-base.c | 15 +++++++++++++++
 include/linux/i2c.h         |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index b4edf10e8fd0..e6623226e393 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2236,6 +2236,21 @@ int i2c_get_device_id(const struct i2c_client *client,
 }
 EXPORT_SYMBOL_GPL(i2c_get_device_id);
 
+/**
+ * i2c_client_get_device_id - get the driver match table entry of a device
+ * @client: the device to query
+ *
+ * Returns a pointer to the matching entry if found, NULL otherwise.
+ */
+const struct i2c_device_id *i2c_client_get_device_id(const struct i2c_client *client)
+{
+	const struct i2c_driver *drv = to_i2c_driver(client->dev.driver);
+
+	return i2c_match_id(drv->id_table, client);
+
+}
+EXPORT_SYMBOL_GPL(i2c_client_get_device_id);
+
 /* ----------------------------------------------------
  * the i2c address scanning function
  * Will not work for 10-bit addresses!
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f7c49bbdb8a1..d84e0e99f084 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -189,6 +189,7 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 					      u8 *values);
 int i2c_get_device_id(const struct i2c_client *client,
 		      struct i2c_device_identity *id);
+const struct i2c_device_id *i2c_client_get_device_id(const struct i2c_client *client);
 #endif /* I2C */
 
 /**
-- 
2.38.1

Re: [RFC PATCH 1/2] i2c: core: Introduce i2c_client_get_device_id helper function

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On Sun, Oct 30, 2022 at 06:52:18PM +0100, Angel Iglesias wrote:
> Introduces new helper function to aid in .probe_new() refactors. In order
> to use existing i2c_get_device_id() on the probe callback, the device
> match table needs to be accessible in that function, which would require
> bigger refactors in some drivers using the deprecated .probe callback.
> 
> This issue was discussed in more detail in the IIO mailing list.
> 
> Link: https://lore.kernel.org/all/20221023132302.911644-11-u.kleine-koenig@pengutronix.de/
> Suggested-by: Nuno Sá <noname.nuno@gmail.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>

Looks good to me!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/2] i2c: core: Introduce i2c_client_get_device_id helper function

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

On Sun, Oct 30, 2022 at 06:52:18PM +0100, Angel Iglesias wrote:
> Introduces new helper function to aid in .probe_new() refactors. In order
> to use existing i2c_get_device_id() on the probe callback, the device
> match table needs to be accessible in that function, which would require
> bigger refactors in some drivers using the deprecated .probe callback.
> 
> This issue was discussed in more detail in the IIO mailing list.
> 
> Link: https://lore.kernel.org/all/20221023132302.911644-11-u.kleine-koenig@pengutronix.de/
> Suggested-by: Nuno Sá <noname.nuno@gmail.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> ---
>  drivers/i2c/i2c-core-base.c | 15 +++++++++++++++
>  include/linux/i2c.h         |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index b4edf10e8fd0..e6623226e393 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2236,6 +2236,21 @@ int i2c_get_device_id(const struct i2c_client *client,
>  }
>  EXPORT_SYMBOL_GPL(i2c_get_device_id);
>  
> +/**
> + * i2c_client_get_device_id - get the driver match table entry of a device
> + * @client: the device to query
> + *
> + * Returns a pointer to the matching entry if found, NULL otherwise.
> + */
> +const struct i2c_device_id *i2c_client_get_device_id(const struct i2c_client *client)
> +{
> +	const struct i2c_driver *drv = to_i2c_driver(client->dev.driver);
> +
> +	return i2c_match_id(drv->id_table, client);
> +

I'd drop the empty line after the return. Is it worth to note in a
comment that it only works for bound clients? (Oopses otherwise)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/2] i2c: core: Introduce i2c_client_get_device_id helper function

[Angel Iglesias]

On Tue, 2022-11-01 at 15:54 +0100, Uwe Kleine-König wrote:
> On Sun, Oct 30, 2022 at 06:52:18PM +0100, Angel Iglesias wrote:
> > Introduces new helper function to aid in .probe_new() refactors. In order
> > to use existing i2c_get_device_id() on the probe callback, the device
> > match table needs to be accessible in that function, which would require
> > bigger refactors in some drivers using the deprecated .probe callback.
> > 
> > This issue was discussed in more detail in the IIO mailing list.
> > 
> > Link:
> > https://lore.kernel.org/all/20221023132302.911644-11-u.kleine-koenig@pengutronix.de/
> > Suggested-by: Nuno Sá <noname.nuno@gmail.com>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> > ---
> >  drivers/i2c/i2c-core-base.c | 15 +++++++++++++++
> >  include/linux/i2c.h         |  1 +
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index b4edf10e8fd0..e6623226e393 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -2236,6 +2236,21 @@ int i2c_get_device_id(const struct i2c_client
> > *client,
> >  }
> >  EXPORT_SYMBOL_GPL(i2c_get_device_id);
> >  
> > +/**
> > + * i2c_client_get_device_id - get the driver match table entry of a device
> > + * @client: the device to query
> > + *
> > + * Returns a pointer to the matching entry if found, NULL otherwise.
> > + */
> > +const struct i2c_device_id *i2c_client_get_device_id(const struct
> > i2c_client *client)
> > +{
> > +       const struct i2c_driver *drv = to_i2c_driver(client->dev.driver);
> > +
> > +       return i2c_match_id(drv->id_table, client);
> > +
> 
> I'd drop the empty line after the return. Is it worth to note in a
> comment that it only works for bound clients? (Oopses otherwise)

Yup, my bad, that newline shouldn't be there.
Sure, seems reasonable to leave warn just in case.

Kind regards,
Angel

> Best regards
> Uwe
> 

[RFC PATCH 2/2] iio: pressure: bmp280: convert to i2c's .probe_new()

[Angel Iglesias]

Use i2c_client_get_device_id() to get the i2c_device_id* parameter in the
.new_probe() callback.

Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
---
 drivers/iio/pressure/bmp280-i2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
index 0c27211f3ea0..20073b09b3e3 100644
--- a/drivers/iio/pressure/bmp280-i2c.c
+++ b/drivers/iio/pressure/bmp280-i2c.c
@@ -5,11 +5,11 @@
 
 #include "bmp280.h"
 
-static int bmp280_i2c_probe(struct i2c_client *client,
-			    const struct i2c_device_id *id)
+static int bmp280_i2c_probe(struct i2c_client *client)
 {
-	struct regmap *regmap;
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	const struct regmap_config *regmap_config;
+	struct regmap *regmap;
 
 	switch (id->driver_data) {
 	case BMP180_CHIP_ID:
@@ -65,7 +65,7 @@ static struct i2c_driver bmp280_i2c_driver = {
 		.of_match_table = bmp280_of_i2c_match,
 		.pm = pm_ptr(&bmp280_dev_pm_ops),
 	},
-	.probe		= bmp280_i2c_probe,
+	.probe_new	= bmp280_i2c_probe,
 	.id_table	= bmp280_i2c_id,
 };
 module_i2c_driver(bmp280_i2c_driver);
-- 
2.38.1

Re: [RFC PATCH 2/2] iio: pressure: bmp280: convert to i2c's .probe_new()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

Hello,

On Sun, Oct 30, 2022 at 06:53:11PM +0100, Angel Iglesias wrote:
> Use i2c_client_get_device_id() to get the i2c_device_id* parameter in the
> .new_probe() callback.
> 
> Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-i2c.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-i2c.c b/drivers/iio/pressure/bmp280-i2c.c
> index 0c27211f3ea0..20073b09b3e3 100644
> --- a/drivers/iio/pressure/bmp280-i2c.c
> +++ b/drivers/iio/pressure/bmp280-i2c.c
> @@ -5,11 +5,11 @@
>  
>  #include "bmp280.h"
>  
> -static int bmp280_i2c_probe(struct i2c_client *client,
> -			    const struct i2c_device_id *id)
> +static int bmp280_i2c_probe(struct i2c_client *client)
>  {
> -	struct regmap *regmap;
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  	const struct regmap_config *regmap_config;
> +	struct regmap *regmap;
>  
>  	switch (id->driver_data) {
>  	case BMP180_CHIP_ID:

What is the motivation for moving regmap? I thought reverse christmas
tree is only a thing in network code? I would have left the regmap
declaration where it is.

> @@ -65,7 +65,7 @@ static struct i2c_driver bmp280_i2c_driver = {
>  		.of_match_table = bmp280_of_i2c_match,
>  		.pm = pm_ptr(&bmp280_dev_pm_ops),
>  	},
> -	.probe		= bmp280_i2c_probe,
> +	.probe_new	= bmp280_i2c_probe,
>  	.id_table	= bmp280_i2c_id,
>  };
>  module_i2c_driver(bmp280_i2c_driver);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/2] iio: pressure: bmp280: convert to i2c's .probe_new()

[Angel Iglesias]

On Tue, 2022-11-01 at 22:52 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Oct 30, 2022 at 06:53:11PM +0100, Angel Iglesias wrote:
> > Use i2c_client_get_device_id() to get the i2c_device_id* parameter in the
> > .new_probe() callback.
> > 
> > Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> > ---
> >  drivers/iio/pressure/bmp280-i2c.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-i2c.c
> > b/drivers/iio/pressure/bmp280-i2c.c
> > index 0c27211f3ea0..20073b09b3e3 100644
> > --- a/drivers/iio/pressure/bmp280-i2c.c
> > +++ b/drivers/iio/pressure/bmp280-i2c.c
> > @@ -5,11 +5,11 @@
> >  
> >  #include "bmp280.h"
> >  
> > -static int bmp280_i2c_probe(struct i2c_client *client,
> > -                           const struct i2c_device_id *id)
> > +static int bmp280_i2c_probe(struct i2c_client *client)
> >  {
> > -       struct regmap *regmap;
> > +       const struct i2c_device_id *id = i2c_client_get_device_id(client);
> >         const struct regmap_config *regmap_config;
> > +       struct regmap *regmap;
> >  
> >         switch (id->driver_data) {
> >         case BMP180_CHIP_ID:
> 
> What is the motivation for moving regmap? I thought reverse christmas
> tree is only a thing in network code? I would have left the regmap
> declaration where it is.

Long story short, I worked previously on a small refactor of this driver to add
support for a new family of sensors. During the different iterations of the
patchset, one thing that was agreed was unifying the driver coding style to
reverse xmas tree. For some extra context, here's the thread:
https://lore.kernel.org/all/20220814145249.701f1261@jic23-huawei/

> > @@ -65,7 +65,7 @@ static struct i2c_driver bmp280_i2c_driver = {
> >                 .of_match_table = bmp280_of_i2c_match,
> >                 .pm = pm_ptr(&bmp280_dev_pm_ops),
> >         },
> > -       .probe          = bmp280_i2c_probe,
> > +       .probe_new      = bmp280_i2c_probe,
> >         .id_table       = bmp280_i2c_id,
> >  };
> >  module_i2c_driver(bmp280_i2c_driver);
> 
> Best regards
> Uwe
> 
Kind regards
Angel

Re: [RFC PATCH 2/2] iio: pressure: bmp280: convert to i2c's .probe_new()

[Jonathan Cameron]

On Wed, 02 Nov 2022 01:16:44 +0100
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> On Tue, 2022-11-01 at 22:52 +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Sun, Oct 30, 2022 at 06:53:11PM +0100, Angel Iglesias wrote:  
> > > Use i2c_client_get_device_id() to get the i2c_device_id* parameter in the
> > > .new_probe() callback.
> > > 
> > > Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> > > ---
> > >  drivers/iio/pressure/bmp280-i2c.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/iio/pressure/bmp280-i2c.c
> > > b/drivers/iio/pressure/bmp280-i2c.c
> > > index 0c27211f3ea0..20073b09b3e3 100644
> > > --- a/drivers/iio/pressure/bmp280-i2c.c
> > > +++ b/drivers/iio/pressure/bmp280-i2c.c
> > > @@ -5,11 +5,11 @@
> > >  
> > >  #include "bmp280.h"
> > >  
> > > -static int bmp280_i2c_probe(struct i2c_client *client,
> > > -                           const struct i2c_device_id *id)
> > > +static int bmp280_i2c_probe(struct i2c_client *client)
> > >  {
> > > -       struct regmap *regmap;
> > > +       const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > >         const struct regmap_config *regmap_config;
> > > +       struct regmap *regmap;
> > >  
> > >         switch (id->driver_data) {
> > >         case BMP180_CHIP_ID:  
> > 
> > What is the motivation for moving regmap? I thought reverse christmas
> > tree is only a thing in network code? I would have left the regmap
> > declaration where it is.  
> 
> Long story short, I worked previously on a small refactor of this driver to add
> support for a new family of sensors. During the different iterations of the
> patchset, one thing that was agreed was unifying the driver coding style to
> reverse xmas tree. For some extra context, here's the thread:
> https://lore.kernel.org/all/20220814145249.701f1261@jic23-huawei/

Not something I feel strongly enough about either way, but has benefit of
consistency. However, it's an unrelated change in this patch, so drop it
to avoid the noise in a patch where you have more significant changes.

Jonathan

> 
> > > @@ -65,7 +65,7 @@ static struct i2c_driver bmp280_i2c_driver = {
> > >                 .of_match_table = bmp280_of_i2c_match,
> > >                 .pm = pm_ptr(&bmp280_dev_pm_ops),
> > >         },
> > > -       .probe          = bmp280_i2c_probe,
> > > +       .probe_new      = bmp280_i2c_probe,
> > >         .id_table       = bmp280_i2c_id,
> > >  };
> > >  module_i2c_driver(bmp280_i2c_driver);  
> > 
> > Best regards
> > Uwe
> >   
> Kind regards
> Angel

Re: [RFC PATCH 0/2] i2c: core: Introduce i2c_client_get_device_id helper

[Andy Shevchenko]

On Sun, Oct 30, 2022 at 06:51:06PM +0100, Angel Iglesias wrote:
> Hello,
> 
> I don't want to step anyone's work here, so I'm sending this RFC to the
> devs involved in the original discussion. I read on Uwe Kleine-König's
> patchset submission thread the necessity for an i2c helper to aid with the
> migration to the new i2c_driver .probe_new callback. Following the
> suggestions made there, I wrote this small patchset implementing the
> suggested helper function and ported the bmp280 IIO i2c probe to the new
> probe using that helper.

For the entire series (please drop RFC in the next version)
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Thanks for your time!
> Angel
> 
> Original discussion thread for additional context:
> https://lore.kernel.org/all/20221023132302.911644-11-u.kleine-koenig@pengutronix.de/
> 
> Angel Iglesias (2):
>   i2c: core: Introduce i2c_client_get_device_id helper function
>   iio: pressure: bmp280: convert to i2c's .probe_new()
> 
>  drivers/i2c/i2c-core-base.c       | 15 +++++++++++++++
>  drivers/iio/pressure/bmp280-i2c.c |  8 ++++----
>  include/linux/i2c.h               |  1 +
>  3 files changed, 20 insertions(+), 4 deletions(-)
> 
> 
> base-commit: c32793afc6976e170f6ab11ca3750fe94fb3454d
> -- 
> 2.38.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] i2c: core: Introduce i2c_client_get_device_id helper

[Jonathan Cameron]

On Tue, 1 Nov 2022 16:58:13 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Oct 30, 2022 at 06:51:06PM +0100, Angel Iglesias wrote:
> > Hello,
> > 
> > I don't want to step anyone's work here, so I'm sending this RFC to the
> > devs involved in the original discussion. I read on Uwe Kleine-König's
> > patchset submission thread the necessity for an i2c helper to aid with the
> > migration to the new i2c_driver .probe_new callback. Following the
> > suggestions made there, I wrote this small patchset implementing the
> > suggested helper function and ported the bmp280 IIO i2c probe to the new
> > probe using that helper.  
> 
> For the entire series (please drop RFC in the next version)
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I'm happy to pick up the next version but a question on 'route' in to the kernel.

I can do an immutable branch with just the new function call in it if
that is useful given I assume this is applicable across a bunch of subsystems?

Jonathan

> 
> > Thanks for your time!
> > Angel
> > 
> > Original discussion thread for additional context:
> > https://lore.kernel.org/all/20221023132302.911644-11-u.kleine-koenig@pengutronix.de/
> > 
> > Angel Iglesias (2):
> >   i2c: core: Introduce i2c_client_get_device_id helper function
> >   iio: pressure: bmp280: convert to i2c's .probe_new()
> > 
> >  drivers/i2c/i2c-core-base.c       | 15 +++++++++++++++
> >  drivers/iio/pressure/bmp280-i2c.c |  8 ++++----
> >  include/linux/i2c.h               |  1 +
> >  3 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > 
> > base-commit: c32793afc6976e170f6ab11ca3750fe94fb3454d
> > -- 
> > 2.38.1
> >   
> 

Re: [RFC PATCH 0/2] i2c: core: Introduce i2c_client_get_device_id helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]


> I can do an immutable branch with just the new function call in it if
> that is useful given I assume this is applicable across a bunch of subsystems?

I'd think I should provide the immutable branch with the new I2C API
call. Feels a bit more logical. Will that work for you as well?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] i2c: core: Introduce i2c_client_get_device_id helper

[Jonathan Cameron]

On Sat, 5 Nov 2022 22:29:29 +0100
Wolfram Sang <wsa@kernel.org> wrote:

> > I can do an immutable branch with just the new function call in it if
> > that is useful given I assume this is applicable across a bunch of subsystems?  
> 
> I'd think I should provide the immutable branch with the new I2C API
> call. Feels a bit more logical. Will that work for you as well?
> 

Absolutely. That's even better - I didn't want to assign you work to do :)

Jonathan