Re: [PATCH 0/2] Cleanup the MAINTAINER's file

[Andi Shyti]

Hi Wolfram,

On Fri, Jul 12, 2024 at 08:34:11AM GMT, Wolfram Sang wrote:
> On Fri, Jul 12, 2024 at 01:19:24AM +0200, Andi Shyti wrote:
> > Hi,
> > 
> > while reviewing Wolfram's series, I received some delivery
> > failure notifications for e-mails that don't exist anymore.
> > 
> > With this series I'm removing:
> > 
> >  - Conghui Chen 
> >  - Thor Thayer 
> 
> Fixes for these two are already in my for-current branch. (And the
> patches were on the i2c list, Andi ;))

oh yes... sorry, completely forgot. Please, ignore, then :-)

Andi

[PATCH 0/2] Cleanup the MAINTAINER's file

[Andi Shyti]

Hi,

while reviewing Wolfram's series, I received some delivery
failure notifications for e-mails that don't exist anymore.

With this series I'm removing:

 - Conghui Chen 
 - Thor Thayer 

unfortunately both from Intel :-(

In the case of Altera's subsystems (except for the i2c), I didn't
really know what to do with them, so that I marked them as
Orphan.

Andi

Cc: Andy Shevchenko 
Cc: Krzysztof Kozlowski 
Cc: Lee Jones 
Cc: Linus Walleij 
Cc: Philipp Zabel 
Cc: Viresh Kumar 
Cc: Wolfram Sang 
Cc: linux-g...@vger.kernel.org
Cc: linux-...@vger.kernel.org
Cc: virtualizat...@lists.linux.dev

Andi Shyti (2):
  MAINTAINERS: i2c-virtio: Drop Conghui Chen from Maintainers
  MAINTAINERS: Drop Thor Thayer from maintainers

 MAINTAINERS | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

-- 
2.45.2

[PATCH 1/2] MAINTAINERS: i2c-virtio: Drop Conghui Chen from Maintainers

[Andi Shyti]

E-mails to Conghui Chen have bounced back:

  : host mgamail.eglb.intel.com[198.175.65.14] said: 550
  #5.1.0 Address rejected. (in reply to RCPT TO command)

Remove him as maintainer of the i2c Virtio driver in the
MAINTAINERS file.

Signed-off-by: Andi Shyti 
Cc: Viresh Kumar 
Cc: Wolfram Sang 
Cc: linux-...@vger.kernel.org
Cc: virtualizat...@lists.linux.dev
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3e26555e52bfd..96745f7971100 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23859,7 +23859,6 @@ S:  Maintained
 F: drivers/vhost/scsi.c
 
 VIRTIO I2C DRIVER
-M: Conghui Chen 
 M: Viresh Kumar 
 L: linux-...@vger.kernel.org
 L: virtualizat...@lists.linux.dev
-- 
2.45.2

Re: [PATCH v2 00/60] i2c: reword first drivers according to newest specification

[Andi Shyti]

Hi Wolfram,

pushed in i2c/i2c-host.

Thanks for this big work, at the end it turned out quite nice and
I'm happy of the outcome!

Thanks
Andi

On Sat, Jul 06, 2024 at 01:20:00PM GMT, Wolfram Sang wrote:
> Start changing the wording of the I2C main header wrt. the newest I2C
> v7 and SMBus 3.2 specifications and replace "master/slave" with more
> appropriate terms. This first step renames the members of struct
> i2c_algorithm. Once all in-tree users are converted, the anonymous union
> will go away again. All this work will also pave the way for finally
> seperating the monolithic header into more fine-grained headers like
> "i2c/clients.h" etc. So, this is not a simple renaming-excercise but
> also a chance to update the I2C core to recent Linux standards.
> 
> Changes since v1:
> 
> * changed wording according to the terminology we agreed on and defined
>   upstream. That means consistent use of "controller/target", and no
>   more "host/client". I added "local/remote target" where necessary.
> * added tags which I kept despite some changes in wording. The approach
>   and code changes (if necessary) did not change.
> * rebased to Andi's for-next branch
> * this series only contains patches which convert the drivers fully. If
>   all goes well, no more updates for them are needed. The previous
>   series converted all users of "master_xfer". But to avoid tons of
>   incremental patches to one driver, I will incrementally improve i2c.h
>   and see which drivers can be fully converted step-by-step.
> * do not mention I3C specs in commit messages, not really relevant here
> 
> Please note that I am not super strict with the 80 char limit. And, as
> agreed, I did not convert occasions where old terminology is used in
> register names or bits etc. or in function names outside of the I2C
> realm.
> 
> The outcome is that before this series 115 drivers use old terminology,
> after this only 54. Hooray.
> 
> And a comment to all janitors: Do not convert I2C drivers outside of
> drivers/i2c yet. Let us first gain experience here and present the
> well-tested results of what we figured out to other maintainers then.
> This ensures they have to deal with way less patch revisions.
> 
> Thanks and happy hacking!
> 
> 
> Wolfram Sang (60):
>   i2c: reword i2c_algorithm according to newest specification
>   i2c: ali15x3: reword according to newest specification
>   i2c: altera: reword according to newest specification
>   i2c: au1550: reword according to newest specification
>   i2c: bcm-kona: reword according to newest specification
>   i2c: bcm2835: reword according to newest specification
>   i2c: brcmstb: reword according to newest specification
>   i2c: cht-wc: reword according to newest specification
>   i2c: cp2615: reword according to newest specification
>   i2c: cros-ec-tunnel: reword according to newest specification
>   i2c: davinci: reword according to newest specification
>   i2c: digicolor: reword according to newest specification
>   i2c: diolan-u2c: reword according to newest specification
>   i2c: dln2: reword according to newest specification
>   i2c: fsi: reword according to newest specification
>   i2c: gpio: reword according to newest specification
>   i2c: highlander: reword according to newest specification
>   i2c: hisi: reword according to newest specification
>   i2c: hix5hd2: reword according to newest specification
>   i2c: i801: reword according to newest specification
>   i2c: ibm_iic: reword according to newest specification
>   i2c: iop3xx: reword according to newest specification
>   i2c: isch: reword according to newest specification
>   i2c: jz4780: reword according to newest specification
>   i2c: kempld: reword according to newest specification
>   i2c: ljca: reword according to newest specification
>   i2c: lpc2k: reword according to newest specification
>   i2c: ls2x: reword according to newest specification
>   i2c: mlxcpld: reword according to newest specification
>   i2c: mpc: reword according to newest specification
>   i2c: mt7621: reword according to newest specification
>   i2c: mv64xxx: reword according to newest specification
>   i2c: ocores: reword according to newest specification
>   i2c: octeon: reword according to newest specification
>   i2c: opal: reword according to newest specification
>   i2c: owl: reword according to newest specification
>   i2c: pasemi: reword according to newest specification
>   i2c: piix4: reword according to newest specification
>   i2c: powermac: reword according to newest specification
>   i2c: pxa-pci: reword according to newest specification
>   i2c: riic: reword according to newest specification
>   i2c: rk3x: reword according to newest specification
>   i2c: robotfuzz-osif: reword according to newest specification
>   i2c: rzv2m: reword according to newest specification
>   i2c: sis5595: reword according to newest specification
>   i2c: sprd: reword according to newest specification
>   i2c: stm32f4: reword according to newest 

Re: [PATCH v2 58/60] i2c: virtio: reword according to newest specification

[Andi Shyti]

Hi Wolfram,

On Sat, Jul 06, 2024 at 01:20:58PM GMT, Wolfram Sang wrote:
> Change the wording of this driver wrt. the newest I2C v7 and SMBus 3.2
> specifications and replace "master/slave" with more appropriate terms.
> 
> Signed-off-by: Wolfram Sang 

Reviewed-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH 00/64] i2c: reword i2c_algorithm according to newest specification

[Andi Shyti]

Hi Wolfram,

> > @Andi: are you okay with this approach? It means you'd need to merge
> > -rc2 into your for-next branch. Or rebase if all fails.
> 
> I think it's a good plan, I'll try to support you with it.

Do you feel more comfortable if I take the patches as soon as
they are reviewd?

So far I have tagged patch 1-4 and I can already merge 2,3,4 as
long as you merge patch 1.

Andi

Re: [PATCH 00/64] i2c: reword i2c_algorithm according to newest specification

[Andi Shyti]

Hi Wolfram,

On Fri, Mar 22, 2024 at 02:24:53PM +0100, Wolfram Sang wrote:
> Okay, we need to begin somewhere...
> 
> Start changing the wording of the I2C main header wrt. the newest I2C
> v7, SMBus 3.2, I3C specifications and replace "master/slave" with more
> appropriate terms. This first step renames the members of struct
> i2c_algorithm. Once all in-tree users are converted, the anonymous union
> will go away again. All this work will also pave the way for finally
> seperating the monolithic header into more fine-grained headers like
> "i2c/clients.h" etc. So, this is not a simple renaming-excercise but
> also a chance to update the I2C core to recent Linux standards.

yes, very good! It's clearly stated in all three documentations
that Target replaces Slave and Controller replaces Master (i3c is
at the 1.1.1 version).

> My motivation is to improve the I2C core API, in general. My motivation
> is not to clean each and every driver. I think this is impossible
> because register names based on official documentation will need to stay
> as they are. But the Linux-internal names should be updated IMO.

Also because some drivers have been written based on previous
specifications where master/slave was used.

> That being said, I worked on 62 drivers in this series beyond plain
> renames inside 'struct i2c_algorithm' because the fruits were so
> low-hanging. Before this series, 112 files in the 'busses/' directory
> contained 'master' and/or 'slave'. After the series, only 57. Why not?
> 
> Next step is updating the drivers outside the 'i2c'-folder regarding
> 'struct i2c_algorithm' so we can remove the anonymous union ASAP. To be
> able to work on this with minimal dependencies, I'd like to apply this
> series between -rc1 and -rc2.
> 
> I hope this will work for you guys. The changes are really minimal. If
> you are not comfortable with changes to your driver or need more time to
> review, please NACK the patch and I will drop the patch and/or address
> the issues separeately.
> 
> @Andi: are you okay with this approach? It means you'd need to merge
> -rc2 into your for-next branch. Or rebase if all fails.

I think it's a good plan, I'll try to support you with it.

> Speaking of Andi, thanks a lot to him taking care of the controller
> drivers these days. His work really gives me the freedom to work on I2C
> core issues again.

Thank you, Wolfram!

Andi

Re: [PATCH v3 1/5] arch,locking/atomic: arc: arch_cmpxchg should check data size

[Andi Shyti]

Hi Wuqiang,

