[PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Sjoerd Simons]

For i2c devices in OF the modalias exposed to userspace is i2c:<node
type>, for the Maxtouch driver this is i2c:maxtouch.

Add maxtouch to the i2c id table such that userspace can correctly
load the module for the device and drop the OF table as it's not
needed for i2c devices.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index db178ed..57ff26d 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -2267,16 +2267,11 @@ static int mxt_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
 
-static const struct of_device_id mxt_of_match[] = {
-	{ .compatible = "atmel,maxtouch", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, mxt_of_match);
-
 static const struct i2c_device_id mxt_id[] = {
 	{ "qt602240_ts", 0 },
 	{ "atmel_mxt_ts", 0 },
 	{ "atmel_mxt_tp", 0 },
+	{ "maxtouch", 0 },
 	{ "mXT224", 0 },
 	{ }
 };
@@ -2286,7 +2281,6 @@ static struct i2c_driver mxt_driver = {
 	.driver = {
 		.name	= "atmel_mxt_ts",
 		.owner	= THIS_MODULE,
-		.of_match_table = of_match_ptr(mxt_of_match),
 		.pm	= &mxt_pm_ops,
 	},
 	.probe		= mxt_probe,
-- 
2.1.0

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Javier Martinez Canillas]

[adding Lee Jones to cc list since I'm referring on a series he posted]

Hello Sjoerd,

On 09/09/2014 09:52 AM, Sjoerd Simons wrote:
> For i2c devices in OF the modalias exposed to userspace is i2c:<node
> type>, for the Maxtouch driver this is i2c:maxtouch.
> 
> Add maxtouch to the i2c id table such that userspace can correctly
> load the module for the device and drop the OF table as it's not
> needed for i2c devices.
> 
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index db178ed..57ff26d 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -2267,16 +2267,11 @@ static int mxt_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
>  
> -static const struct of_device_id mxt_of_match[] = {
> -	{ .compatible = "atmel,maxtouch", },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, mxt_of_match);
> -
>  static const struct i2c_device_id mxt_id[] = {
>  	{ "qt602240_ts", 0 },
>  	{ "atmel_mxt_ts", 0 },
>  	{ "atmel_mxt_tp", 0 },
> +	{ "maxtouch", 0 },
>  	{ "mXT224", 0 },
>  	{ }
>  };
> @@ -2286,7 +2281,6 @@ static struct i2c_driver mxt_driver = {
>  	.driver = {
>  		.name	= "atmel_mxt_ts",
>  		.owner	= THIS_MODULE,
> -		.of_match_table = of_match_ptr(mxt_of_match),
>  		.pm	= &mxt_pm_ops,
>  	},
>  	.probe		= mxt_probe,
> 

I see that Lee is working to allow the I2C subsystem to not need an I2C ID
table to match [0]. I'll let Lee to comment what the future plans are and if
his series are going to solve your issue since I'm not that familiar with the
I2C core.

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/6/20/199

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Nick Dyer]

On 09/09/14 11:21, Javier Martinez Canillas wrote:
> On 09/09/2014 09:52 AM, Sjoerd Simons wrote:
>> For i2c devices in OF the modalias exposed to userspace is i2c:<node
>> type>, for the Maxtouch driver this is i2c:maxtouch.
>>
>> Add maxtouch to the i2c id table such that userspace can correctly
>> load the module for the device and drop the OF table as it's not
>> needed for i2c devices.
> I see that Lee is working to allow the I2C subsystem to not need an I2C ID
> table to match [0]. I'll let Lee to comment what the future plans are and if
> his series are going to solve your issue since I'm not that familiar with the
> I2C core.
> 
> Best regards,
> Javier
> 
> [0]: https://lkml.org/lkml/2014/6/20/199

I can see the benefit of not having the duplication. Am I correct that
you're saying that it might make more sense to remove the i2c ids rather
than the OF table, if Lee's changes are accepted?

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Sjoerd Simons]

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

On Tue, 2014-09-09 at 11:29 +0100, Nick Dyer wrote:
> On 09/09/14 11:21, Javier Martinez Canillas wrote:
> > On 09/09/2014 09:52 AM, Sjoerd Simons wrote:
> >> For i2c devices in OF the modalias exposed to userspace is i2c:<node
> >> type>, for the Maxtouch driver this is i2c:maxtouch.
> >>
> >> Add maxtouch to the i2c id table such that userspace can correctly
> >> load the module for the device and drop the OF table as it's not
> >> needed for i2c devices.
> > I see that Lee is working to allow the I2C subsystem to not need an I2C ID
> > table to match [0]. I'll let Lee to comment what the future plans are and if
> > his series are going to solve your issue since I'm not that familiar with the
> > I2C core.
> 
> I can see the benefit of not having the duplication. Am I correct that
> you're saying that it might make more sense to remove the i2c ids rather
> than the OF table, if Lee's changes are accepted?

You would still need the i2C table for non-OF platforms ofcourse.

I'm not sure what happens with the modalias as exposed to userspace with
Lee's patchset, if that gets changed to prefer an of: type instead of
the current i2c: prefixed ones this patch (at that point) isn't needed.

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.

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

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Lee Jones]

