[PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Rodrigo Ribeiro]

Remove the checkpatch.pl check:

CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?

Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
---
This macro is not used anywhere. Should we just correct the
spelling or remove the macro?

 drivers/staging/iio/cdc/ad7152.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
index 25f51db..c9da6d4 100644
--- a/drivers/staging/iio/cdc/ad7152.c
+++ b/drivers/staging/iio/cdc/ad7152.c
@@ -35,7 +35,7 @@
 #define AD7152_REG_CH2_GAIN_HIGH	12
 #define AD7152_REG_CH2_SETUP		14
 #define AD7152_REG_CFG			15
-#define AD7152_REG_RESEVERD		16
+#define AD7152_REG_RESERVED		16
 #define AD7152_REG_CAPDAC_POS		17
 #define AD7152_REG_CAPDAC_NEG		18
 #define AD7152_REG_CFG2			26
-- 
2.7.4

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Alexandru Ardelean]

On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
>
> Remove the checkpatch.pl check:
>
> CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?

Hey,

A bit curios about this one.
Are you using this chip/driver ?

Thing is: the part is nearing EOL, and it could be an idea to be
marked for removal (since it's still in staging).
But if there are users for this driver, it could be left around for a while.

Thanks
Alex

>
> Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> ---
> This macro is not used anywhere. Should we just correct the
> spelling or remove the macro?
>
>  drivers/staging/iio/cdc/ad7152.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> index 25f51db..c9da6d4 100644
> --- a/drivers/staging/iio/cdc/ad7152.c
> +++ b/drivers/staging/iio/cdc/ad7152.c
> @@ -35,7 +35,7 @@
>  #define AD7152_REG_CH2_GAIN_HIGH       12
>  #define AD7152_REG_CH2_SETUP           14
>  #define AD7152_REG_CFG                 15
> -#define AD7152_REG_RESEVERD            16
> +#define AD7152_REG_RESERVED            16
>  #define AD7152_REG_CAPDAC_POS          17
>  #define AD7152_REG_CAPDAC_NEG          18
>  #define AD7152_REG_CFG2                        26
> --
> 2.7.4
>

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Rodrigo Ribeiro]

Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
<ardeleanalex@gmail.com> escreveu:
>
> On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> >
> > Remove the checkpatch.pl check:
> >
> > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
>
> Hey,

Hi,

Thanks for answering.

> A bit curios about this one.
> Are you using this chip/driver ?

No, I am just a student at USP (University of São Paulo) starting in Linux
Kernel and a new member of the USP Linux Kernel group that has contributed
for a few months.