On Tue, Nov 21, 2023 at 10:23:43PM +0800, wuqiang.matt wrote:
> arch_cmpxchg() should check data size rather than pointer size in case
> CONFIG_ARC_HAS_LLSC is defined. So rename __cmpxchg to __cmpxchg_32 to
> emphasize it's explicit support of 32bit data size with BUILD_BUG_ON()
> added to avoid any possible misuses with unsupported data types.
> 
> In case CONFIG_ARC_HAS_LLSC is undefined, arch_cmpxchg() uses spinlock
> to accomplish SMP-safety, so the BUILD_BUG_ON checking is uncecessary.
> 
> v2 -> v3:
>   - Patches regrouped and has the improvement for xtensa included
>   - Comments refined to address why these changes are needed
> 
> v1 -> v2:
>   - Try using native cmpxchg variants if avaialble, as Arnd advised
> 
> Signed-off-by: wuqiang.matt 
> Reviewed-by: Masami Hiramatsu (Google) 
> ---
>  arch/arc/include/asm/cmpxchg.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arc/include/asm/cmpxchg.h b/arch/arc/include/asm/cmpxchg.h
> index e138fde067de..bf46514f6f12 100644
> --- a/arch/arc/include/asm/cmpxchg.h
> +++ b/arch/arc/include/asm/cmpxchg.h
> @@ -18,14 +18,16 @@
>   * if (*ptr == @old)
>   *  *ptr = @new
>   */
> -#define __cmpxchg(ptr, old, new) \
> +#define __cmpxchg_32(ptr, old, new)  \
>  ({   \
>   __typeof__(*(ptr)) _prev;   \
>   \
> + BUILD_BUG_ON(sizeof(*(ptr)) != 4);  \
> + \
>   __asm__ __volatile__(   \
> - "1: llock  %0, [%1] \n" \
> + "1: llock  %0, [%1] \n" \
>   "   brne   %0, %2, 2f   \n" \
> - "   scond  %3, [%1] \n" \
> + "   scond  %3, [%1] \n" \
>   "   bnz 1b  \n" \
>   "2: \n" \
>   : "="(_prev)  /* Early clobber prevent reg reuse */   \
> @@ -47,7 +49,7 @@
>   \
>   switch(sizeof((_p_))) { \
>   case 4: \
> - _prev_ = __cmpxchg(_p_, _o_, _n_);  \
> + _prev_ = __cmpxchg_32(_p_, _o_, _n_);   \
>   break;  \
>   default:\
>   BUILD_BUG();\
> @@ -65,8 +67,6 @@
>   __typeof__(*(ptr)) _prev_;  \
>   unsigned long __flags;  \
>   \
> - BUILD_BUG_ON(sizeof(_p_) != 4); \
> - \

I think I made some comments here that have not been addressed or
replied.

Thanks,
Andi

>   /*  \
>* spin lock/unlock provide the needed smp_mb() before/after\
>*/ \
> -- 
> 2.40.1

Re: [PATCH v2] input: s6sy761: fix coordinate read bit shift

[Andi Shyti]

Hi Caleb,

On Fri, Mar 05, 2021 at 06:58:10PM +, Caleb Connolly wrote:
> The touch coordinate register contains the following:
> 
> byte 3 byte 2 byte 1
> +++ +-+ +-+
> ||| | | | |
> | X[3:0] | Y[3:0] | | Y[11:4] | | X[11:4] |
> ||| | | | |
> +++ +-+ +-+
> 
> Bytes 2 and 1 need to be shifted left by 4 bits, the least significant
> nibble of each is stored in byte 3. Currently they are only
> being shifted by 3 causing the reported coordinates to be incorrect.
> 
> This matches downstream examples, and has been confirmed on my
> device (OnePlus 7 Pro).
> 
> Fixes: 0145a7141e59 ("Input: add support for the Samsung S6SY761
> touchscreen")
> Signed-off-by: Caleb Connolly 

Reviewed-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH] input: s6sy761: fix coordinate read bit shift

[Andi Shyti]

Hi Caleb,

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256

Please clean up the commit message.

> The touch coordinates are read by shifting a value left by 3,
> this is incorrect and effectively causes the coordinates to
> be half of the correct value.
> 
> Shift by 4 bits instead to report the correct value.
> 
> This matches downstream examples, and has been confirmed on my
> device (OnePlus 7 Pro).

The real reason is that from the register we get:

   byte 3 byte 2 byte 1
+++ +-+ +-+
||| | | | |
| X[3:0] | Y[3:0] | | Y[11:4] | | X[11:4] |
||| | | | |
+++ +-+ +-+

and the 12 bit values have to fit in a 16bit variable.

The upper 8 bits (in event[2] and event[1] need to be shifted
left by '4' and not by '3' in order to leave space to the lower
4 bits (in event[3]).

> 
> Signed-off-by: Caleb Connolly 
> ---
>  drivers/input/touchscreen/s6sy761.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/s6sy761.c 
> b/drivers/input/touchscreen/s6sy761.c
> index b63d7fdf0cd2..85a1f465c097 100644
> --- a/drivers/input/touchscreen/s6sy761.c
> +++ b/drivers/input/touchscreen/s6sy761.c
> @@ -145,8 +145,8 @@ static void s6sy761_report_coordinates(struct 
> s6sy761_data *sdata,
>   u8 major = event[4];
>   u8 minor = event[5];
>   u8 z = event[6] & S6SY761_MASK_Z;
> - u16 x = (event[1] << 3) | ((event[3] & S6SY761_MASK_X) >> 4);
> - u16 y = (event[2] << 3) | (event[3] & S6SY761_MASK_Y);
> + u16 x = (event[1] << 4) | ((event[3] & S6SY761_MASK_X) >> 4);
> + u16 y = (event[2] << 4) | (event[3] & S6SY761_MASK_Y);

the devil knows how that '3' has ended up there :)

Thanks for catching it!

Reviewed-by: Andi Shyti 

Andi

Re: [PATCH -next] Input: stmfts - Fix a & vs && typo

[Andi Shyti]

Hi YueHaibing,

On Wed, Sep 16, 2020 at 10:19:41PM +0800, YueHaibing wrote:
> In stmfts_sysfs_hover_enable_write(), we should check
> value and sdata->hover_enabled is all true.
> 
> Fixes: 78bcac7b2ae1 ("Input: add support for the STMicroelectronics FingerTip 
> touchscreen")
> Signed-off-by: YueHaibing 

Reviewed-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH] spi: s3c24xx: correct kerneldoc comment

[Andi Shyti]

Hi Krzysztof,

On Tue, Aug 04, 2020 at 05:13:56PM +0200, Krzysztof Kozlowski wrote:
> Correct the kerneldoc for structure to fix W=1 compile warning:
> 
> drivers/spi/spi-s3c24xx.c:36: warning: cannot understand function 
> prototype: 'struct s3c24xx_spi_devstate '
> 
> Signed-off-by: Krzysztof Kozlowski 

thanks,

Acked-by: Andi Shyti 

Andi

Re: [Intel-gfx] [PATCH] drm/i915: Fix wrong return value

[Andi Shyti]

> > > diff --git a/drivers/gpu/drm/i915/i915_active.c 
> > > b/drivers/gpu/drm/i915/i915_active.c
> > > index d960d0be5bd2..cc017e3cc9c5 100644
> > > --- a/drivers/gpu/drm/i915/i915_active.c
> > > +++ b/drivers/gpu/drm/i915/i915_active.c
> > > @@ -758,7 +758,7 @@ int i915_active_acquire_preallocate_barrier(struct 
> > > i915_active *ref,
> > >   intel_engine_mask_t tmp, mask = engine->mask;
> > >   struct llist_node *first = NULL, *last = NULL;
> > >   struct intel_gt *gt = engine->gt;
> > > - int err;
> > > + int err = 0;
> > 
> > you don't need the initialization here.
> 
> But it's close enough that I can munge the patch inline.
> Reviewed-by: Chris Wilson 

sure... you can also remove it before merging it and it might
also need:

Fixes: d8af05ff38ae7 ("drm/i915: Allow sharing the idle-barrier from other 
kernel requests)

but, yeah...

Reviewed-by: Andi Shyti 

... as well :)

Andi

Re: [PATCH] drm/i915: Fix wrong return value

[Andi Shyti]

Hi Tianjia,

> diff --git a/drivers/gpu/drm/i915/i915_active.c 
> b/drivers/gpu/drm/i915/i915_active.c
> index d960d0be5bd2..cc017e3cc9c5 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -758,7 +758,7 @@ int i915_active_acquire_preallocate_barrier(struct 
> i915_active *ref,
>   intel_engine_mask_t tmp, mask = engine->mask;
>   struct llist_node *first = NULL, *last = NULL;
>   struct intel_gt *gt = engine->gt;
> - int err;
> + int err = 0;

you don't need the initialization here.

Thanks,
Andi

Re: [PATCH v2] HID: i2c-hid: Use block reads when possible to save power

[Andi Shyti]

Hi Sultan,

> > > > so the only strategy available up until now has been to always retrieve
> > > > the maximum possible report length over i2c, which can be quite
> > > > inefficient. For devices that send reports in block read format, the i2c
> > > > controller driver can read the payload length on the fly and terminate
> > > > the i2c transaction early, resulting in considerable power savings.
> > > > 
> > > > On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the
> > > > touchpad causes psys power readings to go up by about 4W and hover there
> > > > until I remove my finger. With this patch, my psys readings go from 4.7W
> > > > down to 3.1W, yielding about 1.6W in savings. This is because my
> > > > touchpad's max report length is 60 bytes, but all of the regular reports
> > > > it sends for touch events are only 32 bytes, so the i2c transfer is
> > > > roughly halved for the common case.
> > > 
> > > > +   /* Try to do a block read if the size fits in one byte */
> > > > +   flags = size > 255 ? I2C_M_RD : I2C_M_RD | I2C_M_RECV_LEN;
> > > 
> > > AFAIR SMBus specification tells about 256. Why 255?
> > > 
> > > Andi, am I correct?
> > 
> > Actually the SMBUS 3.0 protocol from 2015[*] says 255:
> > 
> > "
> > D.6 255 Bytes in Process Call
> > 
> > The maximum number of bytes allowed in the Block Write-Block Read
> > Process Call (Section 6.5.8) was increased from 32 to 255.
> > "
> > 
> > But why does it matter... I see the patch is detatching itself
> > from smbus.
> > 
> > And, actually, I wonder if this is the right way to fix it, isn't
> > it better to fix smbus instead?
> 
> I think the best solution would be to modify the i2c api to allow passing in a
> function pointer and a payload size length, to specify how to interpret the 
> size
> of the incoming payload, so the adapter could handle both the HID over i2c
> transfer spec and SMBus block reads without needing to read more bytes than
> needed.

Can't you do that by specifying the xfer function?

When you use smbus_read/write in block or byte or whatever, smbus
always checks if there is an xfer function specified and uses
that.

If it's not specified it uses the default smbus functions with
the limitations that come with it.

> For example, for an SMBus block read, the payload size is specified in the 
> first
> byte and it is limited to 32 bytes. However, for HID over i2c, the payload 
> size
> is specified in the first two bytes, and there are also some device quirks
> involved to reinterpret the reported size.

which is wrong. The 32 bytes limitation is outdated: in the link
that I gave before (i.e.  this one [*]), the new SMBUS specifies
255 maximum for read/write block.

> A nice solution would be to pass in how many bytes the i2c payload size can
> contain, as well as a function pointer to evaluate the reported payload size 
> in
> a way that the caller wants. This would require modifying every i2c adapter
> driver to add this functionality, but it would fix the efficiency problem 
> faced
> by i2c-hid and perhaps others.
> 
> > I have a patch ready that fixes the smbus transfer size, perhaps
> > I should rebase, test and send it.
> 
> For the i2c-hid driver?

No, sorry, for smbus.

Now... here you are replacing "i2c_master_recv" with
"i2c_transfer_buffer_flags". I do not really like this change,
although I understand it's necessary, because we are bypassing
the real issue that is that the smbus implementation is outdated.

I have a patch for that that for a matter of time I never sent.

Andi

Re: [PATCH v2] HID: i2c-hid: Use block reads when possible to save power

[Andi Shyti]

Hi Andy,

> > so the only strategy available up until now has been to always retrieve
> > the maximum possible report length over i2c, which can be quite
> > inefficient. For devices that send reports in block read format, the i2c
> > controller driver can read the payload length on the fly and terminate
> > the i2c transaction early, resulting in considerable power savings.
> > 
> > On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the
> > touchpad causes psys power readings to go up by about 4W and hover there
> > until I remove my finger. With this patch, my psys readings go from 4.7W
> > down to 3.1W, yielding about 1.6W in savings. This is because my
> > touchpad's max report length is 60 bytes, but all of the regular reports
> > it sends for touch events are only 32 bytes, so the i2c transfer is
> > roughly halved for the common case.
> 
> > +   /* Try to do a block read if the size fits in one byte */
> > +   flags = size > 255 ? I2C_M_RD : I2C_M_RD | I2C_M_RECV_LEN;
> 
> AFAIR SMBus specification tells about 256. Why 255?
> 
> Andi, am I correct?

Actually the SMBUS 3.0 protocol from 2015[*] says 255:

"
D.6 255 Bytes in Process Call

The maximum number of bytes allowed in the Block Write-Block Read
Process Call (Section 6.5.8) was increased from 32 to 255.
"

But why does it matter... I see the patch is detatching itself
from smbus.

And, actually, I wonder if this is the right way to fix it, isn't
it better to fix smbus instead?

I have a patch ready that fixes the smbus transfer size, perhaps
I should rebase, test and send it.

Andi

[*] http://smbus.org/specs/SMBus_3_0_20141220.pdf

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Dmitry,

> > > > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > > > but it seems abandoned and I do not have any other model for testing.
> > > > Therefore, this patch implements the least instrusive solution
> > > > and only removes I2C_M_NOSTART for MMS345L.
> > > 
> > > Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> > > to work for you and him and it looks like Android drivers are not using
> > > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> > > it was originally developed.
> > 
> > I completely forgot about that patch :)
> > 
> > I should refresh some old work on that device which was left
> > undone.
> > 
> > > Any objections?
> > 
> > It's OK for me. If you can just update my e-mail, please, when
> > applying the patch to "a...@etezian.org". Thanks!
> 
> Andi, any chance you could resend it with the new email? One thing that
> I try to avoid modifying whenever I can is S-O-B strings...

sure, I will resend the patches, but it might take some time (a
few days I hope) because I don't have the devices with me right
now for testing (and I added some small fixes, as well)

Thanks,
Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Stephan,

> > > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > > but it seems abandoned and I do not have any other model for testing.
> > > Therefore, this patch implements the least instrusive solution
> > > and only removes I2C_M_NOSTART for MMS345L.
> > 
> > Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> > to work for you and him and it looks like Android drivers are not using
> > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> > it was originally developed.
> > 
> > Any objections?
> 
> I cannot really speak for any of the other models, but no objections for
> removing I2C_M_NOSTART from my side. I'm actually rather confused by it
> since it is used on the first partial message.
> 
> The documentation [1] says:
>   If you set the I2C_M_NOSTART variable for the first partial message,
>   we do not generate Addr, but we do generate the startbit S.
>   ** This will probably confuse all other clients on your bus,
>   so don't try this. **
> 
> Yet, someone felt like trying this here. ;)

still it should be specified in the i2c protocol of the device,
if it's not, then most probably it's not needed, I guess.

Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Dmitry,

> > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > but it seems abandoned and I do not have any other model for testing.
> > Therefore, this patch implements the least instrusive solution
> > and only removes I2C_M_NOSTART for MMS345L.
> 
> Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> to work for you and him and it looks like Android drivers are not using
> I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> it was originally developed.

I completely forgot about that patch :)

I should refresh some old work on that device which was left
undone.

> Any objections?

It's OK for me. If you can just update my e-mail, please, when
applying the patch to "a...@etezian.org". Thanks!

Thank you,
Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

> > -   if (!i2c_check_functionality(client->adapter,
> > -   I2C_FUNC_PROTOCOL_MANGLING)) {
> > +   type = (enum mms_type)device_get_match_data(>dev);
> 
> you don't need any cast here.

sorry, please ignore :)

Andi

Re: [PATCH 1/3] Input: mms114 - use device_get_match_data

[Andi Shyti]

Hi Stephan,

> device_get_match_data is available now, so we can replace the call
> to of_device_get_match_data and remove the FIXME comment.
> 
> Signed-off-by: Stephan Gerhold 

Reviewed-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH 2/3] dt-bindings: mms114: document melfas,mms345l binding

[Andi Shyti]

Hi Stephan,

On Mon, Oct 07, 2019 at 10:33:42PM +0200, Stephan Gerhold wrote:
> The mms114 driver now supports MMS345L; document the
> melfas,mms345l binding that is used for it.
> 
> Signed-off-by: Stephan Gerhold 

Reviewed-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Stephan,

On Mon, Oct 07, 2019 at 10:50:21PM +0200, Stephan Gerhold wrote:
> MMS345L is another first generation touch screen from Melfas,
> which uses the same registers as MMS152.
> 
> However, using I2C_M_NOSTART for it causes errors when reading:
> 
>   i2c i2c-0: sendbytes: NAK bailout.
>   mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5)
> 
> The driver works fine as soon as I2C_M_NOSTART is removed.
> 
> Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART
> only for MMS114 and MMS152.
> 
> Signed-off-by: Stephan Gerhold 

Reviewed-by: Andi Shyti 

just a nitpick in case you will resend it (but you don't need
to).

> - if (!i2c_check_functionality(client->adapter,
> - I2C_FUNC_PROTOCOL_MANGLING)) {
> + type = (enum mms_type)device_get_match_data(>dev);

you don't need any cast here.

Thanks,
Andi

Re: [PATCH -next 25/36] spi: s3c24xx: use devm_platform_ioremap_resource() to simplify code

[Andi Shyti]

Hi Yuehaibing,

> >> Use devm_platform_ioremap_resource() to simplify the code a bit.
> >> This is detected by coccinelle.
> >>
> >> Reported-by: Hulk Robot 
> > 
> > This tag does not look real... First of all where is the report?
> 
> It is our internal CI robot, which is unavailable to external temporarily.

Hulk Robot is not a person and not accountable for his report.
If it is your internal CI, please write a sentence stating that
the fix has been made using Huawei internal tools.

Credit must be given to tools as well, but not accounts that will
never answer an e-mail.

Otherwise, the patch would look fine.

Andi

Re: [PATCH] Input: tm2-touchkey - acknowledge that setting brightness is a blocking call

[Andi Shyti]

Hi Dmitry,

On Wed, Feb 06, 2019 at 10:16:16AM -0800, Dmitry Torokhov wrote:
> We need to access I2C bus when switching brightness, and that may block,
> therefore we have to set stmfts_brightness_set() as LED's
> brightness_set_blocking() method.
> 
> Signed-off-by: Dmitry Torokhov 

same here:

Acked-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH] Input: stmfts - acknowledge that setting brightness is a blocking call

[Andi Shyti]

Hi Dmitry,

On Tue, Feb 05, 2019 at 02:46:42PM -0800, Dmitry Torokhov wrote:
> We need to turn regulators on and off when switching brightness, and
> that may block, therefore we have to set stmfts_brightness_set() as
> LED's brightness_set_blocking() method.
> 
> Fixes: 78bcac7b2ae1 ("Input: add support for the STMicroelectronics FingerTip 
> touchscreen")
> Signed-off-by: Dmitry Torokhov 

Acked-by: Andi Shyti 

Thanks,

Re: [PATCH 1/2] input: egalax_ts: add system wakeup support

[Andi Shyti]

Hi Anson,

On Wed, Sep 05, 2018 at 05:26:46PM +0800, Anson Huang wrote:
> This patch adds wakeup function support for egalax touch
> screen, if "wakeup-source" is added to device tree's egalax
> touch screen node, the wakeup function will be enabled, and
> egalax touch screen will be able to wakeup system from suspend.
> 
> Signed-off-by: Anson Huang 

I think you messed up the order of the patches, this patch
whould come after the PATCH 2/2.

You first create the property and then use it. If someone checks
out right here, he would find an inconsistency between the dts
property and this code.

You can either send a version 2 with the correct order or check
if Dmitry and Rob are willing to sync for who takes first the
patch first.

Andi

Re: [PATCH 1/2] input: egalax_ts: add system wakeup support

[Andi Shyti]

Hi Anson,

On Wed, Sep 05, 2018 at 05:26:46PM +0800, Anson Huang wrote:
> This patch adds wakeup function support for egalax touch
> screen, if "wakeup-source" is added to device tree's egalax
> touch screen node, the wakeup function will be enabled, and
> egalax touch screen will be able to wakeup system from suspend.
> 
> Signed-off-by: Anson Huang 

I think you messed up the order of the patches, this patch
whould come after the PATCH 2/2.

You first create the property and then use it. If someone checks
out right here, he would find an inconsistency between the dts
property and this code.

You can either send a version 2 with the correct order or check
if Dmitry and Rob are willing to sync for who takes first the
patch first.

Andi

Re: [PATCH v3] dt-bindings: arm: Explicitly mark Samsung Exynos SoC as unstable

[Andi Shyti]

Hi Krzysztof,

On Thu, Aug 30, 2018 at 08:02:05PM +0200, Krzysztof Kozlowski wrote:
> Samsung Exynos SoCs and boards related bindings evolved since the initial
> introduction, but initially the bindings were minimal and a bit incomplete
> (they never described all the hardware modules available in the SoCs).
> Since then some significant (not fully compatible) changes have been
> already committed a few times (like gpio replaced by pinctrl, display ddc,
> mfc reserved memory, some core clocks added to various hardware modules,
> added more required nodes).
> 
> On the other side there are no boards which have device tree embedded in
> the bootloader. Device tree blob is always compiled from the kernel tree
> and updated together with the kernel image.
> 
> Thus to avoid further adding a bunch of workarounds for old/missing
> bindings, make development of new platforms easier and allow to make
> cleanup of the existing code and device tree files, lets mark some
> Samsung Exynos SoC platform bindings as unstable. This means that
> bindings can may change at any time and users should use the dtb file
   ^^^
you have a typo here, other than that I agree with the patch:

Acked-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH v3] dt-bindings: arm: Explicitly mark Samsung Exynos SoC as unstable

[Andi Shyti]

Hi Krzysztof,

On Thu, Aug 30, 2018 at 08:02:05PM +0200, Krzysztof Kozlowski wrote:
> Samsung Exynos SoCs and boards related bindings evolved since the initial
> introduction, but initially the bindings were minimal and a bit incomplete
> (they never described all the hardware modules available in the SoCs).
> Since then some significant (not fully compatible) changes have been
> already committed a few times (like gpio replaced by pinctrl, display ddc,
> mfc reserved memory, some core clocks added to various hardware modules,
> added more required nodes).
> 
> On the other side there are no boards which have device tree embedded in
> the bootloader. Device tree blob is always compiled from the kernel tree
> and updated together with the kernel image.
> 
> Thus to avoid further adding a bunch of workarounds for old/missing
> bindings, make development of new platforms easier and allow to make
> cleanup of the existing code and device tree files, lets mark some
> Samsung Exynos SoC platform bindings as unstable. This means that
> bindings can may change at any time and users should use the dtb file
   ^^^
you have a typo here, other than that I agree with the patch:

Acked-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH] input: olpc_apsp: remove unused pointer 'np'

[Andi Shyti]

Hi Colin,

On Tue, Aug 28, 2018 at 03:56:48PM +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Pointer 'nb' is being assigned but is never used hence it is

'np' here. Other than that,

Reviewed-by: Andi Shyti 

Andi

