[PATCH 0/3] iio: accel: kxcjk-1013: new ACPI id and misc cleanups

[PATCH 0/3] iio: accel: kxcjk-1013: new ACPI id and misc cleanups

[Antonio Ospite]

Hi,

with this set I am adding a new ACPI id to kxcjk-1013 and proposing some
minor cleanups.

CC-ing Bastien because he added the is_smo8500_device flag which
I propose to rename in patch 2.

Thanks,
   Antonio

Antonio Ospite (3):
  iio: accel: kxcjk-1013: add the "KXCJ9000" ACPI id
  iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode
  iio: accel: kxcjk-1013: add some blank lines for readability

 drivers/iio/accel/kxcjk-1013.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH 1/3] iio: accel: kxcjk-1013: add the "KXCJ9000" ACPI id

[Antonio Ospite]

This id has been seen in the DSTD for the Teclast X98 Air 3G tablet
based on Intel Bay Trail.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Bastien Nocera <hadess@hadess.net>
Cc: linux-iio@vger.kernel.org
---
 drivers/iio/accel/kxcjk-1013.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 51da369..5b7a860 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1418,6 +1418,7 @@ static const struct dev_pm_ops kxcjk1013_pm_ops = {
 static const struct acpi_device_id kx_acpi_match[] = {
 	{"KXCJ1013", KXCJK1013},
 	{"KXCJ1008", KXCJ91008},
+	{"KXCJ9000", KXCJ91008},
 	{"KXTJ1009", KXTJ21009},
 	{"SMO8500",  KXCJ91008},
 	{ },
-- 
2.1.4

Re: [PATCH 1/3] iio: accel: kxcjk-1013: add the "KXCJ9000" ACPI id

[Bastien Nocera]

On Wed, 2015-04-29 at 11:01 +0200, Antonio Ospite wrote:
> This id has been seen in the DSTD for the Teclast X98 Air 3G tablet

DSDT.

> based on Intel Bay Trail.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: linux-iio@vger.kernel.org
> ---
>  drivers/iio/accel/kxcjk-1013.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c 
> b/drivers/iio/accel/kxcjk-1013.c
> index 51da369..5b7a860 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -1418,6 +1418,7 @@ static const struct dev_pm_ops 
> kxcjk1013_pm_ops = {
>  static const struct acpi_device_id kx_acpi_match[] = {
>       {"KXCJ1013", KXCJK1013},
>       {"KXCJ1008", KXCJ91008},
> +     {"KXCJ9000", KXCJ91008},

I'm guessing it's not a polling one, right?

>       {"KXTJ1009", KXTJ21009},
>       {"SMO8500",  KXCJ91008},
>       { },

Re: [PATCH 1/3] iio: accel: kxcjk-1013: add the "KXCJ9000" ACPI id

[Antonio Ospite]

On Wed, 29 Apr 2015 12:28:40 +0200
Bastien Nocera <hadess@hadess.net> wrote:

> On Wed, 2015-04-29 at 11:01 +0200, Antonio Ospite wrote:
> > This id has been seen in the DSTD for the Teclast X98 Air 3G tablet
> 
> DSDT.

Right. Thanks :)

Jonathan, let me know if you want me to resend or if you plan to amend
the commit message yourself.

> > based on Intel Bay Trail.
> > 
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Bastien Nocera <hadess@hadess.net>
> > Cc: linux-iio@vger.kernel.org
> > ---
> >  drivers/iio/accel/kxcjk-1013.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/iio/accel/kxcjk-1013.c 
> > b/drivers/iio/accel/kxcjk-1013.c
> > index 51da369..5b7a860 100644
> > --- a/drivers/iio/accel/kxcjk-1013.c
> > +++ b/drivers/iio/accel/kxcjk-1013.c
> > @@ -1418,6 +1418,7 @@ static const struct dev_pm_ops 
> > kxcjk1013_pm_ops = {
> >  static const struct acpi_device_id kx_acpi_match[] = {
> >       {"KXCJ1013", KXCJK1013},
> >       {"KXCJ1008", KXCJ91008},
> > +     {"KXCJ9000", KXCJ91008},
> 
> I'm guessing it's not a polling one, right?
>

It's not.

> >       {"KXTJ1009", KXTJ21009},
> >       {"SMO8500",  KXCJ91008},
> >       { },


-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 1/3] iio: accel: kxcjk-1013: add the "KXCJ9000" ACPI id

[Daniel Baluta]

On Wed, Apr 29, 2015 at 2:01 PM, Antonio Ospite <ao2@ao2.it> wrote:
> On Wed, 29 Apr 2015 12:28:40 +0200
> Bastien Nocera <hadess@hadess.net> wrote:
>
>> On Wed, 2015-04-29 at 11:01 +0200, Antonio Ospite wrote:
>> > This id has been seen in the DSTD for the Teclast X98 Air 3G tablet
>>
>> DSDT.
>
> Right. Thanks :)
>
> Jonathan, let me know if you want me to resend or if you plan to amend
> the commit message yourself.

Please resend.

thanks,
Daniel.

Re: [PATCH 1/3] iio: accel: kxcjk-1013: add the "KXCJ9000" ACPI id

[Jonathan Cameron]

On 29/04/15 16:11, Daniel Baluta wrote:
> On Wed, Apr 29, 2015 at 2:01 PM, Antonio Ospite <ao2@ao2.it> wrote:
>> On Wed, 29 Apr 2015 12:28:40 +0200
>> Bastien Nocera <hadess@hadess.net> wrote:
>>
>>> On Wed, 2015-04-29 at 11:01 +0200, Antonio Ospite wrote:
>>>> This id has been seen in the DSTD for the Teclast X98 Air 3G tablet
>>>
>>> DSDT.
>>
>> Right. Thanks :)
>>
>> Jonathan, let me know if you want me to resend or if you plan to amend
>> the commit message yourself.
> 
> Please resend.
> 
> thanks,
> Daniel.
> 
Sorry, missed this thread entirely and pull request has gone out.
I'd have changed this if it was still in testing, but the moment
it hits the public togreg branch, typos in commit messages are
not a reason to rebase (so tough luck it'll probably be in the
git log forever :)

(there are loads of typos in there so don't worry about it :)

Jonathan

Re: [PATCH 1/3] iio: accel: kxcjk-1013: add the "KXCJ9000" ACPI id

[Jonathan Cameron]

On 29/04/15 16:11, Daniel Baluta wrote:
> On Wed, Apr 29, 2015 at 2:01 PM, Antonio Ospite <ao2@ao2.it> wrote:
>> On Wed, 29 Apr 2015 12:28:40 +0200
>> Bastien Nocera <hadess@hadess.net> wrote:
>>
>>> On Wed, 2015-04-29 at 11:01 +0200, Antonio Ospite wrote:
>>>> This id has been seen in the DSTD for the Teclast X98 Air 3G tablet
>>>
>>> DSDT.
>>
>> Right. Thanks :)
>>
>> Jonathan, let me know if you want me to resend or if you plan to amend
>> the commit message yourself.
> 
> Please resend.
> 
> thanks,
> Daniel.
> 
Ignore my last email. I applied the fixed version without ever seeing
this thread :)

[PATCH 2/3] iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode

[Antonio Ospite]

The purpose of the variable is to remember when an interrupt cannot be
requested and hence the driver must work in polling mode, so tell that
instead of mentioning the specific case when this is needed.

This makes the variable name to be still meaningful when some other
devices requiring this show up.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Bastien Nocera <hadess@hadess.net>
Cc: linux-iio@vger.kernel.org
---
 drivers/iio/accel/kxcjk-1013.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 5b7a860..eccec0e 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -108,7 +108,7 @@ struct kxcjk1013_data {
 	bool motion_trigger_on;
 	int64_t timestamp;
 	enum kx_chipset chipset;
-	bool is_smo8500_device;
+	bool polling_mode;
 };
 
 enum kxcjk1013_axis {
@@ -1140,7 +1140,7 @@ static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
 
 static const char *kxcjk1013_match_acpi_device(struct device *dev,
 					       enum kx_chipset *chipset,
-					       bool *is_smo8500_device)
+					       bool *polling_mode)
 {
 	const struct acpi_device_id *id;
 
@@ -1148,7 +1148,7 @@ static const char *kxcjk1013_match_acpi_device(struct device *dev,
 	if (!id)
 		return NULL;
 	if (strcmp(id->id, "SMO8500") == 0)
-		*is_smo8500_device = true;
+		*polling_mode = true;
 	*chipset = (enum kx_chipset)id->driver_data;
 
 	return dev_name(dev);
@@ -1163,7 +1163,7 @@ static int kxcjk1013_gpio_probe(struct i2c_client *client,
 
 	if (!client)
 		return -EINVAL;
-	if (data->is_smo8500_device)
+	if (data->polling_mode)
 		return -ENOTSUPP;
 
 	dev = &client->dev;
@@ -1211,7 +1211,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
 	} else if (ACPI_HANDLE(&client->dev)) {
 		name = kxcjk1013_match_acpi_device(&client->dev,
 						   &data->chipset,
-						   &data->is_smo8500_device);
+						   &data->polling_mode);
 	} else
 		return -ENODEV;
 
-- 
2.1.4

Re: [PATCH 2/3] iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode

[Bastien Nocera]

On Wed, 2015-04-29 at 11:01 +0200, Antonio Ospite wrote:
> The purpose of the variable is to remember when an interrupt cannot 
> be
> requested and hence the driver must work in polling mode, so tell 
> that
> instead of mentioning the specific case when this is needed.
> 
> This makes the variable name to be still meaningful when some other
> devices requiring this show up.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: linux-iio@vger.kernel.org

Completely fine with that.

Signed-off-by: Bastien Nocera <hadess@hadess.net>

Re: [PATCH 2/3] iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode

[Daniel Baluta]

On Wed, Apr 29, 2015 at 12:01 PM, Antonio Ospite <ao2@ao2.it> wrote:
> The purpose of the variable is to remember when an interrupt cannot be
> requested and hence the driver must work in polling mode, so tell that
> instead of mentioning the specific case when this is needed.
>
> This makes the variable name to be still meaningful when some other
> devices requiring this show up.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: linux-iio@vger.kernel.org
> ---
>  drivers/iio/accel/kxcjk-1013.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 5b7a860..eccec0e 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -108,7 +108,7 @@ struct kxcjk1013_data {
>         bool motion_trigger_on;
>         int64_t timestamp;
>         enum kx_chipset chipset;
> -       bool is_smo8500_device;
> +       bool polling_mode;

I would prefer to call this field use_poll, as introduced in:

http://marc.info/?l=linux-iio&m=142980650422289&w=2

Also use_poll its it's a better suggestion on the type than polling_mode.

Other than that the patch looks good.


thanks,
Daniel.

Re: [PATCH 2/3] iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode

[Antonio Ospite]

On Wed, 29 Apr 2015 18:08:41 +0300
Daniel Baluta <daniel.baluta@gmail.com> wrote:

> On Wed, Apr 29, 2015 at 12:01 PM, Antonio Ospite <ao2@ao2.it> wrote:
> > The purpose of the variable is to remember when an interrupt cannot be
> > requested and hence the driver must work in polling mode, so tell that
> > instead of mentioning the specific case when this is needed.
> >
> > This makes the variable name to be still meaningful when some other
> > devices requiring this show up.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Bastien Nocera <hadess@hadess.net>
> > Cc: linux-iio@vger.kernel.org
> > ---
> >  drivers/iio/accel/kxcjk-1013.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> > index 5b7a860..eccec0e 100644
> > --- a/drivers/iio/accel/kxcjk-1013.c
> > +++ b/drivers/iio/accel/kxcjk-1013.c
> > @@ -108,7 +108,7 @@ struct kxcjk1013_data {
> >         bool motion_trigger_on;
> >         int64_t timestamp;
> >         enum kx_chipset chipset;
> > -       bool is_smo8500_device;
> > +       bool polling_mode;
> 
> I would prefer to call this field use_poll, as introduced in:
>
> http://marc.info/?l=linux-iio&m=142980650422289&w=2
>

Looking at it again I realize that the current meaning  of
is_smo8500_device is more like "gpio interrupt not available" rather
than an explicit indication to use polling mode, which right now is a
userspace thing anyway.

It was mainly to prevent error messages being printed in the logs when
trying to getting a gpio descriptor, is that correct Bastien?

> Also use_poll its it's a better suggestion on the type than polling_mode.
> 

Agreed, polling_mode was not the best name.

How about we drop this patch 2/3 altogether for now and I just resend 1
and 3?

Renaming (or even removing) is_smo8500_device can be re-evaluated when
use_poll gets added.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 2/3] iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode

[Bastien Nocera]

On Thu, 2015-04-30 at 16:25 +0200, Antonio Ospite wrote:
> It was mainly to prevent error messages being printed in the logs 
> when
> trying to getting a gpio descriptor, is that correct Bastien?

Actually, to prevent it failing to init the driver altogether.

Re: [PATCH 2/3] iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode

[Daniel Baluta]

On Thu, Apr 30, 2015 at 5:27 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Thu, 2015-04-30 at 16:25 +0200, Antonio Ospite wrote:
>> It was mainly to prevent error messages being printed in the logs
>> when
>> trying to getting a gpio descriptor, is that correct Bastien?
>
> Actually, to prevent it failing to init the driver altogether.

This is not true :).

I agree with Antonio :), this is just a hack to avoid this error message:

""acpi gpio get index failed"

The initial patch introducing is_smo8500_device shouldn't have been
merged from start :) but it was Jonathan's decision :).