> Thing is: the part is nearing EOL, and it could be an idea to be
> marked for removal (since it's still in staging).
> But if there are users for this driver, it could be left around for a while.

This is my first patch in Linux Kernel, but if the driver will be removed, I
can send a patch for another driver. Is there any driver that I can
fix a style warning?

Thanks


Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
<ardeleanalex@gmail.com> escreveu:
>
> On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> >
> > Remove the checkpatch.pl check:
> >
> > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
>
> Hey,
>
> A bit curios about this one.
> Are you using this chip/driver ?
>
> Thing is: the part is nearing EOL, and it could be an idea to be
> marked for removal (since it's still in staging).
> But if there are users for this driver, it could be left around for a while.
>
> Thanks
> Alex
>
> >
> > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > ---
> > This macro is not used anywhere. Should we just correct the
> > spelling or remove the macro?
> >
> >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> > index 25f51db..c9da6d4 100644
> > --- a/drivers/staging/iio/cdc/ad7152.c
> > +++ b/drivers/staging/iio/cdc/ad7152.c
> > @@ -35,7 +35,7 @@
> >  #define AD7152_REG_CH2_GAIN_HIGH       12
> >  #define AD7152_REG_CH2_SETUP           14
> >  #define AD7152_REG_CFG                 15
> > -#define AD7152_REG_RESEVERD            16
> > +#define AD7152_REG_RESERVED            16
> >  #define AD7152_REG_CAPDAC_POS          17
> >  #define AD7152_REG_CAPDAC_NEG          18
> >  #define AD7152_REG_CFG2                        26
> > --
> > 2.7.4
> >

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Rodrigo Ribeiro]

Em sex, 25 de jan de 2019 às 21:46, Rodrigo Ribeiro
<rodrigorsdc@gmail.com> escreveu:
>
> Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> <ardeleanalex@gmail.com> escreveu:
> >
> > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > >
> > > Remove the checkpatch.pl check:
> > >
> > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> >
> > Hey,
>
> Hi,
>
> Thanks for answering.
>
> > A bit curios about this one.
> > Are you using this chip/driver ?
>
> No, I am just a student at USP (University of São Paulo) starting in Linux
> Kernel and a new member of the USP Linux Kernel group that has contributed
> for a few months.
>
> > Thing is: the part is nearing EOL, and it could be an idea to be
> > marked for removal (since it's still in staging).
> > But if there are users for this driver, it could be left around for a while.
>
> This is my first patch in Linux Kernel, but if the driver will be removed, I
> can send a patch for another driver. Is there any driver that I can
> fix a style warning?

Maybe, one checkstyle patch is enough, right? Which drivers can I truly
contribute to?

> Thanks
>
>
> Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> <ardeleanalex@gmail.com> escreveu:
> >
> > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > >
> > > Remove the checkpatch.pl check:
> > >
> > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> >
> > Hey,
> >
> > A bit curios about this one.
> > Are you using this chip/driver ?
> >
> > Thing is: the part is nearing EOL, and it could be an idea to be
> > marked for removal (since it's still in staging).
> > But if there are users for this driver, it could be left around for a while.
> >
> > Thanks
> > Alex
> >
> > >
> > > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > > ---
> > > This macro is not used anywhere. Should we just correct the
> > > spelling or remove the macro?
> > >
> > >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> > > index 25f51db..c9da6d4 100644
> > > --- a/drivers/staging/iio/cdc/ad7152.c
> > > +++ b/drivers/staging/iio/cdc/ad7152.c
> > > @@ -35,7 +35,7 @@
> > >  #define AD7152_REG_CH2_GAIN_HIGH       12
> > >  #define AD7152_REG_CH2_SETUP           14
> > >  #define AD7152_REG_CFG                 15
> > > -#define AD7152_REG_RESEVERD            16
> > > +#define AD7152_REG_RESERVED            16
> > >  #define AD7152_REG_CAPDAC_POS          17
> > >  #define AD7152_REG_CAPDAC_NEG          18
> > >  #define AD7152_REG_CFG2                        26
> > > --
> > > 2.7.4
> > >

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Jonathan Cameron]

On Fri, 25 Jan 2019 22:14:32 -0200
Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:

> Em sex, 25 de jan de 2019 às 21:46, Rodrigo Ribeiro
> <rodrigorsdc@gmail.com> escreveu:
> >
> > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > <ardeleanalex@gmail.com> escreveu:  
> > >
> > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:  
> > > >
> > > > Remove the checkpatch.pl check:
> > > >
> > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?  
> > >
> > > Hey,  
> >
> > Hi,
> >
> > Thanks for answering.
> >  
> > > A bit curios about this one.
> > > Are you using this chip/driver ?  
> >
> > No, I am just a student at USP (University of São Paulo) starting in Linux
> > Kernel and a new member of the USP Linux Kernel group that has contributed
> > for a few months.
> >  
> > > Thing is: the part is nearing EOL, and it could be an idea to be
> > > marked for removal (since it's still in staging).
> > > But if there are users for this driver, it could be left around for a while.  
> >
> > This is my first patch in Linux Kernel, but if the driver will be removed, I
> > can send a patch for another driver. Is there any driver that I can
> > fix a style warning?  
> 
> Maybe, one checkstyle patch is enough, right? Which drivers can I truly
> contribute to?

How about the ad7150?  That one is still listed as production.
What do you think Alex, you probably have better visibility on the
status of these parts and their drivers than I do!

(note I haven't even opened that one for a few years so no idea
what state the driver is in!)

Jonathan

> 
> > Thanks
> >
> >
> > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > <ardeleanalex@gmail.com> escreveu:  
> > >
> > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:  
> > > >
> > > > Remove the checkpatch.pl check:
> > > >
> > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?  
> > >
> > > Hey,
> > >
> > > A bit curios about this one.
> > > Are you using this chip/driver ?
> > >
> > > Thing is: the part is nearing EOL, and it could be an idea to be
> > > marked for removal (since it's still in staging).
> > > But if there are users for this driver, it could be left around for a while.
> > >
> > > Thanks
> > > Alex
> > >  
> > > >
> > > > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > > > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > > > ---
> > > > This macro is not used anywhere. Should we just correct the
> > > > spelling or remove the macro?
> > > >
> > > >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> > > > index 25f51db..c9da6d4 100644
> > > > --- a/drivers/staging/iio/cdc/ad7152.c
> > > > +++ b/drivers/staging/iio/cdc/ad7152.c
> > > > @@ -35,7 +35,7 @@
> > > >  #define AD7152_REG_CH2_GAIN_HIGH       12
> > > >  #define AD7152_REG_CH2_SETUP           14
> > > >  #define AD7152_REG_CFG                 15
> > > > -#define AD7152_REG_RESEVERD            16
> > > > +#define AD7152_REG_RESERVED            16
> > > >  #define AD7152_REG_CAPDAC_POS          17
> > > >  #define AD7152_REG_CAPDAC_NEG          18
> > > >  #define AD7152_REG_CFG2                        26
> > > > --
> > > > 2.7.4
> > > >  

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Alexandru Ardelean]

On Sat, Jan 26, 2019 at 8:13 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 25 Jan 2019 22:14:32 -0200
> Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
>
> > Em sex, 25 de jan de 2019 às 21:46, Rodrigo Ribeiro
> > <rodrigorsdc@gmail.com> escreveu:
> > >
> > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > <ardeleanalex@gmail.com> escreveu:
> > > >
> > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > > > >
> > > > > Remove the checkpatch.pl check:
> > > > >
> > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> > > >
> > > > Hey,
> > >
> > > Hi,
> > >
> > > Thanks for answering.
> > >
> > > > A bit curios about this one.
> > > > Are you using this chip/driver ?
> > >
> > > No, I am just a student at USP (University of São Paulo) starting in Linux
> > > Kernel and a new member of the USP Linux Kernel group that has contributed
> > > for a few months.
> > >
> > > > Thing is: the part is nearing EOL, and it could be an idea to be
> > > > marked for removal (since it's still in staging).
> > > > But if there are users for this driver, it could be left around for a while.
> > >
> > > This is my first patch in Linux Kernel, but if the driver will be removed, I
> > > can send a patch for another driver. Is there any driver that I can
> > > fix a style warning?
> >
> > Maybe, one checkstyle patch is enough, right? Which drivers can I truly
> > contribute to?
>
> How about the ad7150?  That one is still listed as production.
> What do you think Alex, you probably have better visibility on the
> status of these parts and their drivers than I do!
>
> (note I haven't even opened that one for a few years so no idea
> what state the driver is in!)
>

ad7150 is a good alternative.
At a first glance over the driver it looks like it could do with some
polish/conversions to newer IIO constructs (like IIO triggers, maybe
some newer event handling mechanisms?).
I'll sync with Stefan [Popa] to see about this stuff at a later point in time.

I'd also add here the adxl345 driver.
This is mostly informational for anyone who'd find this interesting.
There are 2 drivers for this chip, one in IIO
[drivers/iio/accel/adxl345] and another one in
"drivers/misc/adxl34x.c" as part of the input sub-system.
What would be interesting here is to finalize the IIO driver [ I think
some features are lacking behind the input driver], and make the input
driver a consumer of the IIO driver.

From my side, both these variants are fine to take on.
The ad7150 is a good idea as a starter project, and the adxl one is
more of a long-term medium-level project.

Thanks
Alex

> Jonathan
>
> >
> > > Thanks
> > >
> > >
> > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > <ardeleanalex@gmail.com> escreveu:
> > > >
> > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > > > >
> > > > > Remove the checkpatch.pl check:
> > > > >
> > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> > > >
> > > > Hey,
> > > >
> > > > A bit curios about this one.
> > > > Are you using this chip/driver ?
> > > >
> > > > Thing is: the part is nearing EOL, and it could be an idea to be
> > > > marked for removal (since it's still in staging).
> > > > But if there are users for this driver, it could be left around for a while.
> > > >
> > > > Thanks
> > > > Alex
> > > >
> > > > >
> > > > > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > > > > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > > > > ---
> > > > > This macro is not used anywhere. Should we just correct the
> > > > > spelling or remove the macro?
> > > > >
> > > > >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> > > > > index 25f51db..c9da6d4 100644
> > > > > --- a/drivers/staging/iio/cdc/ad7152.c
> > > > > +++ b/drivers/staging/iio/cdc/ad7152.c
> > > > > @@ -35,7 +35,7 @@
> > > > >  #define AD7152_REG_CH2_GAIN_HIGH       12
> > > > >  #define AD7152_REG_CH2_SETUP           14
> > > > >  #define AD7152_REG_CFG                 15
> > > > > -#define AD7152_REG_RESEVERD            16
> > > > > +#define AD7152_REG_RESERVED            16
> > > > >  #define AD7152_REG_CAPDAC_POS          17
> > > > >  #define AD7152_REG_CAPDAC_NEG          18
> > > > >  #define AD7152_REG_CFG2                        26
> > > > > --
> > > > > 2.7.4
> > > > >
>

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Popa, Stefan Serban]

On Mi, 2019-02-13 at 22:25 -0200, Rodrigo Ribeiro wrote:
> [External]
> 
> 
> Em ter, 29 de jan de 2019 às 07:10, Alexandru Ardelean <ardeleanalex@gmai
> l.com> escreveu:
> >
> > On Sat, Jan 26, 2019 at 8:13 PM Jonathan Cameron <jic23@kernel.org>
> wrote:
> > >
> > > On Fri, 25 Jan 2019 22:14:32 -0200
> > > Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > >
> > > > Em sex, 25 de jan de 2019 às 21:46, Rodrigo Ribeiro
> > > > <rodrigorsdc@gmail.com> escreveu:
> > > > >
> > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > > > <ardeleanalex@gmail.com> escreveu:
> > > > > >
> > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gm
> ail.com> wrote:
> > > > > > >
> > > > > > > Remove the checkpatch.pl check:
> > > > > > >
> > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> > > > > >
> > > > > > Hey,
> > > > >
> > > > > Hi,
> > > > >
> > > > > Thanks for answering.
> > > > >
> > > > > > A bit curios about this one.
> > > > > > Are you using this chip/driver ?
> > > > >
> > > > > No, I am just a student at USP (University of São Paulo) starting
> in Linux
> > > > > Kernel and a new member of the USP Linux Kernel group that has
> contributed
> > > > > for a few months.
> > > > >
> > > > > > Thing is: the part is nearing EOL, and it could be an idea to
> be
> > > > > > marked for removal (since it's still in staging).
> > > > > > But if there are users for this driver, it could be left around
> for a while.
> > > > >
> > > > > This is my first patch in Linux Kernel, but if the driver will be
> removed, I
> > > > > can send a patch for another driver. Is there any driver that I
> can
> > > > > fix a style warning?
> > > >
> > > > Maybe, one checkstyle patch is enough, right? Which drivers can I
> truly
> > > > contribute to?
> > >
> > > How about the ad7150?  That one is still listed as production.
> > > What do you think Alex, you probably have better visibility on the
> > > status of these parts and their drivers than I do!
> > >
> > > (note I haven't even opened that one for a few years so no idea
> > > what state the driver is in!)
> > >
> >
> > ad7150 is a good alternative.
> > At a first glance over the driver it looks like it could do with some
> > polish/conversions to newer IIO constructs (like IIO triggers, maybe
> > some newer event handling mechanisms?).
> > I'll sync with Stefan [Popa] to see about this stuff at a later point
> in time.
> >
> > I'd also add here the adxl345 driver.
> > This is mostly informational for anyone who'd find this interesting.
> > There are 2 drivers for this chip, one in IIO
> > [drivers/iio/accel/adxl345] and another one in
> > "drivers/misc/adxl34x.c" as part of the input sub-system.
> > What would be interesting here is to finalize the IIO driver [ I think
> > some features are lacking behind the input driver], and make the input
> > driver a consumer of the IIO driver.
> >
> > From my side, both these variants are fine to take on.
> > The ad7150 is a good idea as a starter project, and the adxl one is
> > more of a long-term medium-level project.
> >
> > Thanks
> > Alex
> >
> 
> Hi Alex, thanks for suggestions.
> 
> I read the IIO trigger documentation 
> (https://www.kernel.org/doc/html/v4.12/driver-api/iio/triggers.html) and
> ask one question: What is the difference between events and triggers? 
> They are sounds me same things.
> 
> I am trying to understand how to implement a IIO trigger by reading
> the IIO Simple Dummy code but this driver does not implements IIO
> triggers
> but only IIO events. Is there a didactic example like IIO Simple Dummy
> that implements IIO triggers?
> 
> Thanks
> Rodrigo
> 

Hi Rodrigo,

From what I know, IIO Events are not used for passing readings from devices
to userspace, but rather out of bounds information such as crossing of
voltage thresholds, proximity detection, motion detection, etc.
Triggers are typically used to determine when valid data can be read from a
device which is then stored in the buffer.

After a quick look over the AD7150, I think that using IIO events, might be
the correct approach, since it is a proximity sensing device. 

-Stefan

> > > Jonathan
> > >
> > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > > > <ardeleanalex@gmail.com> escreveu:
> > > > > >
> > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gm
> ail.com> wrote:
> > > > > > >
> > > > > > > Remove the checkpatch.pl check:
> > > > > > >
> > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> > > > > >
> > > > > > Hey,
> > > > > >
> > > > > > A bit curios about this one.
> > > > > > Are you using this chip/driver ?
> > > > > >
> > > > > > Thing is: the part is nearing EOL, and it could be an idea to
> be
> > > > > > marked for removal (since it's still in staging).
> > > > > > But if there are users for this driver, it could be left around
> for a while.
> > > > > >
> > > > > > Thanks
> > > > > > Alex
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > > > > > > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > > > > > > ---
> > > > > > > This macro is not used anywhere. Should we just correct the
> > > > > > > spelling or remove the macro?
> > > > > > >
> > > > > > >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/staging/iio/cdc/ad7152.c
> b/drivers/staging/iio/cdc/ad7152.c
> > > > > > > index 25f51db..c9da6d4 100644
> > > > > > > --- a/drivers/staging/iio/cdc/ad7152.c
> > > > > > > +++ b/drivers/staging/iio/cdc/ad7152.c
> > > > > > > @@ -35,7 +35,7 @@
> > > > > > >  #define AD7152_REG_CH2_GAIN_HIGH       12
> > > > > > >  #define AD7152_REG_CH2_SETUP           14
> > > > > > >  #define AD7152_REG_CFG                 15
> > > > > > > -#define AD7152_REG_RESEVERD            16
> > > > > > > +#define AD7152_REG_RESERVED            16
> > > > > > >  #define AD7152_REG_CAPDAC_POS          17
> > > > > > >  #define AD7152_REG_CAPDAC_NEG          18
> > > > > > >  #define AD7152_REG_CFG2                        26
> > > > > > > --
> > > > > > > 2.7.4
> > > > > > >
> > >

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Jonathan Cameron]

On Thu, 14 Feb 2019 10:51:49 +0000
"Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote:

> On Mi, 2019-02-13 at 22:25 -0200, Rodrigo Ribeiro wrote:
> > [External]
> > 
> > 
> > Em ter, 29 de jan de 2019 às 07:10, Alexandru Ardelean <ardeleanalex@gmai  
> > l.com> escreveu:
> > >
> > > On Sat, Jan 26, 2019 at 8:13 PM Jonathan Cameron <jic23@kernel.org>  
> > wrote:  
> > > >
> > > > On Fri, 25 Jan 2019 22:14:32 -0200
> > > > Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > > >  
> > > > > Em sex, 25 de jan de 2019 às 21:46, Rodrigo Ribeiro
> > > > > <rodrigorsdc@gmail.com> escreveu:  
> > > > > >
> > > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > > > > <ardeleanalex@gmail.com> escreveu:  
> > > > > > >
> > > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gm  
> > ail.com> wrote:  
> > > > > > > >
> > > > > > > > Remove the checkpatch.pl check:
> > > > > > > >
> > > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?  
> > > > > > >
> > > > > > > Hey,  
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Thanks for answering.
> > > > > >  
> > > > > > > A bit curios about this one.
> > > > > > > Are you using this chip/driver ?  
> > > > > >
> > > > > > No, I am just a student at USP (University of São Paulo) starting  
> > in Linux  
> > > > > > Kernel and a new member of the USP Linux Kernel group that has  
> > contributed  
> > > > > > for a few months.
> > > > > >  
> > > > > > > Thing is: the part is nearing EOL, and it could be an idea to  
> > be  
> > > > > > > marked for removal (since it's still in staging).
> > > > > > > But if there are users for this driver, it could be left around  
> > for a while.  
> > > > > >
> > > > > > This is my first patch in Linux Kernel, but if the driver will be  
> > removed, I  
> > > > > > can send a patch for another driver. Is there any driver that I  
> > can  
> > > > > > fix a style warning?  
> > > > >
> > > > > Maybe, one checkstyle patch is enough, right? Which drivers can I  
> > truly  
> > > > > contribute to?  
> > > >
> > > > How about the ad7150?  That one is still listed as production.
> > > > What do you think Alex, you probably have better visibility on the
> > > > status of these parts and their drivers than I do!
> > > >
> > > > (note I haven't even opened that one for a few years so no idea
> > > > what state the driver is in!)
> > > >  
> > >
> > > ad7150 is a good alternative.
> > > At a first glance over the driver it looks like it could do with some
> > > polish/conversions to newer IIO constructs (like IIO triggers, maybe
> > > some newer event handling mechanisms?).
> > > I'll sync with Stefan [Popa] to see about this stuff at a later point  
> > in time.  
> > >
> > > I'd also add here the adxl345 driver.
> > > This is mostly informational for anyone who'd find this interesting.
> > > There are 2 drivers for this chip, one in IIO
> > > [drivers/iio/accel/adxl345] and another one in
> > > "drivers/misc/adxl34x.c" as part of the input sub-system.
> > > What would be interesting here is to finalize the IIO driver [ I think
> > > some features are lacking behind the input driver], and make the input
> > > driver a consumer of the IIO driver.
> > >
> > > From my side, both these variants are fine to take on.
> > > The ad7150 is a good idea as a starter project, and the adxl one is
> > > more of a long-term medium-level project.
> > >
> > > Thanks
> > > Alex
> > >  
> > 
> > Hi Alex, thanks for suggestions.
> > 
> > I read the IIO trigger documentation 
> > (https://www.kernel.org/doc/html/v4.12/driver-api/iio/triggers.html) and
> > ask one question: What is the difference between events and triggers? 
> > They are sounds me same things.
> > 
> > I am trying to understand how to implement a IIO trigger by reading
> > the IIO Simple Dummy code but this driver does not implements IIO
> > triggers
> > but only IIO events. Is there a didactic example like IIO Simple Dummy
> > that implements IIO triggers?
> > 
> > Thanks
> > Rodrigo
> >   
> 
> Hi Rodrigo,
> 
> From what I know, IIO Events are not used for passing readings from devices
> to userspace, but rather out of bounds information such as crossing of
> voltage thresholds, proximity detection, motion detection, etc.
> Triggers are typically used to determine when valid data can be read from a
> device which is then stored in the buffer.
> 
> After a quick look over the AD7150, I think that using IIO events, might be
> the correct approach, since it is a proximity sensing device. 

To answer the question on generic triggers (agreed, probably not relevant here
from what Stefan has said), there are several software triggers under
drivers/iio/triggers.

Wasn't a lot of point in adding another one to the dummy driver given you
can use the hrtimer, or sysfs triggers directly.
(loop will result in craziness given the near zero read time of the
dummy driver ;)

Jonathan

> 
> -Stefan
> 
> > > > Jonathan
> > > >  
> > > > >  
> > > > > > Thanks
> > > > > >
> > > > > >
> > > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > > > > <ardeleanalex@gmail.com> escreveu:  
> > > > > > >
> > > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gm  
> > ail.com> wrote:  
> > > > > > > >
> > > > > > > > Remove the checkpatch.pl check:
> > > > > > > >
> > > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?  
> > > > > > >
> > > > > > > Hey,
> > > > > > >
> > > > > > > A bit curios about this one.
> > > > > > > Are you using this chip/driver ?
> > > > > > >
> > > > > > > Thing is: the part is nearing EOL, and it could be an idea to  
> > be  
> > > > > > > marked for removal (since it's still in staging).
> > > > > > > But if there are users for this driver, it could be left around  
> > for a while.  
> > > > > > >
> > > > > > > Thanks
> > > > > > > Alex
> > > > > > >  
> > > > > > > >
> > > > > > > > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > > > > > > > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > > > > > > > ---
> > > > > > > > This macro is not used anywhere. Should we just correct the
> > > > > > > > spelling or remove the macro?
> > > > > > > >
> > > > > > > >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/staging/iio/cdc/ad7152.c  
> > b/drivers/staging/iio/cdc/ad7152.c  
> > > > > > > > index 25f51db..c9da6d4 100644
> > > > > > > > --- a/drivers/staging/iio/cdc/ad7152.c
> > > > > > > > +++ b/drivers/staging/iio/cdc/ad7152.c
> > > > > > > > @@ -35,7 +35,7 @@
> > > > > > > >  #define AD7152_REG_CH2_GAIN_HIGH       12
> > > > > > > >  #define AD7152_REG_CH2_SETUP           14
> > > > > > > >  #define AD7152_REG_CFG                 15
> > > > > > > > -#define AD7152_REG_RESEVERD            16
> > > > > > > > +#define AD7152_REG_RESERVED            16
> > > > > > > >  #define AD7152_REG_CAPDAC_POS          17
> > > > > > > >  #define AD7152_REG_CAPDAC_NEG          18
> > > > > > > >  #define AD7152_REG_CFG2                        26
> > > > > > > > --
> > > > > > > > 2.7.4
> > > > > > > >  
> > >   

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Rodrigo Ribeiro]

Em seg, 18 de fev de 2019 às 11:46, Jonathan Cameron
<jonathan.cameron@huawei.com> escreveu:
>
> On Thu, 14 Feb 2019 10:51:49 +0000
> "Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote:
>
> > On Mi, 2019-02-13 at 22:25 -0200, Rodrigo Ribeiro wrote:
> > > [External]
> > >
> > >
> > > Em ter, 29 de jan de 2019 às 07:10, Alexandru Ardelean <ardeleanalex@gmai
> > > l.com> escreveu:
> > > >
> > > > On Sat, Jan 26, 2019 at 8:13 PM Jonathan Cameron <jic23@kernel.org>
> > > wrote:
> > > > >
> > > > > On Fri, 25 Jan 2019 22:14:32 -0200
> > > > > Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > > > >
> > > > > > Em sex, 25 de jan de 2019 às 21:46, Rodrigo Ribeiro
> > > > > > <rodrigorsdc@gmail.com> escreveu:
> > > > > > >
> > > > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > > > > > <ardeleanalex@gmail.com> escreveu:
> > > > > > > >
> > > > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gm
> > > ail.com> wrote:
> > > > > > > > >
> > > > > > > > > Remove the checkpatch.pl check:
> > > > > > > > >
> > > > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> > > > > > > >
> > > > > > > > Hey,
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > Thanks for answering.
> > > > > > >
> > > > > > > > A bit curios about this one.
> > > > > > > > Are you using this chip/driver ?
> > > > > > >
> > > > > > > No, I am just a student at USP (University of São Paulo) starting
> > > in Linux
> > > > > > > Kernel and a new member of the USP Linux Kernel group that has
> > > contributed
> > > > > > > for a few months.
> > > > > > >
> > > > > > > > Thing is: the part is nearing EOL, and it could be an idea to
> > > be
> > > > > > > > marked for removal (since it's still in staging).
> > > > > > > > But if there are users for this driver, it could be left around
> > > for a while.
> > > > > > >
> > > > > > > This is my first patch in Linux Kernel, but if the driver will be
> > > removed, I
> > > > > > > can send a patch for another driver. Is there any driver that I
> > > can
> > > > > > > fix a style warning?
> > > > > >
> > > > > > Maybe, one checkstyle patch is enough, right? Which drivers can I
> > > truly
> > > > > > contribute to?
> > > > >
> > > > > How about the ad7150?  That one is still listed as production.
> > > > > What do you think Alex, you probably have better visibility on the
> > > > > status of these parts and their drivers than I do!
> > > > >
> > > > > (note I haven't even opened that one for a few years so no idea
> > > > > what state the driver is in!)
> > > > >
> > > >
> > > > ad7150 is a good alternative.
> > > > At a first glance over the driver it looks like it could do with some
> > > > polish/conversions to newer IIO constructs (like IIO triggers, maybe
> > > > some newer event handling mechanisms?).
> > > > I'll sync with Stefan [Popa] to see about this stuff at a later point
> > > in time.
> > > >
> > > > I'd also add here the adxl345 driver.
> > > > This is mostly informational for anyone who'd find this interesting.
> > > > There are 2 drivers for this chip, one in IIO
> > > > [drivers/iio/accel/adxl345] and another one in
> > > > "drivers/misc/adxl34x.c" as part of the input sub-system.
> > > > What would be interesting here is to finalize the IIO driver [ I think
> > > > some features are lacking behind the input driver], and make the input
> > > > driver a consumer of the IIO driver.
> > > >
> > > > From my side, both these variants are fine to take on.
> > > > The ad7150 is a good idea as a starter project, and the adxl one is
> > > > more of a long-term medium-level project.
> > > >
> > > > Thanks
> > > > Alex
> > > >
> > >
> > > Hi Alex, thanks for suggestions.
> > >
> > > I read the IIO trigger documentation
> > > (https://www.kernel.org/doc/html/v4.12/driver-api/iio/triggers.html) and
> > > ask one question: What is the difference between events and triggers?
> > > They are sounds me same things.
> > >
> > > I am trying to understand how to implement a IIO trigger by reading
> > > the IIO Simple Dummy code but this driver does not implements IIO
> > > triggers
> > > but only IIO events. Is there a didactic example like IIO Simple Dummy
> > > that implements IIO triggers?
> > >
> > > Thanks
> > > Rodrigo
> > >
> >
> > Hi Rodrigo,
> >
> > From what I know, IIO Events are not used for passing readings from devices
> > to userspace, but rather out of bounds information such as crossing of
> > voltage thresholds, proximity detection, motion detection, etc.
> > Triggers are typically used to determine when valid data can be read from a
> > device which is then stored in the buffer.
> >
> > After a quick look over the AD7150, I think that using IIO events, might be
> > the correct approach, since it is a proximity sensing device.
>
> To answer the question on generic triggers (agreed, probably not relevant here
> from what Stefan has said), there are several software triggers under
> drivers/iio/triggers.
>
> Wasn't a lot of point in adding another one to the dummy driver given you
> can use the hrtimer, or sysfs triggers directly.
> (loop will result in craziness given the near zero read time of the
> dummy driver ;)
>
> Jonathan
>
> >
> > -Stefan
> >
> > > > > Jonathan
> > > > >
> > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > Em sex, 25 de jan de 2019 às 06:20, Alexandru Ardelean
> > > > > > > <ardeleanalex@gmail.com> escreveu:
> > > > > > > >
> > > > > > > > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gm
> > > ail.com> wrote:
> > > > > > > > >
> > > > > > > > > Remove the checkpatch.pl check:
> > > > > > > > >
> > > > > > > > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> > > > > > > >
> > > > > > > > Hey,
> > > > > > > >
> > > > > > > > A bit curios about this one.
> > > > > > > > Are you using this chip/driver ?
> > > > > > > >
> > > > > > > > Thing is: the part is nearing EOL, and it could be an idea to
> > > be
> > > > > > > > marked for removal (since it's still in staging).
> > > > > > > > But if there are users for this driver, it could be left around
> > > for a while.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > Alex
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > > > > > > > > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > > > > > > > > ---
> > > > > > > > > This macro is not used anywhere. Should we just correct the
> > > > > > > > > spelling or remove the macro?
> > > > > > > > >
> > > > > > > > >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/staging/iio/cdc/ad7152.c
> > > b/drivers/staging/iio/cdc/ad7152.c
> > > > > > > > > index 25f51db..c9da6d4 100644
> > > > > > > > > --- a/drivers/staging/iio/cdc/ad7152.c
> > > > > > > > > +++ b/drivers/staging/iio/cdc/ad7152.c
> > > > > > > > > @@ -35,7 +35,7 @@
> > > > > > > > >  #define AD7152_REG_CH2_GAIN_HIGH       12
> > > > > > > > >  #define AD7152_REG_CH2_SETUP           14
> > > > > > > > >  #define AD7152_REG_CFG                 15
> > > > > > > > > -#define AD7152_REG_RESEVERD            16
> > > > > > > > > +#define AD7152_REG_RESERVED            16
> > > > > > > > >  #define AD7152_REG_CAPDAC_POS          17
> > > > > > > > >  #define AD7152_REG_CAPDAC_NEG          18
> > > > > > > > >  #define AD7152_REG_CFG2                        26
> > > > > > > > > --
> > > > > > > > > 2.7.4
> > > > > > > > >
> > > >
>
>

Thanks for answering Jonathan and Stefan.

I would like to contribute with the ad7150 driver. Could you please
give me some hints on what can I do to move this driver
out of staging?

I note that device registration (devm_iio_device_register) is not
the last function called at probe function. Should I change order
to call dev_info before the device registration?

Also did not see any device tree documentation at
Documentation/devicetree/bindings/iio for AD7150 driver.

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Jonathan Cameron]

...
> 
> Thanks for answering Jonathan and Stefan.
> 
> I would like to contribute with the ad7150 driver. Could you please
> give me some hints on what can I do to move this driver
> out of staging?
> 
> I note that device registration (devm_iio_device_register) is not
> the last function called at probe function. Should I change order
> to call dev_info before the device registration?
No on this one. Dev_info is just a message print, so there is nothing
to unwind, hence order doesn't matter. Also in this case it is saying
that the probe really succeeded and as devm_iio_device_register
can fail, it should be after that.  However, there is a pretty strong
argument that the print doesn't add anything and should be dropped
anyway.

> 
> Also did not see any device tree documentation at
> Documentation/devicetree/bindings/iio for AD7150 driver.
Agreed. That needs writing.

As for the driver:

> /*
>  * AD7150 capacitive sensor driver supporting AD7150/1/6
>  *
>  * Copyright 2010-2011 Analog Devices Inc.
>  *
>  * Licensed under the GPL-2 or later.

Convert to SPDX.  Note that it's inconsistent between here and below
though.  See recent clarifying patches from Analog and go with what
they said there.  Make sure you include a reference to that thread
in the patch as legal minefields if it isn't clear what your justification
is.

>  */
> 
> #include <linux/interrupt.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> 
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> #include <linux/iio/events.h>
> /*
>  * AD7150 registers definition
>  */
> 
> #define AD7150_STATUS              0
It's not clear what is a register address and what a field entry here.
There are various ways of doing that. One I like would be

#define AD7150_STATUS_REG            0
#define   AD7150_STATUS_OUT1         BIT(3)
etc.  So use REG to make the register addresses clear
and then use indentation to highlight what is a field name.

> #define AD7150_STATUS_OUT1         BIT(3)
> #define AD7150_STATUS_OUT2         BIT(5)
> #define AD7150_CH1_DATA_HIGH       1
> #define AD7150_CH2_DATA_HIGH       3
> #define AD7150_CH1_AVG_HIGH        5
> #define AD7150_CH2_AVG_HIGH        7
> #define AD7150_CH1_SENSITIVITY     9
> #define AD7150_CH1_THR_HOLD_H      9
> #define AD7150_CH1_TIMEOUT         10
> #define AD7150_CH1_SETUP           11
> #define AD7150_CH2_SENSITIVITY     12
> #define AD7150_CH2_THR_HOLD_H      12
> #define AD7150_CH2_TIMEOUT         13
> #define AD7150_CH2_SETUP           14
> #define AD7150_CFG                 15
> #define AD7150_CFG_FIX             BIT(7)
> #define AD7150_PD_TIMER            16
> #define AD7150_CH1_CAPDAC          17
> #define AD7150_CH2_CAPDAC          18
> #define AD7150_SN3                 19
> #define AD7150_SN2                 20
> #define AD7150_SN1                 21
> #define AD7150_SN0                 22
> #define AD7150_ID                  23
> 
> /**
>  * struct ad7150_chip_info - instance specific chip data
>  * @client: i2c client for this device
>  * @current_event: device always has one type of event enabled.
>  *	This element stores the event code of the current one.
>  * @threshold: thresholds for simple capacitance value events
>  * @thresh_sensitivity: threshold for simple capacitance offset
>  *	from 'average' value.
>  * @mag_sensitity: threshold for magnitude of capacitance offset from
>  *	from 'average' value.
>  * @thresh_timeout: a timeout, in samples from the moment an
>  *	adaptive threshold event occurs to when the average
>  *	value jumps to current value.
>  * @mag_timeout: a timeout, in sample from the moment an
>  *	adaptive magnitude event occurs to when the average
>  *	value jumps to the current value.
>  * @old_state: store state from previous event, allowing confirmation
>  *	of new condition.
>  * @conversion_mode: the current conversion mode.
>  * @state_lock: ensure consistent state of this structure wrt the
>  *	hardware.
>  */
> struct ad7150_chip_info {
> 	struct i2c_client *client;
> 	u64 current_event;
> 	u16 threshold[2][2];
> 	u8 thresh_sensitivity[2][2];
> 	u8 mag_sensitivity[2][2];
> 	u8 thresh_timeout[2][2];
> 	u8 mag_timeout[2][2];
> 	int old_state;
> 	char *conversion_mode;
> 	struct mutex state_lock;
> };
> 
> /*
>  * sysfs nodes

It is both a single line comment and inaccurate.  Have a general
clean up of comments to remove those that don't add anything.

>  */
> 
> static const u8 ad7150_addresses[][6] = {
> 	{ AD7150_CH1_DATA_HIGH, AD7150_CH1_AVG_HIGH,
> 	  AD7150_CH1_SETUP, AD7150_CH1_THR_HOLD_H,
> 	  AD7150_CH1_SENSITIVITY, AD7150_CH1_TIMEOUT },
> 	{ AD7150_CH2_DATA_HIGH, AD7150_CH2_AVG_HIGH,
> 	  AD7150_CH2_SETUP, AD7150_CH2_THR_HOLD_H,
> 	  AD7150_CH2_SENSITIVITY, AD7150_CH2_TIMEOUT },
> };
> 
> static int ad7150_read_raw(struct iio_dev *indio_dev,
> 			   struct iio_chan_spec const *chan,
> 			   int *val,
> 			   int *val2,
> 			   long mask)
> {
> 	int ret;
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	int channel = chan->channel;
> 
> 	switch (mask) {
> 	case IIO_CHAN_INFO_RAW:
> 		ret = i2c_smbus_read_word_data(chip->client,
> 					       ad7150_addresses[channel][0]);
> 		if (ret < 0)
> 			return ret;
> 		*val = swab16(ret);

See i2c_smbus_read_word_swapped

https://elixir.bootlin.com/linux/v5.0-rc7/source/include/linux/i2c.h#L167

I suspect this driver predates us putting that little boiler plate removing
function in place.

> 		return IIO_VAL_INT;
> 	case IIO_CHAN_INFO_AVERAGE_RAW:
> 		ret = i2c_smbus_read_word_data(chip->client,
> 					       ad7150_addresses[channel][1]);

Feels like an enum to allow us to pick those addresses using a name
would be nice.

Also, just set a local variable in the switch statement and do the call outside
it to reduce indentation.

> 		if (ret < 0)
> 			return ret;
> 		*val = swab16(ret);
> 		return IIO_VAL_INT;
> 	default:
> 		return -EINVAL;
> 	}
> }
> 
> static int ad7150_read_event_config(struct iio_dev *indio_dev,
> 				    const struct iio_chan_spec *chan,
> 				    enum iio_event_type type,
> 				    enum iio_event_direction dir)
> {
> 	int ret;
> 	u8 threshtype;
> 	bool adaptive;
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 
> 	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> 	if (ret < 0)
> 		return ret;
> 
> 	threshtype = (ret >> 5) & 0x03;

This could do with appropriate define for the mask and use of FIELD_GET to
do the shift and mask in one go.

> 	adaptive = !!(ret & 0x80);

This used to be a common idiom in kernel. Less popular these days.
Also in this case pointless as I'm not sure we need to force it
to 0 or 1.

+ use a define for that bit.


> 
> 	switch (type) {
> 	case IIO_EV_TYPE_MAG_ADAPTIVE:
> 		if (dir == IIO_EV_DIR_RISING)
> 			return adaptive && (threshtype == 0x1);
> 		return adaptive && (threshtype == 0x0);
Hmm. This isn't all that readable. Is the following equivalent?

Note the threshtype values should probably be an enum.

	switch (type) {
	case IIO_EV_TYPE_MAG_ADAPTIVE:
		if (!adaptive)
			return false;
		switch (dir) {
			case IIO_EV_DIR_RISING:
				return threshtype == 0x01; /* use define */
			case IIO_EV_DIR_FALLING:
				return threshtype == 0x00;
			default:
				return -EINVAL;
		}
	case IIO_EV_TYPE_THRESH_ADAPTIVE:
		if (!adaptive)
			return false;
		switch (dir) {
			case IIO_EV_DIR_RISING:
				return threshtype == 0x03; /* use define */
			case IIO_EV_DIR_FALLING:
				return threshtype == 0x02;
			default:
				return -EINVAL;
		}
	}
etc

> 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> 		if (dir == IIO_EV_DIR_RISING)
> 			return adaptive && (threshtype == 0x3);
> 		return adaptive && (threshtype == 0x2);
> 	case IIO_EV_TYPE_THRESH:
> 		if (dir == IIO_EV_DIR_RISING)
> 			return !adaptive && (threshtype == 0x1);
> 		return !adaptive && (threshtype == 0x0);
> 	default:
> 		break;
> 	}
> 	return -EINVAL;
> }
> 
> /* lock should be held */

Which lock?

> static int ad7150_write_event_params(struct iio_dev *indio_dev,
> 				     unsigned int chan,
> 				     enum iio_event_type type,
> 				     enum iio_event_direction dir)
> {
> 	int ret;
> 	u16 value;
> 	u8 sens, timeout;
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	int rising = (dir == IIO_EV_DIR_RISING);
> 	u64 event_code;
> 
> 	event_code = IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, chan, type, dir);
> 
> 	if (event_code != chip->current_event)
> 		return 0;
> 
> 	switch (type) {
> 		/* Note completely different from the adaptive versions */
> 	case IIO_EV_TYPE_THRESH:
> 		value = chip->threshold[rising][chan];
> 		return i2c_smbus_write_word_data(chip->client,
> 						 ad7150_addresses[chan][3],
> 						 swab16(value));

i2c_smbus_write_word_swapped()

> 	case IIO_EV_TYPE_MAG_ADAPTIVE:
> 		sens = chip->mag_sensitivity[rising][chan];
> 		timeout = chip->mag_timeout[rising][chan];
> 		break;
> 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> 		sens = chip->thresh_sensitivity[rising][chan];
> 		timeout = chip->thresh_timeout[rising][chan];
> 		break;
> 	default:
> 		return -EINVAL;
> 	}
> 	ret = i2c_smbus_write_byte_data(chip->client,
> 					ad7150_addresses[chan][4],

As mentioned above, these indices need an enum so we can easily see
we have the right one.

> 					sens);
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = i2c_smbus_write_byte_data(chip->client,
> 					ad7150_addresses[chan][5],
> 					timeout);
> 	if (ret < 0)
> 		return ret;
> 
> 	return 0;
There is no positive option for return from i2c_smbus_write_byte_data
so just do
return i2c_smbus_write_byte_data(...)

On that note it's worth checking all the other if (ret < 0) to see if
if (ret) is better (as lets you tidy up things like this).

> }
> 
> static int ad7150_write_event_config(struct iio_dev *indio_dev,
> 				     const struct iio_chan_spec *chan,
> 				     enum iio_event_type type,
> 				     enum iio_event_direction dir, int state)
> {
> 	u8 thresh_type, cfg, adaptive;
> 	int ret;
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	int rising = (dir == IIO_EV_DIR_RISING);
> 	u64 event_code;
> 
> 	/* Something must always be turned on */
> 	if (!state)
> 		return -EINVAL;
> 
> 	event_code = IIO_UNMOD_EVENT_CODE(chan->type, chan->channel, type, dir);
> 	if (event_code == chip->current_event)
> 		return 0;
> 	mutex_lock(&chip->state_lock);
> 	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> 	if (ret < 0)
> 		goto error_ret;
> 
> 	cfg = ret & ~((0x03 << 5) | BIT(7));

As before, FIELD_PREP etc and masks to define this stuff in a more readable
fashion.

> 
> 	switch (type) {
> 	case IIO_EV_TYPE_MAG_ADAPTIVE:
> 		adaptive = 1;
> 		if (rising)
> 			thresh_type = 0x1;
> 		else
> 			thresh_type = 0x0;
> 		break;
> 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> 		adaptive = 1;
> 		if (rising)
> 			thresh_type = 0x3;
> 		else
> 			thresh_type = 0x2;
> 		break;
> 	case IIO_EV_TYPE_THRESH:
> 		adaptive = 0;
> 		if (rising)
> 			thresh_type = 0x1;
> 		else
> 			thresh_type = 0x0;
> 		break;
> 	default:
> 		ret = -EINVAL;
> 		goto error_ret;
> 	}
> 
> 	cfg |= (!adaptive << 7) | (thresh_type << 5);
> 
> 	ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
> 	if (ret < 0)
> 		goto error_ret;
> 
> 	chip->current_event = event_code;
> 
> 	/* update control attributes */
> 	ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir);
> error_ret:
> 	mutex_unlock(&chip->state_lock);
> 
> 	return ret;
> }
> 
> static int ad7150_read_event_value(struct iio_dev *indio_dev,
> 				   const struct iio_chan_spec *chan,
> 				   enum iio_event_type type,
> 				   enum iio_event_direction dir,
> 				   enum iio_event_info info,
> 				   int *val, int *val2)
> {
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	int rising = (dir == IIO_EV_DIR_RISING);
> 
> 	/* Complex register sharing going on here */
> 	switch (type) {
> 	case IIO_EV_TYPE_MAG_ADAPTIVE:
> 		*val = chip->mag_sensitivity[rising][chan->channel];
> 		return IIO_VAL_INT;
> 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> 		*val = chip->thresh_sensitivity[rising][chan->channel];
> 		return IIO_VAL_INT;
> 	case IIO_EV_TYPE_THRESH:
> 		*val = chip->threshold[rising][chan->channel];
> 		return IIO_VAL_INT;
> 	default:
> 		return -EINVAL;
> 	}
> }
> 
> static int ad7150_write_event_value(struct iio_dev *indio_dev,
> 				    const struct iio_chan_spec *chan,
> 				    enum iio_event_type type,
> 				    enum iio_event_direction dir,
> 				    enum iio_event_info info,
> 				    int val, int val2)
> {
> 	int ret;
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	int rising = (dir == IIO_EV_DIR_RISING);
> 
> 	mutex_lock(&chip->state_lock);
> 	switch (type) {
> 	case IIO_EV_TYPE_MAG_ADAPTIVE:
> 		chip->mag_sensitivity[rising][chan->channel] = val;
> 		break;
> 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> 		chip->thresh_sensitivity[rising][chan->channel] = val;
> 		break;
> 	case IIO_EV_TYPE_THRESH:
> 		chip->threshold[rising][chan->channel] = val;
> 		break;
> 	default:
> 		ret = -EINVAL;
> 		goto error_ret;
> 	}
> 
> 	/* write back if active */
> 	ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir);
> 
> error_ret:
> 	mutex_unlock(&chip->state_lock);
> 	return ret;
> }
> 
> static ssize_t ad7150_show_timeout(struct device *dev,
> 				   struct device_attribute *attr,
> 				   char *buf)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> 	u8 value;
> 
> 	/* use the event code for consistency reasons */
> 	int chan = IIO_EVENT_CODE_EXTRACT_CHAN(this_attr->address);
> 	int rising = !!(IIO_EVENT_CODE_EXTRACT_DIR(this_attr->address)
> 			== IIO_EV_DIR_RISING);
The code is inconsistent on assuming == triggers a 0/1 or not.
This particular idiom is going away in the kernel (Linus commented he
really didn't like it which was the final straw!).

Ternary operating to make it explict.

> 
> 	switch (IIO_EVENT_CODE_EXTRACT_TYPE(this_attr->address)) {
> 	case IIO_EV_TYPE_MAG_ADAPTIVE:
> 		value = chip->mag_timeout[rising][chan];
> 		break;
> 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> 		value = chip->thresh_timeout[rising][chan];
> 		break;
> 	default:
> 		return -EINVAL;
> 	}
> 
> 	return sprintf(buf, "%d\n", value);
> }
> 
> static ssize_t ad7150_store_timeout(struct device *dev,
> 				    struct device_attribute *attr,
> 				    const char *buf,
> 				    size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> 	int chan = IIO_EVENT_CODE_EXTRACT_CHAN(this_attr->address);
> 	enum iio_event_direction dir;
> 	enum iio_event_type type;
> 	int rising;
> 	u8 data;
> 	int ret;
> 
> 	type = IIO_EVENT_CODE_EXTRACT_TYPE(this_attr->address);
> 	dir = IIO_EVENT_CODE_EXTRACT_DIR(this_attr->address);
> 	rising = (dir == IIO_EV_DIR_RISING);

Might as well do these directly into the declarations above as they
are just magic macro manipulation of input variables.

> 
> 	ret = kstrtou8(buf, 10, &data);
> 	if (ret < 0)
> 		return ret;
> 
> 	mutex_lock(&chip->state_lock);
> 	switch (type) {
> 	case IIO_EV_TYPE_MAG_ADAPTIVE:
> 		chip->mag_timeout[rising][chan] = data;
> 		break;
> 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> 		chip->thresh_timeout[rising][chan] = data;
> 		break;
> 	default:
> 		ret = -EINVAL;
> 		goto error_ret;
> 	}
> 
> 	ret = ad7150_write_event_params(indio_dev, chan, type, dir);
> error_ret:
> 	mutex_unlock(&chip->state_lock);
> 
> 	if (ret < 0)
> 		return ret;
> 
> 	return len;
> }
> 
> #define AD7150_TIMEOUT(chan, type, dir, ev_type, ev_dir)		\
> 	IIO_DEVICE_ATTR(in_capacitance##chan##_##type##_##dir##_timeout, \

What is this? Needs documentation and my suspicion is it may match to
existing standard ABI.  I haven't dived into the datasheet to check.

If it isn't matching anything existing it might be standard enough that we
should just add new generic ABI for it.

> 		0644,							\
> 		&ad7150_show_timeout,					\
> 		&ad7150_store_timeout,					\
> 		IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,			\
> 				     chan,				\
> 				     IIO_EV_TYPE_##ev_type,		\
> 				     IIO_EV_DIR_##ev_dir))
> static AD7150_TIMEOUT(0, mag_adaptive, rising, MAG_ADAPTIVE, RISING);
> static AD7150_TIMEOUT(0, mag_adaptive, falling, MAG_ADAPTIVE, FALLING);
> static AD7150_TIMEOUT(1, mag_adaptive, rising, MAG_ADAPTIVE, RISING);
> static AD7150_TIMEOUT(1, mag_adaptive, falling, MAG_ADAPTIVE, FALLING);
> static AD7150_TIMEOUT(0, thresh_adaptive, rising, THRESH_ADAPTIVE, RISING);
> static AD7150_TIMEOUT(0, thresh_adaptive, falling, THRESH_ADAPTIVE, FALLING);
> static AD7150_TIMEOUT(1, thresh_adaptive, rising, THRESH_ADAPTIVE, RISING);
> static AD7150_TIMEOUT(1, thresh_adaptive, falling, THRESH_ADAPTIVE, FALLING);
> 
> static const struct iio_event_spec ad7150_events[] = {
> 	{
> 		.type = IIO_EV_TYPE_THRESH,
> 		.dir = IIO_EV_DIR_RISING,
> 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> 			BIT(IIO_EV_INFO_ENABLE),
> 	}, {
> 		.type = IIO_EV_TYPE_THRESH,
> 		.dir = IIO_EV_DIR_FALLING,
> 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> 			BIT(IIO_EV_INFO_ENABLE),
> 	}, {
> 		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
> 		.dir = IIO_EV_DIR_RISING,
> 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> 			BIT(IIO_EV_INFO_ENABLE),
> 	}, {
> 		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
> 		.dir = IIO_EV_DIR_FALLING,
> 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> 			BIT(IIO_EV_INFO_ENABLE),
> 	}, {
> 		.type = IIO_EV_TYPE_MAG_ADAPTIVE,
> 		.dir = IIO_EV_DIR_RISING,
> 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> 			BIT(IIO_EV_INFO_ENABLE),
> 	}, {
> 		.type = IIO_EV_TYPE_MAG_ADAPTIVE,
> 		.dir = IIO_EV_DIR_FALLING,
> 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> 			BIT(IIO_EV_INFO_ENABLE),
> 	},
> };
> 
> static const struct iio_chan_spec ad7150_channels[] = {
> 	{
> 		.type = IIO_CAPACITANCE,
> 		.indexed = 1,
> 		.channel = 0,
> 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 		BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> 		.event_spec = ad7150_events,
> 		.num_event_specs = ARRAY_SIZE(ad7150_events),

Given two near identical entries, probably worth a macro.

> 	}, {
> 		.type = IIO_CAPACITANCE,
> 		.indexed = 1,
> 		.channel = 1,
> 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 		BIT(IIO_CHAN_INFO_AVERAGE_RAW),
> 		.event_spec = ad7150_events,
> 		.num_event_specs = ARRAY_SIZE(ad7150_events),
> 	},
> };
> 
> /*
>  * threshold events

There have been lots of threshold related functions already so I would
argue this is wrong to have here.  Actually I'd just drop it, these
sort of generic structure comments don't actually add anything.

>  */
> 
> static irqreturn_t ad7150_event_handler(int irq, void *private)
> {
> 	struct iio_dev *indio_dev = private;
> 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
> 	u8 int_status;
> 	s64 timestamp = iio_get_time_ns(indio_dev);
> 	int ret;
> 
> 	ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
> 	if (ret < 0)
> 		return IRQ_HANDLED;
> 
> 	int_status = ret;
> 
> 	if ((int_status & AD7150_STATUS_OUT1) &&
> 	    !(chip->old_state & AD7150_STATUS_OUT1))
> 		iio_push_event(indio_dev,
> 			       IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
> 						    0,
> 						    IIO_EV_TYPE_THRESH,
> 						    IIO_EV_DIR_RISING),
> 				timestamp);
> 	else if ((!(int_status & AD7150_STATUS_OUT1)) &&
> 		 (chip->old_state & AD7150_STATUS_OUT1))
> 		iio_push_event(indio_dev,
> 			       IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
> 						    0,
> 						    IIO_EV_TYPE_THRESH,
> 						    IIO_EV_DIR_FALLING),
> 			       timestamp);
> 
> 	if ((int_status & AD7150_STATUS_OUT2) &&
> 	    !(chip->old_state & AD7150_STATUS_OUT2))
> 		iio_push_event(indio_dev,
> 			       IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
> 						    1,
> 						    IIO_EV_TYPE_THRESH,
> 						    IIO_EV_DIR_RISING),
> 			       timestamp);
> 	else if ((!(int_status & AD7150_STATUS_OUT2)) &&
> 		 (chip->old_state & AD7150_STATUS_OUT2))
> 		iio_push_event(indio_dev,
> 			       IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE,
> 						    1,
> 						    IIO_EV_TYPE_THRESH,
> 						    IIO_EV_DIR_FALLING),
> 			       timestamp);
> 	/* store the status to avoid repushing same events */
> 	chip->old_state = int_status;
> 
> 	return IRQ_HANDLED;
> }
> 
> /* Timeouts not currently handled by core */
> static struct attribute *ad7150_event_attributes[] = {
> 	&iio_dev_attr_in_capacitance0_mag_adaptive_rising_timeout
> 	.dev_attr.attr,
> 	&iio_dev_attr_in_capacitance0_mag_adaptive_falling_timeout
> 	.dev_attr.attr,
> 	&iio_dev_attr_in_capacitance1_mag_adaptive_rising_timeout
> 	.dev_attr.attr,
> 	&iio_dev_attr_in_capacitance1_mag_adaptive_falling_timeout
> 	.dev_attr.attr,
> 	&iio_dev_attr_in_capacitance0_thresh_adaptive_rising_timeout
> 	.dev_attr.attr,
> 	&iio_dev_attr_in_capacitance0_thresh_adaptive_falling_timeout
> 	.dev_attr.attr,
> 	&iio_dev_attr_in_capacitance1_thresh_adaptive_rising_timeout
> 	.dev_attr.attr,
> 	&iio_dev_attr_in_capacitance1_thresh_adaptive_falling_timeout
> 	.dev_attr.attr,
> 	NULL,

Definitely nice to see this as standard ABI.

> };
> 
> static const struct attribute_group ad7150_event_attribute_group = {
> 	.attrs = ad7150_event_attributes,
> 	.name = "events",
> };
> 
> static const struct iio_info ad7150_info = {
> 	.event_attrs = &ad7150_event_attribute_group,
> 	.read_raw = &ad7150_read_raw,
> 	.read_event_config = &ad7150_read_event_config,
> 	.write_event_config = &ad7150_write_event_config,
> 	.read_event_value = &ad7150_read_event_value,
> 	.write_event_value = &ad7150_write_event_value,
> };
> 
> /*
>  * device probe and remove

Comment adds nothing. Drop it.

>  */
> 
> static int ad7150_probe(struct i2c_client *client,
> 			const struct i2c_device_id *id)
> {
> 	int ret;
> 	struct ad7150_chip_info *chip;
> 	struct iio_dev *indio_dev;
> 
> 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> 	if (!indio_dev)
> 		return -ENOMEM;
> 	chip = iio_priv(indio_dev);
> 	mutex_init(&chip->state_lock);
> 	/* this is only used for device removal purposes */
> 	i2c_set_clientdata(client, indio_dev);
> 
> 	chip->client = client;
> 
> 	indio_dev->name = id->name;
> 	indio_dev->channels = ad7150_channels;
> 	indio_dev->num_channels = ARRAY_SIZE(ad7150_channels);
> 	/* Establish that the iio_dev is a child of the i2c device */
> 	indio_dev->dev.parent = &client->dev;
> 
> 	indio_dev->info = &ad7150_info;
> 
> 	indio_dev->modes = INDIO_DIRECT_MODE;
> 
> 	if (client->irq) {
> 		ret = devm_request_threaded_irq(&client->dev, client->irq,
> 						NULL,
> 						&ad7150_event_handler,
> 						IRQF_TRIGGER_RISING |
> 						IRQF_TRIGGER_FALLING |
> 						IRQF_ONESHOT,
> 						"ad7150_irq1",
> 						indio_dev);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	if (client->dev.platform_data) {

I would suggest dropping platform data unless we have any known users.
Better to move out of staging as DT only driver.  Get both interrupts
directly from DT, possibly including the names to allow either one
to be specified without he other.

Is there actually point in having both as it currently stands?

> 		ret = devm_request_threaded_irq(&client->dev, *(unsigned int *)
> 						client->dev.platform_data,
> 						NULL,
> 						&ad7150_event_handler,
> 						IRQF_TRIGGER_RISING |
> 						IRQF_TRIGGER_FALLING |
> 						IRQF_ONESHOT,
> 						"ad7150_irq2",
> 						indio_dev);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> 	if (ret)
> 		return ret;
> 
> 	dev_info(&client->dev, "%s capacitive sensor registered,irq: %d\n",
> 		 id->name, client->irq);
> 
> 	return 0;
> }
> 
> static const struct i2c_device_id ad7150_id[] = {
> 	{ "ad7150", 0 },
> 	{ "ad7151", 0 },
> 	{ "ad7156", 0 },
> 	{}
> };

Add an of_device_id table.

> 
> MODULE_DEVICE_TABLE(i2c, ad7150_id);
> 
> static struct i2c_driver ad7150_driver = {
> 	.driver = {
> 		.name = "ad7150",
> 	},
> 	.probe = ad7150_probe,
> 	.id_table = ad7150_id,
> };
> module_i2c_driver(ad7150_driver);
> 
> MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> MODULE_DESCRIPTION("Analog Devices AD7150/1/6 capacitive sensor driver");
> MODULE_LICENSE("GPL v2");

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[valdis.kletnieks]

On Fri, 25 Jan 2019 22:14:32 -0200, Rodrigo Ribeiro said:
> Maybe, one checkstyle patch is enough, right? Which drivers can I truly
> contribute to?

I'll give you a pointer to the "How to contribute" the Kernel Newbies list and IRC
channel uses:

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.html

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Jonathan Cameron]

On Fri, 25 Jan 2019 10:19:54 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> >
> > Remove the checkpatch.pl check:
> >
> > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?  
> 
> Hey,
> 
> A bit curios about this one.
> Are you using this chip/driver ?
> 
> Thing is: the part is nearing EOL, and it could be an idea to be
> marked for removal (since it's still in staging).
> But if there are users for this driver, it could be left around for a while.

While it might be going away soon, I'm also not that bothered about
the small amount of churn that a tidy up patch like this causes,
and it might not go away ;)

However it is rather odd to have a 'reserved' entry for a register.
can't see that providing any useful information.  Normally I'm
happy to have complete register sets as a form of documentation
if the author wants to do it that way.  This however seems silly.

Alex, we haven't really gone with marking things as 'going away'
before.  I'd suggest we take a guess and remove it if you and the
team an analog don't think it's in use.  Happy to get a patch for
that if you want to send one.  Of course, Rodrigo could do that
patch to get started if that works for everyone? :)

Jonathan
> 
> Thanks
> Alex
> 
> >
> > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > ---
> > This macro is not used anywhere. Should we just correct the
> > spelling or remove the macro?
> >
> >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> > index 25f51db..c9da6d4 100644
> > --- a/drivers/staging/iio/cdc/ad7152.c
> > +++ b/drivers/staging/iio/cdc/ad7152.c
> > @@ -35,7 +35,7 @@
> >  #define AD7152_REG_CH2_GAIN_HIGH       12
> >  #define AD7152_REG_CH2_SETUP           14
> >  #define AD7152_REG_CFG                 15
> > -#define AD7152_REG_RESEVERD            16
> > +#define AD7152_REG_RESERVED            16
> >  #define AD7152_REG_CAPDAC_POS          17
> >  #define AD7152_REG_CAPDAC_NEG          18
> >  #define AD7152_REG_CFG2                        26
> > --
> > 2.7.4
> >  

Re: [PATCH] staging:iio:ad7152: Rename misspelled RESEVERD -> RESERVED

[Alexandru Ardelean]

On Sat, Jan 26, 2019 at 8:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 25 Jan 2019 10:19:54 +0200
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Thu, Jan 24, 2019 at 9:35 PM Rodrigo Ribeiro <rodrigorsdc@gmail.com> wrote:
> > >
> > > Remove the checkpatch.pl check:
> > >
> > > CHECK: 'RESEVERD' may be misspelled - perhaps 'RESERVED'?
> >
> > Hey,
> >
> > A bit curios about this one.
> > Are you using this chip/driver ?
> >
> > Thing is: the part is nearing EOL, and it could be an idea to be
> > marked for removal (since it's still in staging).
> > But if there are users for this driver, it could be left around for a while.
>
> While it might be going away soon, I'm also not that bothered about
> the small amount of churn that a tidy up patch like this causes,
> and it might not go away ;)
>
> However it is rather odd to have a 'reserved' entry for a register.
> can't see that providing any useful information.  Normally I'm
> happy to have complete register sets as a form of documentation
> if the author wants to do it that way.  This however seems silly.
>
> Alex, we haven't really gone with marking things as 'going away'
> before.  I'd suggest we take a guess and remove it if you and the
> team an analog don't think it's in use.  Happy to get a patch for
> that if you want to send one.  Of course, Rodrigo could do that
> patch to get started if that works for everyone? :)
>

We'll also start a discussion about this particular driver internally.
And maybe a procedure for removing drivers that become obsolete [or
come close to it].
We also don't/didn't have one for removing "going away" drivers; I
just remembered that we took a look over this one and decided not to
invest time into it as it's close to being obsolete.

Thanks
Alex

> Jonathan
> >
> > Thanks
> > Alex
> >
> > >
> > > Signed-off-by: Rodrigo Ribeiro <rodrigorsdc@gmail.com>
> > > Signed-off-by: Rafael Tsuha <rafael.tsuha@usp.br>
> > > ---
> > > This macro is not used anywhere. Should we just correct the
> > > spelling or remove the macro?
> > >
> > >  drivers/staging/iio/cdc/ad7152.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7152.c b/drivers/staging/iio/cdc/ad7152.c
> > > index 25f51db..c9da6d4 100644
> > > --- a/drivers/staging/iio/cdc/ad7152.c
> > > +++ b/drivers/staging/iio/cdc/ad7152.c
> > > @@ -35,7 +35,7 @@
> > >  #define AD7152_REG_CH2_GAIN_HIGH       12
> > >  #define AD7152_REG_CH2_SETUP           14
> > >  #define AD7152_REG_CFG                 15
> > > -#define AD7152_REG_RESEVERD            16
> > > +#define AD7152_REG_RESERVED            16
> > >  #define AD7152_REG_CAPDAC_POS          17
> > >  #define AD7152_REG_CAPDAC_NEG          18
> > >  #define AD7152_REG_CFG2                        26
> > > --
> > > 2.7.4
> > >
>