On Tue, 09 Sep 2014, Javier Martinez Canillas wrote:

> [adding Lee Jones to cc list since I'm referring on a series he posted]
> 
> Hello Sjoerd,
> 
> On 09/09/2014 09:52 AM, Sjoerd Simons wrote:
> > For i2c devices in OF the modalias exposed to userspace is i2c:<node
> > type>, for the Maxtouch driver this is i2c:maxtouch.
> > 
> > Add maxtouch to the i2c id table such that userspace can correctly
> > load the module for the device and drop the OF table as it's not
> > needed for i2c devices.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index db178ed..57ff26d 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -2267,16 +2267,11 @@ static int mxt_resume(struct device *dev)
> >  
> >  static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
> >  
> > -static const struct of_device_id mxt_of_match[] = {
> > -	{ .compatible = "atmel,maxtouch", },
> > -	{},
> > -};
> > -MODULE_DEVICE_TABLE(of, mxt_of_match);
> > -
> >  static const struct i2c_device_id mxt_id[] = {
> >  	{ "qt602240_ts", 0 },
> >  	{ "atmel_mxt_ts", 0 },
> >  	{ "atmel_mxt_tp", 0 },
> > +	{ "maxtouch", 0 },
> >  	{ "mXT224", 0 },
> >  	{ }
> >  };
> > @@ -2286,7 +2281,6 @@ static struct i2c_driver mxt_driver = {
> >  	.driver = {
> >  		.name	= "atmel_mxt_ts",
> >  		.owner	= THIS_MODULE,
> > -		.of_match_table = of_match_ptr(mxt_of_match),
> >  		.pm	= &mxt_pm_ops,
> >  	},
> >  	.probe		= mxt_probe,
> > 
> 
> I see that Lee is working to allow the I2C subsystem to not need an I2C ID
> table to match [0]. I'll let Lee to comment what the future plans are and if
> his series are going to solve your issue since I'm not that familiar with the
> I2C core.

It's wrong to expect DT to probe these devices without a compatible
string.  It does so at the moment, but this is a bi-product and not
the correct method.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Lee Jones]

On Tue, 09 Sep 2014, Javier Martinez Canillas wrote:

> [adding Lee Jones to cc list since I'm referring on a series he posted]
> 
> Hello Sjoerd,
> 
> On 09/09/2014 09:52 AM, Sjoerd Simons wrote:
> > For i2c devices in OF the modalias exposed to userspace is i2c:<node
> > type>, for the Maxtouch driver this is i2c:maxtouch.
> > 
> > Add maxtouch to the i2c id table such that userspace can correctly
> > load the module for the device and drop the OF table as it's not
> > needed for i2c devices.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index db178ed..57ff26d 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -2267,16 +2267,11 @@ static int mxt_resume(struct device *dev)
> >  
> >  static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
> >  
> > -static const struct of_device_id mxt_of_match[] = {
> > -	{ .compatible = "atmel,maxtouch", },
> > -	{},
> > -};
> > -MODULE_DEVICE_TABLE(of, mxt_of_match);
> > -
> >  static const struct i2c_device_id mxt_id[] = {
> >  	{ "qt602240_ts", 0 },
> >  	{ "atmel_mxt_ts", 0 },
> >  	{ "atmel_mxt_tp", 0 },
> > +	{ "maxtouch", 0 },
> >  	{ "mXT224", 0 },
> >  	{ }
> >  };
> > @@ -2286,7 +2281,6 @@ static struct i2c_driver mxt_driver = {
> >  	.driver = {
> >  		.name	= "atmel_mxt_ts",
> >  		.owner	= THIS_MODULE,
> > -		.of_match_table = of_match_ptr(mxt_of_match),
> >  		.pm	= &mxt_pm_ops,
> >  	},
> >  	.probe		= mxt_probe,
> > 
> 
> I see that Lee is working to allow the I2C subsystem to not need an I2C ID
> table to match [0]. I'll let Lee to comment what the future plans are and if
> his series are going to solve your issue since I'm not that familiar with the
> I2C core.

It's wrong to expect DT to probe these devices without a compatible
string.  It does so at the moment, but this is a bi-product and not
the correct method.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Sjoerd Simons]

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

Hey Lee,

On Wed, 2014-09-10 at 10:28 +0100, Lee Jones wrote:
> On Tue, 09 Sep 2014, Javier Martinez Canillas wrote:
> 
> > [adding Lee Jones to cc list since I'm referring on a series he posted]
> > 
> > Hello Sjoerd,
> > 
> > On 09/09/2014 09:52 AM, Sjoerd Simons wrote:
> > > For i2c devices in OF the modalias exposed to userspace is i2c:<node
> > > type>, for the Maxtouch driver this is i2c:maxtouch.
> > > 
> > > Add maxtouch to the i2c id table such that userspace can correctly
> > > load the module for the device and drop the OF table as it's not
> > > needed for i2c devices.
> > > 
> > > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > > ---
> > >  drivers/input/touchscreen/atmel_mxt_ts.c | 8 +-------
> > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > index db178ed..57ff26d 100644
> > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > @@ -2267,16 +2267,11 @@ static int mxt_resume(struct device *dev)
> > >  
> > >  static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
> > >  
> > > -static const struct of_device_id mxt_of_match[] = {
> > > -	{ .compatible = "atmel,maxtouch", },
> > > -	{},
> > > -};
> > > -MODULE_DEVICE_TABLE(of, mxt_of_match);
> > > -
> > >  static const struct i2c_device_id mxt_id[] = {
> > >  	{ "qt602240_ts", 0 },
> > >  	{ "atmel_mxt_ts", 0 },
> > >  	{ "atmel_mxt_tp", 0 },
> > > +	{ "maxtouch", 0 },
> > >  	{ "mXT224", 0 },
> > >  	{ }
> > >  };
> > > @@ -2286,7 +2281,6 @@ static struct i2c_driver mxt_driver = {
> > >  	.driver = {
> > >  		.name	= "atmel_mxt_ts",
> > >  		.owner	= THIS_MODULE,
> > > -		.of_match_table = of_match_ptr(mxt_of_match),
> > >  		.pm	= &mxt_pm_ops,
> > >  	},
> > >  	.probe		= mxt_probe,
> > > 
> > 
> > I see that Lee is working to allow the I2C subsystem to not need an I2C ID
> > table to match [0]. I'll let Lee to comment what the future plans are and if
> > his series are going to solve your issue since I'm not that familiar with the
> > I2C core.
> 
> It's wrong to expect DT to probe these devices without a compatible
> string.  It does so at the moment, but this is a bi-product and not
> the correct method.

Ok, which means removing the mxt_of_match table in this patch is wrong..
I'll fix that for for a V2.

However that makes adding the "maxtouch" string to the i2c device table
somewhat cumbersome as it only gets added in this case to ensure
module-autoloading can happen as the modalias presented to userspace is
going still going to be i2c:maxtouch.

Tbh, the bigger problem this is pointing out is that for I2C devices
with only an OF compability tring module auto-loading is broken...

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.

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

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Javier Martinez Canillas]

Hello Lee,

On 09/11/2014 10:00 AM, Sjoerd Simons wrote:
>> > >  
>> > > -static const struct of_device_id mxt_of_match[] = {
>> > > -	{ .compatible = "atmel,maxtouch", },
>> > > -	{},
>> > > -};
>> > > -MODULE_DEVICE_TABLE(of, mxt_of_match);
>> > > -
>> > >  static const struct i2c_device_id mxt_id[] = {
>> > >  	{ "qt602240_ts", 0 },
>> > >  	{ "atmel_mxt_ts", 0 },
>> > >  	{ "atmel_mxt_tp", 0 },
>> > > +	{ "maxtouch", 0 },
>> > >  	{ "mXT224", 0 },
>> > >  	{ }
>> > >  };
>> > > @@ -2286,7 +2281,6 @@ static struct i2c_driver mxt_driver = {
>> > >  	.driver = {
>> > >  		.name	= "atmel_mxt_ts",
>> > >  		.owner	= THIS_MODULE,
>> > > -		.of_match_table = of_match_ptr(mxt_of_match),
>> > >  		.pm	= &mxt_pm_ops,
>> > >  	},
>> > >  	.probe		= mxt_probe,
>> > > 
>> > 
>> > I see that Lee is working to allow the I2C subsystem to not need an I2C ID
>> > table to match [0]. I'll let Lee to comment what the future plans are and if
>> > his series are going to solve your issue since I'm not that familiar with the
>> > I2C core.
>> 
>> It's wrong to expect DT to probe these devices without a compatible
>> string.  It does so at the moment, but this is a bi-product and not
>> the correct method.
> 
> Ok, which means removing the mxt_of_match table in this patch is wrong..
> I'll fix that for for a V2.
> 
> However that makes adding the "maxtouch" string to the i2c device table
> somewhat cumbersome as it only gets added in this case to ensure
> module-autoloading can happen as the modalias presented to userspace is
> going still going to be i2c:maxtouch.
> 
> Tbh, the bigger problem this is pointing out is that for I2C devices
> with only an OF compability tring module auto-loading is broken...
> 

To expand on what Sjoerd already said and just to be sure everyone is on the
same page.

The problem is that right now the driver reports the following modalias:

# cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
i2c:maxtouch

but if you look at the module information, that is not a valid alias:

# modinfo atmel_mxt_ts | grep alias
alias:          i2c:mXT224
alias:          i2c:atmel_mxt_tp
alias:          i2c:atmel_mxt_ts
alias:          i2c:qt602240_ts
alias:          of:N*T*Catmel,maxtouch*

which means that udev/kmod can't load the module automatically based on the
alias information.

The aliases are filled by both MODULE_DEVICE_TABLE(i2c, mxt_id) and
MODULE_DEVICE_TABLE(of, mxt_of_match) so after Sjoerd's patch:

# cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
i2c:maxtouch

# modinfo atmel_mxt_ts | grep alias
alias:          i2c:mXT224
alias:          i2c:maxtouch
alias:          i2c:atmel_mxt_tp
alias:          i2c:atmel_mxt_ts
alias:          i2c:qt602240_ts

which matches the reported uevent so the module will be auto-loaded.

This is because the I2C subsystem hardcodes i2c:<client->name>, if you look at
drivers/i2c/i2c-core.c:

/* uevent helps with hotplug: modprobe -q $(MODALIAS) */
static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
{
...
        if (add_uevent_var(env, "MODALIAS=%s%s",
                           I2C_MODULE_PREFIX, client->name))
...
}

I've looked at Lee's series and AFAICT that remains the same so I second
Sjoerd that module auto-loading will continue to be broken.

Best regards,
Javier

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Nick Dyer]

On 11/09/14 09:38, Javier Martinez Canillas wrote:
> To expand on what Sjoerd already said and just to be sure everyone is on the
> same page.
> 
> The problem is that right now the driver reports the following modalias:
> 
> # cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
> i2c:maxtouch
> 
> but if you look at the module information, that is not a valid alias:
> 
> # modinfo atmel_mxt_ts | grep alias
> alias:          i2c:mXT224
> alias:          i2c:atmel_mxt_tp
> alias:          i2c:atmel_mxt_ts
> alias:          i2c:qt602240_ts
> alias:          of:N*T*Catmel,maxtouch*
> 
> which means that udev/kmod can't load the module automatically based on the
> alias information.
> 
> The aliases are filled by both MODULE_DEVICE_TABLE(i2c, mxt_id) and
> MODULE_DEVICE_TABLE(of, mxt_of_match) so after Sjoerd's patch:
> 
> # cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
> i2c:maxtouch
> 
> # modinfo atmel_mxt_ts | grep alias
> alias:          i2c:mXT224
> alias:          i2c:maxtouch
> alias:          i2c:atmel_mxt_tp
> alias:          i2c:atmel_mxt_ts
> alias:          i2c:qt602240_ts
> 
> which matches the reported uevent so the module will be auto-loaded.
> 
> This is because the I2C subsystem hardcodes i2c:<client->name>, if you look at
> drivers/i2c/i2c-core.c:
> 
> /* uevent helps with hotplug: modprobe -q $(MODALIAS) */
> static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> {
> ...
>         if (add_uevent_var(env, "MODALIAS=%s%s",
>                            I2C_MODULE_PREFIX, client->name))
> ...
> }
> 
> I've looked at Lee's series and AFAICT that remains the same so I second
> Sjoerd that module auto-loading will continue to be broken.

Thanks for the clear explanation.

The i2c aliases are a bit confusing. The original device the driver was
written for was called qt602240, which was renamed by Atmel to mXT224 when
the chip series was called "maXTouch". The driver now actually supports
many other chips which aren't listed (more than 20 devices that I've
personally tested). I could add them all, but it would be an extremely long
list. It may be preferable to use the generic name maXTouch.

So I think the sensible thing to do here would be to add "maxtouch" to the
i2c list to fix the module autoload issue.

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Javier Martinez Canillas]

Hello Nick,

On 09/11/2014 11:19 AM, Nick Dyer wrote:
> 
> Thanks for the clear explanation.
> 
> The i2c aliases are a bit confusing. The original device the driver was
> written for was called qt602240, which was renamed by Atmel to mXT224 when
> the chip series was called "maXTouch". The driver now actually supports
> many other chips which aren't listed (more than 20 devices that I've
> personally tested). I could add them all, but it would be an extremely long
> list. It may be preferable to use the generic name maXTouch.
> 
> So I think the sensible thing to do here would be to add "maxtouch" to the
> i2c list to fix the module autoload issue.
> 