See: http://marc.info/?l=linux-iio&m=141520240809938&w=2

I think dropping patch 2/3 is the best option for now.

thanks,
Daniel.

Re: [PATCH 2/3] iio: accel: kxcjk-1013: rename is_smo8500_device to polling_mode

[Antonio Ospite]

On Thu, 30 Apr 2015 16:27:49 +0200
Bastien Nocera <hadess@hadess.net> wrote:

> On Thu, 2015-04-30 at 16:25 +0200, Antonio Ospite wrote:
> > It was mainly to prevent error messages being printed in the logs 
> > when
> > trying to getting a gpio descriptor, is that correct Bastien?
> 
> Actually, to prevent it failing to init the driver altogether.

Then I am afraid I am failing to follow the code (4.1-rc1), from what
I read:

  - is_smo8500_device is only used in kxcjk1013_gpio_probe() to make it
    return -ENOTSUPP; i.e. client->irq = -ENOTSUPP;

  - when client->irq < 0 (even after kxcjk1013_gpio_probe), the driver
    does not request an interrupt  and does not set up triggers but it
    can still finish initialization.

So it looks to me that is_smo8500_device just prevents
devm_gpiod_get_index() from being called, but even if it was called it
(or gpiod_to_irq) should return an error.

Mmh, maybe the problem is "just" in checking the return value of
devm_gpiod_get_index() in kxcjk1013_gpio_probe(), maybe it's NULL in
your case and that was causing your original problem; BTW if it is NULL
then desc_to_gpio() in the debug print will Oops after commit b457f5.

I'll double check and get back to you.

Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH 3/3] iio: accel: kxcjk-1013: add some blank lines for readability

[Antonio Ospite]

Some extra blank lines between if checks don't hurt and improve
readability.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Bastien Nocera <hadess@hadess.net>
Cc: linux-iio@vger.kernel.org
---
 drivers/iio/accel/kxcjk-1013.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index eccec0e..2519692 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1147,8 +1147,10 @@ static const char *kxcjk1013_match_acpi_device(struct device *dev,
 	id = acpi_match_device(dev->driver->acpi_match_table, dev);
 	if (!id)
 		return NULL;
+
 	if (strcmp(id->id, "SMO8500") == 0)
 		*polling_mode = true;
+
 	*chipset = (enum kx_chipset)id->driver_data;
 
 	return dev_name(dev);
@@ -1163,6 +1165,7 @@ static int kxcjk1013_gpio_probe(struct i2c_client *client,
 
 	if (!client)
 		return -EINVAL;
+
 	if (data->polling_mode)
 		return -ENOTSUPP;
 
-- 
2.1.4