> redundant and can be removed.
> 
> Cleans up clang warning:
> warning: variable 'np' set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/input/serio/olpc_apsp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
> index 8e9a4209fcad..8bd8ed25946f 100644
> --- a/drivers/input/serio/olpc_apsp.c
> +++ b/drivers/input/serio/olpc_apsp.c
> @@ -172,7 +172,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
>   struct serio *kb_serio, *pad_serio;
>   struct olpc_apsp *priv;
>   struct resource *res;
> - struct device_node *np;
>   unsigned long l;
>   int error;
>  
> @@ -180,7 +179,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
>   if (!priv)
>   return -ENOMEM;
>  
> - np = pdev->dev.of_node;
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   priv->base = devm_ioremap_resource(>dev, res);
>   if (IS_ERR(priv->base)) {
> -- 
> 2.17.1

Re: [PATCH] input: olpc_apsp: remove unused pointer 'np'

[Andi Shyti]

Hi Colin,

On Tue, Aug 28, 2018 at 03:56:48PM +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Pointer 'nb' is being assigned but is never used hence it is

'np' here. Other than that,

Reviewed-by: Andi Shyti 

Andi

> redundant and can be removed.
> 
> Cleans up clang warning:
> warning: variable 'np' set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/input/serio/olpc_apsp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
> index 8e9a4209fcad..8bd8ed25946f 100644
> --- a/drivers/input/serio/olpc_apsp.c
> +++ b/drivers/input/serio/olpc_apsp.c
> @@ -172,7 +172,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
>   struct serio *kb_serio, *pad_serio;
>   struct olpc_apsp *priv;
>   struct resource *res;
> - struct device_node *np;
>   unsigned long l;
>   int error;
>  
> @@ -180,7 +179,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
>   if (!priv)
>   return -ENOMEM;
>  
> - np = pdev->dev.of_node;
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   priv->base = devm_ioremap_resource(>dev, res);
>   if (IS_ERR(priv->base)) {
> -- 
> 2.17.1

Re: [PATCH] Input: elants_i2c - Fix sw reset delays

[Andi Shyti]

Hi Derek,

next time, could you please avoid using html mails when replying
to the mailing list? They are not clear.

On Fri, Aug 24, 2018 at 04:07:41PM -0700, dbasehore . wrote:
> 
> 
> On Fri, Aug 24, 2018 at 1:49 AM Andi Shyti  wrote:
> 
> Hi Derek,
> 
> > > > On Thu, Aug 23, 2018 at 04:10:13PM -0700, Derek Basehore wrote:
> > > > > We only need to wait 10ms instead of 30ms before starting fastboot
> or
> > > > > sending IAP on the touchscreen. Also, instead of delaying 
> everytime
> > > > > sw_reset is called, this delays 10ms in the function that starts
> > > > > fastboot. There's also an explicit 20ms delay before sending IAP
> when
> > > > > updating the firmware, so no additional delay is needed there. 
> This
> > > > > change also has the benefit of not delaying when wakeup is enabled
> > > > > during suspend. This is because sw_reset is called, yet fastboot
> > > > > isn't.
> 
> ...
> 
> > > > > -       /*
> > > > > -        * We should wait at least 10 msec (but no more than 40)
> before
> > > > > -        * sending fastboot or IAP command to the device.
> > > > > -        */
> > > > > -       msleep(30);
> > > > > -
> 
> moving from 30 to 0 is a bit alarming... what does the datasheet
> say?
> 
> 
> We're moving from 30msec to 10msec. We used to do this in an older kernel.

That's not what you are saying in your commit message:

  "... This change also has the benefit of not delaying when
  wakeup is enabled during suspend... "

and indeed, in resume the "elants_i2c_sw_reset()" function would
be called without any msleep. Or am I missing something? Would
it be correct based on the datasheet?

>  
> 
> 
> Sometimes delays are implicit in the system where you are testing
> the driver, so that without any msleep it might work in your
> system but it might not on others.
> 
> > +             /*
> > +              * We should wait at least 10 msec (but no more than 40)
> before
> > +              * sending IAP command to the device.
> > +              */
> >               msleep(20);
> 
> I agree though that it's not nice to wait twice here (even though
> as Dmitry says it doesn't hurt so much). Wouldn't it make more
> sense to remove this msleep instead?
> This way...
> 
> 
> We still need this if the delay is removed from the sw_reset function. We have
> a total of 20msecs delayed between sw_reset and sending IAP in our old kernel
> which should be well tested. There seems to be no reason why the delays
> increased.

Yes, that's what I'm saying, removing this msleep to me looks
more correct than removing it from sw_reset().

> > +     /*
> > +      * We should wait at least 10 msec (but no more than 40) before
> sending
> > +      * fastboot command to the device.
> > +      */
> > +     usleep_range(10 * 1000, 11 * 1000);
> > +
> >       error = elants_i2c_send(client, boot_cmd, sizeof(boot_cmd));
> 
> ... you do not need to add an extra sleep here.
> 
> 
> I added it here since I removed the 30msec sleep in sw_reset. This prevents us
> from sleeping on resume when the touch controller is already on for wake up
> capabilities. This is the same delay that we have in our old 3.8 kernel, so
> it's pretty well tested and shipped in production.

And here we fall in my first question (and also Dmitry's
concern), Are we sure we don't need to sleep after resume? Again,
what does the datasheet say?

I don't know the answer, you might be right, but I need to know
why you are removing the sleep after resume and why, instead,
the original developer added it there.

Two more nitpicks on this patch:

1. by adding the usleep_range() outside from the
elants_i2c_sw_reset() function, to another developer reading the
code, might not really look clear why we are waiting. The
comment you copy pasted does not, indeed, refer that we are
waiting because a reset has been performed and we need to wait
for the _reset time_ before sending IAP commands.

2. this is a matter of taste: to me "1" looks more clear than
"10 * 1000", which doesn't add any better readability of the
number itself.

Thanks,
Andi

Re: [PATCH] Input: elants_i2c - Fix sw reset delays

[Andi Shyti]

Hi Derek,

next time, could you please avoid using html mails when replying
to the mailing list? They are not clear.

On Fri, Aug 24, 2018 at 04:07:41PM -0700, dbasehore . wrote:
> 
> 
> On Fri, Aug 24, 2018 at 1:49 AM Andi Shyti  wrote:
> 
> Hi Derek,
> 
> > > > On Thu, Aug 23, 2018 at 04:10:13PM -0700, Derek Basehore wrote:
> > > > > We only need to wait 10ms instead of 30ms before starting fastboot
> or
> > > > > sending IAP on the touchscreen. Also, instead of delaying 
> everytime
> > > > > sw_reset is called, this delays 10ms in the function that starts
> > > > > fastboot. There's also an explicit 20ms delay before sending IAP
> when
> > > > > updating the firmware, so no additional delay is needed there. 
> This
> > > > > change also has the benefit of not delaying when wakeup is enabled
> > > > > during suspend. This is because sw_reset is called, yet fastboot
> > > > > isn't.
> 
> ...
> 
> > > > > -       /*
> > > > > -        * We should wait at least 10 msec (but no more than 40)
> before
> > > > > -        * sending fastboot or IAP command to the device.
> > > > > -        */
> > > > > -       msleep(30);
> > > > > -
> 
> moving from 30 to 0 is a bit alarming... what does the datasheet
> say?
> 
> 
> We're moving from 30msec to 10msec. We used to do this in an older kernel.

That's not what you are saying in your commit message:

  "... This change also has the benefit of not delaying when
  wakeup is enabled during suspend... "

and indeed, in resume the "elants_i2c_sw_reset()" function would
be called without any msleep. Or am I missing something? Would
it be correct based on the datasheet?

>  
> 
> 
> Sometimes delays are implicit in the system where you are testing
> the driver, so that without any msleep it might work in your
> system but it might not on others.
> 
> > +             /*
> > +              * We should wait at least 10 msec (but no more than 40)
> before
> > +              * sending IAP command to the device.
> > +              */
> >               msleep(20);
> 
> I agree though that it's not nice to wait twice here (even though
> as Dmitry says it doesn't hurt so much). Wouldn't it make more
> sense to remove this msleep instead?
> This way...
> 
> 
> We still need this if the delay is removed from the sw_reset function. We have
> a total of 20msecs delayed between sw_reset and sending IAP in our old kernel
> which should be well tested. There seems to be no reason why the delays
> increased.

Yes, that's what I'm saying, removing this msleep to me looks
more correct than removing it from sw_reset().

> > +     /*
> > +      * We should wait at least 10 msec (but no more than 40) before
> sending
> > +      * fastboot command to the device.
> > +      */
> > +     usleep_range(10 * 1000, 11 * 1000);
> > +
> >       error = elants_i2c_send(client, boot_cmd, sizeof(boot_cmd));
> 
> ... you do not need to add an extra sleep here.
> 
> 
> I added it here since I removed the 30msec sleep in sw_reset. This prevents us
> from sleeping on resume when the touch controller is already on for wake up
> capabilities. This is the same delay that we have in our old 3.8 kernel, so
> it's pretty well tested and shipped in production.

And here we fall in my first question (and also Dmitry's
concern), Are we sure we don't need to sleep after resume? Again,
what does the datasheet say?

I don't know the answer, you might be right, but I need to know
why you are removing the sleep after resume and why, instead,
the original developer added it there.

Two more nitpicks on this patch:

1. by adding the usleep_range() outside from the
elants_i2c_sw_reset() function, to another developer reading the
code, might not really look clear why we are waiting. The
comment you copy pasted does not, indeed, refer that we are
waiting because a reset has been performed and we need to wait
for the _reset time_ before sending IAP commands.

2. this is a matter of taste: to me "1" looks more clear than
"10 * 1000", which doesn't add any better readability of the
number itself.

Thanks,
Andi

Re: [PATCH] Input: elants_i2c - Fix sw reset delays

[Andi Shyti]

Hi Derek,

> > > On Thu, Aug 23, 2018 at 04:10:13PM -0700, Derek Basehore wrote:
> > > > We only need to wait 10ms instead of 30ms before starting fastboot or
> > > > sending IAP on the touchscreen. Also, instead of delaying everytime
> > > > sw_reset is called, this delays 10ms in the function that starts
> > > > fastboot. There's also an explicit 20ms delay before sending IAP when
> > > > updating the firmware, so no additional delay is needed there. This
> > > > change also has the benefit of not delaying when wakeup is enabled
> > > > during suspend. This is because sw_reset is called, yet fastboot
> > > > isn't.

...

> > > > -   /*
> > > > -* We should wait at least 10 msec (but no more than 40) before
> > > > -* sending fastboot or IAP command to the device.
> > > > -*/
> > > > -   msleep(30);
> > > > -

moving from 30 to 0 is a bit alarming... what does the datasheet
say?

Sometimes delays are implicit in the system where you are testing
the driver, so that without any msleep it might work in your
system but it might not on others.

> + /*
> +  * We should wait at least 10 msec (but no more than 40) before
> +  * sending IAP command to the device.
> +  */
>   msleep(20);

I agree though that it's not nice to wait twice here (even though
as Dmitry says it doesn't hurt so much). Wouldn't it make more
sense to remove this msleep instead?
This way...

> + /*
> +  * We should wait at least 10 msec (but no more than 40) before sending
> +  * fastboot command to the device.
> +  */
> + usleep_range(10 * 1000, 11 * 1000);
> +
>   error = elants_i2c_send(client, boot_cmd, sizeof(boot_cmd));

... you do not need to add an extra sleep here.

Andi

Re: [PATCH] Input: elants_i2c - Fix sw reset delays

[Andi Shyti]

Hi Derek,

> > > On Thu, Aug 23, 2018 at 04:10:13PM -0700, Derek Basehore wrote:
> > > > We only need to wait 10ms instead of 30ms before starting fastboot or
> > > > sending IAP on the touchscreen. Also, instead of delaying everytime
> > > > sw_reset is called, this delays 10ms in the function that starts
> > > > fastboot. There's also an explicit 20ms delay before sending IAP when
> > > > updating the firmware, so no additional delay is needed there. This
> > > > change also has the benefit of not delaying when wakeup is enabled
> > > > during suspend. This is because sw_reset is called, yet fastboot
> > > > isn't.

...

> > > > -   /*
> > > > -* We should wait at least 10 msec (but no more than 40) before
> > > > -* sending fastboot or IAP command to the device.
> > > > -*/
> > > > -   msleep(30);
> > > > -

moving from 30 to 0 is a bit alarming... what does the datasheet
say?

Sometimes delays are implicit in the system where you are testing
the driver, so that without any msleep it might work in your
system but it might not on others.

> + /*
> +  * We should wait at least 10 msec (but no more than 40) before
> +  * sending IAP command to the device.
> +  */
>   msleep(20);

I agree though that it's not nice to wait twice here (even though
as Dmitry says it doesn't hurt so much). Wouldn't it make more
sense to remove this msleep instead?
This way...

> + /*
> +  * We should wait at least 10 msec (but no more than 40) before sending
> +  * fastboot command to the device.
> +  */
> + usleep_range(10 * 1000, 11 * 1000);
> +
>   error = elants_i2c_send(client, boot_cmd, sizeof(boot_cmd));

... you do not need to add an extra sleep here.

Andi

[PATCH v2] ARM: dts: exynos: Update x and y properties for mms114 touchscreen

[Andi Shyti]

The mms114 binding [1] specifies that the 'x' and 'y' should be
called respectively 'touchscreen-size-x' and 'touchscreen-size-y'
in coherence with the touchscreen [2] binding.

Update the mms114 node for trats2 and trats dts according to the
binding.

[1] Documentation/devicetree/bindings/input/touchscreen/mms114.txt
[2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 arch/arm/boot/dts/exynos4210-trats.dts  | 4 ++--
 arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-trats.dts 
b/arch/arm/boot/dts/exynos4210-trats.dts
index eaeeb4f6b84a..6f1d76cb7951 100644
--- a/arch/arm/boot/dts/exynos4210-trats.dts
+++ b/arch/arm/boot/dts/exynos4210-trats.dts
@@ -259,8 +259,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
diff --git a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi 
b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi
index 606946a264da..30eee5942eff 100644
--- a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi
+++ b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi
@@ -118,8 +118,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
-- 
2.17.0

[PATCH v2] ARM: dts: exynos: Update x and y properties for mms114 touchscreen

[Andi Shyti]

The mms114 binding [1] specifies that the 'x' and 'y' should be
called respectively 'touchscreen-size-x' and 'touchscreen-size-y'
in coherence with the touchscreen [2] binding.

Update the mms114 node for trats2 and trats dts according to the
binding.

[1] Documentation/devicetree/bindings/input/touchscreen/mms114.txt
[2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Signed-off-by: Andi Shyti 
---
 arch/arm/boot/dts/exynos4210-trats.dts  | 4 ++--
 arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-trats.dts 
b/arch/arm/boot/dts/exynos4210-trats.dts
index eaeeb4f6b84a..6f1d76cb7951 100644
--- a/arch/arm/boot/dts/exynos4210-trats.dts
+++ b/arch/arm/boot/dts/exynos4210-trats.dts
@@ -259,8 +259,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
diff --git a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi 
b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi
index 606946a264da..30eee5942eff 100644
--- a/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi
+++ b/arch/arm/boot/dts/exynos4412-galaxy-s3.dtsi
@@ -118,8 +118,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
-- 
2.17.0

Re: [PATCH] Input: ili251x - add support for Ilitek ILI251x touchscreens

[Andi Shyti]

Hi Philipp,

I had a fast look to your driver and I have few comments.

>  .../bindings/input/touchscreen/ili251x.txt|  35 ++
>  drivers/input/touchscreen/Kconfig |  12 +
>  drivers/input/touchscreen/Makefile|   1 +
>  drivers/input/touchscreen/ili251x.c   | 350 ++
>  4 files changed, 398 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/input/touchscreen/ili251x.txt
>  create mode 100644 drivers/input/touchscreen/ili251x.c

Please split the patch, the bindig should be on a separate patch
and must come before the driver.

> +#define MAX_FINGERS  10
> +#define REG_TOUCHDATA0x10
> +#define TOUCHDATA_FINGERS6
> +#define REG_TOUCHDATA2   0x14
> +#define TOUCHDATA2_FINGERS   4
> +#define REG_PANEL_INFO   0x20
> +#define REG_FIRMWARE_VERSION 0x40
> +#define REG_PROTO_VERSION0x42
> +#define REG_CALIBRATE0xcc

Can you please group and sort these definitions by type? REGs
with REGs and others together?

Please start the defines names with a unique identifier,
ILI251X_REG_* instead of just REG_*

> +struct finger {
> + u8 x_high:6;
> + u8 dummy:1;
> + u8 status:1;
> + u8 x_low;
> + u8 y_high;
> + u8 y_low;
> + u8 pressure;
> +} __packed;
> +
> +struct touchdata {
> + u8 status;
> + struct finger fingers[MAX_FINGERS];
> +} __packed;
> +
> +struct panel_info {
> + u8 x_low;
> + u8 x_high;
> + u8 y_low;
> + u8 y_high;
> + u8 xchannel_num;
> + u8 ychannel_num;
> + u8 max_fingers;
> +} __packed;
> +
> +struct firmware_version {
> + u8 id;
> + u8 major;
> + u8 minor;
> +} __packed;
> +
> +struct protocol_version {
> + u8 major;
> + u8 minor;
> +} __packed;

panel_info, firmware_version and protocol_version are used very
little in the driver (just in probe). Is it really necessary to
keep a definition?

Is there a way to get rid of them?

> +struct ili251x_data {
> + struct i2c_client *client;
> + struct input_dev *input;
> + unsigned int max_fingers;
> + bool use_pressure;
> + struct gpio_desc *reset_gpio;
> +};

Please start also the strct definitions with the unique
identifier ili251x_* like you did with ili251x_data.

> +
> +static int ili251x_read_reg(struct i2c_client *client, u8 reg, void *buf,
> + size_t len)
> +{
> + struct i2c_msg msg[2] = {
> + {
> + .addr   = client->addr,
> + .flags  = 0,
> + .len= 1,
> + .buf= ,
> + },
> + {
> + .addr   = client->addr,
> + .flags  = I2C_M_RD,
> + .len= len,
> + .buf= buf,
> + }
> + };
> +
> + if (i2c_transfer(client->adapter, msg, 2) != 2) {
> + dev_err(>dev, "i2c transfer failed\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}

I do not see the need for a ili251x_read_reg function. You are
not reading more than 240 bytes per time, am I right?

In this case I would use the smbus functions (at least whenever
possible in case I miscalculated the 240b), this is ju a
duplicated code.

> +static void ili251x_report_events(struct ili251x_data *data,
> +   const struct touchdata *touchdata)
> +{
> + struct input_dev *input = data->input;
> + unsigned int i;
> + bool touch;
> + unsigned int x, y;
> + const struct finger *finger;
> + unsigned int reported_fingers = 0;
> +
> + /* the touch chip does not count the real fingers but switches between
> +  * 0, 6 and 10 reported fingers *
> +  *
> +  * FIXME: With a tested ili2511 we received only garbage for fingers
> +  *6-9. As workaround we add a device tree option to limit the
> +  *handled number of fingers
> +  */
> + if (touchdata->status == 1)
> + reported_fingers = 6;
> + else if (touchdata->status == 2)
> + reported_fingers = 10;
> +
> + for (i = 0; i < reported_fingers && i < data->max_fingers; i++) {
> + input_mt_slot(input, i);
> +
> + finger = >fingers[i];
> +
> + touch = finger->status;
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
> + x = finger->x_low | (finger->x_high << 8);
> + y = finger->y_low | (finger->y_high << 8);

the x and y calculation can go uinside the if() below, right?

> + if (touch) {
> + input_report_abs(input, ABS_MT_POSITION_X, x);
> + input_report_abs(input, ABS_MT_POSITION_Y, y);
> + if (data->use_pressure)
> + input_report_abs(input, ABS_MT_PRESSURE,
> +  finger->pressure);
> +
> + }

just a 

Re: [PATCH] Input: ili251x - add support for Ilitek ILI251x touchscreens

[Andi Shyti]

Hi Philipp,

I had a fast look to your driver and I have few comments.

>  .../bindings/input/touchscreen/ili251x.txt|  35 ++
>  drivers/input/touchscreen/Kconfig |  12 +
>  drivers/input/touchscreen/Makefile|   1 +
>  drivers/input/touchscreen/ili251x.c   | 350 ++
>  4 files changed, 398 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/input/touchscreen/ili251x.txt
>  create mode 100644 drivers/input/touchscreen/ili251x.c

Please split the patch, the bindig should be on a separate patch
and must come before the driver.

> +#define MAX_FINGERS  10
> +#define REG_TOUCHDATA0x10
> +#define TOUCHDATA_FINGERS6
> +#define REG_TOUCHDATA2   0x14
> +#define TOUCHDATA2_FINGERS   4
> +#define REG_PANEL_INFO   0x20
> +#define REG_FIRMWARE_VERSION 0x40
> +#define REG_PROTO_VERSION0x42
> +#define REG_CALIBRATE0xcc

Can you please group and sort these definitions by type? REGs
with REGs and others together?

Please start the defines names with a unique identifier,
ILI251X_REG_* instead of just REG_*

> +struct finger {
> + u8 x_high:6;
> + u8 dummy:1;
> + u8 status:1;
> + u8 x_low;
> + u8 y_high;
> + u8 y_low;
> + u8 pressure;
> +} __packed;
> +
> +struct touchdata {
> + u8 status;
> + struct finger fingers[MAX_FINGERS];
> +} __packed;
> +
> +struct panel_info {
> + u8 x_low;
> + u8 x_high;
> + u8 y_low;
> + u8 y_high;
> + u8 xchannel_num;
> + u8 ychannel_num;
> + u8 max_fingers;
> +} __packed;
> +
> +struct firmware_version {
> + u8 id;
> + u8 major;
> + u8 minor;
> +} __packed;
> +
> +struct protocol_version {
> + u8 major;
> + u8 minor;
> +} __packed;

panel_info, firmware_version and protocol_version are used very
little in the driver (just in probe). Is it really necessary to
keep a definition?

Is there a way to get rid of them?

> +struct ili251x_data {
> + struct i2c_client *client;
> + struct input_dev *input;
> + unsigned int max_fingers;
> + bool use_pressure;
> + struct gpio_desc *reset_gpio;
> +};

Please start also the strct definitions with the unique
identifier ili251x_* like you did with ili251x_data.

> +
> +static int ili251x_read_reg(struct i2c_client *client, u8 reg, void *buf,
> + size_t len)
> +{
> + struct i2c_msg msg[2] = {
> + {
> + .addr   = client->addr,
> + .flags  = 0,
> + .len= 1,
> + .buf= ,
> + },
> + {
> + .addr   = client->addr,
> + .flags  = I2C_M_RD,
> + .len= len,
> + .buf= buf,
> + }
> + };
> +
> + if (i2c_transfer(client->adapter, msg, 2) != 2) {
> + dev_err(>dev, "i2c transfer failed\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}

I do not see the need for a ili251x_read_reg function. You are
not reading more than 240 bytes per time, am I right?

In this case I would use the smbus functions (at least whenever
possible in case I miscalculated the 240b), this is ju a
duplicated code.

> +static void ili251x_report_events(struct ili251x_data *data,
> +   const struct touchdata *touchdata)
> +{
> + struct input_dev *input = data->input;
> + unsigned int i;
> + bool touch;
> + unsigned int x, y;
> + const struct finger *finger;
> + unsigned int reported_fingers = 0;
> +
> + /* the touch chip does not count the real fingers but switches between
> +  * 0, 6 and 10 reported fingers *
> +  *
> +  * FIXME: With a tested ili2511 we received only garbage for fingers
> +  *6-9. As workaround we add a device tree option to limit the
> +  *handled number of fingers
> +  */
> + if (touchdata->status == 1)
> + reported_fingers = 6;
> + else if (touchdata->status == 2)
> + reported_fingers = 10;
> +
> + for (i = 0; i < reported_fingers && i < data->max_fingers; i++) {
> + input_mt_slot(input, i);
> +
> + finger = >fingers[i];
> +
> + touch = finger->status;
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
> + x = finger->x_low | (finger->x_high << 8);
> + y = finger->y_low | (finger->y_high << 8);

the x and y calculation can go uinside the if() below, right?

> + if (touch) {
> + input_report_abs(input, ABS_MT_POSITION_X, x);
> + input_report_abs(input, ABS_MT_POSITION_Y, y);
> + if (data->use_pressure)
> + input_report_abs(input, ABS_MT_PRESSURE,
> +  finger->pressure);
> +
> + }

just a 

[PATCH v2] media: ir-spi: update Andi's e-mail

[Andi Shyti]

From: Andi Shyti <andi.sh...@samsung.com>

Because I will be leaving Samsung soon, update my e-mail to the
etezian.org mail.

CC: Mauro Carvalho Chehab <mche...@kernel.org>
CC: Sean Young <s...@mess.org>
Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
Hi Sean,

thanks for the review and sorry for the late reply. Here is the
patch with my mail changed also in the MODULE_AUTHOR macro.

Thanks,
Andi

v1 - v2
changed the mail also in the MODULE_AUTHOR macro

 drivers/media/rc/ir-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
index 7163d5ce2e64..66334e8d63ba 100644
--- a/drivers/media/rc/ir-spi.c
+++ b/drivers/media/rc/ir-spi.c
@@ -2,7 +2,7 @@
 // SPI driven IR LED device driver
 //
 // Copyright (c) 2016 Samsung Electronics Co., Ltd.
-// Copyright (c) Andi Shyti <andi.sh...@samsung.com>
+// Copyright (c) Andi Shyti <a...@etezian.org>
 
 #include 
 #include 
@@ -173,6 +173,6 @@ static struct spi_driver ir_spi_driver = {
 
 module_spi_driver(ir_spi_driver);
 
-MODULE_AUTHOR("Andi Shyti <andi.sh...@samsung.com>");
+MODULE_AUTHOR("Andi Shyti <a...@etezian.org>");
 MODULE_DESCRIPTION("SPI IR LED");
 MODULE_LICENSE("GPL v2");
-- 
2.17.0

[PATCH v2] media: ir-spi: update Andi's e-mail

[Andi Shyti]

From: Andi Shyti 

Because I will be leaving Samsung soon, update my e-mail to the
etezian.org mail.

CC: Mauro Carvalho Chehab 
CC: Sean Young 
Signed-off-by: Andi Shyti 
---
Hi Sean,

thanks for the review and sorry for the late reply. Here is the
patch with my mail changed also in the MODULE_AUTHOR macro.

Thanks,
Andi

v1 - v2
changed the mail also in the MODULE_AUTHOR macro

 drivers/media/rc/ir-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
index 7163d5ce2e64..66334e8d63ba 100644
--- a/drivers/media/rc/ir-spi.c
+++ b/drivers/media/rc/ir-spi.c
@@ -2,7 +2,7 @@
 // SPI driven IR LED device driver
 //
 // Copyright (c) 2016 Samsung Electronics Co., Ltd.
-// Copyright (c) Andi Shyti 
+// Copyright (c) Andi Shyti 
 
 #include 
 #include 
@@ -173,6 +173,6 @@ static struct spi_driver ir_spi_driver = {
 
 module_spi_driver(ir_spi_driver);
 
-MODULE_AUTHOR("Andi Shyti ");
+MODULE_AUTHOR("Andi Shyti ");
 MODULE_DESCRIPTION("SPI IR LED");
 MODULE_LICENSE("GPL v2");
-- 
2.17.0

[PATCH 0/2] Licensing fixes and updates for the mms114 driver

[Andi Shyti]

Hi Dmitry,

I will send this two as a new patchset. The first one is the
messed SPDX patch that you reverted, the second one is about the
license incohorency that Marcus has pointed out.

Andi

Andi Shyti (2):
  Input: mms114 - add SPDX identifier
  Input: mms114 - fix license module information

 drivers/input/touchscreen/mms114.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

-- 
2.15.1

[PATCH 0/2] Licensing fixes and updates for the mms114 driver

[Andi Shyti]

Hi Dmitry,

I will send this two as a new patchset. The first one is the
messed SPDX patch that you reverted, the second one is about the
license incohorency that Marcus has pointed out.

Andi

Andi Shyti (2):
  Input: mms114 - add SPDX identifier
  Input: mms114 - fix license module information

 drivers/input/touchscreen/mms114.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

-- 
2.15.1

[PATCH 2/2] Input: mms114 - fix license module information

[Andi Shyti]

The driver has been released with GNU Public License v2 as stated
in the header, but the module license information has been tagged
as "GPL" (GNU Public License v2 or later).

Fix the module license information so that it matches the one in
the header as "GPL v2".

Fixes: 07b8481d4aff ("Input: add MELFAS mms114 touchscreen driver")
Reported-by: Marcus Folkesson <marcus.folkes...@gmail.com>
Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index be7926555432..a5ab774da4cc 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -621,4 +621,4 @@ module_i2c_driver(mms114_driver);
 /* Module information */
 MODULE_AUTHOR("Joonyoung Shim <jy0922.s...@samsung.com>");
 MODULE_DESCRIPTION("MELFAS mms114 Touchscreen driver");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
-- 
2.15.1

[PATCH 2/2] Input: mms114 - fix license module information

[Andi Shyti]

The driver has been released with GNU Public License v2 as stated
in the header, but the module license information has been tagged
as "GPL" (GNU Public License v2 or later).

Fix the module license information so that it matches the one in
the header as "GPL v2".

Fixes: 07b8481d4aff ("Input: add MELFAS mms114 touchscreen driver")
Reported-by: Marcus Folkesson 
Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index be7926555432..a5ab774da4cc 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -621,4 +621,4 @@ module_i2c_driver(mms114_driver);
 /* Module information */
 MODULE_AUTHOR("Joonyoung Shim ");
 MODULE_DESCRIPTION("MELFAS mms114 Touchscreen driver");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
-- 
2.15.1

[PATCH 1/2] Input: mms114 - add SPDX identifier

[Andi Shyti]

Replace the original license statement with the SPDX identifier.
Add also one line of description as recommended by the COPYING
file.

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index db4f6bb502e3..be7926555432 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -1,11 +1,8 @@
-/*
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim <jy0922.s...@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
+// SPDX-License-Identifier: GPL-2.0
+// Melfas MMS114/MMS152 touchscreen device driver
+//
+// Copyright (c) 2012 Samsung Electronics Co., Ltd.
+// Author: Joonyoung Shim <jy0922.s...@samsung.com>
 
 #include 
 #include 
-- 
2.15.1

[PATCH 1/2] Input: mms114 - add SPDX identifier

[Andi Shyti]

Replace the original license statement with the SPDX identifier.
Add also one line of description as recommended by the COPYING
file.

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index db4f6bb502e3..be7926555432 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -1,11 +1,8 @@
-/*
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
+// SPDX-License-Identifier: GPL-2.0
+// Melfas MMS114/MMS152 touchscreen device driver
+//
+// Copyright (c) 2012 Samsung Electronics Co., Ltd.
+// Author: Joonyoung Shim 
 
 #include 
 #include 
-- 
2.15.1

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

> > > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > > - * Author: Joonyoung Shim 
> > > > > - *
> > > > > - * This program is free software; you can redistribute it and/or 
> > > > > modify
> > > > > - * it under the terms of the GNU General Public License version 2 as
> > > > > - * published by the Free Software Foundation.
> > > > > - */
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +// Samsung S6SY761 Touchscreen device driver
> > > 
> > > I'm very sorry, but my distrcation will kill me one day.
> > > 
> > > Is it possible to revert this patch or do you want me to send a
> > > fix?
> > 
> 
> OK, I dropped the patch, please resend the correct version.

Thanks, will do!

Andi

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

> > > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > > - * Author: Joonyoung Shim 
> > > > > - *
> > > > > - * This program is free software; you can redistribute it and/or 
> > > > > modify
> > > > > - * it under the terms of the GNU General Public License version 2 as
> > > > > - * published by the Free Software Foundation.
> > > > > - */
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +// Samsung S6SY761 Touchscreen device driver
> > > 
> > > I'm very sorry, but my distrcation will kill me one day.
> > > 
> > > Is it possible to revert this patch or do you want me to send a
> > > fix?
> > 
> 
> OK, I dropped the patch, please resend the correct version.

Thanks, will do!

Andi

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Marcus,

> > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > - * Author: Joonyoung Shim 
> > > > - *
> > > > - * This program is free software; you can redistribute it and/or modify
> > > > - * it under the terms of the GNU General Public License version 2 as
> > > > - * published by the Free Software Foundation.
> > > > - */
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +// Samsung S6SY761 Touchscreen device driver
> > 
> > I'm very sorry, but my distrcation will kill me one day.
> 
> More coffee or sleep - your choice ;-)

I see that beer is not working, indeed :)

> > Is it possible to revert this patch or do you want me to send a
> > fix?
> 
> When you are on it;
> 
> Change
> MODULE_LICENSE("GPL");
> 
> to 
> MODULE_LICENSE("GPL v2");
> To match the former license text and SPDX-identifier.
> 
> See include/linux/module.h:
>  *"GPL"   [GNU Public License v2 or later]
>  *"GPL v2"[GNU Public License v2]

I haven't noticed that. I will fix it in a separate patch.

Thanks,
Andi

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Marcus,

> > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > - * Author: Joonyoung Shim 
> > > > - *
> > > > - * This program is free software; you can redistribute it and/or modify
> > > > - * it under the terms of the GNU General Public License version 2 as
> > > > - * published by the Free Software Foundation.
> > > > - */
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +// Samsung S6SY761 Touchscreen device driver
> > 
> > I'm very sorry, but my distrcation will kill me one day.
> 
> More coffee or sleep - your choice ;-)

I see that beer is not working, indeed :)

> > Is it possible to revert this patch or do you want me to send a
> > fix?
> 
> When you are on it;
> 
> Change
> MODULE_LICENSE("GPL");
> 
> to 
> MODULE_LICENSE("GPL v2");
> To match the former license text and SPDX-identifier.
> 
> See include/linux/module.h:
>  *"GPL"   [GNU Public License v2 or later]
>  *"GPL v2"[GNU Public License v2]

I haven't noticed that. I will fix it in a separate patch.

Thanks,
Andi

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

> > -/*
> > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > - * Author: Joonyoung Shim 
> > - *
> > - * This program is free software; you can redistribute it and/or modify
> > - * it under the terms of the GNU General Public License version 2 as
> > - * published by the Free Software Foundation.
> > - */
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Samsung S6SY761 Touchscreen device driver

I'm very sorry, but my distrcation will kill me one day.

Is it possible to revert this patch or do you want me to send a
fix?

Sorry,
Andi

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

> > -/*
> > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > - * Author: Joonyoung Shim 
> > - *
> > - * This program is free software; you can redistribute it and/or modify
> > - * it under the terms of the GNU General Public License version 2 as
> > - * published by the Free Software Foundation.
> > - */
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Samsung S6SY761 Touchscreen device driver

I'm very sorry, but my distrcation will kill me one day.

Is it possible to revert this patch or do you want me to send a
fix?

Sorry,
Andi

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 03:33:35PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 30, 2018 at 08:29:23AM +0900, Andi Shyti wrote:
> > Hi Dmitry,
> > 
> > On Mon, Jan 29, 2018 at 10:56:31AM -0800, Dmitry Torokhov wrote:
> > > On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> > > > Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
> > > 
> > > But why?
> > 
> > I think this patch is trivial and the subject itself is quite
> > self-explanatory and the commit message would be a copy-paste of
> > it.
> 
> Here is where we disagree. Why lowercase is preferred over uppercase? Do
> we have this in coding style (and I completely forgot about it)? Or is
> it your preference (mine is too to be fair, but I also do not care
> about churning the code for it)?

I think this is the case where the cultural habits become law, I've
been writing lower case hex values from years and my eyes are so
used to it that it looks wrong to have them written upper case.
Many maintainers reject upper case hex values.

As well as many reject the " ? : " conditional form.

In any case, you're basically right here, at this point I
don't mind if you drop it :)

Thanks,
Andi

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 03:33:35PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 30, 2018 at 08:29:23AM +0900, Andi Shyti wrote:
> > Hi Dmitry,
> > 
> > On Mon, Jan 29, 2018 at 10:56:31AM -0800, Dmitry Torokhov wrote:
> > > On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> > > > Signed-off-by: Andi Shyti 
> > > 
> > > But why?
> > 
> > I think this patch is trivial and the subject itself is quite
> > self-explanatory and the commit message would be a copy-paste of
> > it.
> 
> Here is where we disagree. Why lowercase is preferred over uppercase? Do
> we have this in coding style (and I completely forgot about it)? Or is
> it your preference (mine is too to be fair, but I also do not care
> about churning the code for it)?

I think this is the case where the cultural habits become law, I've
been writing lower case hex values from years and my eyes are so
used to it that it looks wrong to have them written upper case.
Many maintainers reject upper case hex values.

As well as many reject the " ? : " conditional form.

In any case, you're basically right here, at this point I
don't mind if you drop it :)

Thanks,
Andi

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 10:56:31AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> > Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
> 
> But why?

I think this patch is trivial and the subject itself is quite
self-explanatory and the commit message would be a copy-paste of
it.

OK, if you want I will add "some comment".

Andi

> > ---
> >  drivers/input/touchscreen/mms114.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/mms114.c 
> > b/drivers/input/touchscreen/mms114.c
> > index 11dba8bb48e3..fdf23bc416af 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -20,7 +20,7 @@
> >  
> >  /* Write only registers */
> >  #define MMS114_MODE_CONTROL0x01
> > -#define MMS114_OPERATION_MODE_MASK 0xE
> > +#define MMS114_OPERATION_MODE_MASK 0xe
> >  #define MMS114_ACTIVE  (1 << 1)
> >  
> >  #define MMS114_XY_RESOLUTION_H 0x02
> > @@ -30,12 +30,12 @@
> >  #define MMS114_MOVING_THRESHOLD0x06
> >  
> >  /* Read only registers */
> > -#define MMS114_PACKET_SIZE 0x0F
> > +#define MMS114_PACKET_SIZE 0x0f
> >  #define MMS114_INFOMATION  0x10
> > -#define MMS114_TSP_REV 0xF0
> > +#define MMS114_TSP_REV 0xf0
> >  
> > -#define MMS152_FW_REV  0xE1
> > -#define MMS152_COMPAT_GROUP0xF2
> > +#define MMS152_FW_REV  0xe1
> > +#define MMS152_COMPAT_GROUP0xf2
> >  
> >  /* 200ms needs after power on */
> >  #define MMS114_POWERON_DELAY   200
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Dmitry
> 

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 10:56:31AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> > Signed-off-by: Andi Shyti 
> 
> But why?

I think this patch is trivial and the subject itself is quite
self-explanatory and the commit message would be a copy-paste of
it.

OK, if you want I will add "some comment".

Andi

> > ---
> >  drivers/input/touchscreen/mms114.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/mms114.c 
> > b/drivers/input/touchscreen/mms114.c
> > index 11dba8bb48e3..fdf23bc416af 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -20,7 +20,7 @@
> >  
> >  /* Write only registers */
> >  #define MMS114_MODE_CONTROL0x01
> > -#define MMS114_OPERATION_MODE_MASK 0xE
> > +#define MMS114_OPERATION_MODE_MASK 0xe
> >  #define MMS114_ACTIVE  (1 << 1)
> >  
> >  #define MMS114_XY_RESOLUTION_H 0x02
> > @@ -30,12 +30,12 @@
> >  #define MMS114_MOVING_THRESHOLD0x06
> >  
> >  /* Read only registers */
> > -#define MMS114_PACKET_SIZE 0x0F
> > +#define MMS114_PACKET_SIZE 0x0f
> >  #define MMS114_INFOMATION  0x10
> > -#define MMS114_TSP_REV 0xF0
> > +#define MMS114_TSP_REV 0xf0
> >  
> > -#define MMS152_FW_REV  0xE1
> > -#define MMS152_COMPAT_GROUP0xF2
> > +#define MMS152_FW_REV  0xe1
> > +#define MMS152_COMPAT_GROUP0xf2
> >  
> >  /* 200ms needs after power on */
> >  #define MMS114_POWERON_DELAY   200
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Dmitry
> 

Re: [PATCH 4/8] Input: mms114 - remove unused variable

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 10:43:09AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:19PM +0900, Andi Shyti wrote:
> > '__packed' is not used anywhere, remove it.
> 
> Umm, this is not a variable, this is type annotation meaning that the
> structure is packed. Still not needed, as we are not using anything but
> u8 data elements, but justification is completely wrong.

Oh dammit! :)

of course! The original idea was the alignment of the structure,
but I must have got confused while re-editing all the commits.

Sorry, I'll fix it.

Andi

Re: [PATCH 4/8] Input: mms114 - remove unused variable

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 10:43:09AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:19PM +0900, Andi Shyti wrote:
> > '__packed' is not used anywhere, remove it.
> 
> Umm, this is not a variable, this is type annotation meaning that the
> structure is packed. Still not needed, as we are not using anything but
> u8 data elements, but justification is completely wrong.

Oh dammit! :)

of course! The original idea was the alignment of the structure,
but I must have got confused while re-editing all the commits.

Sorry, I'll fix it.

Andi

Re: [PATCH 2/8] Input: mms114 - get read of custm i2c read/write functions

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 11:01:41AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:17PM +0900, Andi Shyti wrote:
> > The 'mms114_read_reg' and 'mms114_write_reg' are used when
> > reading or writing to the 'MMS114_MODE_CONTROL' register for
> > updating the 'cache_mode_control' variable.
> > 
> > Update the 'cache_mode_control' variable in the calling
> > mms114_set_active() function and get rid of all the custom i2c
> > read/write functions.
> > 
> > With this remove also the redundant sleep of MMS114_I2C_DELAY
> > (50us) between i2c operations. The waiting should to be handled
> > by the i2c driver.
> > 
> > Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
> > ---
> >  drivers/input/touchscreen/mms114.c | 87 
> > +-
> >  1 file changed, 10 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/mms114.c 
> > b/drivers/input/touchscreen/mms114.c
> > index 0b8b1f0e8ba6..94a97049d711 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -37,9 +37,6 @@
> >  #define MMS152_FW_REV  0xE1
> >  #define MMS152_COMPAT_GROUP0xF2
> >  
> > -/* Minimum delay time is 50us between stop and start signal of i2c */
> > -#define MMS114_I2C_DELAY   50
> > -
> >  /* 200ms needs after power on */
> >  #define MMS114_POWERON_DELAY   200
> >  
> > @@ -83,76 +80,6 @@ struct mms114_touch {
> > u8 reserved[2];
> >  } __packed;
> >  
> > -static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
> > -unsigned int len, u8 *val)
> > -{
> > -   struct i2c_client *client = data->client;
> > -   struct i2c_msg xfer[2];
> > -   u8 buf = reg & 0xff;
> > -   int error;
> > -
> > -   if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
> > -   BUG();
> > -
> > -   /* Write register: use repeated start */
> > -   xfer[0].addr = client->addr;
> > -   xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
> 
> So the chip does not use 10-bit addressing? What about I2C_M_NOSTART? It
> is not needed also?

>From the datasheet I have no, indeed I don't understand why this
is coming. That's why I asked Simon to test it on his device as
well.

On my device this patch works just fine.

> > -   xfer[0].len = 1;
> > -   xfer[0].buf = 
> > -
> > -   /* Read data */
> > -   xfer[1].addr = client->addr;
> > -   xfer[1].flags = I2C_M_RD;
> > -   xfer[1].len = len;
> > -   xfer[1].buf = val;
> > -
> > -   error = i2c_transfer(client->adapter, xfer, 2);
> > -   if (error != 2) {
> > -   dev_err(>dev,
> > -   "%s: i2c transfer failed (%d)\n", __func__, error);
> > -   return error < 0 ? error : -EIO;
> > -   }
> > -   udelay(MMS114_I2C_DELAY);
> > -
> > -   return 0;
> > -}
> > -
> > -static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
> > -{
> > -   u8 val;
> > -   int error;
> > -
> > -   if (reg == MMS114_MODE_CONTROL)
> > -   return data->cache_mode_control;
> > -
> > -   error = __mms114_read_reg(data, reg, 1, );
> > -   return error < 0 ? error : val;
> > -}
> > -
> > -static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
> > -   unsigned int val)
> > -{
> > -   struct i2c_client *client = data->client;
> > -   u8 buf[2];
> > -   int error;
> > -
> > -   buf[0] = reg & 0xff;
> > -   buf[1] = val & 0xff;
> > -
> > -   error = i2c_master_send(client, buf, 2);
> > -   if (error != 2) {
> > -   dev_err(>dev,
> > -   "%s: i2c send failed (%d)\n", __func__, error);
> > -   return error < 0 ? error : -EIO;
> > -   }
> > -   udelay(MMS114_I2C_DELAY);
> > -
> > -   if (reg == MMS114_MODE_CONTROL)
> > -   data->cache_mode_control = val;
> > -
> > -   return 0;
> > -}
> > -
> >  static void mms114_process_mt(struct mms114_data *data, struct 
> > mms114_touch *touch)
> >  {
> > struct i2c_client *client = data->client;
> > @@ -231,19 +158,25 @@ static irqreturn_t mms114_interrupt(int irq, void 
> > *dev_id)
> >  
> >  static int mms114_set_active(struct mms114_data *data, bool active)
> >  {
> > -   int val;
> > +   int val, err;
> >  
> > -   val = mms114_read_reg(data, MMS114_MODE_CONTROL);
> > +   val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL);
> 
> If I  understand the original commit for the driver, the control
> register is write only and is not to be read from, that is why we have
> this cached value. With your change you read form it.

Still in my datasheet thee MMS114_MODE_CONTROL is a R/W register.
Still I don't understand the original choice, but it doesn't
change much anyway, I can keep the original idea of never reading
from it. Or I can rework this function in a better way.

> By the way, have you looked into converting it all to regmap?

I could.

Andi

Re: [PATCH 2/8] Input: mms114 - get read of custm i2c read/write functions

[Andi Shyti]

Hi Dmitry,

On Mon, Jan 29, 2018 at 11:01:41AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:17PM +0900, Andi Shyti wrote:
> > The 'mms114_read_reg' and 'mms114_write_reg' are used when
> > reading or writing to the 'MMS114_MODE_CONTROL' register for
> > updating the 'cache_mode_control' variable.
> > 
> > Update the 'cache_mode_control' variable in the calling
> > mms114_set_active() function and get rid of all the custom i2c
> > read/write functions.
> > 
> > With this remove also the redundant sleep of MMS114_I2C_DELAY
> > (50us) between i2c operations. The waiting should to be handled
> > by the i2c driver.
> > 
> > Signed-off-by: Andi Shyti 
> > ---
> >  drivers/input/touchscreen/mms114.c | 87 
> > +-
> >  1 file changed, 10 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/mms114.c 
> > b/drivers/input/touchscreen/mms114.c
> > index 0b8b1f0e8ba6..94a97049d711 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -37,9 +37,6 @@
> >  #define MMS152_FW_REV  0xE1
> >  #define MMS152_COMPAT_GROUP0xF2
> >  
> > -/* Minimum delay time is 50us between stop and start signal of i2c */
> > -#define MMS114_I2C_DELAY   50
> > -
> >  /* 200ms needs after power on */
> >  #define MMS114_POWERON_DELAY   200
> >  
> > @@ -83,76 +80,6 @@ struct mms114_touch {
> > u8 reserved[2];
> >  } __packed;
> >  
> > -static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
> > -unsigned int len, u8 *val)
> > -{
> > -   struct i2c_client *client = data->client;
> > -   struct i2c_msg xfer[2];
> > -   u8 buf = reg & 0xff;
> > -   int error;
> > -
> > -   if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
> > -   BUG();
> > -
> > -   /* Write register: use repeated start */
> > -   xfer[0].addr = client->addr;
> > -   xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
> 
> So the chip does not use 10-bit addressing? What about I2C_M_NOSTART? It
> is not needed also?

>From the datasheet I have no, indeed I don't understand why this
is coming. That's why I asked Simon to test it on his device as
well.

On my device this patch works just fine.

> > -   xfer[0].len = 1;
> > -   xfer[0].buf = 
> > -
> > -   /* Read data */
> > -   xfer[1].addr = client->addr;
> > -   xfer[1].flags = I2C_M_RD;
> > -   xfer[1].len = len;
> > -   xfer[1].buf = val;
> > -
> > -   error = i2c_transfer(client->adapter, xfer, 2);
> > -   if (error != 2) {
> > -   dev_err(>dev,
> > -   "%s: i2c transfer failed (%d)\n", __func__, error);
> > -   return error < 0 ? error : -EIO;
> > -   }
> > -   udelay(MMS114_I2C_DELAY);
> > -
> > -   return 0;
> > -}
> > -
> > -static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
> > -{
> > -   u8 val;
> > -   int error;
> > -
> > -   if (reg == MMS114_MODE_CONTROL)
> > -   return data->cache_mode_control;
> > -
> > -   error = __mms114_read_reg(data, reg, 1, );
> > -   return error < 0 ? error : val;
> > -}
> > -
> > -static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
> > -   unsigned int val)
> > -{
> > -   struct i2c_client *client = data->client;
> > -   u8 buf[2];
> > -   int error;
> > -
> > -   buf[0] = reg & 0xff;
> > -   buf[1] = val & 0xff;
> > -
> > -   error = i2c_master_send(client, buf, 2);
> > -   if (error != 2) {
> > -   dev_err(>dev,
> > -   "%s: i2c send failed (%d)\n", __func__, error);
> > -   return error < 0 ? error : -EIO;
> > -   }
> > -   udelay(MMS114_I2C_DELAY);
> > -
> > -   if (reg == MMS114_MODE_CONTROL)
> > -   data->cache_mode_control = val;
> > -
> > -   return 0;
> > -}
> > -
> >  static void mms114_process_mt(struct mms114_data *data, struct 
> > mms114_touch *touch)
> >  {
> > struct i2c_client *client = data->client;
> > @@ -231,19 +158,25 @@ static irqreturn_t mms114_interrupt(int irq, void 
> > *dev_id)
> >  
> >  static int mms114_set_active(struct mms114_data *data, bool active)
> >  {
> > -   int val;
> > +   int val, err;
> >  
> > -   val = mms114_read_reg(data, MMS114_MODE_CONTROL);
> > +   val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL);
> 
> If I  understand the original commit for the driver, the control
> register is write only and is not to be read from, that is why we have
> this cached value. With your change you read form it.

Still in my datasheet thee MMS114_MODE_CONTROL is a R/W register.
Still I don't understand the original choice, but it doesn't
change much anyway, I can keep the original idea of never reading
from it. Or I can rework this function in a better way.

> By the way, have you looked into converting it all to regmap?

I could.

Andi

[PATCH 8/8] Input: mms114 - fix typo in definition

[Andi Shyti]

It's 'MMS114_INFORMATION', not 'MMS114_INFOMATION'

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 3230c92de1ed..5b609531b3aa 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -28,7 +28,7 @@
 
 /* Read only registers */
 #define MMS114_PACKET_SIZE 0x0f
-#define MMS114_INFOMATION  0x10
+#define MMS114_INFORMATION 0x10
 #define MMS114_TSP_REV 0xf0
 
 #define MMS152_FW_REV  0xe1
@@ -138,7 +138,7 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
 
touch_size = packet_size / MMS114_PACKET_NUM;
 
-   error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFOMATION,
+   error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFORMATION,
  packet_size, (u8 *)touch);
if (error < 0)
goto out;
-- 
2.15.1

[PATCH 8/8] Input: mms114 - fix typo in definition

[Andi Shyti]

It's 'MMS114_INFORMATION', not 'MMS114_INFOMATION'

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 3230c92de1ed..5b609531b3aa 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -28,7 +28,7 @@
 
 /* Read only registers */
 #define MMS114_PACKET_SIZE 0x0f
-#define MMS114_INFOMATION  0x10
+#define MMS114_INFORMATION 0x10
 #define MMS114_TSP_REV 0xf0
 
 #define MMS152_FW_REV  0xe1
@@ -138,7 +138,7 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
 
touch_size = packet_size / MMS114_PACKET_NUM;
 
-   error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFOMATION,
+   error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFORMATION,
  packet_size, (u8 *)touch);
if (error < 0)
goto out;
-- 
2.15.1

[PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 11dba8bb48e3..fdf23bc416af 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -20,7 +20,7 @@
 
 /* Write only registers */
 #define MMS114_MODE_CONTROL0x01
-#define MMS114_OPERATION_MODE_MASK 0xE
+#define MMS114_OPERATION_MODE_MASK 0xe
 #define MMS114_ACTIVE  (1 << 1)
 
 #define MMS114_XY_RESOLUTION_H 0x02
@@ -30,12 +30,12 @@
 #define MMS114_MOVING_THRESHOLD0x06
 
 /* Read only registers */
-#define MMS114_PACKET_SIZE 0x0F
+#define MMS114_PACKET_SIZE 0x0f
 #define MMS114_INFOMATION  0x10
-#define MMS114_TSP_REV 0xF0
+#define MMS114_TSP_REV 0xf0
 
-#define MMS152_FW_REV  0xE1
-#define MMS152_COMPAT_GROUP0xF2
+#define MMS152_FW_REV  0xe1
+#define MMS152_COMPAT_GROUP0xf2
 
 /* 200ms needs after power on */
 #define MMS114_POWERON_DELAY   200
-- 
2.15.1

[PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Replace the original license statement with the SPDX identifier.

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index d70c03adf148..3230c92de1ed 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -1,11 +1,8 @@
-/*
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim <jy0922.s...@samsung.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
+// SPDX-License-Identifier: GPL-2.0
+// Samsung S6SY761 Touchscreen device driver
+//
+// Copyright (c) 2012 Samsung Electronics Co., Ltd.
+// Author: Joonyoung Shim <jy0922.s...@samsung.com>
 
 #include 
 #include 
-- 
2.15.1

[PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 11dba8bb48e3..fdf23bc416af 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -20,7 +20,7 @@
 
 /* Write only registers */
 #define MMS114_MODE_CONTROL0x01
-#define MMS114_OPERATION_MODE_MASK 0xE
+#define MMS114_OPERATION_MODE_MASK 0xe
 #define MMS114_ACTIVE  (1 << 1)
 
 #define MMS114_XY_RESOLUTION_H 0x02
@@ -30,12 +30,12 @@
 #define MMS114_MOVING_THRESHOLD0x06
 
 /* Read only registers */
-#define MMS114_PACKET_SIZE 0x0F
+#define MMS114_PACKET_SIZE 0x0f
 #define MMS114_INFOMATION  0x10
-#define MMS114_TSP_REV 0xF0
+#define MMS114_TSP_REV 0xf0
 
-#define MMS152_FW_REV  0xE1
-#define MMS152_COMPAT_GROUP0xF2
+#define MMS152_FW_REV  0xe1
+#define MMS152_COMPAT_GROUP0xf2
 
 /* 200ms needs after power on */
 #define MMS114_POWERON_DELAY   200
-- 
2.15.1

[PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Replace the original license statement with the SPDX identifier.

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index d70c03adf148..3230c92de1ed 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -1,11 +1,8 @@
-/*
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
+// SPDX-License-Identifier: GPL-2.0
+// Samsung S6SY761 Touchscreen device driver
+//
+// Copyright (c) 2012 Samsung Electronics Co., Ltd.
+// Author: Joonyoung Shim 
 
 #include 
 #include 
-- 
2.15.1

[PATCH 4/8] Input: mms114 - remove unused variable

[Andi Shyti]

'__packed' is not used anywhere, remove it.

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index fb4435ae506b..11dba8bb48e3 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -78,7 +78,7 @@ struct mms114_touch {
u8 width;
u8 strength;
u8 reserved[2];
-} __packed;
+};
 
 static void mms114_process_mt(struct mms114_data *data, struct mms114_touch 
*touch)
 {
-- 
2.15.1

[PATCH 4/8] Input: mms114 - remove unused variable

[Andi Shyti]

'__packed' is not used anywhere, remove it.

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index fb4435ae506b..11dba8bb48e3 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -78,7 +78,7 @@ struct mms114_touch {
u8 width;
u8 strength;
u8 reserved[2];
-} __packed;
+};
 
 static void mms114_process_mt(struct mms114_data *data, struct mms114_touch 
*touch)
 {
-- 
2.15.1

[PATCH 1/8] Input: mms114 - use smbus functions whenever possible

[Andi Shyti]

The exchange of data to and from the mms114 touchscreen never
exceeds 256 bytes. In the worst case it goes up to 80 bytes in
the interrupt handler while reading the events.

Thus it's not needed to make use of custom read/write functions
for accessing the i2c. Replace, whenever possible, the use of
custom functions with the more standard smbus ones.

It's not possible only in one case, in the mms114_set_active()
function where the 'cache_mode_control' variable is updated
according to the value in the register 'MMS114_MODE_CONTROL'
register.

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index c54f4afe1103..0b8b1f0e8ba6 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -207,14 +207,15 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
}
mutex_unlock(_dev->mutex);
 
-   packet_size = mms114_read_reg(data, MMS114_PACKET_SIZE);
+   packet_size = i2c_smbus_read_byte_data(data->client,
+  MMS114_PACKET_SIZE);
if (packet_size <= 0)
goto out;
 
touch_size = packet_size / MMS114_PACKET_NUM;
 
-   error = __mms114_read_reg(data, MMS114_INFOMATION, packet_size,
-   (u8 *)touch);
+   error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFOMATION,
+ packet_size, (u8 *)touch);
if (error < 0)
goto out;
 
@@ -254,7 +255,8 @@ static int mms114_get_version(struct mms114_data *data)
 
switch (data->type) {
case TYPE_MMS152:
-   error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+   error = i2c_smbus_read_i2c_block_data(data->client,
+ MMS152_FW_REV, 3, buf);
if (error)
return error;
 
@@ -268,7 +270,8 @@ static int mms114_get_version(struct mms114_data *data)
break;
 
case TYPE_MMS114:
-   error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
+   error = i2c_smbus_read_i2c_block_data(data->client,
+ MMS114_TSP_REV, 6, buf);
if (error)
return error;
 
@@ -300,30 +303,35 @@ static int mms114_setup_regs(struct mms114_data *data)
 
val = (props->max_x >> 8) & 0xf;
val |= ((props->max_y >> 8) & 0xf) << 4;
-   error = mms114_write_reg(data, MMS114_XY_RESOLUTION_H, val);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_XY_RESOLUTION_H, val);
if (error < 0)
return error;
 
val = props->max_x & 0xff;
-   error = mms114_write_reg(data, MMS114_X_RESOLUTION, val);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_X_RESOLUTION, val);
if (error < 0)
return error;
 
val = props->max_x & 0xff;
-   error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_Y_RESOLUTION, val);
if (error < 0)
return error;
 
if (data->contact_threshold) {
-   error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD,
-   data->contact_threshold);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_CONTACT_THRESHOLD,
+ data->contact_threshold);
if (error < 0)
return error;
}
 
if (data->moving_threshold) {
-   error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD,
-   data->moving_threshold);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_MOVING_THRESHOLD,
+ data->moving_threshold);
if (error < 0)
return error;
}
-- 
2.15.1

[PATCH 1/8] Input: mms114 - use smbus functions whenever possible

[Andi Shyti]

The exchange of data to and from the mms114 touchscreen never
exceeds 256 bytes. In the worst case it goes up to 80 bytes in
the interrupt handler while reading the events.

Thus it's not needed to make use of custom read/write functions
for accessing the i2c. Replace, whenever possible, the use of
custom functions with the more standard smbus ones.

It's not possible only in one case, in the mms114_set_active()
function where the 'cache_mode_control' variable is updated
according to the value in the register 'MMS114_MODE_CONTROL'
register.

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index c54f4afe1103..0b8b1f0e8ba6 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -207,14 +207,15 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
}
mutex_unlock(_dev->mutex);
 
-   packet_size = mms114_read_reg(data, MMS114_PACKET_SIZE);
+   packet_size = i2c_smbus_read_byte_data(data->client,
+  MMS114_PACKET_SIZE);
if (packet_size <= 0)
goto out;
 
touch_size = packet_size / MMS114_PACKET_NUM;
 
-   error = __mms114_read_reg(data, MMS114_INFOMATION, packet_size,
-   (u8 *)touch);
+   error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFOMATION,
+ packet_size, (u8 *)touch);
if (error < 0)
goto out;
 
@@ -254,7 +255,8 @@ static int mms114_get_version(struct mms114_data *data)
 
switch (data->type) {
case TYPE_MMS152:
-   error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+   error = i2c_smbus_read_i2c_block_data(data->client,
+ MMS152_FW_REV, 3, buf);
if (error)
return error;
 
@@ -268,7 +270,8 @@ static int mms114_get_version(struct mms114_data *data)
break;
 
case TYPE_MMS114:
-   error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
+   error = i2c_smbus_read_i2c_block_data(data->client,
+ MMS114_TSP_REV, 6, buf);
if (error)
return error;
 
@@ -300,30 +303,35 @@ static int mms114_setup_regs(struct mms114_data *data)
 
val = (props->max_x >> 8) & 0xf;
val |= ((props->max_y >> 8) & 0xf) << 4;
-   error = mms114_write_reg(data, MMS114_XY_RESOLUTION_H, val);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_XY_RESOLUTION_H, val);
if (error < 0)
return error;
 
val = props->max_x & 0xff;
-   error = mms114_write_reg(data, MMS114_X_RESOLUTION, val);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_X_RESOLUTION, val);
if (error < 0)
return error;
 
val = props->max_x & 0xff;
-   error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_Y_RESOLUTION, val);
if (error < 0)
return error;
 
if (data->contact_threshold) {
-   error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD,
-   data->contact_threshold);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_CONTACT_THRESHOLD,
+ data->contact_threshold);
if (error < 0)
return error;
}
 
if (data->moving_threshold) {
-   error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD,
-   data->moving_threshold);
+   error = i2c_smbus_write_byte_data(data->client,
+ MMS114_MOVING_THRESHOLD,
+ data->moving_threshold);
if (error < 0)
return error;
}
-- 
2.15.1

[PATCH 6/8] Input: mms114 - Use BIT() macro instead of explicit shifting

[Andi Shyti]

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index fdf23bc416af..d70c03adf148 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -21,7 +21,7 @@
 /* Write only registers */
 #define MMS114_MODE_CONTROL0x01
 #define MMS114_OPERATION_MODE_MASK 0xe
-#define MMS114_ACTIVE  (1 << 1)
+#define MMS114_ACTIVE  BIT(1)
 
 #define MMS114_XY_RESOLUTION_H 0x02
 #define MMS114_X_RESOLUTION0x03
-- 
2.15.1

[PATCH 6/8] Input: mms114 - Use BIT() macro instead of explicit shifting

[Andi Shyti]

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index fdf23bc416af..d70c03adf148 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -21,7 +21,7 @@
 /* Write only registers */
 #define MMS114_MODE_CONTROL0x01
 #define MMS114_OPERATION_MODE_MASK 0xe
-#define MMS114_ACTIVE  (1 << 1)
+#define MMS114_ACTIVE  BIT(1)
 
 #define MMS114_XY_RESOLUTION_H 0x02
 #define MMS114_X_RESOLUTION0x03
-- 
2.15.1

[PATCH 3/8] Input: mms114 - replace mdelay with msleep

[Andi Shyti]

200ms seconds is a very long time to keep the CPU busy looping.
Use msleep instead.

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 94a97049d711..fb4435ae506b 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -290,7 +290,7 @@ static int mms114_start(struct mms114_data *data)
return error;
}
 
-   mdelay(MMS114_POWERON_DELAY);
+   msleep(MMS114_POWERON_DELAY);
 
error = mms114_setup_regs(data);
if (error < 0) {
-- 
2.15.1

[PATCH 3/8] Input: mms114 - replace mdelay with msleep

[Andi Shyti]

200ms seconds is a very long time to keep the CPU busy looping.
Use msleep instead.

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 94a97049d711..fb4435ae506b 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -290,7 +290,7 @@ static int mms114_start(struct mms114_data *data)
return error;
}
 
-   mdelay(MMS114_POWERON_DELAY);
+   msleep(MMS114_POWERON_DELAY);
 
error = mms114_setup_regs(data);
if (error < 0) {
-- 
2.15.1

[PATCH 0/8] Melfas MMS114 touchscreen cleanups

[Andi Shyti]

Hi Dmitry,

this patchset contains some cleanups for the mms114 driver. The
first two patches are the most important because they get rid of
the custom i2c read/write functions and use the smbus instead.

The others come from a "cleanup rush" I felt into.

I tested the patchest on the mms114 touchscreen in trats2, I
would appreaciate if Simon can test it on mms152.

Thanks,
Andi

Andi Shyti (8):
  Input: mms114 - use smbus functions whenever possible
  Input: mms114 - get read of custm i2c read/write functions
  Input: mms114 - replace mdelay with msleep
  Input: mms114 - remove unused variable
  Input: mms114 - use lower case for hexadecimal values
  Input: mms114 - Use BIT() macro instead of explicit shifting
  Input: mms114 - add SPDX identifier
  Input: mms114 - fix typo in definition

 drivers/input/touchscreen/mms114.c | 150 +++--
 1 file changed, 44 insertions(+), 106 deletions(-)

-- 
2.15.1

[PATCH 2/8] Input: mms114 - get read of custm i2c read/write functions

[Andi Shyti]

The 'mms114_read_reg' and 'mms114_write_reg' are used when
reading or writing to the 'MMS114_MODE_CONTROL' register for
updating the 'cache_mode_control' variable.

Update the 'cache_mode_control' variable in the calling
mms114_set_active() function and get rid of all the custom i2c
read/write functions.

With this remove also the redundant sleep of MMS114_I2C_DELAY
(50us) between i2c operations. The waiting should to be handled
by the i2c driver.

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
 drivers/input/touchscreen/mms114.c | 87 +-
 1 file changed, 10 insertions(+), 77 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 0b8b1f0e8ba6..94a97049d711 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -37,9 +37,6 @@
 #define MMS152_FW_REV  0xE1
 #define MMS152_COMPAT_GROUP0xF2
 
-/* Minimum delay time is 50us between stop and start signal of i2c */
-#define MMS114_I2C_DELAY   50
-
 /* 200ms needs after power on */
 #define MMS114_POWERON_DELAY   200
 
@@ -83,76 +80,6 @@ struct mms114_touch {
u8 reserved[2];
 } __packed;
 
-static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
-unsigned int len, u8 *val)
-{
-   struct i2c_client *client = data->client;
-   struct i2c_msg xfer[2];
-   u8 buf = reg & 0xff;
-   int error;
-
-   if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
-   BUG();
-
-   /* Write register: use repeated start */
-   xfer[0].addr = client->addr;
-   xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
-   xfer[0].len = 1;
-   xfer[0].buf = 
-
-   /* Read data */
-   xfer[1].addr = client->addr;
-   xfer[1].flags = I2C_M_RD;
-   xfer[1].len = len;
-   xfer[1].buf = val;
-
-   error = i2c_transfer(client->adapter, xfer, 2);
-   if (error != 2) {
-   dev_err(>dev,
-   "%s: i2c transfer failed (%d)\n", __func__, error);
-   return error < 0 ? error : -EIO;
-   }
-   udelay(MMS114_I2C_DELAY);
-
-   return 0;
-}
-
-static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
-{
-   u8 val;
-   int error;
-
-   if (reg == MMS114_MODE_CONTROL)
-   return data->cache_mode_control;
-
-   error = __mms114_read_reg(data, reg, 1, );
-   return error < 0 ? error : val;
-}
-
-static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
-   unsigned int val)
-{
-   struct i2c_client *client = data->client;
-   u8 buf[2];
-   int error;
-
-   buf[0] = reg & 0xff;
-   buf[1] = val & 0xff;
-
-   error = i2c_master_send(client, buf, 2);
-   if (error != 2) {
-   dev_err(>dev,
-   "%s: i2c send failed (%d)\n", __func__, error);
-   return error < 0 ? error : -EIO;
-   }
-   udelay(MMS114_I2C_DELAY);
-
-   if (reg == MMS114_MODE_CONTROL)
-   data->cache_mode_control = val;
-
-   return 0;
-}
-
 static void mms114_process_mt(struct mms114_data *data, struct mms114_touch 
*touch)
 {
struct i2c_client *client = data->client;
@@ -231,19 +158,25 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
 
 static int mms114_set_active(struct mms114_data *data, bool active)
 {
-   int val;
+   int val, err;
 
-   val = mms114_read_reg(data, MMS114_MODE_CONTROL);
+   val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL);
if (val < 0)
return val;
 
-   val &= ~MMS114_OPERATION_MODE_MASK;
+   val = data->cache_mode_control &= ~MMS114_OPERATION_MODE_MASK;
 
/* If active is false, sleep mode */
if (active)
val |= MMS114_ACTIVE;
 
-   return mms114_write_reg(data, MMS114_MODE_CONTROL, val);
+   err = i2c_smbus_write_byte_data(data->client, MMS114_MODE_CONTROL, val);
+   if (err < 0)
+   return err;
+
+   data->cache_mode_control = val;
+
+   return 0;
 }
 
 static int mms114_get_version(struct mms114_data *data)
-- 
2.15.1

[PATCH 2/8] Input: mms114 - get read of custm i2c read/write functions

[Andi Shyti]

The 'mms114_read_reg' and 'mms114_write_reg' are used when
reading or writing to the 'MMS114_MODE_CONTROL' register for
updating the 'cache_mode_control' variable.

Update the 'cache_mode_control' variable in the calling
mms114_set_active() function and get rid of all the custom i2c
read/write functions.

With this remove also the redundant sleep of MMS114_I2C_DELAY
(50us) between i2c operations. The waiting should to be handled
by the i2c driver.

Signed-off-by: Andi Shyti 
---
 drivers/input/touchscreen/mms114.c | 87 +-
 1 file changed, 10 insertions(+), 77 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c 
b/drivers/input/touchscreen/mms114.c
index 0b8b1f0e8ba6..94a97049d711 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -37,9 +37,6 @@
 #define MMS152_FW_REV  0xE1
 #define MMS152_COMPAT_GROUP0xF2
 
-/* Minimum delay time is 50us between stop and start signal of i2c */
-#define MMS114_I2C_DELAY   50
-
 /* 200ms needs after power on */
 #define MMS114_POWERON_DELAY   200
 
@@ -83,76 +80,6 @@ struct mms114_touch {
u8 reserved[2];
 } __packed;
 
-static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
-unsigned int len, u8 *val)
-{
-   struct i2c_client *client = data->client;
-   struct i2c_msg xfer[2];
-   u8 buf = reg & 0xff;
-   int error;
-
-   if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
-   BUG();
-
-   /* Write register: use repeated start */
-   xfer[0].addr = client->addr;
-   xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
-   xfer[0].len = 1;
-   xfer[0].buf = 
-
-   /* Read data */
-   xfer[1].addr = client->addr;
-   xfer[1].flags = I2C_M_RD;
-   xfer[1].len = len;
-   xfer[1].buf = val;
-
-   error = i2c_transfer(client->adapter, xfer, 2);
-   if (error != 2) {
-   dev_err(>dev,
-   "%s: i2c transfer failed (%d)\n", __func__, error);
-   return error < 0 ? error : -EIO;
-   }
-   udelay(MMS114_I2C_DELAY);
-
-   return 0;
-}
-
-static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
-{
-   u8 val;
-   int error;
-
-   if (reg == MMS114_MODE_CONTROL)
-   return data->cache_mode_control;
-
-   error = __mms114_read_reg(data, reg, 1, );
-   return error < 0 ? error : val;
-}
-
-static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
-   unsigned int val)
-{
-   struct i2c_client *client = data->client;
-   u8 buf[2];
-   int error;
-
-   buf[0] = reg & 0xff;
-   buf[1] = val & 0xff;
-
-   error = i2c_master_send(client, buf, 2);
-   if (error != 2) {
-   dev_err(>dev,
-   "%s: i2c send failed (%d)\n", __func__, error);
-   return error < 0 ? error : -EIO;
-   }
-   udelay(MMS114_I2C_DELAY);
-
-   if (reg == MMS114_MODE_CONTROL)
-   data->cache_mode_control = val;
-
-   return 0;
-}
-
 static void mms114_process_mt(struct mms114_data *data, struct mms114_touch 
*touch)
 {
struct i2c_client *client = data->client;
@@ -231,19 +158,25 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
 
 static int mms114_set_active(struct mms114_data *data, bool active)
 {
-   int val;
+   int val, err;
 
-   val = mms114_read_reg(data, MMS114_MODE_CONTROL);
+   val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL);
if (val < 0)
return val;
 
-   val &= ~MMS114_OPERATION_MODE_MASK;
+   val = data->cache_mode_control &= ~MMS114_OPERATION_MODE_MASK;
 
/* If active is false, sleep mode */
if (active)
val |= MMS114_ACTIVE;
 
-   return mms114_write_reg(data, MMS114_MODE_CONTROL, val);
+   err = i2c_smbus_write_byte_data(data->client, MMS114_MODE_CONTROL, val);
+   if (err < 0)
+   return err;
+
+   data->cache_mode_control = val;
+
+   return 0;
 }
 
 static int mms114_get_version(struct mms114_data *data)
-- 
2.15.1

[PATCH 0/8] Melfas MMS114 touchscreen cleanups

[Andi Shyti]

Hi Dmitry,

this patchset contains some cleanups for the mms114 driver. The
first two patches are the most important because they get rid of
the custom i2c read/write functions and use the smbus instead.

The others come from a "cleanup rush" I felt into.

I tested the patchest on the mms114 touchscreen in trats2, I
would appreaciate if Simon can test it on mms152.

Thanks,
Andi

Andi Shyti (8):
  Input: mms114 - use smbus functions whenever possible
  Input: mms114 - get read of custm i2c read/write functions
  Input: mms114 - replace mdelay with msleep
  Input: mms114 - remove unused variable
  Input: mms114 - use lower case for hexadecimal values
  Input: mms114 - Use BIT() macro instead of explicit shifting
  Input: mms114 - add SPDX identifier
  Input: mms114 - fix typo in definition

 drivers/input/touchscreen/mms114.c | 150 +++--
 1 file changed, 44 insertions(+), 106 deletions(-)

-- 
2.15.1

[PATCH] ARM: dts: exynos: Update x and y properties for mms114 touchscreen

[Andi Shyti]

The mms114 binding [1] specifies that the 'x' and 'y' should be
called respectively 'touchscreen-size-x' and 'touchscreen-size-y'
in coherence with the touchscreen [2] binding.

Update the mms114 node for trats2 and trats dts according to the
binding.

[1] Documentation/devicetree/bindings/input/touchscreen/mms114.txt
[2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
---
Hi Krzysztof,

this patch depends on Simon's patchset [1] and must be applied after
that. If you want I can ping you when the patch gets or you can take
it in the next cycle.

Andi

[1] https://marc.info/?l=linux-kernel=151682273403098=2

 arch/arm/boot/dts/exynos4210-trats.dts  | 4 ++--
 arch/arm/boot/dts/exynos4412-trats2.dts | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-trats.dts 
b/arch/arm/boot/dts/exynos4210-trats.dts
index 7b6ab7265110..fc71a493b5cb 100644
--- a/arch/arm/boot/dts/exynos4210-trats.dts
+++ b/arch/arm/boot/dts/exynos4210-trats.dts
@@ -261,8 +261,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts 
b/arch/arm/boot/dts/exynos4412-trats2.dts
index 220cdf109405..1808c4951538 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -635,8 +635,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
-- 
2.15.1

[PATCH] ARM: dts: exynos: Update x and y properties for mms114 touchscreen

[Andi Shyti]

The mms114 binding [1] specifies that the 'x' and 'y' should be
called respectively 'touchscreen-size-x' and 'touchscreen-size-y'
in coherence with the touchscreen [2] binding.

Update the mms114 node for trats2 and trats dts according to the
binding.

[1] Documentation/devicetree/bindings/input/touchscreen/mms114.txt
[2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Signed-off-by: Andi Shyti 
---
Hi Krzysztof,

this patch depends on Simon's patchset [1] and must be applied after
that. If you want I can ping you when the patch gets or you can take
it in the next cycle.

Andi

[1] https://marc.info/?l=linux-kernel=151682273403098=2

 arch/arm/boot/dts/exynos4210-trats.dts  | 4 ++--
 arch/arm/boot/dts/exynos4412-trats2.dts | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-trats.dts 
b/arch/arm/boot/dts/exynos4210-trats.dts
index 7b6ab7265110..fc71a493b5cb 100644
--- a/arch/arm/boot/dts/exynos4210-trats.dts
+++ b/arch/arm/boot/dts/exynos4210-trats.dts
@@ -261,8 +261,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts 
b/arch/arm/boot/dts/exynos4412-trats2.dts
index 220cdf109405..1808c4951538 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -635,8 +635,8 @@
reg = <0x48>;
interrupt-parent = <>;
interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
-   x-size = <720>;
-   y-size = <1280>;
+   touchscreen-size-x = <720>;
+   touchscreen-size-y = <1280>;
avdd-supply = <_reg>;
vdd-supply = <_reg>;
};
-- 
2.15.1

Re: [PATCH 4/4 v2] Input: mms114 - add support for mms152

[Andi Shyti]

Hi Simon and Dmitry,

On Wed, Jan 24, 2018 at 01:32:01PM -0800, Dmitry Torokhov wrote:
> From: Simon Shields <si...@lineageos.org>
> 
> MMS152 has no configuration registers, but the packet format used in
> interrupts is identical to mms114.
> 
> Signed-off-by: Simon Shields <si...@lineageos.org>
> Patchwork-Id: 10125841
> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

also here

Reviewed-by: Andi Shyti <andi.sh...@samsung.com>
Tested-by: Andi Shyti <andi.sh...@samsung.com>

one small nitpick:

> @@ -239,14 +249,33 @@ static int mms114_get_version(struct mms114_data *data)
>  {
>   struct device *dev = >client->dev;
>   u8 buf[6];
> + int group;
>   int error;

do we really need to define a new 'group' variable?

Andi

> - error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
> - if (error < 0)
> - return error;
> + switch (data->type) {
> + case TYPE_MMS152:
> + error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
> + if (error)
> + return error;
> +
> + group = i2c_smbus_read_byte_data(data->client,
> +   MMS152_COMPAT_GROUP);
> + if (group < 0)
> + return group;
> +
> + dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 
> 0x%x, Compat group: %c\n",
> +  buf[0], buf[1], buf[2], group);
> + break;
> +
> + case TYPE_MMS114:
> + error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
> + if (error)
> + return error;
>  
> - dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 0x%x\n",
> -  buf[0], buf[1], buf[3]);
> + dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 
> 0x%x\n",
> +  buf[0], buf[1], buf[3]);
> + break;
> + }
>  
>   return 0;
>  }

Re: [PATCH 4/4 v2] Input: mms114 - add support for mms152

[Andi Shyti]

Hi Simon and Dmitry,

On Wed, Jan 24, 2018 at 01:32:01PM -0800, Dmitry Torokhov wrote:
> From: Simon Shields 
> 
> MMS152 has no configuration registers, but the packet format used in
> interrupts is identical to mms114.
> 
> Signed-off-by: Simon Shields 
> Patchwork-Id: 10125841
> Signed-off-by: Dmitry Torokhov 

also here

Reviewed-by: Andi Shyti 
Tested-by: Andi Shyti 

one small nitpick:

> @@ -239,14 +249,33 @@ static int mms114_get_version(struct mms114_data *data)
>  {
>   struct device *dev = >client->dev;
>   u8 buf[6];
> + int group;
>   int error;

do we really need to define a new 'group' variable?

Andi

> - error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
> - if (error < 0)
> - return error;
> + switch (data->type) {
> + case TYPE_MMS152:
> + error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
> + if (error)
> + return error;
> +
> + group = i2c_smbus_read_byte_data(data->client,
> +   MMS152_COMPAT_GROUP);
> + if (group < 0)
> + return group;
> +
> + dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 
> 0x%x, Compat group: %c\n",
> +  buf[0], buf[1], buf[2], group);
> + break;
> +
> + case TYPE_MMS114:
> + error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
> + if (error)
> + return error;
>  
> - dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 0x%x\n",
> -  buf[0], buf[1], buf[3]);
> + dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 
> 0x%x\n",
> +  buf[0], buf[1], buf[3]);
> + break;
> + }
>  
>   return 0;
>  }