While this will actually fix the module auto-load issue on the atmel driver,
I'm concerned that the I2C core is not reporting the correct module
'of:N*T*Catmel,maxtouch*' alias when probing using DT.

Since as Lee said on his cover letter for the mentioned series [0], an I2C ID
table shouldn't be mandatory for drivers that only support DT based platforms
(e.g: a driver that depends on OF) but in that case I2C module auto-loading
would not work AFAICT.

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/6/20/199

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Wolfram Sang]

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


Funny timing. I am just reviewing the series from Lee and also stumbled
over modaliases, too...

On Thu, Sep 11, 2014 at 10:19:54AM +0100, Nick Dyer wrote:
> On 11/09/14 09:38, Javier Martinez Canillas wrote:
> > To expand on what Sjoerd already said and just to be sure everyone is on the
> > same page.
> > 
> > The problem is that right now the driver reports the following modalias:
> > 
> > # cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
> > i2c:maxtouch
> > 
> > but if you look at the module information, that is not a valid alias:
> > 
> > # modinfo atmel_mxt_ts | grep alias
> > alias:          i2c:mXT224
> > alias:          i2c:atmel_mxt_tp
> > alias:          i2c:atmel_mxt_ts
> > alias:          i2c:qt602240_ts
> > alias:          of:N*T*Catmel,maxtouch*
> > 
> > which means that udev/kmod can't load the module automatically based on the
> > alias information.
> > 
> > The aliases are filled by both MODULE_DEVICE_TABLE(i2c, mxt_id) and
> > MODULE_DEVICE_TABLE(of, mxt_of_match) so after Sjoerd's patch:
> > 
> > # cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
> > i2c:maxtouch
> > 
> > # modinfo atmel_mxt_ts | grep alias
> > alias:          i2c:mXT224
> > alias:          i2c:maxtouch
> > alias:          i2c:atmel_mxt_tp
> > alias:          i2c:atmel_mxt_ts
> > alias:          i2c:qt602240_ts
> > 
> > which matches the reported uevent so the module will be auto-loaded.
> > 
> > This is because the I2C subsystem hardcodes i2c:<client->name>, if you look at
> > drivers/i2c/i2c-core.c:
> > 
> > /* uevent helps with hotplug: modprobe -q $(MODALIAS) */
> > static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > {
> > ...
> >         if (add_uevent_var(env, "MODALIAS=%s%s",
> >                            I2C_MODULE_PREFIX, client->name))
> > ...
> > }
> > 
> > I've looked at Lee's series and AFAICT that remains the same so I second
> > Sjoerd that module auto-loading will continue to be broken.

