[PATCH 0/2 v2] Add OF match tables for the ChromeOs EC subdevices

[PATCH 0/2 v2] Add OF match tables for the ChromeOs EC subdevices

[Sjoerd Simons]

[ This is essentially a resend, adding Javiers reviewed-by and fixing some small
identation issues in the second patch. Also added Wolfgang and Dimitry in the
to, as i missed them last time ]

The ChromeOS EC MFD driver registers its sub-devices with both a (platform)
name and an OF compatibility string. As a result of this the modalias passed on
to user-space will be based on the OF compatibility string. Thus to be able to
rely on autoloading in case the subdevices are build as modules they need to
export the necessary module aliases based to match the of information.

The two patches in these series add the requird of match information to the EC
subdevices

I guess the first patch should through the i2c tree while the second one would
be for the input tree?

Sjoerd Simons (2):
  i2c: cros-ec-tunnel: Add of match table
  input: cros_ec_keyb: Add of match table

 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 9 +++++++++
 drivers/input/keyboard/cros_ec_keyb.c   | 9 +++++++++
 2 files changed, 18 insertions(+)

-- 
2.1.0

[PATCH 1/2 v2] i2c: cros-ec-tunnel: Add of match table

[Sjoerd Simons]

To enable the cros-ec-tunnel driver to be auto-loaded when build as a
module add an of match table (and export it) to match the modalias
information passed on to userspace as the Cros EC MFD driver registers
the MFD subdevices with an of_compatibility string.