Re: [PATCH 3/4] Input: mms114 - drop platform data and use generic APIs

[Andi Shyti]

Hi Simon and Dmitry,

On Wed, Jan 24, 2018 at 11:38:03AM -0800, Dmitry Torokhov wrote:
> From: Simon Shields <si...@lineageos.org>
> 
> The MMS114 platform data has no in-tree users, so drop it.
> 
> Switch to using the standard touchscreen properties via
> touchscreen_parse_properties(), and move the old DT parsing code
> to use device_property_*() APIs.
> 
> Finally, use touchscreen_report_pos to report x/y coordinates
> and drop the custom x/y inversion code.
> 
> Signed-off-by: Simon Shields <si...@lineageos.org>
> Reviewed-by: Rob Herring <r...@kernel.org>
> Patchwork-Id: 10162101
> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

yes, looks better. I'm happy you dropped the
(mms114_parse_dt(data) < 0).

Reviewed-by: Andi Shyti <andi.sh...@samsung.com>
Tested-by: Andi Shyti <andi.sh...@samsung.com>

Thanks,
Andi

> ---
>  .../bindings/input/touchscreen/mms114.txt  |  29 ++--
>  drivers/input/touchscreen/mms114.c | 147 
> +
>  include/linux/platform_data/mms114.h   |  24 
>  3 files changed, 82 insertions(+), 118 deletions(-)
>  delete mode 100644 include/linux/platform_data/mms114.h
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt 
> b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
> index 89d4c56c56711..8f9f9f38eff4a 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
> @@ -4,14 +4,18 @@ Required properties:
>  - compatible: must be "melfas,mms114"
>  - reg: I2C address of the chip
>  - interrupts: interrupt to which the chip is connected
> -- x-size: horizontal resolution of touchscreen
> -- y-size: vertical resolution of touchscreen
> +- touchscreen-size-x: See [1]
> +- touchscreen-size-y: See [1]
>  
>  Optional properties:
> -- contact-threshold:
> -- moving-threshold:
> -- x-invert: invert X axis
> -- y-invert: invert Y axis
> +- touchscreen-fuzz-x: See [1]
> +- touchscreen-fuzz-y: See [1]
> +- touchscreen-fuzz-pressure: See [1]
> +- touchscreen-inverted-x: See [1]
> +- touchscreen-inverted-y: See [1]
> +- touchscreen-swapped-x-y: See [1]
> +
> +[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>  
>  Example:
>  
> @@ -22,12 +26,13 @@ Example:
>   compatible = "melfas,mms114";
>   reg = <0x48>;
>   interrupts = <39 0>;
> - x-size = <720>;
> - y-size = <1280>;
> - contact-threshold = <10>;
> - moving-threshold = <10>;
> - x-invert;
> - y-invert;
> + touchscreen-size-x = <720>;
> + touchscreen-size-y = <1280>;
> + touchscreen-fuzz-x = <10>;
> + touchscreen-fuzz-y = <10>;
> + touchscreen-fuzz-pressure = <10>;
> + touchscreen-inverted-x;
> + touchscreen-inverted-y;
>   };
>  
>   /* ... */
> diff --git a/drivers/input/touchscreen/mms114.c 
> b/drivers/input/touchscreen/mms114.c
> index c3480db5d21ed..69e4288bf8aa3 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -12,8 +12,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> -#include 
>  #include 
>  #include 
>  
> @@ -55,7 +55,9 @@ struct mms114_data {
>   struct input_dev*input_dev;
>   struct regulator*core_reg;
>   struct regulator*io_reg;
> - const struct mms114_platform_data   *pdata;
> + struct touchscreen_properties props;
> + unsigned intcontact_threshold;
> + unsigned intmoving_threshold;
>  
>   /* Use cache data for mode control register(write only) */
>   u8  cache_mode_control;
> @@ -143,7 +145,6 @@ static int mms114_write_reg(struct mms114_data *data, 
> unsigned int reg,
>  
>  static void mms114_process_mt(struct mms114_data *data, struct mms114_touch 
> *touch)
>  {
> - const struct mms114_platform_data *pdata = data->pdata;
>   struct i2c_client *client = data->client;
>   struct input_dev *input_dev = data->input_dev;
>   unsigned int id;
> @@ -163,16 +164,6 @@ static void mms114_process_mt(struct mms114_data *data, 
> struct mms114_touch *tou
>   id = touch->id - 1;
>   x = touch->x_l

Re: [PATCH 3/4] Input: mms114 - drop platform data and use generic APIs

[Andi Shyti]

Hi Simon and Dmitry,

On Wed, Jan 24, 2018 at 11:38:03AM -0800, Dmitry Torokhov wrote:
> From: Simon Shields 
> 
> The MMS114 platform data has no in-tree users, so drop it.
> 
> Switch to using the standard touchscreen properties via
> touchscreen_parse_properties(), and move the old DT parsing code
> to use device_property_*() APIs.
> 
> Finally, use touchscreen_report_pos to report x/y coordinates
> and drop the custom x/y inversion code.
> 
> Signed-off-by: Simon Shields 
> Reviewed-by: Rob Herring 
> Patchwork-Id: 10162101
> Signed-off-by: Dmitry Torokhov 

yes, looks better. I'm happy you dropped the
(mms114_parse_dt(data) < 0).

Reviewed-by: Andi Shyti 
Tested-by: Andi Shyti 

Thanks,
Andi

> ---
>  .../bindings/input/touchscreen/mms114.txt  |  29 ++--
>  drivers/input/touchscreen/mms114.c | 147 
> +
>  include/linux/platform_data/mms114.h   |  24 
>  3 files changed, 82 insertions(+), 118 deletions(-)
>  delete mode 100644 include/linux/platform_data/mms114.h
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt 
> b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
> index 89d4c56c56711..8f9f9f38eff4a 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
> @@ -4,14 +4,18 @@ Required properties:
>  - compatible: must be "melfas,mms114"
>  - reg: I2C address of the chip
>  - interrupts: interrupt to which the chip is connected
> -- x-size: horizontal resolution of touchscreen
> -- y-size: vertical resolution of touchscreen
> +- touchscreen-size-x: See [1]
> +- touchscreen-size-y: See [1]
>  
>  Optional properties:
> -- contact-threshold:
> -- moving-threshold:
> -- x-invert: invert X axis
> -- y-invert: invert Y axis
> +- touchscreen-fuzz-x: See [1]
> +- touchscreen-fuzz-y: See [1]
> +- touchscreen-fuzz-pressure: See [1]
> +- touchscreen-inverted-x: See [1]
> +- touchscreen-inverted-y: See [1]
> +- touchscreen-swapped-x-y: See [1]
> +
> +[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>  
>  Example:
>  
> @@ -22,12 +26,13 @@ Example:
>   compatible = "melfas,mms114";
>   reg = <0x48>;
>   interrupts = <39 0>;
> - x-size = <720>;
> - y-size = <1280>;
> - contact-threshold = <10>;
> - moving-threshold = <10>;
> - x-invert;
> - y-invert;
> + touchscreen-size-x = <720>;
> + touchscreen-size-y = <1280>;
> + touchscreen-fuzz-x = <10>;
> + touchscreen-fuzz-y = <10>;
> + touchscreen-fuzz-pressure = <10>;
> + touchscreen-inverted-x;
> + touchscreen-inverted-y;
>   };
>  
>   /* ... */
> diff --git a/drivers/input/touchscreen/mms114.c 
> b/drivers/input/touchscreen/mms114.c
> index c3480db5d21ed..69e4288bf8aa3 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -12,8 +12,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> -#include 
>  #include 
>  #include 
>  
> @@ -55,7 +55,9 @@ struct mms114_data {
>   struct input_dev*input_dev;
>   struct regulator*core_reg;
>   struct regulator*io_reg;
> - const struct mms114_platform_data   *pdata;
> + struct touchscreen_properties props;
> + unsigned intcontact_threshold;
> + unsigned intmoving_threshold;
>  
>   /* Use cache data for mode control register(write only) */
>   u8  cache_mode_control;
> @@ -143,7 +145,6 @@ static int mms114_write_reg(struct mms114_data *data, 
> unsigned int reg,
>  
>  static void mms114_process_mt(struct mms114_data *data, struct mms114_touch 
> *touch)
>  {
> - const struct mms114_platform_data *pdata = data->pdata;
>   struct i2c_client *client = data->client;
>   struct input_dev *input_dev = data->input_dev;
>   unsigned int id;
> @@ -163,16 +164,6 @@ static void mms114_process_mt(struct mms114_data *data, 
> struct mms114_touch *tou
>   id = touch->id - 1;
>   x = touch->x_lo | touch->x_hi << 8;
>   y = touch->y_lo | touch->y_hi << 8;
> - if (x > pdata->x_size || y > pdata->y_size) {
> - 

Re: [PATCH 2/4] Input: mms114 - mark as direct input device

[Andi Shyti]

Hi Dmitry,

On Wed, Jan 24, 2018 at 11:38:02AM -0800, Dmitry Torokhov wrote:
> mms14 is a touchscreen and thus a direct input device; let's mark it
> as such. This also allows us to drop some initialization code as
> input_init_mt_slots() will do that for us.
> 
> Also add error handling for input_mt_init_slots().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

Reviewed-by: Andi Shyti <andi.sh...@samsung.com>
Tested-by: Andi Shyti <andi.sh...@samsung.com>

Thanks,
Andi

Re: [PATCH 2/4] Input: mms114 - mark as direct input device

[Andi Shyti]

Hi Dmitry,

On Wed, Jan 24, 2018 at 11:38:02AM -0800, Dmitry Torokhov wrote:
> mms14 is a touchscreen and thus a direct input device; let's mark it
> as such. This also allows us to drop some initialization code as
> input_init_mt_slots() will do that for us.
> 
> Also add error handling for input_mt_init_slots().
> 
> Signed-off-by: Dmitry Torokhov 

Reviewed-by: Andi Shyti 
Tested-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH 1/4] Input: mms114 - do not clobber interrupt trigger

[Andi Shyti]

Hi Dmitry,

On Wed, Jan 24, 2018 at 11:38:01AM -0800, Dmitry Torokhov wrote:
> Rely on the platform (device tree, ACPI, etc) to properly configure
> interrupt trigger/polarity instead of hardcoding the falling edge.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

Reviewed-by: Andi Shyti <andi.sh...@samsung.com>
Tested-by: Andi Shyti <andi.sh...@samsung.com>

Thanks,
Andi

Re: [PATCH 1/4] Input: mms114 - do not clobber interrupt trigger

[Andi Shyti]

Hi Dmitry,

On Wed, Jan 24, 2018 at 11:38:01AM -0800, Dmitry Torokhov wrote:
> Rely on the platform (device tree, ACPI, etc) to properly configure
> interrupt trigger/polarity instead of hardcoding the falling edge.
> 
> Signed-off-by: Dmitry Torokhov 

Reviewed-by: Andi Shyti 
Tested-by: Andi Shyti 

Thanks,
Andi

Re: [PATCH] Input: edt-ft5x06 - fix error handling for factory mode on non-M06

[Andi Shyti]

Hi Dmitry,

On Tue, Jan 23, 2018 at 10:52:36AM -0800, Dmitry Torokhov wrote:
> When attempting enter factory mode on firmware that does not support it,
> we'd error out, but leave the device with interrupts disabled, and thus
> touch not working. Fix it by moving the check before we disable
> interrupts/allocate memory for debug buffers.
> 
> Fixes: fd335ab04b3f ("Input: edt-ft5x06 - add support for M09 firmware 
> version")
> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

makes sense!

Reviewed-by: Andi Shyti <a...@etezian.org>

Andi

> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c 
> b/drivers/input/touchscreen/edt-ft5x06.c
> index c53a3d7239e72..1e18ca0d1b4e1 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -511,6 +511,12 @@ static int edt_ft5x06_factory_mode(struct 
> edt_ft5x06_ts_data *tsdata)
>   int ret;
>   int error;
>  
> + if (tsdata->version != EDT_M06) {
> + dev_err(>dev,
> + "No factory mode support for non-M06 devices\n");
> + return -EINVAL;
> + }
> +
>   disable_irq(client->irq);
>  
>   if (!tsdata->raw_buffer) {
> @@ -524,9 +530,6 @@ static int edt_ft5x06_factory_mode(struct 
> edt_ft5x06_ts_data *tsdata)
>   }
>  
>   /* mode register is 0x3c when in the work mode */
> - if (tsdata->version != EDT_M06)
> - goto m09_out;
> -
>   error = edt_ft5x06_register_write(tsdata, WORK_REGISTER_OPMODE, 0x03);
>   if (error) {
>   dev_err(>dev,
> @@ -559,11 +562,6 @@ static int edt_ft5x06_factory_mode(struct 
> edt_ft5x06_ts_data *tsdata)
>   enable_irq(client->irq);
>  
>   return error;
> -
> -m09_out:
> - dev_err(>dev, "No factory mode support for 
> M09/M12/GENERIC_FT\n");
> - return -EINVAL;
> -
>  }
>  
>  static int edt_ft5x06_work_mode(struct edt_ft5x06_ts_data *tsdata)
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 
> 
> -- 
> Dmitry

Re: [PATCH] Input: edt-ft5x06 - fix error handling for factory mode on non-M06

[Andi Shyti]

Hi Dmitry,

On Tue, Jan 23, 2018 at 10:52:36AM -0800, Dmitry Torokhov wrote:
> When attempting enter factory mode on firmware that does not support it,
> we'd error out, but leave the device with interrupts disabled, and thus
> touch not working. Fix it by moving the check before we disable
> interrupts/allocate memory for debug buffers.
> 
> Fixes: fd335ab04b3f ("Input: edt-ft5x06 - add support for M09 firmware 
> version")
> Signed-off-by: Dmitry Torokhov 

makes sense!

Reviewed-by: Andi Shyti 

Andi

> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c 
> b/drivers/input/touchscreen/edt-ft5x06.c
> index c53a3d7239e72..1e18ca0d1b4e1 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -511,6 +511,12 @@ static int edt_ft5x06_factory_mode(struct 
> edt_ft5x06_ts_data *tsdata)
>   int ret;
>   int error;
>  
> + if (tsdata->version != EDT_M06) {
> + dev_err(>dev,
> + "No factory mode support for non-M06 devices\n");
> + return -EINVAL;
> + }
> +
>   disable_irq(client->irq);
>  
>   if (!tsdata->raw_buffer) {
> @@ -524,9 +530,6 @@ static int edt_ft5x06_factory_mode(struct 
> edt_ft5x06_ts_data *tsdata)
>   }
>  
>   /* mode register is 0x3c when in the work mode */
> - if (tsdata->version != EDT_M06)
> - goto m09_out;
> -
>   error = edt_ft5x06_register_write(tsdata, WORK_REGISTER_OPMODE, 0x03);
>   if (error) {
>   dev_err(>dev,
> @@ -559,11 +562,6 @@ static int edt_ft5x06_factory_mode(struct 
> edt_ft5x06_ts_data *tsdata)
>   enable_irq(client->irq);
>  
>   return error;
> -
> -m09_out:
> - dev_err(>dev, "No factory mode support for 
> M09/M12/GENERIC_FT\n");
> - return -EINVAL;
> -
>  }
>  
>  static int edt_ft5x06_work_mode(struct edt_ft5x06_ts_data *tsdata)
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 
> 
> -- 
> Dmitry

Re: [PATCH] Input: edt-ft5x06: Delete an error message for a failed memory allocation in edt_ft5x06_ts_probe()

[Andi Shyti]

Hi Markus,

On Sun, Jan 21, 2018 at 08:19:00PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Sun, 21 Jan 2018 20:10:03 +0100
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Acked-by: Andi Shyti <a...@etezian.org>

Thanks,
Andi

> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c 
> b/drivers/input/touchscreen/edt-ft5x06.c
> index c53a3d7239e7..9d2799c90150 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -978,10 +978,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>   dev_dbg(>dev, "probing for EDT FT5x06 I2C\n");
>  
>   tsdata = devm_kzalloc(>dev, sizeof(*tsdata), GFP_KERNEL);
> - if (!tsdata) {
> - dev_err(>dev, "failed to allocate driver data.\n");
> + if (!tsdata)
>   return -ENOMEM;
> - }
>  
>   chip_data = of_device_get_match_data(>dev);
>   if (!chip_data)
> -- 
> 2.16.0

Re: [PATCH] Input: edt-ft5x06: Delete an error message for a failed memory allocation in edt_ft5x06_ts_probe()

[Andi Shyti]

Hi Markus,

On Sun, Jan 21, 2018 at 08:19:00PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sun, 21 Jan 2018 20:10:03 +0100
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 

Acked-by: Andi Shyti 

Thanks,
Andi

> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c 
> b/drivers/input/touchscreen/edt-ft5x06.c
> index c53a3d7239e7..9d2799c90150 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -978,10 +978,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>   dev_dbg(>dev, "probing for EDT FT5x06 I2C\n");
>  
>   tsdata = devm_kzalloc(>dev, sizeof(*tsdata), GFP_KERNEL);
> - if (!tsdata) {
> - dev_err(>dev, "failed to allocate driver data.\n");
> + if (!tsdata)
>   return -ENOMEM;
> - }
>  
>   chip_data = of_device_get_match_data(>dev);
>   if (!chip_data)
> -- 
> 2.16.0

Re: [PATCH] Input: stmfts,s6sy671 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

On Fri, Jan 05, 2018 at 08:49:58AM -0800, Dmitry Torokhov wrote:
> Hi Andi,
> 
> On Fri, Jan 05, 2018 at 06:57:15PM +0900, Andi Shyti wrote:
> > Hi Dmitry,
> > 
> > this is a kind ping, would you also mind giving me a feedback to
> 
> Yes, sorry. Could you please split the patch for each driver
> individually? Also, until we have an update to the CodingStyle doc
> mandating the C++ style comments, I'd prefer keeping the original style
> of comments. So // for the SPDX line and /* */ for the rest.

I was actually following Linus guideline [1] [2]. I also had the
same discussion previously in another context [3].

Please let me know if you still want a mixed style of comments in
the next patch.

Thanks,
Andi

[1] https://marc.info/?l=linux-kernel=151163713125320=2
...
└-> https://marc.info/?l=linux-kernel=151163867325641=2
...
└-> https://marc.info/?l=linux-kernel=151163867325641=2

[2] https://marc.info/?l=linux-kernel=150964359922353
[3] https://marc.info/?l=linux-kernel=151312974503109=2

Re: [PATCH] Input: stmfts,s6sy671 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

On Fri, Jan 05, 2018 at 08:49:58AM -0800, Dmitry Torokhov wrote:
> Hi Andi,
> 
> On Fri, Jan 05, 2018 at 06:57:15PM +0900, Andi Shyti wrote:
> > Hi Dmitry,
> > 
> > this is a kind ping, would you also mind giving me a feedback to
> 
> Yes, sorry. Could you please split the patch for each driver
> individually? Also, until we have an update to the CodingStyle doc
> mandating the C++ style comments, I'd prefer keeping the original style
> of comments. So // for the SPDX line and /* */ for the rest.

I was actually following Linus guideline [1] [2]. I also had the
same discussion previously in another context [3].

Please let me know if you still want a mixed style of comments in
the next patch.

Thanks,
Andi

[1] https://marc.info/?l=linux-kernel=151163713125320=2
...
└-> https://marc.info/?l=linux-kernel=151163867325641=2
...
└-> https://marc.info/?l=linux-kernel=151163867325641=2

[2] https://marc.info/?l=linux-kernel=150964359922353
[3] https://marc.info/?l=linux-kernel=151312974503109=2

Re: [PATCH] Input: stmfts,s6sy671 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

this is a kind ping, would you also mind giving me a feedback to
all the previous patches I sent?

Thanks,
Andi

On Tue, Dec 12, 2017 at 04:41:49PM +0900, Andi Shyti wrote:
> Replace the original license statement with the SPDX identifier.
> 
> Update also the copyright owner adding myself as co-owner of the
> copyright.
> 
> Signed-off-by: Andi Shyti <andi.sh...@samsung.com>
> ---
>  drivers/input/touchscreen/s6sy761.c | 15 +--
>  drivers/input/touchscreen/stmfts.c  | 15 +--
>  2 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/s6sy761.c 
> b/drivers/input/touchscreen/s6sy761.c
> index 26b1cb8a88ec..675efa93d444 100644
> --- a/drivers/input/touchscreen/s6sy761.c
> +++ b/drivers/input/touchscreen/s6sy761.c
> @@ -1,13 +1,8 @@
> -/*
> - * Copyright (c) 2017 Samsung Electronics Co., Ltd.
> - * Author: Andi Shyti <andi.sh...@samsung.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * Samsung S6SY761 Touchscreen device driver
> - */
> +// SPDX-License-Identifier: GPL-2.0
> +// Samsung S6SY761 Touchscreen device driver
> +//
> +// Copyright (c) 2017 Samsung Electronics Co., Ltd.
> +// Copyright (c) 2017 Andi Shyti <andi.sh...@samsung.com>
>  
>  #include 
>  #include 
> diff --git a/drivers/input/touchscreen/stmfts.c 
> b/drivers/input/touchscreen/stmfts.c
> index c12d01899939..2a123e20a42e 100644
> --- a/drivers/input/touchscreen/stmfts.c
> +++ b/drivers/input/touchscreen/stmfts.c
> @@ -1,13 +1,8 @@
> -/*
> - * Copyright (c) 2017 Samsung Electronics Co., Ltd.
> - * Author: Andi Shyti <andi.sh...@samsung.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * STMicroelectronics FTS Touchscreen device driver
> - */
> +// SPDX-License-Identifier: GPL-2.0
> +// STMicroelectronics FTS Touchscreen device driver
> +//
> +// Copyright (c) 2017 Samsung Electronics Co., Ltd.
> +// Copyright (c) 2017 Andi Shyti <andi.sh...@samsung.com>
>  
>  #include 
>  #include 
> -- 
> 2.15.1
> 

Re: [PATCH] Input: stmfts,s6sy671 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

this is a kind ping, would you also mind giving me a feedback to
all the previous patches I sent?

Thanks,
Andi

On Tue, Dec 12, 2017 at 04:41:49PM +0900, Andi Shyti wrote:
> Replace the original license statement with the SPDX identifier.
> 
> Update also the copyright owner adding myself as co-owner of the
> copyright.
> 
> Signed-off-by: Andi Shyti 
> ---
>  drivers/input/touchscreen/s6sy761.c | 15 +--
>  drivers/input/touchscreen/stmfts.c  | 15 +--
>  2 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/s6sy761.c 
> b/drivers/input/touchscreen/s6sy761.c
> index 26b1cb8a88ec..675efa93d444 100644
> --- a/drivers/input/touchscreen/s6sy761.c
> +++ b/drivers/input/touchscreen/s6sy761.c
> @@ -1,13 +1,8 @@
> -/*
> - * Copyright (c) 2017 Samsung Electronics Co., Ltd.
> - * Author: Andi Shyti 
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * Samsung S6SY761 Touchscreen device driver
> - */
> +// SPDX-License-Identifier: GPL-2.0
> +// Samsung S6SY761 Touchscreen device driver
> +//
> +// Copyright (c) 2017 Samsung Electronics Co., Ltd.
> +// Copyright (c) 2017 Andi Shyti 
>  
>  #include 
>  #include 
> diff --git a/drivers/input/touchscreen/stmfts.c 
> b/drivers/input/touchscreen/stmfts.c
> index c12d01899939..2a123e20a42e 100644
> --- a/drivers/input/touchscreen/stmfts.c
> +++ b/drivers/input/touchscreen/stmfts.c
> @@ -1,13 +1,8 @@
> -/*
> - * Copyright (c) 2017 Samsung Electronics Co., Ltd.
> - * Author: Andi Shyti 
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * STMicroelectronics FTS Touchscreen device driver
> - */
> +// SPDX-License-Identifier: GPL-2.0
> +// STMicroelectronics FTS Touchscreen device driver
> +//
> +// Copyright (c) 2017 Samsung Electronics Co., Ltd.
> +// Copyright (c) 2017 Andi Shyti 
>  
>  #include 
>  #include 
> -- 
> 2.15.1
> 

Re: [PATCH v3 1/6] media: rc: update sunxi-ir driver to get base clock frequency from devicetree

[Andi Shyti]

Hi Philipp,

On Tue, Dec 19, 2017 at 09:07:42AM +0100, Philipp Rossak wrote:
> This patch updates the sunxi-ir driver to set the base clock frequency from
> devicetree.
> 
> This is necessary since there are different ir receivers on the
> market, that operate with different frequencies. So this value could be
> set if the attached ir receiver needs a different base clock frequency,
> than the default 8 MHz.
> 
> Signed-off-by: Philipp Rossak <embe...@gmail.com>

feel free to add

Reviewed-by: Andi Shyti <andi.sh...@samsung.com>

Andi

Re: [PATCH v3 1/6] media: rc: update sunxi-ir driver to get base clock frequency from devicetree

[Andi Shyti]

Hi Philipp,

On Tue, Dec 19, 2017 at 09:07:42AM +0100, Philipp Rossak wrote:
> This patch updates the sunxi-ir driver to set the base clock frequency from
> devicetree.
> 
> This is necessary since there are different ir receivers on the
> market, that operate with different frequencies. So this value could be
> set if the attached ir receiver needs a different base clock frequency,
> than the default 8 MHz.
> 
> Signed-off-by: Philipp Rossak 

feel free to add

Reviewed-by: Andi Shyti 

Andi