I think the above code in the i2c core needs a fix. Will have a closer
look after lunch.

> The i2c aliases are a bit confusing. The original device the driver was
> written for was called qt602240, which was renamed by Atmel to mXT224 when
> the chip series was called "maXTouch". The driver now actually supports
> many other chips which aren't listed (more than 20 devices that I've
> personally tested). I could add them all, but it would be an extremely long
> list. It may be preferable to use the generic name maXTouch.

This is probably true for some more I2C devices. Like RTCs being
compatible or, most prominent, EEPROMS. I don't want to have a list of
all vendors producing 24c02s if they are all compatible. If generic
entries are frowned upon, I'd agree on a "first come, first served" policy:
Somebody provides one compatible-property with the vendor which happens
to be on that board, and the others have to reuse that
compatible-property since they are, well, compatible.

> So I think the sensible thing to do here would be to add "maxtouch" to the
> i2c list to fix the module autoload issue.

This is a workaround. It would make sense, however, to add it because we
want to support i2c_board_info structures.

Regards,

   Wolfram


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

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Javier Martinez Canillas]

Hello Wolfram,

On 09/11/2014 01:08 PM, Wolfram Sang wrote:
> 
> Funny timing. I am just reviewing the series from Lee and also stumbled
> over modaliases, too...
> 
> On Thu, Sep 11, 2014 at 10:19:54AM +0100, Nick Dyer wrote:
>> On 11/09/14 09:38, Javier Martinez Canillas wrote:
>> > To expand on what Sjoerd already said and just to be sure everyone is on the
>> > same page.
>> > 
>> > The problem is that right now the driver reports the following modalias:
>> > 
>> > # cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
>> > i2c:maxtouch
>> > 
>> > but if you look at the module information, that is not a valid alias:
>> > 
>> > # modinfo atmel_mxt_ts | grep alias
>> > alias:          i2c:mXT224
>> > alias:          i2c:atmel_mxt_tp
>> > alias:          i2c:atmel_mxt_ts
>> > alias:          i2c:qt602240_ts
>> > alias:          of:N*T*Catmel,maxtouch*
>> > 
>> > which means that udev/kmod can't load the module automatically based on the
>> > alias information.
>> > 
>> > The aliases are filled by both MODULE_DEVICE_TABLE(i2c, mxt_id) and
>> > MODULE_DEVICE_TABLE(of, mxt_of_match) so after Sjoerd's patch:
>> > 
>> > # cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
>> > i2c:maxtouch
>> > 
>> > # modinfo atmel_mxt_ts | grep alias
>> > alias:          i2c:mXT224
>> > alias:          i2c:maxtouch
>> > alias:          i2c:atmel_mxt_tp
>> > alias:          i2c:atmel_mxt_ts
>> > alias:          i2c:qt602240_ts
>> > 
>> > which matches the reported uevent so the module will be auto-loaded.
>> > 
>> > This is because the I2C subsystem hardcodes i2c:<client->name>, if you look at
>> > drivers/i2c/i2c-core.c:
>> > 
>> > /* uevent helps with hotplug: modprobe -q $(MODALIAS) */
>> > static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>> > {
>> > ...
>> >         if (add_uevent_var(env, "MODALIAS=%s%s",
>> >                            I2C_MODULE_PREFIX, client->name))
>> > ...
>> > }
>> > 
>> > I've looked at Lee's series and AFAICT that remains the same so I second
>> > Sjoerd that module auto-loading will continue to be broken.
> 
> I think the above code in the i2c core needs a fix. Will have a closer
> look after lunch.
> 