Signed-off-by: Sjoerd Simons <sjoerd.simons-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
Reviewed-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---
 Changes in v2: none

 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 05e033c..0403ec1 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -310,11 +310,20 @@ static int ec_i2c_remove(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_i2c_of_match[] = {
+	{ .compatible = "google,cros-ec-i2c-tunnel" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cros_ec_i2c_of_match);
+#endif
+
 static struct platform_driver ec_i2c_tunnel_driver = {
 	.probe = ec_i2c_probe,
 	.remove = ec_i2c_remove,
 	.driver = {
 		.name = "cros-ec-i2c-tunnel",
+		.of_match_table = of_match_ptr(cros_ec_i2c_of_match),
 	},
 };
 
-- 
2.1.0

Re: [PATCH 1/2 v2] i2c: cros-ec-tunnel: Add of match table

[Wolfram Sang]

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

On Fri, Sep 19, 2014 at 10:08:12AM +0200, Sjoerd Simons wrote:
> To enable the cros-ec-tunnel driver to be auto-loaded when build as a
> module add an of match table (and export it) to match the modalias
> information passed on to userspace as the Cros EC MFD driver registers
> the MFD subdevices with an of_compatibility string.
> 
> Signed-off-by: Sjoerd Simons <sjoerd.simons-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> Reviewed-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

Applied to for-next, thanks!

However, since you are working on this driver: sparse rightfully
complains about:

drivers/i2c/busses/i2c-cros-ec-tunnel.c:97:44: warning: cast truncates bits from constant value (10000 becomes 0)

I don't know the EC protocol, can you check?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2 v2] i2c: cros-ec-tunnel: Add of match table

[Javier Martinez Canillas]

Hello Wolfram,

On 09/29/2014 10:59 PM, Wolfram Sang wrote:
> On Fri, Sep 19, 2014 at 10:08:12AM +0200, Sjoerd Simons wrote:
>> To enable the cros-ec-tunnel driver to be auto-loaded when build as a
>> module add an of match table (and export it) to match the modalias
>> information passed on to userspace as the Cros EC MFD driver registers
>> the MFD subdevices with an of_compatibility string.
>> 
>> Signed-off-by: Sjoerd Simons <sjoerd.simons-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
>> Reviewed-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> 
> Applied to for-next, thanks!
> 
> However, since you are working on this driver: sparse rightfully
> complains about:
> 
> drivers/i2c/busses/i2c-cros-ec-tunnel.c:97:44: warning: cast truncates bits from constant value (10000 becomes 0)
> 
> I don't know the EC protocol, can you check?
> 

Doug posted a patch [0] to fix the issue you are reporting but it was not
picked. I tested that the patch applies on top of linux-next + the latest
cros_ec cleanups (that includes a patch for this i2c driver and was taken
by Lee. So I think is safe for you take Doug's patch through your tree
since it seems to not cause any conflict with the mfd tree.

Or if you prefer, I can re-post that patch for 3.19.

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/6/23/830

Re: [PATCH 1/2 v2] i2c: cros-ec-tunnel: Add of match table

[Wolfram Sang]

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


> Doug posted a patch [0] to fix the issue you are reporting but it was not
> picked. I tested that the patch applies on top of linux-next + the latest
> cros_ec cleanups (that includes a patch for this i2c driver and was taken
> by Lee. So I think is safe for you take Doug's patch through your tree
> since it seems to not cause any conflict with the mfd tree.

OK, thanks for the heads up. I picked it.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] input: cros_ec_keyb: Add of match table

[Sjoerd Simons]

To enable the cros_ec_keyb driver to be auto-loaded when build as
module add an of match table (and export it) to match the modalias
information passed on to userspace as the Cros EC MFD driver registers
the MFD subdevices with an of_compatibility string.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Changes in v2: Fixed some indentation issues

 drivers/input/keyboard/cros_ec_keyb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 791781a..e1903ec 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -342,10 +342,19 @@ static int cros_ec_keyb_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
 
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_keyb_of_match[] = {
+	{ .compatible = "google,cros-ec-keyb" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cros_ec_keyb_of_match);
+#endif
+
 static struct platform_driver cros_ec_keyb_driver = {
 	.probe = cros_ec_keyb_probe,
 	.driver = {
 		.name = "cros-ec-keyb",
+		.of_match_table = of_match_ptr(cros_ec_keyb_of_match),
 		.pm	= &cros_ec_keyb_pm_ops,
 	},
 };
-- 
2.1.0

Re: [PATCH 2/2] input: cros_ec_keyb: Add of match table

[Dmitry Torokhov]

On Fri, Sep 19, 2014 at 10:08:13AM +0200, Sjoerd Simons wrote:
> To enable the cros_ec_keyb driver to be auto-loaded when build as
> module add an of match table (and export it) to match the modalias
> information passed on to userspace as the Cros EC MFD driver registers
> the MFD subdevices with an of_compatibility string.
> 
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Applied, thank you.

> ---
>  Changes in v2: Fixed some indentation issues
> 
>  drivers/input/keyboard/cros_ec_keyb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 791781a..e1903ec 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -342,10 +342,19 @@ static int cros_ec_keyb_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id cros_ec_keyb_of_match[] = {
> +	{ .compatible = "google,cros-ec-keyb" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_keyb_of_match);
> +#endif
> +
>  static struct platform_driver cros_ec_keyb_driver = {
>  	.probe = cros_ec_keyb_probe,
>  	.driver = {
>  		.name = "cros-ec-keyb",
> +		.of_match_table = of_match_ptr(cros_ec_keyb_of_match),
>  		.pm	= &cros_ec_keyb_pm_ops,
>  	},
>  };
> -- 
> 2.1.0
> 

-- 
Dmitry

[PATCH 0/2] Add of match tables for the ChromeOs EC subdevices

[Sjoerd Simons]

The ChromeOS EC MFD driver registers its sub-devices with both a (platform)
name and an of compatibility string. As a result of this the modalias passed on
to user-space will be based on the of compatibility string. Thus to be able to
rely on autoloading in case the subdevices are build as modules they need to
export the necessary module aliases based to match the of information.

The two patches in these series add the requird of match information to the EC
subdevices

Sjoerd Simons (2):
  i2c: cros-ec-tunnel: Add of match table
  input: cros_ec_keyb: Add of match table

 drivers/i2c/busses/i2c-cros-ec-tunnel.c |  9 +++++++++
 drivers/input/keyboard/cros_ec_keyb.c   | 10 ++++++++++
 2 files changed, 19 insertions(+)

-- 
2.1.0

[PATCH 2/2] input: cros_ec_keyb: Add of match table

[Sjoerd Simons]

To enable the cros_ec_keyb driver to be auto-loaded when build as
module add an of match table (and export it) to match the modalias
information passed on to userspace as the Cros EC MFD driver registers
the MFD subdevices with an of_compatibility string.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 drivers/input/keyboard/cros_ec_keyb.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 791781a..0bdbf2d 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -342,10 +342,20 @@ static int cros_ec_keyb_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
 
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_keyb_of_match[] = {
+	{ .compatible = "google,cros-ec-keyb" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cros_ec_keyb_of_match);
+#endif
+
+
 static struct platform_driver cros_ec_keyb_driver = {
 	.probe = cros_ec_keyb_probe,
 	.driver = {
 		.name = "cros-ec-keyb",
+		.of_match_table = of_match_ptr (cros_ec_keyb_of_match),
 		.pm	= &cros_ec_keyb_pm_ops,
 	},
 };
-- 
2.1.0

Re: [PATCH 2/2] input: cros_ec_keyb: Add of match table

[Sergei Shtylyov]

Hello.

On 8/23/2014 3:03 AM, Sjoerd Simons wrote:

> To enable the cros_ec_keyb driver to be auto-loaded when build as
> module add an of match table (and export it) to match the modalias
> information passed on to userspace as the Cros EC MFD driver registers
> the MFD subdevices with an of_compatibility string.

> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>   drivers/input/keyboard/cros_ec_keyb.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)

> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 791781a..0bdbf2d 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -342,10 +342,20 @@ static int cros_ec_keyb_resume(struct device *dev)
>
>   static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id cros_ec_keyb_of_match[] = {

    Perhaps better to use '__maybe_unused' instead of #ifdef...

> +	{ .compatible = "google,cros-ec-keyb" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_keyb_of_match);
> +#endif
> +
> +

    Too many empty lines.

>   static struct platform_driver cros_ec_keyb_driver = {
>   	.probe = cros_ec_keyb_probe,
>   	.driver = {
>   		.name = "cros-ec-keyb",
> +		.of_match_table = of_match_ptr (cros_ec_keyb_of_match),

    There shouldn't be space before (.

>   		.pm	= &cros_ec_keyb_pm_ops,
>   	},
>   };

WBR, Sergei

Re: [PATCH 2/2] input: cros_ec_keyb: Add of match table

[Sjoerd Simons]

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

Hey,

On Sat, 2014-08-23 at 17:42 +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 8/23/2014 3:03 AM, Sjoerd Simons wrote:
> 
> > To enable the cros_ec_keyb driver to be auto-loaded when build as
> > module add an of match table (and export it) to match the modalias
> > information passed on to userspace as the Cros EC MFD driver registers
> > the MFD subdevices with an of_compatibility string.
> 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> >   drivers/input/keyboard/cros_ec_keyb.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> 
> > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> > index 791781a..0bdbf2d 100644
> > --- a/drivers/input/keyboard/cros_ec_keyb.c
> > +++ b/drivers/input/keyboard/cros_ec_keyb.c
> > @@ -342,10 +342,20 @@ static int cros_ec_keyb_resume(struct device *dev)
> >
> >   static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
> >
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id cros_ec_keyb_of_match[] = {

>     Perhaps better to use '__maybe_unused' instead of #ifdef...

Hmm, looks like the rtc-ds1742.c driver is the only one in the kernel
tree using that strategy, while all others use #ifdef CONFIG_OF. So i'm
inclined to keep the #ifdef here, ooi what is your rationale behind
suggesting __maybe_unused?


> > +	{ .compatible = "google,cros-ec-keyb" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_ec_keyb_of_match);
> > +#endif
> > +
> > +
> 
>     Too many empty lines.



> >   static struct platform_driver cros_ec_keyb_driver = {
> >   	.probe = cros_ec_keyb_probe,
> >   	.driver = {
> >   		.name = "cros-ec-keyb",
> > +		.of_match_table = of_match_ptr (cros_ec_keyb_of_match),
> 
>     There shouldn't be space before (.

Will fix the identation issues in a v2.

Thanks for the review,
 Sjoerd

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6170 bytes --]

Re: [PATCH 2/2] input: cros_ec_keyb: Add of match table

[Javier Martinez Canillas]

Hello,

On Mon, Aug 25, 2014 at 9:34 AM, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
>> >
>> >   static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
>> >
>> > +#ifdef CONFIG_OF
>> > +static const struct of_device_id cros_ec_keyb_of_match[] = {
>
>>     Perhaps better to use '__maybe_unused' instead of #ifdef...
>
> Hmm, looks like the rtc-ds1742.c driver is the only one in the kernel
> tree using that strategy, while all others use #ifdef CONFIG_OF. So i'm
> inclined to keep the #ifdef here, ooi what is your rationale behind
> suggesting __maybe_unused?
>

I agree with Sjoerd on this. Not only using the #ifdef guards makes it
more evident when reading the code that this depends on OF being
enabled but also if using __maybe_unused an entry in the struct
of_device_id table will be added for no reason.

>
>> > +   { .compatible = "google,cros-ec-keyb" },
>> > +   {},
>> > +};
>> > +MODULE_DEVICE_TABLE(of, cros_ec_keyb_of_match);
>> > +#endif
>> > +
>> > +
>>
>>     Too many empty lines.
>
>
>
>> >   static struct platform_driver cros_ec_keyb_driver = {
>> >     .probe = cros_ec_keyb_probe,
>> >     .driver = {
>> >             .name = "cros-ec-keyb",
>> > +           .of_match_table = of_match_ptr (cros_ec_keyb_of_match),
>>
>>     There shouldn't be space before (.
>
> Will fix the identation issues in a v2.
>
> Thanks for the review,
>  Sjoerd

After fixing the empty lines:

Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Best regards,
Javier