Agreed, I just posted an RFC patch [0] with the fix but as Sjoerd pointed out
on an internal review, changing that will regress all the drivers that were
relying on the old behavior.

>> The i2c aliases are a bit confusing. The original device the driver was
>> written for was called qt602240, which was renamed by Atmel to mXT224 when
>> the chip series was called "maXTouch". The driver now actually supports
>> many other chips which aren't listed (more than 20 devices that I've
>> personally tested). I could add them all, but it would be an extremely long
>> list. It may be preferable to use the generic name maXTouch.
> 
> This is probably true for some more I2C devices. Like RTCs being
> compatible or, most prominent, EEPROMS. I don't want to have a list of
> all vendors producing 24c02s if they are all compatible. If generic
> entries are frowned upon, I'd agree on a "first come, first served" policy:
> Somebody provides one compatible-property with the vendor which happens
> to be on that board, and the others have to reuse that
> compatible-property since they are, well, compatible.
> 

Agreed.

>> So I think the sensible thing to do here would be to add "maxtouch" to the
>> i2c list to fix the module autoload issue.
> 
> This is a workaround. It would make sense, however, to add it because we
> want to support i2c_board_info structures.
> 

I think it really depends if an IP block can be used on non-DT platforms
(which I think is true for this trackpad) but if a driver is for an IP block
that can only be used on a DT-only platform (e.g: a PMIC that is controlled
over I2C and is only compatible with a DT-only SoC) then I don't think we need
to support the i2c_board_info structure and can get rid of the I2C ID table on
these drivers once Lee series land.

> Regards,
> 
>    Wolfram
> 

Best regards,
Javier

[0]: https://patchwork.ozlabs.org/patch/388200/

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Wolfram Sang]

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


> > This is a workaround. It would make sense, however, to add it because we
> > want to support i2c_board_info structures.
> > 
> 
> I think it really depends if an IP block can be used on non-DT platforms
> (which I think is true for this trackpad) but if a driver is for an IP block
> that can only be used on a DT-only platform (e.g: a PMIC that is controlled
> over I2C and is only compatible with a DT-only SoC) then I don't think we need
> to support the i2c_board_info structure and can get rid of the I2C ID table on
> these drivers once Lee series land.

That is exactly what I meant. It should be only added if there is a
reason other than "workaround". If you say, it doesn't make sense on
non-DT, then it should not be added.


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

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Javier Martinez Canillas]

Hello Wolfram,

On 09/11/2014 01:35 PM, Wolfram Sang wrote:
> 
>> > This is a workaround. It would make sense, however, to add it because we
>> > want to support i2c_board_info structures.
>> > 
>> 
>> I think it really depends if an IP block can be used on non-DT platforms
>> (which I think is true for this trackpad) but if a driver is for an IP block
>> that can only be used on a DT-only platform (e.g: a PMIC that is controlled
>> over I2C and is only compatible with a DT-only SoC) then I don't think we need
>> to support the i2c_board_info structure and can get rid of the I2C ID table on
>> these drivers once Lee series land.
> 
> That is exactly what I meant. It should be only added if there is a
> reason other than "workaround". If you say, it doesn't make sense on
> non-DT, then it should not be added.
> 

Sorry for explaining myself badly. I just tried to say that this is a decision
that has to be made on a per-driver basis but I don't really know if makes
sense or not in this case since I don't know if this device is (or could be)
shipped on non-DT platforms. Nick is in a much better position to answer that
question.

Best regards,
Javier

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Nick Dyer]

On 11/09/14 12:41, Javier Martinez Canillas wrote:
> On 09/11/2014 01:35 PM, Wolfram Sang wrote:
>>>> This is a workaround. It would make sense, however, to add it because we
>>>> want to support i2c_board_info structures.
>>>>
>>>
>>> I think it really depends if an IP block can be used on non-DT platforms
>>> (which I think is true for this trackpad) but if a driver is for an IP block
>>> that can only be used on a DT-only platform (e.g: a PMIC that is controlled
>>> over I2C and is only compatible with a DT-only SoC) then I don't think we need
>>> to support the i2c_board_info structure and can get rid of the I2C ID table on
>>> these drivers once Lee series land.
>>
>> That is exactly what I meant. It should be only added if there is a
>> reason other than "workaround". If you say, it doesn't make sense on
>> non-DT, then it should not be added.
> 
> Sorry for explaining myself badly. I just tried to say that this is a decision
> that has to be made on a per-driver basis but I don't really know if makes
> sense or not in this case since I don't know if this device is (or could be)
> shipped on non-DT platforms. Nick is in a much better position to answer that
> question.

There are plenty of out-of-tree users of this driver which don't use DT.
The most significant use I'm aware of is on Chromium OS systems.

Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

[Nick Dyer]

On 09/09/14 08:52, Sjoerd Simons wrote:
> For i2c devices in OF the modalias exposed to userspace is i2c:<node
> type>, for the Maxtouch driver this is i2c:maxtouch.
> 
> Add maxtouch to the i2c id table such that userspace can correctly
> load the module for the device and drop the OF table as it's not
> needed for i2c devices.
> 
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

I've tested this and it does work. In fact, it seems that you can use
	compatible = "i2c:atmel,maxtouch";

because the "atmel," is ignored in the matching.

Before I could ack this patch, we will need to update this dts file:
	arch/arm/boot/dts/s5pv210-goni.dts
and this documentation file:
	Documentation/devicetree/bindings/input/atmel,maxtouch.txt

thanks!