Re: pcm|dmaengine|imx-sdma race condition on i.MX6

[Richard Leitner]

On Fri, Aug 21, 2020 at 09:21:37AM +, Robin Gong wrote:
> On 2020/08/21 12:34 Richard Leitner  wrote: 
> > On Thu, Aug 20, 2020 at 03:01:44PM +, Robin Gong wrote:
> > > On 2020/08/19 22:26 Benjamin Bara - SKIDATA 
> > wrote:
> > > >
> > > > @Robin:
> > > > Is it possible to tag the commits for the stable-tree
> > > > Cc: sta...@vger.kernel.org?
> > > Could my patch work in your side? If yes, I will add
> > > Cc: sta...@vger.kernel.org
> > 
> > I've tested the patches 3 & 4 (removing sdmac->context_loaded) of the series
> > you mentioned and sent Tested-by tags for them [1,2], as they fix the EIO
> > problems for our use case.
> > 
> > So from our side they are fine for stable.
> > 
> Okay, I thought that's just decrease the issue in your side not totally fix, 
> and the patch

As Benjamin mentioned the issue isn't "fixed" for us from the logical/
technical side.
Nonetheless the EIO error won't occur anymore with the patches applied.
Therefore the issue is for me "fixed from a userspace point of view", as
they don't get/see the error anymore.

> I post in https://www.spinics.net/lists/arm-kernel/msg829972.html
> could resolve the potential next descriptor wrongly freed by 
> vchan_get_all_descriptors
> in sdma_channel_terminate_work. Anyway, I'll add ' Cc: 
> sta...@vger.kernel.org' and
> your Tested-by tags in 3&4, then resend it again, thanks.

Great. Thank you!

regards;rl

Re: pcm|dmaengine|imx-sdma race condition on i.MX6

[Richard Leitner]

On Thu, Aug 20, 2020 at 03:01:44PM +, Robin Gong wrote:
> On 2020/08/19 22:26 Benjamin Bara - SKIDATA  
> wrote: 
> > 
> > @Robin:
> > Is it possible to tag the commits for the stable-tree
> > Cc: sta...@vger.kernel.org?
> Could my patch work in your side? If yes, I will add
> Cc: sta...@vger.kernel.org 

I've tested the patches 3 & 4 (removing sdmac->context_loaded) of the
series you mentioned and sent Tested-by tags for them [1,2], as they
fix the EIO problems for our use case.

So from our side they are fine for stable.

[1] https://lore.kernel.org/dmaengine/20200817053813.GA551027@pcleri/T/#u
[2] https://lore.kernel.org/dmaengine/20200817053820.GB551027@pcleri/T/#u

regards;rl

Re: [PATCH v3] Add two new configuration drivers for Microchip USB hubs

[Richard Leitner]

On Mon, Jul 27, 2020 at 10:33:29AM +0200, Christian Eggers wrote:
> On Sonday, greg k-h wrote:
> > Please resend the whole series, not just a single patch, as it makes it
> > very difficult to pick the "correct" patches to be applied...

Hi Christian,
sorry for the late reply. My MUA somehow didn't show me that series
earlier...

I haven't looked into the patches in detail, but at a first glance it
looks like a lot copy-n-paste.
Have you thought about merging the (after your series) 3 hub drivers
into one? Something like a "microchip i2c usb hub driver"?
Would that be feasible for your point of view?

regards;rl

> 
> Changes in v3:
>  - none (only resend the whole series)
> 
> Changes in v2:
>  - added property description for ocs-min-width-ms
>  - fixed property description for oc-delay-ns
> 

Re: pcm|dmaengine|imx-sdma race condition on i.MX6

[Richard Leitner]

On Fri, Aug 14, 2020 at 11:18:10AM +0200, Robin Gong wrote:
> 
> On 2020/08/13 19:23: Richard Leitner  wrote:
> > Hi,
> > we've found a race condition with the PCM on the i.MX6 which results 
> > in an -EIO for the SNDRV_PCM_IOCTL_READI_FRAMES ioctl after an -EPIPE 
> > (XRUN).
> > 
> > A possible reproduction may look like the following reduced call graph 
> > during a PCM capture:
> > 
> > us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES)
> >   - wait_for_avail()
> > - schedule_timeout()
> >-> snd_pcm_update_hw_ptr0()
> >   - snd_pcm_update_state: EPIPE (XRUN)
> >   - sdma_disable_channel_async() # get's scheduled away due to sleep us
> > <- ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns -EPIPE
> > us -> ioctl(SNDRV_PCM_IOCTL_PREPARE) # as reaction to the EPIPE (XRUN)
> > us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) # next try to capture frames
> >   - sdma_prep_dma_cyclic()
> > - sdma_load_context() # not loaded as context_loaded is 1
> >   - wait_for_avail()
> > - schedule_timeout()
> > # now the sdma_channel_terminate_work() comes back and sets # 
> > context_loaded = false and frees in vchan_dma_desc_free_list().
> > us <- ioctl returns -EIO (capture write error (DMA or IRQ trouble?))
>
> Seems the write error caused by context_loaded not set to false before
> next transfer start? If yes, please have a try with the 03/04 of the below
> patch set, anyway, could you post your failure log?
> https://lkml.org/lkml/2020/8/11/111

Thanks for the pointer to those patches. I've tested them on top of the
v5.8 tag during the weekend. It seems those patches are mitigiating
the described EIO issue.

Nonetheless IMHO this patches are not fixing the root cause of the
problem (unsynchronized sdma_disable_channel_async / sdma_prep_dma_cyclic).
Do you (or somebody else) have any suggestions/comments/objections on that?

regards;rl

> 
> > 
> > 
> > What we have found out, based on our understanding:
> > The dmaengine docu states that a dmaengine_terminate_async() must be 
> > followed by a dmaengine_synchronize().
> > However, in the pcm_dmaengine.c, only dmaengine_terminate_async() is 
> > called (for performance reasons and because it might be called from an 
> > interrupt handler).
> > 
> > In our tests, we saw that the user-space immediately calls
> > ioctl(SNDRV_PCM_IOCTL_PREPARE) as a handler for the happened xrun 
> > (previous ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns with -EPIPE). In 
> > our case (imx-sdma.c), the terminate really happens asynchronously 
> > with a worker thread which is not awaited/synchronized by the
> > ioctl(SNDRV_PCM_IOCTL_PREPARE) call.
> > 
> > Since the syscall immediately enters an atomic context 
> > (snd_pcm_stream_lock_irq()), we are not able to flush the work of the 
> > termination worker from within the DMA context. This leads to an 
> > unterminated DMA getting re-initialized and then terminated.
> > 
> > On the i.MX6 platform the problem is (if I got it correctly) that the
> > sdma_channel_terminate_work() called after the -EPIPE gets scheduled 
> > away (for the 1-2ms sleep [1]). During that time the userspace already 
> > sends in the
> > ioctl(SNDRV_PCM_IOCTL_PREPARE) and
> > ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
> > As none of them are anyhow synchronized to the terminate_worker the
> > vchan_dma_desc_free_list() [2] and "sdmac->context_loaded = false;" 
> > [3] are executed during the wait_for_avail() [4] of the 
> > ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
> > 
> > To make sure we identified the problem correctly we've tested to add a 
> > "dmaengine_synchronize()" before the snd_pcm_prepare() in [5]. This 
> > fixed the race condition in all our tests. (Before we were able to 
> > reproduce it in 100% of the test runs).
> > 
> > Based on our understanding, there are two different points to ensure 
> > the
> > termination:
> > Either ensure that the termination is finished within the previous 
> > SNDRV_PCM_IOCTL_READI_FRAMES call (inside the DMA context) or 
> > finishing it in the SNDRV_PCM_IOCTL_PREPARE call (and all other 
> > applicable ioclts) before entering the atomic context (from the PCM 
> > context).
> > 
> > We initially thought about implementing the first approach, basically 
> > splitting up the dma_device terminate_all operation into a sync
> > (busy-wait) and a async one. This would align the operations with the 
> > DMAengine interface and would enable a sync termination variant from 
> > atomic contexts.
> > However, we saw that the dma_free_attrs() function has a WARN_ON on 
> > irqs disabled, which would be the case for the sync variant.
> > Side note: We found this issue on the current v5.4.y LTS branch, but 
> > it also affects v5.8.y.
> > 
> > Any feedback or pointers how we may fix the problem are warmly welcome!
> > If anything is unclear please just ask :-)
> > 
> > regards;
> > Richard Leitner
> > Benjamin Bara

Re: [PATCH v12 04/12] dmaengine: imx-sdma: remove duplicated sdma_load_context

[Richard Leitner]

On Tue, Aug 11, 2020 at 11:53:43PM +0800, Robin Gong wrote:
> Since sdma_transfer_init() will do sdma_load_context before any
> sdma transfer, no need once more in sdma_config_channel().
> 
> Signed-off-by: Robin Gong 
> Acked-by: Vinod Koul 

Hi Robin,
thanks for the pointer to this patch.

As you suggested I've tested the two patches on my custom i.MX6DL
board. Therefore please feel free to add:

Tested-by: Richard Leitner 

regards;rl

> ---
>  drivers/dma/imx-sdma.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index d305b80..5411e01e 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1137,7 +1137,6 @@ static void sdma_set_watermarklevel_for_p2p(struct 
> sdma_channel *sdmac)
>  static int sdma_config_channel(struct dma_chan *chan)
>  {
>   struct sdma_channel *sdmac = to_sdma_chan(chan);
> - int ret;
>  
>   sdma_disable_channel(chan);
>  
> @@ -1177,9 +1176,7 @@ static int sdma_config_channel(struct dma_chan *chan)
>   sdmac->watermark_level = 0; /* FIXME: M3_BASE_ADDRESS */
>   }
>  
> - ret = sdma_load_context(sdmac);
> -
> - return ret;
> + return 0;
>  }
>  
>  static int sdma_set_channel_priority(struct sdma_channel *sdmac,
> -- 
> 2.7.4
> 
> 

-- 
Richard Leitner
Hardware R&D / Senior Embedded Linux Engineer

SKIDATA | Driving Your Digital Future

SKIDATA GmbH
Untersbergstraße 40 | 5083 Grödig/Salzburg | Austria
[t] +43-6246-888-4245 | [m] +43-664-88616370
[w] www.skidata.com

Re: [PATCH v12 03/12] Revert "dmaengine: imx-sdma: refine to load context only once"

[Richard Leitner]

On Tue, Aug 11, 2020 at 11:53:42PM +0800, Robin Gong wrote:
> This reverts commit ad0d92d7ba6aecbe2705907c38ff8d8be4da1e9c, because
> in spi-imx case, burst length may be changed dynamically.
> 
> Signed-off-by: Robin Gong 
> Acked-by: Sascha Hauer 

Hi Robin,
thanks for the pointer to this patch.

As you suggested I've tested the two patches on my custom i.MX6DL
board. Therefore please feel free to add:

Tested-by: Richard Leitner 

regards;rl

> ---
>  drivers/dma/imx-sdma.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 270992c..d305b80 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -377,7 +377,6 @@ struct sdma_channel {
>   unsigned long   watermark_level;
>   u32 shp_addr, per_addr;
>   enum dma_status status;
> - boolcontext_loaded;
>   struct imx_dma_data data;
>   struct work_struct  terminate_worker;
>  };
> @@ -984,9 +983,6 @@ static int sdma_load_context(struct sdma_channel *sdmac)
>   int ret;
>   unsigned long flags;
>  
> - if (sdmac->context_loaded)
> - return 0;
> -
>   if (sdmac->direction == DMA_DEV_TO_MEM)
>   load_address = sdmac->pc_from_device;
>   else if (sdmac->direction == DMA_DEV_TO_DEV)
> @@ -1029,8 +1025,6 @@ static int sdma_load_context(struct sdma_channel *sdmac)
>  
>   spin_unlock_irqrestore(&sdma->channel_0_lock, flags);
>  
> - sdmac->context_loaded = true;
> -
>   return ret;
>  }
>  
> @@ -1069,7 +1063,6 @@ static void sdma_channel_terminate_work(struct 
> work_struct *work)
>   vchan_get_all_descriptors(&sdmac->vc, &head);
>   spin_unlock_irqrestore(&sdmac->vc.lock, flags);
>   vchan_dma_desc_free_list(&sdmac->vc, &head);
> - sdmac->context_loaded = false;
>  }
>  
>  static int sdma_terminate_all(struct dma_chan *chan)
> @@ -1337,7 +1330,6 @@ static void sdma_free_chan_resources(struct dma_chan 
> *chan)
>  
>   sdmac->event_id0 = 0;
>   sdmac->event_id1 = 0;
> - sdmac->context_loaded = false;
>  
>   sdma_set_channel_priority(sdmac, 0);
>  
> -- 
> 2.7.4
> 
> 

-- 
Richard Leitner
Hardware R&D / Senior Embedded Linux Engineer

SKIDATA | Driving Your Digital Future

SKIDATA GmbH
Untersbergstraße 40 | 5083 Grödig/Salzburg | Austria
[t] +43-6246-888-4245 | [m] +43-664-88616370
[w] www.skidata.com

pcm|dmaengine|imx-sdma race condition on i.MX6

[Richard Leitner]

Hi,
we've found a race condition with the PCM on the i.MX6 which results in
an -EIO for the SNDRV_PCM_IOCTL_READI_FRAMES ioctl after an -EPIPE (XRUN).

A possible reproduction may look like the following reduced call graph
during a PCM capture:

us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES)
  - wait_for_avail()
- schedule_timeout()
   -> snd_pcm_update_hw_ptr0()
  - snd_pcm_update_state: EPIPE (XRUN)
  - sdma_disable_channel_async() # get's scheduled away due to sleep
us <- ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns -EPIPE
us -> ioctl(SNDRV_PCM_IOCTL_PREPARE) # as reaction to the EPIPE (XRUN)
us -> ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) # next try to capture frames
  - sdma_prep_dma_cyclic()
- sdma_load_context() # not loaded as context_loaded is 1
  - wait_for_avail()
- schedule_timeout()
# now the sdma_channel_terminate_work() comes back and sets
# context_loaded = false and frees in vchan_dma_desc_free_list().
us <- ioctl returns -EIO (capture write error (DMA or IRQ trouble?))


What we have found out, based on our understanding:
The dmaengine docu states that a dmaengine_terminate_async() must be
followed by a dmaengine_synchronize().
However, in the pcm_dmaengine.c, only dmaengine_terminate_async() is
called (for performance reasons and because it might be called from an
interrupt handler).

In our tests, we saw that the user-space immediately calls
ioctl(SNDRV_PCM_IOCTL_PREPARE) as a handler for the happened xrun
(previous ioctl(SNDRV_PCM_IOCTL_READI_FRAMES) returns with -EPIPE). In
our case (imx-sdma.c), the terminate really happens asynchronously with
a worker thread which is not awaited/synchronized by the
ioctl(SNDRV_PCM_IOCTL_PREPARE) call.

Since the syscall immediately enters an atomic context
(snd_pcm_stream_lock_irq()), we are not able to flush the work of the
termination worker from within the DMA context. This leads to an
unterminated DMA getting re-initialized and then terminated.

On the i.MX6 platform the problem is (if I got it correctly) that the
sdma_channel_terminate_work() called after the -EPIPE gets scheduled
away (for the 1-2ms sleep [1]). During that time the userspace already
sends in the ioctl(SNDRV_PCM_IOCTL_PREPARE) and
ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).
As none of them are anyhow synchronized to the terminate_worker the
vchan_dma_desc_free_list() [2] and "sdmac->context_loaded = false;" [3]
are executed during the wait_for_avail() [4] of the
ioctl(SNDRV_PCM_IOCTL_READI_FRAMES).

To make sure we identified the problem correctly we've tested to add a
"dmaengine_synchronize()" before the snd_pcm_prepare() in [5]. This
fixed the race condition in all our tests. (Before we were able to
reproduce it in 100% of the test runs).

Based on our understanding, there are two different points to ensure
the termination:
Either ensure that the termination is finished within the previous
SNDRV_PCM_IOCTL_READI_FRAMES call (inside the DMA context) or finishing
it in the SNDRV_PCM_IOCTL_PREPARE call (and all other applicable ioclts)
before entering the atomic context (from the PCM context).

We initially thought about implementing the first approach, basically
splitting up the dma_device terminate_all operation into a sync
(busy-wait) and a async one. This would align the operations with the
DMAengine interface and would enable a sync termination variant from
atomic contexts.
However, we saw that the dma_free_attrs() function has a WARN_ON on irqs
disabled, which would be the case for the sync variant.

Side note: We found this issue on the current v5.4.y LTS branch,
but it also affects v5.8.y.

Any feedback or pointers how we may fix the problem are warmly welcome!
If anything is unclear please just ask :-)

regards;
Richard Leitner
Benjamin Bara

[1]https://elixir.bootlin.com/linux/v5.8/source/drivers/dma/imx-sdma.c#L1066
[2]https://elixir.bootlin.com/linux/v5.8/source/drivers/dma/imx-sdma.c#L1071
[3]https://elixir.bootlin.com/linux/v5.8/source/drivers/dma/imx-sdma.c#L1072
[4]https://elixir.bootlin.com/linux/v5.8/source/sound/core/pcm_lib.c#L1825
[5]https://elixir.bootlin.com/linux/v5.8/source/sound/core/pcm_native.c#L3226

Re: [PATCH 5.3 112/112] ASoC: sgtl5000: add ADC mute control

[Richard Leitner]

On 17/10/2019 01:23, Greg Kroah-Hartman wrote:

On Wed, Oct 16, 2019 at 11:35:18PM +0100, Mark Brown wrote:

On Wed, Oct 16, 2019 at 03:10:25PM -0700, Greg Kroah-Hartman wrote:

On Wed, Oct 16, 2019 at 11:00:44PM +0100, Mark Brown wrote:

On Wed, Oct 16, 2019 at 02:51:44PM -0700, Greg Kroah-Hartman wrote:

From: Oleksandr Suvorov 



commit 694b14554d75f2a1ae111202e71860d58b434a21 upstream.



This control mute/unmute the ADC input of SGTL5000
using its CHIP_ANA_CTRL register.



This seems like a new feature and not an obvious candidate for stable?



there was a long email from Richard that said:
Upstream commit 631bc8f0134a ("ASoC: sgtl5000: Fix of unmute
outputs on probe"), which is e9f621efaebd in v5.3 replaced
snd_soc_component_write with snd_soc_component_update_bits and
therefore no longer cleared the MUTE_ADC flag. This caused the
ADC to stay muted and recording doesn't work any longer. This
patch fixes this problem by adding a Switch control for
MUTE_ADC.



That's why I took this.  If this isn't true, I'll be glad to drop this.


That's probably not an appropriate fix for stable - it's going to add a
new control which users will need to manually set (or hope their
userspace automatically figures out that it should set for them, more
advanced userspaces like PulseAudio should) which isn't a drop in fix.
You could either drop the backport that was done for zero cross or take
a new patch that clears the MUTE_ADC flag (rather than punting to
userspace to do so), or just be OK with what you've got at the minute
which might be fine given the lack of user reports.


Ok, I'll gladly go drop it, thanks!


Mark, thanks for the clarification! I haven't thought of breaking 
anything with the backport as it worked fine for our application.


Greg, just to be sure:

Are you going to drop this patch and revert e9f621efaebd for v5.3?

Or should I send a patch which clears the MUTE_ADC flag like Mark
suggested?

thanks & regards;Richard.L

[PATCH v5.3] ASoC: sgtl5000: add ADC mute control

[Richard Leitner]

From: Oleksandr Suvorov 

Upstream commit 631bc8f0134a ("ASoC: sgtl5000: Fix of unmute outputs on
probe"), which is e9f621efaebd in v5.3 replaced snd_soc_component_write
with snd_soc_component_update_bits and therefore no longer cleared the
MUTE_ADC flag. This caused the ADC to stay muted and recording doesn't
work any longer. This patch fixes this problem by adding a Switch control
for MUTE_ADC.

commit 694b14554d75 ("ASoC: sgtl5000: add ADC mute control") upstream

This control mute/unmute the ADC input of SGTL5000
using its CHIP_ANA_CTRL register.

Signed-off-by: Oleksandr Suvorov 
Reviewed-by: Marcel Ziswiler 
Reviewed-by: Igor Opaniuk 
Reviewed-by: Fabio Estevam 
Link: 
https://lore.kernel.org/r/20190719100524.23300-5-oleksandr.suvo...@toradex.com
Signed-off-by: Mark Brown 
Signed-off-by: Richard Leitner 
Fixes: e9f621efaebd ("ASoC: sgtl5000: Fix of unmute outputs on probe")
---
 sound/soc/codecs/sgtl5000.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 3f28e7862b5b..b65232521ea8 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -720,6 +720,7 @@ static const struct snd_kcontrol_new 
sgtl5000_snd_controls[] = {
SGTL5000_CHIP_ANA_ADC_CTRL,
8, 1, 0, capture_6db_attenuate),
SOC_SINGLE("Capture ZC Switch", SGTL5000_CHIP_ANA_CTRL, 1, 1, 0),
+   SOC_SINGLE("Capture Switch", SGTL5000_CHIP_ANA_CTRL, 0, 1, 1),
 
SOC_DOUBLE_TLV("Headphone Playback Volume",
SGTL5000_CHIP_ANA_HP_CTRL,
-- 
2.21.0

[PATCH v2 4/4] rtc: s35390a: change FLAG defines to use BIT macro

[Richard Leitner]

To be consistent change the S35390A_FLAG defines to use the BIT
macro (like the S35390A_INT2_MODE defines).

Signed-off-by: Richard Leitner 
---
 drivers/rtc/rtc-s35390a.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 7293dcdea692..b18ce5610a94 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -36,14 +36,14 @@
 #define S35390A_ALRM_BYTE_MINS 2
 
 /* flags for STATUS1 */
-#define S35390A_FLAG_POC   0x01
-#define S35390A_FLAG_BLD   0x02
-#define S35390A_FLAG_INT2  0x04
-#define S35390A_FLAG_24H   0x40
-#define S35390A_FLAG_RESET 0x80
+#define S35390A_FLAG_POC   BIT(0)
+#define S35390A_FLAG_BLD   BIT(1)
+#define S35390A_FLAG_INT2  BIT(2)
+#define S35390A_FLAG_24H   BIT(6)
+#define S35390A_FLAG_RESET BIT(7)
 
 /* flag for STATUS2 */
-#define S35390A_FLAG_TEST  0x01
+#define S35390A_FLAG_TEST  BIT(0)
 
 /* INT2 pin output mode */
 #define S35390A_INT2_MODE_MASK 0x0E
-- 
2.20.1

[PATCH v2 2/4] rtc: s35390a: set uie_unsupported

[Richard Leitner]

Alarms are only supported on a per minute basis. This is why
uie_unsupported is set. Furthermore issue a warning when a second based
alarm is requested.

Signed-off-by: Richard Leitner 
---
 drivers/rtc/rtc-s35390a.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index fb795c454077..4ca37f281ed9 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -289,6 +289,9 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct 
rtc_wkalrm *alm)
alm->time.tm_min, alm->time.tm_hour, alm->time.tm_mday,
alm->time.tm_mon, alm->time.tm_year, alm->time.tm_wday);
 
+   if (alm->time.tm_sec != 0)
+   dev_warn(&client->dev, "Alarms are only supported on a per 
minute basis!\n");
+
/* disable interrupt (which deasserts the irq line) */
err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
if (err < 0)
@@ -500,6 +503,9 @@ static int s35390a_probe(struct i2c_client *client,
goto exit_dummy;
}
 
+   /* supports per-minute alarms only, therefore set uie_unsupported */
+   s35390a->rtc->uie_unsupported = 1;
+
if (status1 & S35390A_FLAG_INT2)
rtc_update_irq(s35390a->rtc, 1, RTC_AF);
 
-- 
2.20.1

[PATCH v2 0/4] rtc: s35390a: uie_unsupported and minor fixes

[Richard Leitner]

As the s35390a does only support per-minute based alarms we have to
set the uie_unsupported flag. Otherwise it delays for 10sec and 
fails afterwards with modern hwclock versions.

Furthermore some other minor changes are made.

All patches were tested on an i.MX6 platform.

CHANGES v2:
 - use BIT in "clarify INT2 pin output modes"
 - add "change FLAG defines to use BIT macro"

Richard Leitner (4):
  rtc: s35390a: clarify INT2 pin output modes
  rtc: s35390a: set uie_unsupported
  rtc: s35390a: introduce struct device in probe
  rtc: s35390a: change FLAG defines to use BIT macro

 drivers/rtc/rtc-s35390a.c | 55 +--
 1 file changed, 29 insertions(+), 26 deletions(-)

-- 
2.20.1

[PATCH v2 3/4] rtc: s35390a: introduce struct device in probe

[Richard Leitner]

To simplify access and shorten code introduce a struct device pointer in
the s35390a probe function.

Signed-off-by: Richard Leitner 
---
 drivers/rtc/rtc-s35390a.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 4ca37f281ed9..7293dcdea692 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -436,14 +436,14 @@ static int s35390a_probe(struct i2c_client *client,
unsigned int i;
struct s35390a *s35390a;
char buf, status1;
+   struct device *dev = &client->dev;
 
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
err = -ENODEV;
goto exit;
}
 
-   s35390a = devm_kzalloc(&client->dev, sizeof(struct s35390a),
-   GFP_KERNEL);
+   s35390a = devm_kzalloc(dev, sizeof(struct s35390a), GFP_KERNEL);
if (!s35390a) {
err = -ENOMEM;
goto exit;
@@ -457,8 +457,8 @@ static int s35390a_probe(struct i2c_client *client,
s35390a->client[i] = i2c_new_dummy(client->adapter,
client->addr + i);
if (!s35390a->client[i]) {
-   dev_err(&client->dev, "Address %02x unavailable\n",
-   client->addr + i);
+   dev_err(dev, "Address %02x unavailable\n",
+   client->addr + i);
err = -EBUSY;
goto exit_dummy;
}
@@ -467,7 +467,7 @@ static int s35390a_probe(struct i2c_client *client,
err_read = s35390a_read_status(s35390a, &status1);
if (err_read < 0) {
err = err_read;
-   dev_err(&client->dev, "error resetting chip\n");
+   dev_err(dev, "error resetting chip\n");
goto exit_dummy;
}
 
@@ -481,22 +481,21 @@ static int s35390a_probe(struct i2c_client *client,
buf = 0;
err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
if (err < 0) {
-   dev_err(&client->dev, "error disabling alarm");
+   dev_err(dev, "error disabling alarm");
goto exit_dummy;
}
} else {
err = s35390a_disable_test_mode(s35390a);
if (err < 0) {
-   dev_err(&client->dev, "error disabling test mode\n");
+   dev_err(dev, "error disabling test mode\n");
goto exit_dummy;
}
}
 
-   device_set_wakeup_capable(&client->dev, 1);
+   device_set_wakeup_capable(dev, 1);
 
-   s35390a->rtc = devm_rtc_device_register(&client->dev,
-   s35390a_driver.driver.name,
-   &s35390a_rtc_ops, THIS_MODULE);
+   s35390a->rtc = devm_rtc_device_register(dev, s35390a_driver.driver.name,
+   &s35390a_rtc_ops, THIS_MODULE);
 
if (IS_ERR(s35390a->rtc)) {
err = PTR_ERR(s35390a->rtc);
-- 
2.20.1

[PATCH v2 1/4] rtc: s35390a: clarify INT2 pin output modes

[Richard Leitner]

Fix the INT2 mode mask to not include the "TEST" flag. Furthermore
remove the not needed reversion of bits when parsing the INT2 modes.
Instead reverse the INT2_MODE defines to match the bit order from the
datasheet.

Additionally mention the flag names from the datasheet for the different
modes in the comments.

Signed-off-by: Richard Leitner 
---
 drivers/rtc/rtc-s35390a.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 3c64dbb08109..fb795c454077 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -45,12 +45,13 @@
 /* flag for STATUS2 */
 #define S35390A_FLAG_TEST  0x01
 
-#define S35390A_INT2_MODE_MASK 0xF0
-
+/* INT2 pin output mode */
+#define S35390A_INT2_MODE_MASK 0x0E
 #define S35390A_INT2_MODE_NOINTR   0x00
-#define S35390A_INT2_MODE_FREQ 0x10
-#define S35390A_INT2_MODE_ALARM0x40
-#define S35390A_INT2_MODE_PMIN_EDG 0x20
+#define S35390A_INT2_MODE_ALARMBIT(1) /* INT2AE */
+#define S35390A_INT2_MODE_PMIN_EDG BIT(2) /* INT2ME */
+#define S35390A_INT2_MODE_FREQ BIT(3) /* INT2FE */
+#define S35390A_INT2_MODE_PMIN (BIT(3) | BIT(2)) /* INT2FE | INT2ME */
 
 static const struct i2c_device_id s35390a_id[] = {
{ "s35390a", 0 },
@@ -303,9 +304,6 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct 
rtc_wkalrm *alm)
else
sts = S35390A_INT2_MODE_NOINTR;
 
-   /* This chip expects the bits of each byte to be in reverse order */
-   sts = bitrev8(sts);
-
/* set interupt mode*/
err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
if (err < 0)
@@ -343,7 +341,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, 
struct rtc_wkalrm *alm)
if (err < 0)
return err;
 
-   if ((bitrev8(sts) & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) 
{
+   if ((sts & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) {
/*
 * When the alarm isn't enabled, the register to configure
 * the alarm time isn't accessible.
-- 
2.20.1

Re: [PATCH 1/3] rtc: s35390a: clarify INT2 pin output modes

[Richard Leitner]

On 21/05/2019 17:54, Alexandre Belloni wrote:

Hello,

This seems good to me but...

On 21/05/2019 16:20:22+0200, Richard Leitner wrote:



--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -45,12 +45,13 @@
  /* flag for STATUS2 */
  #define S35390A_FLAG_TEST 0x01
  
-#define S35390A_INT2_MODE_MASK		0xF0

-
+/* INT2 pin output mode */
+#define S35390A_INT2_MODE_MASK 0x0E
  #define S35390A_INT2_MODE_NOINTR  0x00
-#define S35390A_INT2_MODE_FREQ 0x10
-#define S35390A_INT2_MODE_ALARM0x40
-#define S35390A_INT2_MODE_PMIN_EDG 0x20
+#define S35390A_INT2_MODE_ALARM0x02 /* INT2AE */
+#define S35390A_INT2_MODE_PMIN_EDG 0x04 /* INT2ME */
+#define S35390A_INT2_MODE_FREQ 0x08 /* INT2FE */
+#define S35390A_INT2_MODE_PMIN 0x0C /* INT2ME | INT2FE */
  


While you are at it you may as well use BIT().


Sure, will change that for v2.

regards;Richard.L

[PATCH 2/3] rtc: s35390a: set uie_unsupported

[Richard Leitner]

Alarms are only supported on a per minute basis. This is why
uie_unsupported is set. Furthermore issue a warning when a second based
alarm is requested.

Signed-off-by: Richard Leitner 
---
 drivers/rtc/rtc-s35390a.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 6fb6d835b178..462407570750 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -289,6 +289,9 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct 
rtc_wkalrm *alm)
alm->time.tm_min, alm->time.tm_hour, alm->time.tm_mday,
alm->time.tm_mon, alm->time.tm_year, alm->time.tm_wday);
 
+   if (alm->time.tm_sec != 0)
+   dev_warn(&client->dev, "Alarms are only supported on a per 
minute basis!\n");
+
/* disable interrupt (which deasserts the irq line) */
err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
if (err < 0)
@@ -500,6 +503,9 @@ static int s35390a_probe(struct i2c_client *client,
goto exit_dummy;
}
 
+   /* supports per-minute alarms only, therefore set uie_unsupported */
+   s35390a->rtc->uie_unsupported = 1;
+
if (status1 & S35390A_FLAG_INT2)
rtc_update_irq(s35390a->rtc, 1, RTC_AF);
 
-- 
2.20.1

[PATCH 3/3] rtc: s35390a: introduce struct device in probe

[Richard Leitner]

To simplify access and shorten code introduce a struct device pointer in
the s35390a probe function.

Signed-off-by: Richard Leitner 
---
 drivers/rtc/rtc-s35390a.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 462407570750..5cc2d194645b 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -436,14 +436,14 @@ static int s35390a_probe(struct i2c_client *client,
unsigned int i;
struct s35390a *s35390a;
char buf, status1;
+   struct device *dev = &client->dev;
 
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
err = -ENODEV;
goto exit;
}
 
-   s35390a = devm_kzalloc(&client->dev, sizeof(struct s35390a),
-   GFP_KERNEL);
+   s35390a = devm_kzalloc(dev, sizeof(struct s35390a), GFP_KERNEL);
if (!s35390a) {
err = -ENOMEM;
goto exit;
@@ -457,8 +457,8 @@ static int s35390a_probe(struct i2c_client *client,
s35390a->client[i] = i2c_new_dummy(client->adapter,
client->addr + i);
if (!s35390a->client[i]) {
-   dev_err(&client->dev, "Address %02x unavailable\n",
-   client->addr + i);
+   dev_err(dev, "Address %02x unavailable\n",
+   client->addr + i);
err = -EBUSY;
goto exit_dummy;
}
@@ -467,7 +467,7 @@ static int s35390a_probe(struct i2c_client *client,
err_read = s35390a_read_status(s35390a, &status1);
if (err_read < 0) {
err = err_read;
-   dev_err(&client->dev, "error resetting chip\n");
+   dev_err(dev, "error resetting chip\n");
goto exit_dummy;
}
 
@@ -481,22 +481,21 @@ static int s35390a_probe(struct i2c_client *client,
buf = 0;
err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
if (err < 0) {
-   dev_err(&client->dev, "error disabling alarm");
+   dev_err(dev, "error disabling alarm");
goto exit_dummy;
}
} else {
err = s35390a_disable_test_mode(s35390a);
if (err < 0) {
-   dev_err(&client->dev, "error disabling test mode\n");
+   dev_err(dev, "error disabling test mode\n");
goto exit_dummy;
}
}
 
-   device_set_wakeup_capable(&client->dev, 1);
+   device_set_wakeup_capable(dev, 1);
 
-   s35390a->rtc = devm_rtc_device_register(&client->dev,
-   s35390a_driver.driver.name,
-   &s35390a_rtc_ops, THIS_MODULE);
+   s35390a->rtc = devm_rtc_device_register(dev, s35390a_driver.driver.name,
+   &s35390a_rtc_ops, THIS_MODULE);
 
if (IS_ERR(s35390a->rtc)) {
err = PTR_ERR(s35390a->rtc);
-- 
2.20.1

[PATCH 0/3] rtc: s35390a: uie_unsupported and minor fixes

[Richard Leitner]

As the s35390a does only support per-minute based alarms we have to
set the uie_unsupported flag. Otherwise it delays for 10sec and 
fails afterwards with modern hwclock versions.

Furthermore some other minor changes are made.

All patches were tested on an i.MX6 platform.

Richard Leitner (3):
  rtc: s35390a: clarify INT2 pin output modes
  rtc: s35390a: set uie_unsupported
  rtc: s35390a: introduce struct device in probe

 drivers/rtc/rtc-s35390a.c | 43 +--
 1 file changed, 23 insertions(+), 20 deletions(-)

-- 
2.20.1

[PATCH 1/3] rtc: s35390a: clarify INT2 pin output modes

[Richard Leitner]

Fix the INT2 mode mask to not include the "TEST" flag. Furthermore
remove the not needed reversion of bits when parsing the INT2 modes.
Instead reverse the INT2_MODE defines.

Additionally mention the flag names from the datasheet for the different
modes in the comments.

Signed-off-by: Richard Leitner 
---
 drivers/rtc/rtc-s35390a.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 3c64dbb08109..6fb6d835b178 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -45,12 +45,13 @@
 /* flag for STATUS2 */
 #define S35390A_FLAG_TEST  0x01
 
-#define S35390A_INT2_MODE_MASK 0xF0
-
+/* INT2 pin output mode */
+#define S35390A_INT2_MODE_MASK 0x0E
 #define S35390A_INT2_MODE_NOINTR   0x00
-#define S35390A_INT2_MODE_FREQ 0x10
-#define S35390A_INT2_MODE_ALARM0x40
-#define S35390A_INT2_MODE_PMIN_EDG 0x20
+#define S35390A_INT2_MODE_ALARM0x02 /* INT2AE */
+#define S35390A_INT2_MODE_PMIN_EDG 0x04 /* INT2ME */
+#define S35390A_INT2_MODE_FREQ 0x08 /* INT2FE */
+#define S35390A_INT2_MODE_PMIN 0x0C /* INT2ME | INT2FE */
 
 static const struct i2c_device_id s35390a_id[] = {
{ "s35390a", 0 },
@@ -303,9 +304,6 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct 
rtc_wkalrm *alm)
else
sts = S35390A_INT2_MODE_NOINTR;
 
-   /* This chip expects the bits of each byte to be in reverse order */
-   sts = bitrev8(sts);
-
/* set interupt mode*/
err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
if (err < 0)
@@ -343,7 +341,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, 
struct rtc_wkalrm *alm)
if (err < 0)
return err;
 
-   if ((bitrev8(sts) & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) 
{
+   if ((sts & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) {
/*
 * When the alarm isn't enabled, the register to configure
 * the alarm time isn't accessible.
-- 
2.20.1

Re: [RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Richard Leitner]

Hi Joe,

On 29/01/2019 06:40, Joe Perches wrote:

On Mon, 2019-01-28 at 16:25 -0800, Dmitry Torokhov wrote:

On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote:

Some of the #defined register values are one-bit flags. Convert them to
use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
readability and clarifies the intent.

Signed-off-by: Richard Leitner 


Applied, thank you.


Not so sure this should be applied.


diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c

[]

@@ -46,7 +47,7 @@

[]

  /* bits for I2C_REG_IRQSRC */
-#define IRQ_PENTOUCH_TOUCHCONVDONE 0x08
-#define IRQ_PENRELEASE 0x04
+#define IRQ_PENTOUCH_TOUCHCONVDONE BIT(7)
+#define IRQ_PENRELEASE BIT(6)


Shouldn't this be BIT(3) and BIT(2)
or did you mean to change the values too?

If so, this change should be noted in the commit message.



That's true, those values should stay the same. Thanks for the catch!

@Dimitry: Should I send an updated version or do you fix it yourself?

regards;Richard.L

Re: [RESEND PATCH v2 0/8] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Hi,
another 3 weeks and still no response :-(

The last comment on this patchset was in October by Dimitry/Rob. In the
meantime I did a "ping" and a resend...

So what's up? I'm perfectly fine with a "we don't want this is mainline
because XXX" or "I have no time and will come back to it in XXX"...

But please give at least some kind of response... Thanks!

regards;Richard.L

On 18/12/2018 09:35, Richard Leitner wrote:

Add reset-gpio, sx8654[056] and common of_touchscreen functions support
for the sx8654 driver.

Changes RESEND v2:
- added "Reviewed-by: Rob Herring " tags

Changes v2:
- use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF
- convert flags to BIT() in a separate patch
- replace hrtimer with "regular" timer
- use of_device_get_match_data instead of of_match_device
- add driver data to i2c_device_id table for non-DT fallback
- fix sequence of common touchscreen initialization
- div. minor stlye changes

Richard Leitner (8):
   dt-bindings: input: touchscreen: sx8654: add reset-gpio property
   Input: sx8654 - add reset-gpio support
   dt-bindings: input: touchscreen: sx8654: add compatible models
   Input: sx8654 - add sx8655 and sx8656 to compatibles
   dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
   Input: sx8654 - add sx8650 support
   Input: sx8654 - use common of_touchscreen functions
   Input: sx8654 - convert #defined flags to BIT(x)

  .../bindings/input/touchscreen/sx8654.txt  |  10 +-
  drivers/input/touchscreen/sx8654.c | 245 ++---
  2 files changed, 229 insertions(+), 26 deletions(-)

[RESEND PATCH v2 7/8] Input: sx8654 - use common of_touchscreen functions

[Richard Leitner]

of_touchscreen.c provides a common interface for a axis invertion and
swapping of touchscreens. Therefore use it in the sx8654 driver.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index 4939863efbef..b7b263ed52af 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -101,6 +102,7 @@ struct sx8654 {
 
spinlock_t  lock; /* for input reporting from irq/timer */
struct timer_list   timer;
+   struct touchscreen_properties props;
 
const struct sx865x_data *data;
 };
@@ -178,8 +180,7 @@ static irqreturn_t sx8650_irq(int irq, void *handle)
 chdata);
}
 
-   input_report_abs(ts->input, ABS_X, x);
-   input_report_abs(ts->input, ABS_Y, y);
+   touchscreen_report_pos(ts->input, &ts->props, x, y, false);
input_report_key(ts->input, BTN_TOUCH, 1);
input_sync(ts->input);
dev_dbg(dev, "point(%4d,%4d)\n", x, y);
@@ -226,8 +227,8 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
x = ((data[0] & 0xf) << 8) | (data[1]);
y = ((data[2] & 0xf) << 8) | (data[3]);
 
-   input_report_abs(sx8654->input, ABS_X, x);
-   input_report_abs(sx8654->input, ABS_Y, y);
+   touchscreen_report_pos(sx8654->input, &sx8654->props, x, y,
+  false);
input_report_key(sx8654->input, BTN_TOUCH, 1);
input_sync(sx8654->input);
 
@@ -377,6 +378,8 @@ static int sx8654_probe(struct i2c_client *client,
input_set_abs_params(input, ABS_X, 0, MAX_12BIT, 0, 0);
input_set_abs_params(input, ABS_Y, 0, MAX_12BIT, 0, 0);
 
+   touchscreen_parse_properties(input, false, &sx8654->props);
+
sx8654->client = client;
sx8654->input = input;
 
-- 
2.11.0

[RESEND PATCH v2 6/8] Input: sx8654 - add sx8650 support

[Richard Leitner]

The sx8654 and sx8650 are quite similar, therefore add support for the
sx8650 within the sx8654 driver.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 193 +
 1 file changed, 173 insertions(+), 20 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index afa4da138fe9..4939863efbef 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -29,7 +29,7 @@
 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -44,9 +44,11 @@
 #define I2C_REG_IRQSRC 0x23
 #define I2C_REG_SOFTRESET  0x3f
 
+#define I2C_REG_SX8650_STAT0x05
+#define SX8650_STAT_CONVIRQ0x80
+
 /* commands */
 #define CMD_READ_REGISTER  0x40
-#define CMD_MANUAL 0xc0
 #define CMD_PENTRG 0xe0
 
 /* value for I2C_REG_SOFTRESET */
@@ -58,6 +60,7 @@
 
 /* bits for RegTouch1 */
 #define CONDIRQ0x20
+#define RPDNT_100K 0x00
 #define FILT_7SA   0x03
 
 /* bits for I2C_REG_CHANMASK */
@@ -71,14 +74,122 @@
 /* power delay: lower nibble of CTRL0 register */
 #define POWDLY_1_1MS   0x0b
 
+/* for sx8650, as we have no pen release IRQ there: timeout in ns following the
+ * last PENIRQ after which we assume the pen is lifted.
+ */
+#define SX8650_PENIRQ_TIMEOUT  msecs_to_jiffies(10)
+
 #define MAX_12BIT  ((1 << 12) - 1)
+#define MAX_I2C_READ_LEN   10 /* see datasheet section 5.1.5 */
+
+/* channel definition */
+#define CH_X   0x00
+#define CH_Y   0x01
+
+struct sx865x_data {
+   u8 cmd_manual;
+   u8 chan_mask;
+   u8 has_irq_penrelease;
+   u8 has_reg_irqmask;
+   irq_handler_t irqh;
+};
 
 struct sx8654 {
struct input_dev *input;
struct i2c_client *client;
struct gpio_desc *gpio_reset;
+
+   spinlock_t  lock; /* for input reporting from irq/timer */
+   struct timer_list   timer;
+
+   const struct sx865x_data *data;
 };
 
+static inline void sx865x_penrelease(struct sx8654 *ts)
+{
+   struct input_dev *input_dev = ts->input;
+
+   input_report_key(input_dev, BTN_TOUCH, 0);
+   input_sync(input_dev);
+}
+
+static void sx865x_penrelease_timer_handler(struct timer_list *t)
+{
+   struct sx8654 *ts = from_timer(ts, t, timer);
+   unsigned long flags;
+
+   spin_lock_irqsave(&ts->lock, flags);
+   sx865x_penrelease(ts);
+   spin_unlock_irqrestore(&ts->lock, flags);
+   dev_dbg(&ts->client->dev, "penrelease by timer\n");
+}
+
+static irqreturn_t sx8650_irq(int irq, void *handle)
+{
+   struct sx8654 *ts = handle;
+   struct device *dev = &ts->client->dev;
+   int len, i;
+   unsigned long flags;
+   u8 stat;
+   u16 x, y;
+   u8 data[MAX_I2C_READ_LEN];
+   u16 ch;
+   u16 chdata;
+   u8 readlen = hweight32(ts->data->chan_mask) * 2;
+
+   stat = i2c_smbus_read_byte_data(ts->client, CMD_READ_REGISTER
+   | I2C_REG_SX8650_STAT);
+
+   if (!(stat & SX8650_STAT_CONVIRQ)) {
+   dev_dbg(dev, "%s ignore stat [0x%02x]", __func__, stat);
+   return IRQ_HANDLED;
+   }
+
+   len = i2c_master_recv(ts->client, data, readlen);
+   if (len != readlen) {
+   dev_dbg(dev, "ignore short recv (%d)\n", len);
+   return IRQ_HANDLED;
+   }
+
+   spin_lock_irqsave(&ts->lock, flags);
+
+   x = 0;
+   y = 0;
+   for (i = 0; (i + 1) < len; i++) {
+   chdata = data[i] << 8;
+   i++;
+   chdata += data[i];
+
+   if (unlikely(chdata == 0x)) {
+   dev_dbg(dev, "invalid qualified data @ %d\n", i);
+   continue;
+   } else if (unlikely(chdata & 0x8000)) {
+   dev_warn(dev, "hibit @ %d [0x%04x]\n", i, chdata);
+   continue;
+   }
+
+   ch = chdata >> 12;
+   if  (ch == CH_X)
+   x = chdata & MAX_12BIT;
+   else if (ch == CH_Y)
+   y = chdata & MAX_12BIT;
+   else
+   dev_warn(dev, "unknown channel %d [0x%04x]\n", ch,
+chdata);
+   }
+
+   input_report_abs(ts->input, ABS_X, x);
+   input_report_abs(ts->input, ABS_Y, y);
+   input_report_key(ts->input, BTN_TOUCH, 1);
+   input_sync(ts->input);
+   dev_dbg(dev, "point(%4d,%4d)\n", x, y);
+
+   mod_timer(&ts->timer, jiffies 

[RESEND PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Richard Leitner]

Some of the #defined register values are one-bit flags. Convert them to
use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
readability and clarifies the intent.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index b7b263ed52af..3746ea855f94 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -46,7 +47,7 @@
 #define I2C_REG_SOFTRESET  0x3f
 
 #define I2C_REG_SX8650_STAT0x05
-#define SX8650_STAT_CONVIRQ0x80
+#define SX8650_STAT_CONVIRQBIT(7)
 
 /* commands */
 #define CMD_READ_REGISTER  0x40
@@ -56,8 +57,8 @@
 #define SOFTRESET_VALUE0xde
 
 /* bits for I2C_REG_IRQSRC */
-#define IRQ_PENTOUCH_TOUCHCONVDONE 0x08
-#define IRQ_PENRELEASE 0x04
+#define IRQ_PENTOUCH_TOUCHCONVDONE BIT(7)
+#define IRQ_PENRELEASE BIT(6)
 
 /* bits for RegTouch1 */
 #define CONDIRQ0x20
@@ -65,8 +66,8 @@
 #define FILT_7SA   0x03
 
 /* bits for I2C_REG_CHANMASK */
-#define CONV_X 0x80
-#define CONV_Y 0x40
+#define CONV_X BIT(7)
+#define CONV_Y BIT(6)
 
 /* coordinates rate: higher nibble of CTRL0 register */
 #define RATE_MANUAL0x00
-- 
2.11.0

[RESEND PATCH v2 5/8] dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles

[Richard Leitner]

As the sx8650 is quite similar to the sx8654 support for it will be
added in the driver. Therefore add it to the compatibles.

Signed-off-by: Richard Leitner 
Reviewed-by: Rob Herring 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index a538678424dd..0ebe6dd043c7 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -2,6 +2,7 @@
 
 Required properties:
 - compatible: must be one of the following, depending on the model:
+   "semtech,sx8650"
"semtech,sx8654"
"semtech,sx8655"
"semtech,sx8656"
-- 
2.11.0

[RESEND PATCH v2 3/8] dt-bindings: input: touchscreen: sx8654: add compatible models

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner 
Reviewed-by: Rob Herring 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index ca521d8f7d65..a538678424dd 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -1,7 +1,10 @@
 * Semtech SX8654 I2C Touchscreen Controller
 
 Required properties:
-- compatible: must be "semtech,sx8654"
+- compatible: must be one of the following, depending on the model:
+   "semtech,sx8654"
+   "semtech,sx8655"
+   "semtech,sx8656"
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
-- 
2.11.0

[RESEND PATCH v2 4/8] Input: sx8654 - add sx8655 and sx8656 to compatibles

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index 238f56b1581b..afa4da138fe9 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -293,6 +293,8 @@ static int sx8654_probe(struct i2c_client *client,
 #ifdef CONFIG_OF
 static const struct of_device_id sx8654_of_match[] = {
{ .compatible = "semtech,sx8654", },
+   { .compatible = "semtech,sx8655", },
+   { .compatible = "semtech,sx8656", },
{ },
 };
 MODULE_DEVICE_TABLE(of, sx8654_of_match);
@@ -300,6 +302,8 @@ MODULE_DEVICE_TABLE(of, sx8654_of_match);
 
 static const struct i2c_device_id sx8654_id_table[] = {
{ "semtech_sx8654", 0 },
+   { "semtech_sx8655", 0 },
+   { "semtech_sx8656", 0 },
{ },
 };
 MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
-- 
2.11.0

[RESEND PATCH v2 2/8] Input: sx8654 - add reset-gpio support

[Richard Leitner]

The sx8654 features a NRST input which may be connected to a GPIO.
Therefore add support for hard-resetting the sx8654 via this NRST.

If the reset-gpio property is provided the sx8654 is resetted via NRST
instead of the soft-reset via I2C.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 40 +++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index ed29db3ec731..238f56b1581b 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -33,6 +33,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -74,6 +76,7 @@
 struct sx8654 {
struct input_dev *input;
struct i2c_client *client;
+   struct gpio_desc *gpio_reset;
 };
 
 static irqreturn_t sx8654_irq(int irq, void *handle)
@@ -124,6 +127,27 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
return IRQ_HANDLED;
 }
 
+static int sx8654_reset(struct sx8654 *ts)
+{
+   int err;
+
+   if (ts->gpio_reset) {
+   gpiod_set_value_cansleep(ts->gpio_reset, 1);
+   udelay(2); /* Tpulse > 1µs */
+   gpiod_set_value_cansleep(ts->gpio_reset, 0);
+
+   return 0;
+   }
+
+   dev_dbg(&ts->client->dev, "NRST unavailable, try softreset\n");
+   err = i2c_smbus_write_byte_data(ts->client, I2C_REG_SOFTRESET,
+   SOFTRESET_VALUE);
+   if (err)
+   return err;
+   else
+   return 0;
+}
+
 static int sx8654_open(struct input_dev *dev)
 {
struct sx8654 *sx8654 = input_get_drvdata(dev);
@@ -186,6 +210,17 @@ static int sx8654_probe(struct i2c_client *client,
if (!sx8654)
return -ENOMEM;
 
+   sx8654->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset",
+GPIOD_OUT_HIGH);
+   if (IS_ERR(sx8654->gpio_reset)) {
+   error = PTR_ERR(sx8654->gpio_reset);
+   if (error != -EPROBE_DEFER)
+   dev_err(&client->dev, "unable to get reset-gpio: %d\n",
+   error);
+   return error;
+   }
+   dev_dbg(&client->dev, "got GPIO reset pin\n");
+
input = devm_input_allocate_device(&client->dev);
if (!input)
return -ENOMEM;
@@ -206,10 +241,9 @@ static int sx8654_probe(struct i2c_client *client,
 
input_set_drvdata(sx8654->input, sx8654);
 
-   error = i2c_smbus_write_byte_data(client, I2C_REG_SOFTRESET,
- SOFTRESET_VALUE);
+   error = sx8654_reset(sx8654);
if (error) {
-   dev_err(&client->dev, "writing softreset value failed");
+   dev_err(&client->dev, "reset failed");
return error;
}
 
-- 
2.11.0

[RESEND PATCH v2 1/8] dt-bindings: input: touchscreen: sx8654: add reset-gpio property

[Richard Leitner]

Document the reset-gpio property for the sx8654 touchscreen controller
driver.

Signed-off-by: Richard Leitner 
Reviewed-by: Rob Herring 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index 4886c4aa2906..ca521d8f7d65 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -5,6 +5,9 @@ Required properties:
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
+Optional properties:
+ - reset-gpios: GPIO specification for the NRST input
+
 Example:
 
sx8654@48 {
@@ -12,4 +15,5 @@ Example:
reg = <0x48>;
interrupt-parent = <&gpio6>;
interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+   reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
};
-- 
2.11.0

[RESEND PATCH v2 0/8] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Add reset-gpio, sx8654[056] and common of_touchscreen functions support
for the sx8654 driver.

Changes RESEND v2:
- added "Reviewed-by: Rob Herring " tags

Changes v2:
- use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF
- convert flags to BIT() in a separate patch
- replace hrtimer with "regular" timer
- use of_device_get_match_data instead of of_match_device
- add driver data to i2c_device_id table for non-DT fallback
- fix sequence of common touchscreen initialization
- div. minor stlye changes

Richard Leitner (8):
  dt-bindings: input: touchscreen: sx8654: add reset-gpio property
  Input: sx8654 - add reset-gpio support
  dt-bindings: input: touchscreen: sx8654: add compatible models
  Input: sx8654 - add sx8655 and sx8656 to compatibles
  dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
  Input: sx8654 - add sx8650 support
  Input: sx8654 - use common of_touchscreen functions
  Input: sx8654 - convert #defined flags to BIT(x)

 .../bindings/input/touchscreen/sx8654.txt  |  10 +-
 drivers/input/touchscreen/sx8654.c | 245 ++---
 2 files changed, 229 insertions(+), 26 deletions(-)

-- 
2.11.0

Re: [PATCH v2 0/7] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Hi,
another friendly reminder for this patchset...

Any comments/objections?

regards;Richard.L

On 17.10.18 14:51, Richard Leitner wrote:
> Add reset-gpio, sx8654[056] and common of_touchscreen functions support
> for the sx8654 driver.
> 
> Changes v2:
>   - use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF
>   - convert flags to BIT() in a separate patch
>   - replace hrtimer with "regular" timer
>   - use of_device_get_match_data instead of of_match_device
>   - add driver data to i2c_device_id table for non-DT fallback
>   - fix sequence of common touchscreen initialization
>       - div. minor stlye changes
> 
> Richard Leitner (8):
>   dt-bindings: input: touchscreen: sx8654: add reset-gpio property
>   Input: sx8654 - add reset-gpio support
>   dt-bindings: input: touchscreen: sx8654: add compatible models
>   Input: sx8654 - add sx8655 and sx8656 to compatibles
>   dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
>   Input: sx8654 - add sx8650 support
>   Input: sx8654 - use common of_touchscreen functions
>   Input: sx8654 - convert #defined flags to BIT(x)
> 
>  .../bindings/input/touchscreen/sx8654.txt  |  10 +-
>  drivers/input/touchscreen/sx8654.c | 245 
> ++---
>  2 files changed, 229 insertions(+), 26 deletions(-)
> 

Re: [PATCH v2 0/7] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Hi,
friendly question for the status of this patchset of mine.

thanks®ards;Richard.L

On 10/17/18 2:51 PM, Richard Leitner wrote:

Add reset-gpio, sx8654[056] and common of_touchscreen functions support
for the sx8654 driver.

Changes v2:
- use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF
- convert flags to BIT() in a separate patch
- replace hrtimer with "regular" timer
- use of_device_get_match_data instead of of_match_device
- add driver data to i2c_device_id table for non-DT fallback
- fix sequence of common touchscreen initialization
- div. minor stlye changes

Richard Leitner (8):
   dt-bindings: input: touchscreen: sx8654: add reset-gpio property
   Input: sx8654 - add reset-gpio support
   dt-bindings: input: touchscreen: sx8654: add compatible models
   Input: sx8654 - add sx8655 and sx8656 to compatibles
   dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
   Input: sx8654 - add sx8650 support
   Input: sx8654 - use common of_touchscreen functions
   Input: sx8654 - convert #defined flags to BIT(x)

  .../bindings/input/touchscreen/sx8654.txt  |  10 +-
  drivers/input/touchscreen/sx8654.c | 245 ++---
  2 files changed, 229 insertions(+), 26 deletions(-)

[PATCH v2 4/8] Input: sx8654 - add sx8655 and sx8656 to compatibles

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index 238f56b1581b..afa4da138fe9 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -293,6 +293,8 @@ static int sx8654_probe(struct i2c_client *client,
 #ifdef CONFIG_OF
 static const struct of_device_id sx8654_of_match[] = {
{ .compatible = "semtech,sx8654", },
+   { .compatible = "semtech,sx8655", },
+   { .compatible = "semtech,sx8656", },
{ },
 };
 MODULE_DEVICE_TABLE(of, sx8654_of_match);
@@ -300,6 +302,8 @@ MODULE_DEVICE_TABLE(of, sx8654_of_match);
 
 static const struct i2c_device_id sx8654_id_table[] = {
{ "semtech_sx8654", 0 },
+   { "semtech_sx8655", 0 },
+   { "semtech_sx8656", 0 },
{ },
 };
 MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
-- 
2.11.0

[PATCH v2 3/8] dt-bindings: input: touchscreen: sx8654: add compatible models

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index ca521d8f7d65..a538678424dd 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -1,7 +1,10 @@
 * Semtech SX8654 I2C Touchscreen Controller
 
 Required properties:
-- compatible: must be "semtech,sx8654"
+- compatible: must be one of the following, depending on the model:
+   "semtech,sx8654"
+   "semtech,sx8655"
+   "semtech,sx8656"
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
-- 
2.11.0

[PATCH v2 1/8] dt-bindings: input: touchscreen: sx8654: add reset-gpio property

[Richard Leitner]

Document the reset-gpio property for the sx8654 touchscreen controller
driver.

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index 4886c4aa2906..ca521d8f7d65 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -5,6 +5,9 @@ Required properties:
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
+Optional properties:
+ - reset-gpios: GPIO specification for the NRST input
+
 Example:
 
sx8654@48 {
@@ -12,4 +15,5 @@ Example:
reg = <0x48>;
interrupt-parent = <&gpio6>;
interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+   reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
};
-- 
2.11.0

[PATCH v2 2/8] Input: sx8654 - add reset-gpio support

[Richard Leitner]

The sx8654 features a NRST input which may be connected to a GPIO.
Therefore add support for hard-resetting the sx8654 via this NRST.

If the reset-gpio property is provided the sx8654 is resetted via NRST
instead of the soft-reset via I2C.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 40 +++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index ed29db3ec731..238f56b1581b 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -33,6 +33,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -74,6 +76,7 @@
 struct sx8654 {
struct input_dev *input;
struct i2c_client *client;
+   struct gpio_desc *gpio_reset;
 };
 
 static irqreturn_t sx8654_irq(int irq, void *handle)
@@ -124,6 +127,27 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
return IRQ_HANDLED;
 }
 
+static int sx8654_reset(struct sx8654 *ts)
+{
+   int err;
+
+   if (ts->gpio_reset) {
+   gpiod_set_value_cansleep(ts->gpio_reset, 1);
+   udelay(2); /* Tpulse > 1µs */
+   gpiod_set_value_cansleep(ts->gpio_reset, 0);
+
+   return 0;
+   }
+
+   dev_dbg(&ts->client->dev, "NRST unavailable, try softreset\n");
+   err = i2c_smbus_write_byte_data(ts->client, I2C_REG_SOFTRESET,
+   SOFTRESET_VALUE);
+   if (err)
+   return err;
+   else
+   return 0;
+}
+
 static int sx8654_open(struct input_dev *dev)
 {
struct sx8654 *sx8654 = input_get_drvdata(dev);
@@ -186,6 +210,17 @@ static int sx8654_probe(struct i2c_client *client,
if (!sx8654)
return -ENOMEM;
 
+   sx8654->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset",
+GPIOD_OUT_HIGH);
+   if (IS_ERR(sx8654->gpio_reset)) {
+   error = PTR_ERR(sx8654->gpio_reset);
+   if (error != -EPROBE_DEFER)
+   dev_err(&client->dev, "unable to get reset-gpio: %d\n",
+   error);
+   return error;
+   }
+   dev_dbg(&client->dev, "got GPIO reset pin\n");
+
input = devm_input_allocate_device(&client->dev);
if (!input)
return -ENOMEM;
@@ -206,10 +241,9 @@ static int sx8654_probe(struct i2c_client *client,
 
input_set_drvdata(sx8654->input, sx8654);
 
-   error = i2c_smbus_write_byte_data(client, I2C_REG_SOFTRESET,
- SOFTRESET_VALUE);
+   error = sx8654_reset(sx8654);
if (error) {
-   dev_err(&client->dev, "writing softreset value failed");
+   dev_err(&client->dev, "reset failed");
return error;
}
 
-- 
2.11.0

[PATCH v2 0/7] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Add reset-gpio, sx8654[056] and common of_touchscreen functions support
for the sx8654 driver.

Changes v2:
- use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF
- convert flags to BIT() in a separate patch
- replace hrtimer with "regular" timer
- use of_device_get_match_data instead of of_match_device
- add driver data to i2c_device_id table for non-DT fallback
- fix sequence of common touchscreen initialization
- div. minor stlye changes

Richard Leitner (8):
  dt-bindings: input: touchscreen: sx8654: add reset-gpio property
  Input: sx8654 - add reset-gpio support
  dt-bindings: input: touchscreen: sx8654: add compatible models
  Input: sx8654 - add sx8655 and sx8656 to compatibles
  dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
  Input: sx8654 - add sx8650 support
  Input: sx8654 - use common of_touchscreen functions
  Input: sx8654 - convert #defined flags to BIT(x)

 .../bindings/input/touchscreen/sx8654.txt  |  10 +-
 drivers/input/touchscreen/sx8654.c | 245 ++---
 2 files changed, 229 insertions(+), 26 deletions(-)

-- 
2.11.0

[PATCH v2 7/8] Input: sx8654 - use common of_touchscreen functions

[Richard Leitner]

of_touchscreen.c provides a common interface for a axis invertion and
swapping of touchscreens. Therefore use it in the sx8654 driver.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index 4939863efbef..b7b263ed52af 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -101,6 +102,7 @@ struct sx8654 {
 
spinlock_t  lock; /* for input reporting from irq/timer */
struct timer_list   timer;
+   struct touchscreen_properties props;
 
const struct sx865x_data *data;
 };
@@ -178,8 +180,7 @@ static irqreturn_t sx8650_irq(int irq, void *handle)
 chdata);
}
 
-   input_report_abs(ts->input, ABS_X, x);
-   input_report_abs(ts->input, ABS_Y, y);
+   touchscreen_report_pos(ts->input, &ts->props, x, y, false);
input_report_key(ts->input, BTN_TOUCH, 1);
input_sync(ts->input);
dev_dbg(dev, "point(%4d,%4d)\n", x, y);
@@ -226,8 +227,8 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
x = ((data[0] & 0xf) << 8) | (data[1]);
y = ((data[2] & 0xf) << 8) | (data[3]);
 
-   input_report_abs(sx8654->input, ABS_X, x);
-   input_report_abs(sx8654->input, ABS_Y, y);
+   touchscreen_report_pos(sx8654->input, &sx8654->props, x, y,
+  false);
input_report_key(sx8654->input, BTN_TOUCH, 1);
input_sync(sx8654->input);
 
@@ -377,6 +378,8 @@ static int sx8654_probe(struct i2c_client *client,
input_set_abs_params(input, ABS_X, 0, MAX_12BIT, 0, 0);
input_set_abs_params(input, ABS_Y, 0, MAX_12BIT, 0, 0);
 
+   touchscreen_parse_properties(input, false, &sx8654->props);
+
sx8654->client = client;
sx8654->input = input;
 
-- 
2.11.0

[PATCH v2 8/8] Input: sx8654 - convert #defined flags to BIT(x)

[Richard Leitner]

Some of the #defined register values are one-bit flags. Convert them to
use the BIT(x) macro instead of 1 byte hexadecimal values. This improves
readability and clarifies the intent.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index b7b263ed52af..3746ea855f94 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -46,7 +47,7 @@
 #define I2C_REG_SOFTRESET  0x3f
 
 #define I2C_REG_SX8650_STAT0x05
-#define SX8650_STAT_CONVIRQ0x80
+#define SX8650_STAT_CONVIRQBIT(7)
 
 /* commands */
 #define CMD_READ_REGISTER  0x40
@@ -56,8 +57,8 @@
 #define SOFTRESET_VALUE0xde
 
 /* bits for I2C_REG_IRQSRC */
-#define IRQ_PENTOUCH_TOUCHCONVDONE 0x08
-#define IRQ_PENRELEASE 0x04
+#define IRQ_PENTOUCH_TOUCHCONVDONE BIT(7)
+#define IRQ_PENRELEASE BIT(6)
 
 /* bits for RegTouch1 */
 #define CONDIRQ0x20
@@ -65,8 +66,8 @@
 #define FILT_7SA   0x03
 
 /* bits for I2C_REG_CHANMASK */
-#define CONV_X 0x80
-#define CONV_Y 0x40
+#define CONV_X BIT(7)
+#define CONV_Y BIT(6)
 
 /* coordinates rate: higher nibble of CTRL0 register */
 #define RATE_MANUAL0x00
-- 
2.11.0

[PATCH v2 6/8] Input: sx8654 - add sx8650 support

[Richard Leitner]

The sx8654 and sx8650 are quite similar, therefore add support for the
sx8650 within the sx8654 driver.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 193 +
 1 file changed, 173 insertions(+), 20 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index afa4da138fe9..4939863efbef 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -29,7 +29,7 @@
 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -44,9 +44,11 @@
 #define I2C_REG_IRQSRC 0x23
 #define I2C_REG_SOFTRESET  0x3f
 
+#define I2C_REG_SX8650_STAT0x05
+#define SX8650_STAT_CONVIRQ0x80
+
 /* commands */
 #define CMD_READ_REGISTER  0x40
-#define CMD_MANUAL 0xc0
 #define CMD_PENTRG 0xe0
 
 /* value for I2C_REG_SOFTRESET */
@@ -58,6 +60,7 @@
 
 /* bits for RegTouch1 */
 #define CONDIRQ0x20
+#define RPDNT_100K 0x00
 #define FILT_7SA   0x03
 
 /* bits for I2C_REG_CHANMASK */
@@ -71,14 +74,122 @@
 /* power delay: lower nibble of CTRL0 register */
 #define POWDLY_1_1MS   0x0b
 
+/* for sx8650, as we have no pen release IRQ there: timeout in ns following the
+ * last PENIRQ after which we assume the pen is lifted.
+ */
+#define SX8650_PENIRQ_TIMEOUT  msecs_to_jiffies(10)
+
 #define MAX_12BIT  ((1 << 12) - 1)
+#define MAX_I2C_READ_LEN   10 /* see datasheet section 5.1.5 */
+
+/* channel definition */
+#define CH_X   0x00
+#define CH_Y   0x01
+
+struct sx865x_data {
+   u8 cmd_manual;
+   u8 chan_mask;
+   u8 has_irq_penrelease;
+   u8 has_reg_irqmask;
+   irq_handler_t irqh;
+};
 
 struct sx8654 {
struct input_dev *input;
struct i2c_client *client;
struct gpio_desc *gpio_reset;
+
+   spinlock_t  lock; /* for input reporting from irq/timer */
+   struct timer_list   timer;
+
+   const struct sx865x_data *data;
 };
 
+static inline void sx865x_penrelease(struct sx8654 *ts)
+{
+   struct input_dev *input_dev = ts->input;
+
+   input_report_key(input_dev, BTN_TOUCH, 0);
+   input_sync(input_dev);
+}
+
+static void sx865x_penrelease_timer_handler(struct timer_list *t)
+{
+   struct sx8654 *ts = from_timer(ts, t, timer);
+   unsigned long flags;
+
+   spin_lock_irqsave(&ts->lock, flags);
+   sx865x_penrelease(ts);
+   spin_unlock_irqrestore(&ts->lock, flags);
+   dev_dbg(&ts->client->dev, "penrelease by timer\n");
+}
+
+static irqreturn_t sx8650_irq(int irq, void *handle)
+{
+   struct sx8654 *ts = handle;
+   struct device *dev = &ts->client->dev;
+   int len, i;
+   unsigned long flags;
+   u8 stat;
+   u16 x, y;
+   u8 data[MAX_I2C_READ_LEN];
+   u16 ch;
+   u16 chdata;
+   u8 readlen = hweight32(ts->data->chan_mask) * 2;
+
+   stat = i2c_smbus_read_byte_data(ts->client, CMD_READ_REGISTER
+   | I2C_REG_SX8650_STAT);
+
+   if (!(stat & SX8650_STAT_CONVIRQ)) {
+   dev_dbg(dev, "%s ignore stat [0x%02x]", __func__, stat);
+   return IRQ_HANDLED;
+   }
+
+   len = i2c_master_recv(ts->client, data, readlen);
+   if (len != readlen) {
+   dev_dbg(dev, "ignore short recv (%d)\n", len);
+   return IRQ_HANDLED;
+   }
+
+   spin_lock_irqsave(&ts->lock, flags);
+
+   x = 0;
+   y = 0;
+   for (i = 0; (i + 1) < len; i++) {
+   chdata = data[i] << 8;
+   i++;
+   chdata += data[i];
+
+   if (unlikely(chdata == 0x)) {
+   dev_dbg(dev, "invalid qualified data @ %d\n", i);
+   continue;
+   } else if (unlikely(chdata & 0x8000)) {
+   dev_warn(dev, "hibit @ %d [0x%04x]\n", i, chdata);
+   continue;
+   }
+
+   ch = chdata >> 12;
+   if  (ch == CH_X)
+   x = chdata & MAX_12BIT;
+   else if (ch == CH_Y)
+   y = chdata & MAX_12BIT;
+   else
+   dev_warn(dev, "unknown channel %d [0x%04x]\n", ch,
+chdata);
+   }
+
+   input_report_abs(ts->input, ABS_X, x);
+   input_report_abs(ts->input, ABS_Y, y);
+   input_report_key(ts->input, BTN_TOUCH, 1);
+   input_sync(ts->input);
+   dev_dbg(dev, "point(%4d,%4d)\n", x, y);
+
+   mod_timer(&ts->timer, jiffies 

[PATCH v2 5/8] dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles

[Richard Leitner]

As the sx8650 is quite similar to the sx8654 support for it will be
added in the driver. Therefore add it to the compatibles.

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index a538678424dd..0ebe6dd043c7 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -2,6 +2,7 @@
 
 Required properties:
 - compatible: must be one of the following, depending on the model:
+   "semtech,sx8650"
"semtech,sx8654"
"semtech,sx8655"
"semtech,sx8656"
-- 
2.11.0

Re: [PATCH 6/7] Input: sx8654 - add sx8650 support

[Richard Leitner]

On 10/16/18 7:48 PM, Dmitry Torokhov wrote:

On Tue, Oct 16, 2018 at 11:22:48AM +0200, Richard Leitner wrote:

The sx8654 and sx8650 are quite similar, therefore add support for the
sx8650 within the sx8654 driver.



...

  
  /* bits for I2C_REG_CHANMASK */

-#define CONV_X 0x80
-#define CONV_Y 0x40
+#define CONV_X BIT(7)
+#define CONV_Y BIT(6)


If you are using BIT() you need to include include/linux/bitops.h

I also would prefer conversion to using BIT() as a separate patch.


OK. I'll take it out of this patch.

Should I convert them (and the other #defines where it makes sense) to 
BIT() at all?




  
  /* coordinates rate: higher nibble of CTRL0 register */

  #define RATE_MANUAL   0x00
@@ -71,14 +75,110 @@
  /* power delay: lower nibble of CTRL0 register */
  #define POWDLY_1_1MS  0x0b
  
+/* for sx8650, as we have no pen release IRQ there: timeout in ns following the

+ * last PENIRQ after which we assume the pen is lifted.
+ */
+#define SX8650_PENIRQ_TIMEOUT  (80 * 1100 * 1000)
+
  #define MAX_12BIT ((1 << 12) - 1)
+#define MAX_I2C_READ_LEN   10 /* see datasheet section 5.1.5 */
+
+/* channel definition */
+#define CH_X   0x00
+#define CH_Y   0x01
+
+struct sx865x_data {
+   u8 cmd_manual;
+   u8 chan_mask;
+   u8 has_irq_penrelease;
+   u8 has_reg_irqmask;
+   irq_handler_t irqh;
+};
  
  struct sx8654 {

struct input_dev *input;
struct i2c_client *client;
struct gpio_desc *gpio_reset;
+   struct hrtimer timer;


Does this have to be hrtimer? Can regular timer be used?


I'll check again if it's feasible to reduce the timer delay to something 
below the "normal" jiffy. If not I'll replace the hrtimer with a timer.





+
+   const struct sx865x_data *data;
  };
  
+static enum hrtimer_restart sx865x_penrelease_timer_handler(struct hrtimer *h)

+{
+   struct sx8654 *ts = container_of(h, struct sx8654, timer);
+
+   input_report_key(ts->input, BTN_TOUCH, 0);
+   input_sync(ts->input);
+   dev_dbg(&ts->client->dev, "penrelease by timer\n");
+
+   return HRTIMER_NORESTART;
+}
+


...


@@ -196,10 +312,30 @@ static void sx8654_close(struct input_dev *dev)
  }
  
  #ifdef CONFIG_OF

+static const struct of_device_id sx8654_of_match[] = {
+   {
+   .compatible = "semtech,sx8650",
+   .data = &sx8650_data,
+   }, {
+   .compatible = "semtech,sx8654",
+   .data = &sx8654_data,
+   }, {
+   .compatible = "semtech,sx8655",
+   .data = &sx8654_data,
+   }, {
+   .compatible = "semtech,sx8656",
+   .data = &sx8654_data,
+   }, {},
+};
+MODULE_DEVICE_TABLE(of, sx8654_of_match);
+
  static int sx8654_get_ofdata(struct sx8654 *ts)
  {
struct device *dev = &ts->client->dev;
struct device_node *node = dev->of_node;
+   const struct of_device_id *of_id = of_match_device(sx8654_of_match,
+  dev);


If you use of_device_get_match_data() you do not need to move device
table around.


Thanks for that hint. I haven't known there's something like this ;-)




+   const struct sx865x_data *data = (struct sx865x_data *)of_id->data;
int err;
  
  	if (!node) {


...


@@ -327,6 +466,7 @@ static int sx8654_probe(struct i2c_client *client,
  }
  
  static const struct i2c_device_id sx8654_id_table[] = {

+   { "semtech_sx8650", 0 },


Can we add the data here as well?


Does it generate any benefit if we add it? Who should be using it?

I found in other drivers that it's used as a fallback when 
of_device_get_match_data() returns NULL... Should I also implement it 
that way?





{ "semtech_sx8654", 0 },
{ "semtech_sx8655", 0 },
{ "semtech_sx8656", 0 },
--
2.11.0



Thanks.



thanks®ards;Richard.L

Re: [PATCH 2/7] Input: sx8654 - add reset-gpio support

[Richard Leitner]

Hi Dimitry,
thanks for the quick reply!

On 10/16/18 7:39 PM, Dmitry Torokhov wrote:

Hi Richard,

On Tue, Oct 16, 2018 at 11:16:48AM +0200, Richard Leitner wrote:

The sx8654 features a NRST input which may be connected to a GPIO.
Therefore add support for hard-resetting the sx8654 via this NRST.

If the reset-gpio property is provided the sx8654 is resetted via NRST
instead of the soft-reset via I2C.



...


+
+   dev_dbg(&ts->client->dev, "NRST unavailable, try softreset\n");
+   err = i2c_smbus_write_byte_data(ts->client, I2C_REG_SOFTRESET,
+   SOFTRESET_VALUE);
+   if (err)
+   return err;


Have this in an "else" branch?


If that's the preferred way... of course.




+
+   return 0;
+}
+
  static int sx8654_open(struct input_dev *dev)
  {
struct sx8654 *sx8654 = input_get_drvdata(dev);
@@ -171,6 +195,44 @@ static void sx8654_close(struct input_dev *dev)
}
  }
  
+#ifdef CONFIG_OF

+static int sx8654_get_ofdata(struct sx8654 *ts)
+{
+   struct device *dev = &ts->client->dev;
+   struct device_node *node = dev->of_node;
+   int err;
+
+   if (!node) {
+   ts->gpio_reset = NULL;
+   return 0;
+   }


There is no need to wrap this into CONFIG_OF or test "node". Just use
devm_gpiod_get_optional() and it will work equally well on DT, ACPI, and
static board systems.


Ok. Thank you for that information!
I haven't been aware of this before :-)




+
+   ts->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+   if (PTR_ERR(ts->gpio_reset) == -EPROBE_DEFER) {
+   return -EPROBE_DEFER;
+   } else if (IS_ERR(ts->gpio_reset)) {
+   err = PTR_ERR(ts->gpio_reset);


I'd do:

if (err != -EPROBE_DEFER)
dev_err(...)

here instead of separate branch.


Indeed... Looks like a more sexy solution. Thanks.




+   dev_err(dev, "unable to request GPIO reset pin (%d)\n", err);
+   return err;
+   }
+   dev_dbg(dev, "got GPIO reset pin\n");


Also, I'd move all this code directly into probe() since all this
#ifdefery is not needed.


Without the #ifdef'ery and node checking this sounds reasonable.




+
+   return 0;
+}
+
+static const struct of_device_id sx8654_of_match[] = {
+   { .compatible = "semtech,sx8654", },
+   { },
+};


Please keep the device table wehere it was.


OK.


@@ -186,10 +248,20 @@ static int sx8654_probe(struct i2c_client *client,
if (!sx8654)
return -ENOMEM;
  
+	sx8654->client = client;

+
+   error = sx8654_get_ofdata(sx8654);
+   if (error) {
+   dev_err(&client->dev, "get_ofdata failed: %d\n", error);
+   return error;
+   }
+
input = devm_input_allocate_device(&client->dev);
if (!input)
return -ENOMEM;
  
+	sx8654->input = input;

+
input->name = "SX8654 I2C Touchscreen";
input->id.bustype = BUS_I2C;
input->dev.parent = &client->dev;
@@ -201,15 +273,11 @@ static int sx8654_probe(struct i2c_client *client,
input_set_abs_params(input, ABS_X, 0, MAX_12BIT, 0, 0);
input_set_abs_params(input, ABS_Y, 0, MAX_12BIT, 0, 0);
  
-	sx8654->client = client;

-   sx8654->input = input;
-


Why do we need to move this?


This was because sx8654_get_ofdata needed ts->client->dev.
But as I will integrate the reset gpio getting in the probe function 
I'll move it back.


...



Thanks.



I'll send a v2 of the series as soon as I have fixed the other patches.

regards;Richard.L

[PATCH 4/7] Input: sx8654 - add sx8655 and sx8656 to compatibles

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index 059127490c8d..622283b49d7c 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -222,6 +222,8 @@ static int sx8654_get_ofdata(struct sx8654 *ts)
 
 static const struct of_device_id sx8654_of_match[] = {
{ .compatible = "semtech,sx8654", },
+   { .compatible = "semtech,sx8655", },
+   { .compatible = "semtech,sx8656", },
{ },
 };
 MODULE_DEVICE_TABLE(of, sx8654_of_match);
@@ -326,6 +328,8 @@ static int sx8654_probe(struct i2c_client *client,
 
 static const struct i2c_device_id sx8654_id_table[] = {
{ "semtech_sx8654", 0 },
+   { "semtech_sx8655", 0 },
+   { "semtech_sx8656", 0 },
{ },
 };
 MODULE_DEVICE_TABLE(i2c, sx8654_id_table);
-- 
2.11.0

[PATCH 0/7] Input: sx8654 - reset-gpio, sx865[056] support, etc.

[Richard Leitner]

Add reset-gpio, sx8654[056] and common of_touchscreen functions support
for the sx8654 driver.

Richard Leitner (7):
  dt-bindings: input: touchscreen: sx8654: add reset-gpio property
  Input: sx8654 - add reset-gpio support
  dt-bindings: input: touchscreen: sx8654: add compatible models
  Input: sx8654 - add sx8655 and sx8656 to compatibles
  dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles
  Input: sx8654 - add sx8650 support
  Input: sx8654 - use common of_touchscreen functions

 .../bindings/input/touchscreen/sx8654.txt  |  10 +-
 drivers/input/touchscreen/sx8654.c | 267 ++---
 2 files changed, 245 insertions(+), 32 deletions(-)

-- 
2.11.0

[PATCH 1/7] dt-bindings: input: touchscreen: sx8654: add reset-gpio property

[Richard Leitner]

Document the reset-gpio property for the sx8654 touchscreen controller
driver.

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index 4886c4aa2906..ca521d8f7d65 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -5,6 +5,9 @@ Required properties:
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
+Optional properties:
+ - reset-gpios: GPIO specification for the NRST input
+
 Example:
 
sx8654@48 {
@@ -12,4 +15,5 @@ Example:
reg = <0x48>;
interrupt-parent = <&gpio6>;
interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+   reset-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
};
-- 
2.11.0

[PATCH 3/7] dt-bindings: input: touchscreen: sx8654: add compatible models

[Richard Leitner]

As the sx865[456] share the same datasheet and differ only in the
presence of a "capacitive proximity detection circuit" and a "haptics
motor driver for LRA/ERM" add them to the compatbiles. As the driver
doesn't implement these features it should be no problem.

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index ca521d8f7d65..a538678424dd 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -1,7 +1,10 @@
 * Semtech SX8654 I2C Touchscreen Controller
 
 Required properties:
-- compatible: must be "semtech,sx8654"
+- compatible: must be one of the following, depending on the model:
+   "semtech,sx8654"
+   "semtech,sx8655"
+   "semtech,sx8656"
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 
-- 
2.11.0

[PATCH 2/7] Input: sx8654 - add reset-gpio support

[Richard Leitner]

The sx8654 features a NRST input which may be connected to a GPIO.
Therefore add support for hard-resetting the sx8654 via this NRST.

If the reset-gpio property is provided the sx8654 is resetted via NRST
instead of the soft-reset via I2C.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 88 --
 1 file changed, 74 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index ed29db3ec731..059127490c8d 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -33,6 +33,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -74,6 +76,7 @@
 struct sx8654 {
struct input_dev *input;
struct i2c_client *client;
+   struct gpio_desc *gpio_reset;
 };
 
 static irqreturn_t sx8654_irq(int irq, void *handle)
@@ -124,6 +127,27 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
return IRQ_HANDLED;
 }
 
+static int sx8654_reset(struct sx8654 *ts)
+{
+   int err;
+
+   if (ts->gpio_reset) {
+   gpiod_set_value_cansleep(ts->gpio_reset, 1);
+   udelay(2); /* Tpulse > 1µs */
+   gpiod_set_value_cansleep(ts->gpio_reset, 0);
+
+   return 0;
+   }
+
+   dev_dbg(&ts->client->dev, "NRST unavailable, try softreset\n");
+   err = i2c_smbus_write_byte_data(ts->client, I2C_REG_SOFTRESET,
+   SOFTRESET_VALUE);
+   if (err)
+   return err;
+
+   return 0;
+}
+
 static int sx8654_open(struct input_dev *dev)
 {
struct sx8654 *sx8654 = input_get_drvdata(dev);
@@ -171,6 +195,44 @@ static void sx8654_close(struct input_dev *dev)
}
 }
 
+#ifdef CONFIG_OF
+static int sx8654_get_ofdata(struct sx8654 *ts)
+{
+   struct device *dev = &ts->client->dev;
+   struct device_node *node = dev->of_node;
+   int err;
+
+   if (!node) {
+   ts->gpio_reset = NULL;
+   return 0;
+   }
+
+   ts->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+   if (PTR_ERR(ts->gpio_reset) == -EPROBE_DEFER) {
+   return -EPROBE_DEFER;
+   } else if (IS_ERR(ts->gpio_reset)) {
+   err = PTR_ERR(ts->gpio_reset);
+   dev_err(dev, "unable to request GPIO reset pin (%d)\n", err);
+   return err;
+   }
+   dev_dbg(dev, "got GPIO reset pin\n");
+
+   return 0;
+}
+
+static const struct of_device_id sx8654_of_match[] = {
+   { .compatible = "semtech,sx8654", },
+   { },
+};
+MODULE_DEVICE_TABLE(of, sx8654_of_match);
+#else /* CONFIG_OF */
+static int sx8654_get_ofdata(struct sx8654 *ts)
+{
+   ts->gpio_reset = NULL;
+   return 0;
+}
+#endif /* CONFIG_OF */
+
 static int sx8654_probe(struct i2c_client *client,
const struct i2c_device_id *id)
 {
@@ -186,10 +248,20 @@ static int sx8654_probe(struct i2c_client *client,
if (!sx8654)
return -ENOMEM;
 
+   sx8654->client = client;
+
+   error = sx8654_get_ofdata(sx8654);
+   if (error) {
+   dev_err(&client->dev, "get_ofdata failed: %d\n", error);
+   return error;
+   }
+
input = devm_input_allocate_device(&client->dev);
if (!input)
return -ENOMEM;
 
+   sx8654->input = input;
+
input->name = "SX8654 I2C Touchscreen";
input->id.bustype = BUS_I2C;
input->dev.parent = &client->dev;
@@ -201,15 +273,11 @@ static int sx8654_probe(struct i2c_client *client,
input_set_abs_params(input, ABS_X, 0, MAX_12BIT, 0, 0);
input_set_abs_params(input, ABS_Y, 0, MAX_12BIT, 0, 0);
 
-   sx8654->client = client;
-   sx8654->input = input;
-
input_set_drvdata(sx8654->input, sx8654);
 
-   error = i2c_smbus_write_byte_data(client, I2C_REG_SOFTRESET,
- SOFTRESET_VALUE);
+   error = sx8654_reset(sx8654);
if (error) {
-   dev_err(&client->dev, "writing softreset value failed");
+   dev_err(&client->dev, "reset failed");
return error;
}
 
@@ -256,14 +324,6 @@ static int sx8654_probe(struct i2c_client *client,
return 0;
 }
 
-#ifdef CONFIG_OF
-static const struct of_device_id sx8654_of_match[] = {
-   { .compatible = "semtech,sx8654", },
-   { },
-};
-MODULE_DEVICE_TABLE(of, sx8654_of_match);
-#endif
-
 static const struct i2c_device_id sx8654_id_table[] = {
{ "semtech_sx8654", 0 },
{ },
-- 
2.11.0

[PATCH 7/7] Input: sx8654 - use common of_touchscreen functions

[Richard Leitner]

of_touchscreen.c provides a common interface for a axis invertion and
swapping of touchscreens. Therefore use it in the sx8654 driver.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 37 +++--
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index 33f7a0be4ef8..08b064f96b4e 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -100,6 +101,7 @@ struct sx8654 {
struct i2c_client *client;
struct gpio_desc *gpio_reset;
struct hrtimer timer;
+   struct touchscreen_properties props;
 
const struct sx865x_data *data;
 };
@@ -168,8 +170,7 @@ static irqreturn_t sx8650_irq(int irq, void *handle)
 chdata);
}
 
-   input_report_abs(ts->input, ABS_X, x);
-   input_report_abs(ts->input, ABS_Y, y);
+   touchscreen_report_pos(ts->input, &ts->props, x, y, false);
input_report_key(ts->input, BTN_TOUCH, 1);
input_sync(ts->input);
dev_dbg(dev, "point(%4d,%4d)\n", x, y);
@@ -215,8 +216,8 @@ static irqreturn_t sx8654_irq(int irq, void *handle)
x = ((data[0] & 0xf) << 8) | (data[1]);
y = ((data[2] & 0xf) << 8) | (data[3]);
 
-   input_report_abs(sx8654->input, ABS_X, x);
-   input_report_abs(sx8654->input, ABS_Y, y);
+   touchscreen_report_pos(sx8654->input, &sx8654->props, x, y,
+  false);
input_report_key(sx8654->input, BTN_TOUCH, 1);
input_sync(sx8654->input);
 
@@ -355,6 +356,8 @@ static int sx8654_get_ofdata(struct sx8654 *ts)
}
dev_dbg(dev, "got GPIO reset pin\n");
 
+   touchscreen_parse_properties(ts->input, false, &ts->props);
+
return 0;
 }
 #else /* CONFIG_OF */
@@ -380,25 +383,11 @@ static int sx8654_probe(struct i2c_client *client,
sx8654 = devm_kzalloc(&client->dev, sizeof(*sx8654), GFP_KERNEL);
if (!sx8654)
return -ENOMEM;
-
sx8654->client = client;
 
-   error = sx8654_get_ofdata(sx8654);
-   if (error) {
-   dev_err(&client->dev, "get_ofdata failed: %d\n", error);
-   return error;
-   }
-
-   if (!sx8654->data->has_irq_penrelease) {
-   dev_dbg(&client->dev, "use timer for penrelease\n");
-   hrtimer_init(&sx8654->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-   sx8654->timer.function = sx865x_penrelease_timer_handler;
-   }
-
input = devm_input_allocate_device(&client->dev);
if (!input)
return -ENOMEM;
-
sx8654->input = input;
 
input->name = "SX8654 I2C Touchscreen";
@@ -414,6 +403,18 @@ static int sx8654_probe(struct i2c_client *client,
 
input_set_drvdata(sx8654->input, sx8654);
 
+   error = sx8654_get_ofdata(sx8654);
+   if (error) {
+   dev_err(&client->dev, "get_ofdata failed: %d\n", error);
+   return error;
+   }
+
+   if (!sx8654->data->has_irq_penrelease) {
+   dev_dbg(&client->dev, "use timer for penrelease\n");
+   hrtimer_init(&sx8654->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+   sx8654->timer.function = sx865x_penrelease_timer_handler;
+   }
+
error = sx8654_reset(sx8654);
if (error) {
dev_err(&client->dev, "reset failed");
-- 
2.11.0

[PATCH 6/7] Input: sx8654 - add sx8650 support

[Richard Leitner]

The sx8654 and sx8650 are quite similar, therefore add support for the
sx8650 within the sx8654 driver.

Signed-off-by: Richard Leitner 
---
 drivers/input/touchscreen/sx8654.c | 186 -
 1 file changed, 163 insertions(+), 23 deletions(-)

diff --git a/drivers/input/touchscreen/sx8654.c 
b/drivers/input/touchscreen/sx8654.c
index 622283b49d7c..33f7a0be4ef8 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -29,12 +29,13 @@
 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 
 /* register addresses */
 #define I2C_REG_TOUCH0 0x00
@@ -44,9 +45,11 @@
 #define I2C_REG_IRQSRC 0x23
 #define I2C_REG_SOFTRESET  0x3f
 
+#define I2C_REG_SX8650_STAT0x05
+#define SX8650_STAT_CONVIRQBIT(7)
+
 /* commands */
 #define CMD_READ_REGISTER  0x40
-#define CMD_MANUAL 0xc0
 #define CMD_PENTRG 0xe0
 
 /* value for I2C_REG_SOFTRESET */
@@ -58,11 +61,12 @@
 
 /* bits for RegTouch1 */
 #define CONDIRQ0x20
+#define RPDNT_100K 0x00
 #define FILT_7SA   0x03
 
 /* bits for I2C_REG_CHANMASK */
-#define CONV_X 0x80
-#define CONV_Y 0x40
+#define CONV_X BIT(7)
+#define CONV_Y BIT(6)
 
 /* coordinates rate: higher nibble of CTRL0 register */
 #define RATE_MANUAL0x00
@@ -71,14 +75,110 @@
 /* power delay: lower nibble of CTRL0 register */
 #define POWDLY_1_1MS   0x0b
 
+/* for sx8650, as we have no pen release IRQ there: timeout in ns following the
+ * last PENIRQ after which we assume the pen is lifted.
+ */
+#define SX8650_PENIRQ_TIMEOUT  (80 * 1100 * 1000)
+
 #define MAX_12BIT  ((1 << 12) - 1)
+#define MAX_I2C_READ_LEN   10 /* see datasheet section 5.1.5 */
+
+/* channel definition */
+#define CH_X   0x00
+#define CH_Y   0x01
+
+struct sx865x_data {
+   u8 cmd_manual;
+   u8 chan_mask;
+   u8 has_irq_penrelease;
+   u8 has_reg_irqmask;
+   irq_handler_t irqh;
+};
 
 struct sx8654 {
struct input_dev *input;
struct i2c_client *client;
struct gpio_desc *gpio_reset;
+   struct hrtimer timer;
+
+   const struct sx865x_data *data;
 };
 
+static enum hrtimer_restart sx865x_penrelease_timer_handler(struct hrtimer *h)
+{
+   struct sx8654 *ts = container_of(h, struct sx8654, timer);
+
+   input_report_key(ts->input, BTN_TOUCH, 0);
+   input_sync(ts->input);
+   dev_dbg(&ts->client->dev, "penrelease by timer\n");
+
+   return HRTIMER_NORESTART;
+}
+
+static irqreturn_t sx8650_irq(int irq, void *handle)
+{
+   struct sx8654 *ts = handle;
+   struct device *dev = &ts->client->dev;
+   int len, i, ret;
+   u8 stat;
+   u16 x, y;
+   u8 data[MAX_I2C_READ_LEN];
+   u16 ch;
+   u16 chdata;
+   u8 readlen = hweight32(ts->data->chan_mask) * 2;
+
+   stat = i2c_smbus_read_byte_data(ts->client, CMD_READ_REGISTER
+   | I2C_REG_SX8650_STAT);
+
+   if (!(stat & SX8650_STAT_CONVIRQ)) {
+   dev_dbg(dev, "%s ignore stat [0x%02x]", __func__, stat);
+   return IRQ_HANDLED;
+   }
+
+   len = i2c_master_recv(ts->client, data, readlen);
+   if (len != readlen) {
+   dev_dbg(dev, "ignore short recv (%d)\n", len);
+   return IRQ_HANDLED;
+   }
+
+   ret = hrtimer_try_to_cancel(&ts->timer);
+
+   x = 0;
+   y = 0;
+   for (i = 0; (i + 1) < len; i++) {
+   chdata = data[i] << 8;
+   i++;
+   chdata += data[i];
+
+   if (unlikely(chdata == 0x)) {
+   dev_dbg(dev, "invalid qualified data @ %d\n", i);
+   continue;
+   } else if (unlikely(chdata & 0x8000)) {
+   dev_warn(dev, "hibit @ %d [0x%04x]\n", i, chdata);
+   continue;
+   }
+
+   ch = chdata >> 12;
+   if  (ch == CH_X)
+   x = chdata & MAX_12BIT;
+   else if (ch == CH_Y)
+   y = chdata & MAX_12BIT;
+   else
+   dev_warn(dev, "unknown channel %d [0x%04x]\n", ch,
+chdata);
+   }
+
+   input_report_abs(ts->input, ABS_X, x);
+   input_report_abs(ts->input, ABS_Y, y);
+   input_report_key(ts->input, BTN_TOUCH, 1);
+   input_sync(ts->input);
+   dev_dbg(dev, "point(%

[PATCH 5/7] dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles

[Richard Leitner]

As the sx8650 is quite similar to the sx8654 support for it will be
added in the driver. Therefore add it to the compatibles.

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/input/touchscreen/sx8654.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt 
b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
index a538678424dd..0ebe6dd043c7 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sx8654.txt
@@ -2,6 +2,7 @@
 
 Required properties:
 - compatible: must be one of the following, depending on the model:
+   "semtech,sx8650"
"semtech,sx8654"
"semtech,sx8655"
"semtech,sx8656"
-- 
2.11.0

[PATCH v4] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner 

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant.

In our case we have a series of USB hardware (using the cp210x driver)
which communicates using a proprietary protocol. These devices sometimes
trigger an over-current situation on some hubs. In case of such an
over-current situation the USB devices offer an interface for reducing
the max used power. As these conditions are quite rare and imply
performance reductions of the device we don't want to reduce the max
power always.

Therefore give user-space applications the possibility to react
adequately by introducing an over_current_counter in the usb port struct
which is exported via sysfs.

Signed-off-by: Richard Leitner 
---
Changes v4:
- reintroduce forgotten Changelog
Changes v3:
- Improve sysfs file description as recommended by greg k-h
Changes v2:
- rename oc_count to over_current_count
- add entry to Documentation/ABI
- add detailled description to commit message
---
 Documentation/ABI/testing/sysfs-bus-usb | 10 ++
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..c702c78f24d8 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,16 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
 
+What:  /sys/bus/usb/devices/.../(hub 
interface)/portX/over_current_count
+Date:      February 2018
+Contact:   Richard Leitner 
+Description:
+   Most hubs are able to detect over-current situations on their
+   ports and report them to the kernel. This attribute is to expose
+   the number of over-current situation occurred on a specific port
+   to user space. This file will contain an unsigned 32 bit value
+   which wraps to 0 after its maximum is reached.
+
 What:  /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
 Date:  November 2015
 Contact:   Lu Baolu 
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..6f779b518e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->over_current_count++;
 
-   dev_dbg(&port_dev->dev, "over-current change\n");
+   dev_dbg(&port_dev->dev, "over-current change #%u\n",
+   port_dev->over_current_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..78d7f4dad618 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int over_current_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..6979bde87d31 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t over_current_count_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->over_current_count);
+}
+static DEVICE_ATTR_RO(over_current_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
&dev_attr_connect_type.attr,
+   &dev_attr_over_current_count.attr,
NULL,
 };
 
-- 
2.11.0

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

On 03/15/2018 10:26 AM, Oliver Neukum wrote:
> Am Mittwoch, den 14.03.2018, 16:44 +0100 schrieb Richard Leitner:
>> On 03/14/2018 04:27 PM, Oliver Neukum wrote:
>>> Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner:
>>>>
>>> Well, but it does not. Removing a redundant definition is a clear
>>> benefit. But you are not removing a definition. You are introducing
>>> a preprocessor constant. Why?
>>> What is its benefit?
>>
>> AFAIK pci_ids.h collects PCI vendor and device IDs in one single 
>> point. As the PCI vendor ID of Netlogic is used in multiple files
>> IMHO it would be a good idea to add it to pci_ids.h and furthermore
>> remove it from arch/mips/include/asm/netlogic/xlp-hal/iomap.h (where
>> it's currently defined).
>>
>> Or am I getting things wrong?
> 
> I think so, yes. We are giving names to constants as a form
> of comment or to change them at multiple places at once and
> consistently.
> 
> So
> 
> #define XYZ_NETDEV_RESET_RETRIES  2
> 
> makes clearly sense. So does
> 
> #define XYZ_MAGIC_VALUE1  0xab4e
> 
> because it tells you that you have a magic value.
> But you will never redefine a PCI vendor ID. In fact you
> must not. And if you have a comparison like
> 
> dev->vID == 0x1234
> 
> if you change this to
> 
> dev->vID == SOME_VENDOR_ID
> 
> what good does this to you? You already knew it was a vendor ID.
> Now you can name it at a glance. So what? If you have a device
> you will have to check whether you have some OEM version. You
> will always go and check the raw number. And if you have a log
> and need to check whether the check will be true, you will have
> a number.
> Using a constant there is nothing but trouble. Yet one more grep.

Thank you for that explanation. But IMHO it was clearer with a
human-readable name in such comparisons...

For example in the following I see at the first glance which
device from which vendor is affected and I don't need any
additional comments or ID databases...
   if (pdev->vendor == PCI_VENDOR_ID_TI &&
   pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)

...but if that's not the preferred way of doing things I'm
perfectly fine with that.

Furthermore to me it sounds you are saying that the complete
pci_ids.h should be thrown over-board?!?

regards;richard.l

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

On 03/14/2018 04:27 PM, Oliver Neukum wrote:
> Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner:
>> Hi Oliver,
>> thank you for your feedback!
>>
>> On 03/14/2018 01:17 PM, Oliver Neukum wrote:
>>> Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner:
>>>> From: Richard Leitner 
>>>>
>>>> Replace the hardcoded PCI vendor ID of Netlogic with a definition in
>>>> pci_ids.h
>>>
>>> Hi,
>>>
>>> in general, why?
>>> Does this patch generate any benefit for any developer
>>> reading the source? I don't see it. Does it cause an
>>> issue for anybody who has a log file with the nummerical
>>> ID and needs to grep for it? Yes it does.
>>
>> I'll send a v2 where I also use this definition in 
>> arch/mips/netlogic/xlp/ instead of PCI_VENDOR_NETLOGIC from
>> arch/mips/include/asm/netlogic/xlp-hal/iomap.h.
>>
>> Therefore it will remove this definition from the iomap.h
>> and move it to pci_ids.h
>>
>> This will IMHO be a clear benefit as it removes a redundant
>> definition.
> 
> Well, but it does not. Removing a redundant definition is a clear
> benefit. But you are not removing a definition. You are introducing
> a preprocessor constant. Why?
> What is its benefit?

AFAIK pci_ids.h collects PCI vendor and device IDs in one single 
point. As the PCI vendor ID of Netlogic is used in multiple files
IMHO it would be a good idea to add it to pci_ids.h and furthermore
remove it from arch/mips/include/asm/netlogic/xlp-hal/iomap.h (where
it's currently defined).

Or am I getting things wrong?

regards;richard.l

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

Hi Oliver,
thank you for your feedback!

On 03/14/2018 01:17 PM, Oliver Neukum wrote:
> Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner:
>> From: Richard Leitner 
>>
>> Replace the hardcoded PCI vendor ID of Netlogic with a definition in
>> pci_ids.h
> 
> Hi,
> 
> in general, why?
> Does this patch generate any benefit for any developer
> reading the source? I don't see it. Does it cause an
> issue for anybody who has a log file with the nummerical
> ID and needs to grep for it? Yes it does.

I'll send a v2 where I also use this definition in 
arch/mips/netlogic/xlp/ instead of PCI_VENDOR_NETLOGIC from
arch/mips/include/asm/netlogic/xlp-hal/iomap.h.

Therefore it will remove this definition from the iomap.h
and move it to pci_ids.h

This will IMHO be a clear benefit as it removes a redundant
definition.

> 
> Where is the point of this patch?
> 
>   Regards
>   Oliver
> 

regards;Richard.L

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

On 03/14/2018 11:48 AM, Greg KH wrote:
> On Wed, Mar 14, 2018 at 11:29:32AM +0100, Richard Leitner wrote:
>> From: Richard Leitner 
>>
>> Replace the hardcoded PCI vendor ID of Netlogic with a definition in
>> pci_ids.h
> 
> Why?  It's only being used in one file, so it should not be in
> pci_ids.h, right?

It's also used as PCI_VENDOR_NETLOGIC in arch/mips/netlogic/xlp/.

Should this be replaced with PCI_VENDOR_ID_NETLOGIC from pci_ids.h?

> 
> thanks,
> 
> greg k-h
> 

thank you!

regards;Richard.L

Re: [PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

On 03/14/2018 11:49 AM, Greg KH wrote:
> On Wed, Mar 14, 2018 at 11:29:33AM +0100, Richard Leitner wrote:
>> From: Richard Leitner 
>>
>> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
>> the harcoded values with them.
>>
>> Signed-off-by: Richard Leitner 
>> ---
>>  drivers/usb/host/pci-quirks.c | 6 --
>>  drivers/usb/host/xhci-pci.c   | 4 ++--
>>  include/linux/pci_ids.h   | 2 ++
>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>

> Now this patch was fine :)
> 
> Care to redo this series?

Sure. Will send a v2 soon.

> 
> thanks,
> 
> greg k-h
> 

thank you!

regards;Richard.L

[PATCH 1/3] usb: host: pci: use existing Intel PCI ID macros

[Richard Leitner]

From: Richard Leitner 

Instead of the hardcoded hexadecimal PCI IDs use the existing macros
from pci_ids.h for Intel IDs.

Signed-off-by: Richard Leitner 
---
 drivers/usb/host/pci-quirks.c | 10 +-
 drivers/usb/host/xhci-pci.c   |  3 ++-
 include/linux/pci_ids.h   |  1 +
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..4f4a9f36a68e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -858,8 +858,8 @@ static void ehci_bios_handoff(struct pci_dev *pdev,
 *
 * The HASEE E200 hangs when the semaphore is set (bugzilla #77021).
 */
-   if (pdev->vendor == 0x8086 && (pdev->device == 0x283a ||
-   pdev->device == 0x27cc)) {
+   if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+   (pdev->device == 0x283a || pdev->device == 0x27cc)) {
if (dmi_check_system(ehci_dmi_nohandoff_table))
try_handoff = 0;
}
@@ -1168,9 +1168,9 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
val = readl(base + ext_cap_offset);
 
/* Auto handoff never worked for these devices. Force it and continue */
-   if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
-   (pdev->vendor == PCI_VENDOR_ID_RENESAS
-&& pdev->device == 0x0014)) {
+   if ((pdev->vendor == PCI_VENDOR_ID_TI &&
+pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
+   (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
writel(val, base + ext_cap_offset);
}
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5262fa571a5d..a5bfd890190c 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -213,7 +213,8 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
pdev->device == PCI_DEVICE_ID_ASMEDIA_1042A_XHCI)
xhci->quirks |= XHCI_ASMEDIA_MODIFY_FLOWCONTROL;
 
-   if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
+   if (pdev->vendor == PCI_VENDOR_ID_TI &&
+   pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
 
if (xhci->quirks & XHCI_RESET_ON_RESUME)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index a6b30667a331..e8d1af82a688 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -838,6 +838,7 @@
 #define PCI_DEVICE_ID_TI_XX12  0x8039
 #define PCI_DEVICE_ID_TI_XX12_FM   0x803b
 #define PCI_DEVICE_ID_TI_XIO2000A  0x8231
+#define PCI_DEVICE_ID_TI_TUSB73X0  0x8241
 #define PCI_DEVICE_ID_TI_1130  0xac12
 #define PCI_DEVICE_ID_TI_1031  0xac13
 #define PCI_DEVICE_ID_TI_1131  0xac15
-- 
2.11.0

[PATCH 0/3] usb: host: pci: PCI ID consolidation

[Richard Leitner]

From: Richard Leitner 

Centralize some hardcoded PCI IDs as definitions in the global
include/linux/pci_ids.h file. This is done to reduce the amount of
scattered PCI ID definitions and hardcoded values across the kernel.

Richard Leitner (3):
  usb: host: pci: use existing Intel PCI ID macros
  usb: host: pci: introduce PCI vendor ID for Netlogic
  usb: host: pci: replace hardcoded renesas PCI IDs

 drivers/usb/host/pci-quirks.c | 16 +---
 drivers/usb/host/xhci-pci.c   |  7 ---
 include/linux/pci_ids.h   |  5 +
 3 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

From: Richard Leitner 

Replace the hardcoded PCI vendor ID of Netlogic with a definition in
pci_ids.h

Signed-off-by: Richard Leitner 
---
 drivers/usb/host/pci-quirks.c | 2 +-
 include/linux/pci_ids.h   | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 4f4a9f36a68e..39d163729b89 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1243,7 +1243,7 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
/* Skip Netlogic mips SoC's internal PCI USB controller.
 * This device does not need/support EHCI/OHCI handoff
 */
-   if (pdev->vendor == 0x184e) /* vendor Netlogic */
+   if (pdev->vendor == PCI_VENDOR_ID_NETLOGIC)
return;
if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI &&
pdev->class != PCI_CLASS_SERIAL_USB_OHCI &&
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index e8d1af82a688..d23a97868dee 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -3067,4 +3067,6 @@
 
 #define PCI_VENDOR_ID_OCZ  0x1b85
 
+#define PCI_VENDOR_ID_NETLOGIC 0x184e
+
 #endif /* _LINUX_PCI_IDS_H */
-- 
2.11.0

[PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

From: Richard Leitner 

Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
the harcoded values with them.

Signed-off-by: Richard Leitner 
---
 drivers/usb/host/pci-quirks.c | 6 --
 drivers/usb/host/xhci-pci.c   | 4 ++--
 include/linux/pci_ids.h   | 2 ++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 39d163729b89..5e1ad523622e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1170,7 +1170,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
/* Auto handoff never worked for these devices. Force it and continue */
if ((pdev->vendor == PCI_VENDOR_ID_TI &&
 pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
-   (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
+   (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
+pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)) {
val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
writel(val, base + ext_cap_offset);
}
@@ -1282,7 +1283,8 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
 * quirk, or the system will be in a rather bad state.
 */
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   (pdev->device == 0x0014 || pdev->device == 0x0015))
+   (pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201 ||
+pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202))
return true;
 
return false;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a5bfd890190c..a453e4c35ac7 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -189,10 +189,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
xhci->quirks |= XHCI_BROKEN_STREAMS;
}
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0014)
+   pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0015)
+   pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202)
xhci->quirks |= XHCI_RESET_ON_RESUME;
if (pdev->vendor == PCI_VENDOR_ID_VIA)
xhci->quirks |= XHCI_RESET_ON_RESUME;
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index d23a97868dee..eb52f0e9b651 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2427,6 +2427,8 @@
 #define PCI_DEVICE_ID_RENESAS_SH7763   0x0004
 #define PCI_DEVICE_ID_RENESAS_SH7785   0x0007
 #define PCI_DEVICE_ID_RENESAS_SH7786   0x0010
+#define PCI_DEVICE_ID_RENESAS_UPD7202010x0014
+#define PCI_DEVICE_ID_RENESAS_UPD7202020x0015
 
 #define PCI_VENDOR_ID_SOLARFLARE   0x1924
 #define PCI_DEVICE_ID_SOLARFLARE_SFC4000A_00x0703
-- 
2.11.0

[PATCH v3] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner 

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant.

In our case we have a series of USB hardware (using the cp210x driver)
which communicates using a proprietary protocol. These devices sometimes
trigger an over-current situation on some hubs. In case of such an
over-current situation the USB devices offer an interface for reducing
the max used power. As these conditions are quite rare and imply
performance reductions of the device we don't want to reduce the max
power always.

Therefore give user-space applications the possibility to react
adequately by introducing an over_current_counter in the usb port struct
which is exported via sysfs.

Signed-off-by: Richard Leitner 
---
 Documentation/ABI/testing/sysfs-bus-usb | 10 ++
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..c702c78f24d8 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,16 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
 
+What:  /sys/bus/usb/devices/.../(hub 
interface)/portX/over_current_count
+Date:      February 2018
+Contact:   Richard Leitner 
+Description:
+   Most hubs are able to detect over-current situations on their
+   ports and report them to the kernel. This attribute is to expose
+   the number of over-current situation occurred on a specific port
+   to user space. This file will contain an unsigned 32 bit value
+   which wraps to 0 after its maximum is reached.
+
 What:  /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
 Date:  November 2015
 Contact:   Lu Baolu 
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..6f779b518e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->over_current_count++;
 
-   dev_dbg(&port_dev->dev, "over-current change\n");
+   dev_dbg(&port_dev->dev, "over-current change #%u\n",
+   port_dev->over_current_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..78d7f4dad618 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int over_current_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..6979bde87d31 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t over_current_count_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->over_current_count);
+}
+static DEVICE_ATTR_RO(over_current_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
&dev_attr_connect_type.attr,
+   &dev_attr_over_current_count.attr,
NULL,
 };
 
-- 
2.11.0

Re: [PATCH v2] usb: core: introduce per-port over-current counters

[Richard Leitner]

Hi Greg,

On 03/09/2018 06:19 PM, Greg KH wrote:
>> diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
>> b/Documentation/ABI/testing/sysfs-bus-usb
>> index 0bd731cbb50c..27020293c85b 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-usb
>> +++ b/Documentation/ABI/testing/sysfs-bus-usb
>> @@ -189,6 +189,15 @@ Description:
>>  The file will read "hotplug", "wired" and "not used" if the
>>  information is available, and "unknown" otherwise.
>>  
>> +What:   /sys/bus/usb/devices/.../(hub 
>> interface)/portX/over_current_count
>> +Date:   February 2018
>> +Contact:Richard Leitner 
>> +Description:
>> +Most hubs are able to detect over-current situations on their
>> +ports and report them to the kernel. This attribute is to expose
>> +the number of over-current situation occurred on a specific port
>> +to user space. This file will contain an unsigned int.
> "unsigned int" is very vague :)
> 
> How about "this is a 32bit value"?
> 
> And what happens when this wraps?  Is that allowed?

Thank you for your feedback. I'll improve the description for v3!

> 
> thanks,
> 
> greg k-h

regards;Richard.L

Re: [PATCH v2] usb: core: introduce per-port over-current counters

[Richard Leitner]

On 02/20/2018 12:55 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Richard Leitner  writes:
>> From: Richard Leitner 
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant.
>>
>> In our case we have a series of USB hardware (using the cp210x driver)
>> which communicates using a proprietary protocol. These devices sometimes
>> trigger an over-current situation on some hubs. In case of such an
>> over-current situation the USB devices offer an interface for reducing
>> the max used power. As these conditions are quite rare and imply
>> performance reductions of the device we don't want to reduce the max
>> power always.
>>
>> Therefore give user-space applications the possibility to react
>> adequately by introducing an over_current_counter in the usb port struct
>> which is exported via sysfs.
> 
> why don't you just provide more than one configuration with several
> bMaxPower fields? Then host OS should choose correct configuration based
> on power budget.

Thank you for that suggestion!
Generally speaking that would be possible. Nonetheless we
a) don't have the possibility to change the firmware or the USB 
configuration of the device.
b) in those corner-cases (where the over-current situation occurs)
the device consumes more than 500mA (which couldn't be 
configured in bMaxPower AFAIK?). Nonetheless most
hubs don't detect the over-current (AFAIK) due to
electrical tolerances of their components. Our problem are
the hubs which trigger the over-current situation (which
is fine from a USB perspective).

I know it's probably not a very nice solution, so if anybody has a better
proposal please let me know :-)

Thanks!

regards;Richard.L

[PATCH v2] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner 

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant.

In our case we have a series of USB hardware (using the cp210x driver)
which communicates using a proprietary protocol. These devices sometimes
trigger an over-current situation on some hubs. In case of such an
over-current situation the USB devices offer an interface for reducing
the max used power. As these conditions are quite rare and imply
performance reductions of the device we don't want to reduce the max
power always.

Therefore give user-space applications the possibility to react
adequately by introducing an over_current_counter in the usb port struct
which is exported via sysfs.

Signed-off-by: Richard Leitner 
---
Tested on an i.MX6DL based board

Changes v2:
- rename oc_count to over_current_count
- add entry to Documentation/ABI
- add detailled description to commit message
---
 Documentation/ABI/testing/sysfs-bus-usb |  9 +
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..27020293c85b 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,15 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
 
+What:  /sys/bus/usb/devices/.../(hub 
interface)/portX/over_current_count
+Date:      February 2018
+Contact:   Richard Leitner 
+Description:
+   Most hubs are able to detect over-current situations on their
+   ports and report them to the kernel. This attribute is to expose
+   the number of over-current situation occurred on a specific port
+   to user space. This file will contain an unsigned int.
+
 What:  /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
 Date:  November 2015
 Contact:   Lu Baolu 
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..6f779b518e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->over_current_count++;
 
-   dev_dbg(&port_dev->dev, "over-current change\n");
+   dev_dbg(&port_dev->dev, "over-current change #%u\n",
+   port_dev->over_current_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..78d7f4dad618 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int over_current_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..6979bde87d31 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t over_current_count_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->over_current_count);
+}
+static DEVICE_ATTR_RO(over_current_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
&dev_attr_connect_type.attr,
+   &dev_attr_over_current_count.attr,
NULL,
 };
 
-- 
2.11.0

Re: [PATCH] usb: core: introduce per-port over-current counters

[Richard Leitner]

On 02/19/2018 04:59 PM, Greg KH wrote:
> On Mon, Feb 19, 2018 at 01:01:07PM +0100, Richard Leitner wrote:
>> From: Richard Leitner 
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant. Therefore
>> introduce a oc_counter in the usb port struct which is exported via
>> sysfs.
>>
>> Signed-off-by: Richard Leitner 
>> ---
>> Tested on an i.MX6DL based board.
>> ---
>>  drivers/usb/core/hub.c  |  4 +++-
>>  drivers/usb/core/hub.h  |  1 +
>>  drivers/usb/core/port.c | 10 ++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> When you add/remove/modify a sysfs attribute, you always have to
> document in Documentation/ABI/

Ok. Thank you. Seems I missed that, Sorry!

>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index c5c1f6cf3228..448fba1e1827 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
>>  
>>  if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
>>  u16 status = 0, unused;
>> +port_dev->oc_count++;
>>  
>> -dev_dbg(&port_dev->dev, "over-current change\n");
>> +dev_dbg(&port_dev->dev, "over-current change #%u\n",
>> +port_dev->oc_count);
>>  usb_clear_port_feature(hdev, port1,
>>  USB_PORT_FEAT_C_OVER_CURRENT);
>>  msleep(100);/* Cool down */
>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
>> index 2a700ccc868c..b5cf567bf9e2 100644
>> --- a/drivers/usb/core/hub.h
>> +++ b/drivers/usb/core/hub.h
>> @@ -100,6 +100,7 @@ struct usb_port {
>>  unsigned int is_superspeed:1;
>>  unsigned int usb3_lpm_u1_permit:1;
>>  unsigned int usb3_lpm_u2_permit:1;
>> +unsigned int oc_count;
>>  };
>>  
>>  #define to_usb_port(_dev) \
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 1a01e9ad3804..0bfe410eb8a7 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RO(connect_type);
>>  
>> +static ssize_t oc_count_show(struct device *dev, struct device_attribute 
>> *attr,
>> + char *buf)
>> +{
>> +struct usb_port *port_dev = to_usb_port(dev);
>> +
>> +return sprintf(buf, "%u\n", port_dev->oc_count);
>> +}
>> +static DEVICE_ATTR_RO(oc_count);
> 
> I don't see what userspace can do with this number, as there's not much
> it can do with it.

I've answered this question to Felipe already:

On 02/19/2018 03:12 PM, Richard Leitner wrote:
> In our case we have a series of USB hardware (using the cp210x driver) which
> communicates using a proprietary protocol. These devices sometimes trigger an
> over-current situation on some hubs. In case of such an over-current situation
> the USB devices offer an interface for reducing the max used power.
> As these conditions are quite rare and imply performance reductions of the
> device we don't want to use reduce the max power always.
> 
> With this patch I want to give the user-space application the possibility to
> react adequately to such over-current situations.
> 
> I hope this explains my motivation for this patch in a comprehensible way.

I'm of course always open for a better/cleaner/safer way of doing this ;-)

Thank you!

regards;Richard.L

Re: [PATCH] usb: core: introduce per-port over-current counters

[Richard Leitner]

Hi,

On 02/19/2018 02:53 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Richard Leitner  writes:
> 
>> From: Richard Leitner 
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant. Therefore
>> introduce a oc_counter in the usb port struct which is exported via
>> sysfs.
> 
> relevant how? What can the application do with that knowledge?

In our case we have a series of USB hardware (using the cp210x driver) which
communicates using a proprietary protocol. These devices sometimes trigger an
over-current situation on some hubs. In case of such an over-current situation
the USB devices offer an interface for reducing the max used power.
As these conditions are quite rare and imply performance reductions of the
device we don't want to use reduce the max power always.

With this patch I want to give the user-space application the possibility to
react adequately to such over-current situations.

I hope this explains my motivation for this patch in a comprehensible way.

> 
>> Signed-off-by: Richard Leitner 
>> ---
>> Tested on an i.MX6DL based board.
>> ---
>>  drivers/usb/core/hub.c  |  4 +++-
>>  drivers/usb/core/hub.h  |  1 +
>>  drivers/usb/core/port.c | 10 ++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index c5c1f6cf3228..448fba1e1827 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
>>  
>>  if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
>>  u16 status = 0, unused;
>> +port_dev->oc_count++;
>>  
>> -dev_dbg(&port_dev->dev, "over-current change\n");
>> +dev_dbg(&port_dev->dev, "over-current change #%u\n",
>> +port_dev->oc_count);
>>  usb_clear_port_feature(hdev, port1,
>>  USB_PORT_FEAT_C_OVER_CURRENT);
>>  msleep(100);/* Cool down */
>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
>> index 2a700ccc868c..b5cf567bf9e2 100644
>> --- a/drivers/usb/core/hub.h
>> +++ b/drivers/usb/core/hub.h
>> @@ -100,6 +100,7 @@ struct usb_port {
>>  unsigned int is_superspeed:1;
>>  unsigned int usb3_lpm_u1_permit:1;
>>  unsigned int usb3_lpm_u2_permit:1;
>> +unsigned int oc_count;
>>  };
>>  
>>  #define to_usb_port(_dev) \
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 1a01e9ad3804..0bfe410eb8a7 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RO(connect_type);
>>  
>> +static ssize_t oc_count_show(struct device *dev, struct device_attribute 
>> *attr,
>> + char *buf)
>> +{
>> +struct usb_port *port_dev = to_usb_port(dev);
>> +
>> +return sprintf(buf, "%u\n", port_dev->oc_count);
>> +}
>> +static DEVICE_ATTR_RO(oc_count);
> 
> I would actually spell this out human readable: over_current_count
> 

Would be fine for me.

regards;Richard.L

[PATCH] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner 

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant. Therefore
introduce a oc_counter in the usb port struct which is exported via
sysfs.

Signed-off-by: Richard Leitner 
---
Tested on an i.MX6DL based board.
---
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..448fba1e1827 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->oc_count++;
 
-   dev_dbg(&port_dev->dev, "over-current change\n");
+   dev_dbg(&port_dev->dev, "over-current change #%u\n",
+   port_dev->oc_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..b5cf567bf9e2 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int oc_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..0bfe410eb8a7 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t oc_count_show(struct device *dev, struct device_attribute *attr,
+char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->oc_count);
+}
+static DEVICE_ATTR_RO(oc_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
&dev_attr_connect_type.attr,
+   &dev_attr_oc_count.attr,
NULL,
 };
 
-- 
2.11.0

[PATCH net-next] phylib: rename reset-(post-)delay-us to reset-(de)assert-us

[Richard Leitner]

From: Richard Leitner 

As suggested by Rob Herring [1] rename the previously introduced
reset-{,post-}delay-us bindings to the clearer reset-{,de}assert-us

[1] https://patchwork.kernel.org/patch/10104905/

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/net/phy.txt | 8 
 drivers/net/phy/mdio_device.c | 2 +-
 drivers/of/of_mdio.c  | 7 ---
 include/linux/mdio.h  | 4 ++--
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt 
b/Documentation/devicetree/bindings/net/phy.txt
index 72860ce7f610..d2169a56f5e3 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -55,10 +55,10 @@ Optional Properties:
 
 - reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
 
-- reset-delay-us: Delay after the reset was asserted in microseconds.
+- reset-assert-us: Delay after the reset was asserted in microseconds.
   If this property is missing the delay will be skipped.
 
-- reset-post-delay-us: Delay after the reset was deasserted in microseconds.
+- reset-deassert-us: Delay after the reset was deasserted in microseconds.
   If this property is missing the delay will be skipped.
 
 Example:
@@ -70,6 +70,6 @@ ethernet-phy@0 {
reg = <0>;
 
reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
-   reset-delay-us = <1000>;
-   reset-post-delay-us = <2000>;
+   reset-assert-us = <1000>;
+   reset-deassert-us = <2000>;
 };
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 843c1dde93e4..c924700cf37b 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -126,7 +126,7 @@ void mdio_device_reset(struct mdio_device *mdiodev, int 
value)
 
gpiod_set_value(mdiodev->reset, value);
 
-   d = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
+   d = value ? mdiodev->reset_assert_delay : mdiodev->reset_deassert_delay;
if (d)
usleep_range(d, d + max_t(unsigned int, d / 10, 100));
 }
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index f4c73292b304..1b9ef35cf0d9 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -77,9 +77,10 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
if (of_property_read_bool(child, "broken-turn-around"))
mdio->phy_ignore_ta_mask |= 1 << addr;
 
-   of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay);
-   of_property_read_u32(child, "reset-post-delay-us",
-&phy->mdio.reset_post_delay);
+   of_property_read_u32(child, "reset-assert-us",
+&phy->mdio.reset_assert_delay);
+   of_property_read_u32(child, "reset-deassert-us",
+&phy->mdio.reset_deassert_delay);
 
/* Associate the OF node with the device structure so it
 * can be looked up later */
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index e37c21d8eb19..268aad47ecd3 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -41,8 +41,8 @@ struct mdio_device {
int addr;
int flags;
struct gpio_desc *reset;
-   unsigned int reset_delay;
-   unsigned int reset_post_delay;
+   unsigned int reset_assert_delay;
+   unsigned int reset_deassert_delay;
 };
 #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
 
-- 
2.11.0

Re: [PATCH net-next v5 1/4] phylib: Add device reset delay support

[Richard Leitner]

Hi Rob,

On 12/15/2017 11:17 PM, Rob Herring wrote:
> On Mon, Dec 11, 2017 at 01:16:57PM +0100, Richard Leitner wrote:
>> From: Richard Leitner 
>>
>> Some PHYs need a minimum time after the reset gpio was asserted and/or
>> deasserted. To ensure we meet these timing requirements add two new
>> optional devicetree parameters for the phy: reset-delay-us and
>> reset-post-delay-us.
>>
>> Signed-off-by: Richard Leitner 
>> Reviewed-by: Geert Uytterhoeven 
>> ---
>>  Documentation/devicetree/bindings/net/phy.txt | 10 ++
>>  drivers/net/phy/mdio_device.c | 13 +++--
>>  drivers/of/of_mdio.c  |  4 
>>  include/linux/mdio.h  |  2 ++
>>  4 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/phy.txt 
>> b/Documentation/devicetree/bindings/net/phy.txt
>> index c05479f5ac7c..72860ce7f610 100644
>> --- a/Documentation/devicetree/bindings/net/phy.txt
>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>> @@ -55,6 +55,12 @@ Optional Properties:
>>  
>>  - reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
>>  
>> +- reset-delay-us: Delay after the reset was asserted in microseconds.
>> +  If this property is missing the delay will be skipped.
>> +
>> +- reset-post-delay-us: Delay after the reset was deasserted in microseconds.
>> +  If this property is missing the delay will be skipped.
> 
> I think these names could be clearer as to exactly what they mean. 
> Looking at existing properties with "reset-delay" there's a mixture of 
> definitions whether it is the assert time or the time after deassert.
> 
> So I'd call these "reset-assert-us" and "reset-deassert-us".

Ok, that would be fine with me, but are you sure that we should omit the
"-delay" term completely?

What would be the best approach to post this change (as the patchset was
already merged to net-next)? A separate patch or a v6 of the complete
patchset?

> 
> Rob
> 

regards;Richard.L

[PATCH net-next v5 2/4] phylib: add reset after clk enable support

[Richard Leitner]

From: Richard Leitner 

Some PHYs need the refclk to be a continuous clock. Therefore they don't
allow turning it off and on again during operation. Nonetheless such a
clock switching is performed by some ETH drivers (namely FEC [1]) for
power saving reasons. An example for an affected PHY is the
SMSC/Microchip LAN8720 in "REF_CLK In Mode".

In order to provide a uniform method to overcome this problem this patch
adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
function phy_reset_after_clk_enable() to the phylib. These should be
used to trigger reset of the PHY after the refclk is switched on again.

[1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save 
power")

Signed-off-by: Richard Leitner 
Reviewed-by: Andrew Lunn 
---
 drivers/net/phy/phy_device.c | 24 
 include/linux/phy.h  |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1de5e242b8b4..462c17ed87b8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1218,6 +1218,30 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+/**
+ * phy_reset_after_clk_enable - perform a PHY reset if needed
+ * @phydev: target phy_device struct
+ *
+ * Description: Some PHYs are known to need a reset after their refclk was
+ *   enabled. This function evaluates the flags and perform the reset if it's
+ *   needed. Returns < 0 on error, 0 if the phy wasn't reset and 1 if the phy
+ *   was reset.
+ */
+int phy_reset_after_clk_enable(struct phy_device *phydev)
+{
+   if (!phydev || !phydev->drv)
+   return -ENODEV;
+
+   if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
+   phy_device_reset(phydev, 1);
+   phy_device_reset(phydev, 0);
+   return 1;
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL(phy_reset_after_clk_enable);
+
 /* Generic PHY support and helper functions */
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index d3037e2ffbc4..c4b4715caa21 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -59,6 +59,7 @@
 
 #define PHY_HAS_INTERRUPT  0x0001
 #define PHY_IS_INTERNAL0x0002
+#define PHY_RST_AFTER_CLK_EN   0x0004
 #define MDIO_DEVICE_IS_PHY 0x8000
 
 /* Interface Mode definitions */
@@ -853,6 +854,7 @@ int phy_aneg_done(struct phy_device *phydev);
 
 int phy_stop_interrupts(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
+int phy_reset_after_clk_enable(struct phy_device *phydev);
 
 static inline void phy_device_reset(struct phy_device *phydev, int value)
 {
-- 
2.11.0

[PATCH net-next v5 0/4] net: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

From: Richard Leitner 

This patch series fixes the use of the SMSC LAN8710/20 with a Freescale ETH
when the refclk is generated by the FSL.

This patchset depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov, which was merged to
net-next as commit bafbdd527d56 ("phylib: Add device reset GPIO support").

Changes v5:
- fix reset delay calculation (max_t instead of min_t)

Changes v4:
- simplify dts parsing
- simplify reset delay evaluation and execution
- fec: ensure to only reset once during fec_enet_open()
- remove dependency notes from commit message
- add reviews and acks

Changes v3:
- use phylib to hard-reset the PHY
- implement reset delays in phylib
- add new phylib API & flag (PHY_RST_AFTER_CLK_EN) to determine if
  a PHY is affected

Changes v2:
- simplify and fix fec_reset_phy function to support multiple calls
- include: linux: phy: harmonize phy_id{,_mask} type
- reset the phy instead of not turning the clock on and off
  (which would have caused a power consumption regression)

Richard Leitner (4):
  phylib: Add device reset delay support
  phylib: add reset after clk enable support
  net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag
  net: fec: add phy_reset_after_clk_enable() support

 Documentation/devicetree/bindings/net/phy.txt | 10 ++
 drivers/net/ethernet/freescale/fec_main.c | 20 
 drivers/net/phy/mdio_device.c | 13 +++--
 drivers/net/phy/phy_device.c  | 24 
 drivers/net/phy/smsc.c|  2 +-
 drivers/of/of_mdio.c  |  4 
 include/linux/mdio.h  |  2 ++
 include/linux/phy.h   |  2 ++
 8 files changed, 74 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH net-next v5 4/4] net: fec: add phy_reset_after_clk_enable() support

[Richard Leitner]

From: Richard Leitner 

Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
the refclk on and off again during operation (according to their
datasheet). Nonetheless exactly this behaviour was introduced for power
saving reasons by commit e8fcfcd5684a ("net: fec: optimize the clock management 
to save power").
Therefore add support for the phy_reset_after_clk_enable function from
phylib to mitigate this issue.

Generally speaking this issue is only relevant if the ref clk for the
PHY is generated by the SoC and therefore the PHY is configured to
"REF_CLK In Mode". In our specific case (PCB) this problem does occur at
about every 10th to 50th POR of an LAN8710 connected to an i.MX6SOLO
SoC. The typical symptom of this problem is a "swinging" ethernet link.
Similar issues were reported by users of the NXP forum:
https://community.nxp.com/thread/389902
https://community.nxp.com/message/309354
With this patch applied the issue didn't occur for at least a few
hundret PORs of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
Signed-off-by: Richard Leitner 
---
 drivers/net/ethernet/freescale/fec_main.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 610573855213..2d1b06579c1a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
ret = clk_prepare_enable(fep->clk_ref);
if (ret)
goto failed_clk_ref;
+
+   phy_reset_after_clk_enable(ndev->phydev);
} else {
clk_disable_unprepare(fep->clk_ahb);
clk_disable_unprepare(fep->clk_enet_out);
@@ -2834,6 +2836,7 @@ fec_enet_open(struct net_device *ndev)
 {
struct fec_enet_private *fep = netdev_priv(ndev);
int ret;
+   bool reset_again;
 
ret = pm_runtime_get_sync(&fep->pdev->dev);
if (ret < 0)
@@ -2844,6 +2847,17 @@ fec_enet_open(struct net_device *ndev)
if (ret)
goto clk_enable;
 
+   /* During the first fec_enet_open call the PHY isn't probed at this
+* point. Therefore the phy_reset_after_clk_enable() call within
+* fec_enet_clk_enable() fails. As we need this reset in order to be
+* sure the PHY is working correctly we check if we need to reset again
+* later when the PHY is probed
+*/
+   if (ndev->phydev && ndev->phydev->drv)
+   reset_again = false;
+   else
+   reset_again = true;
+
/* I should reset the ring buffers here, but I don't yet know
 * a simple way to do that.
 */
@@ -2860,6 +2874,12 @@ fec_enet_open(struct net_device *ndev)
if (ret)
goto err_enet_mii_probe;
 
+   /* Call phy_reset_after_clk_enable() again if it failed during
+* phy_reset_after_clk_enable() before because the PHY wasn't probed.
+*/
+   if (reset_again)
+   phy_reset_after_clk_enable(ndev->phydev);
+
if (fep->quirks & FEC_QUIRK_ERR006687)
imx6q_cpuidle_fec_irqs_used();
 
-- 
2.11.0

[PATCH net-next v5 3/4] net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag

[Richard Leitner]

From: Richard Leitner 

The Microchip/SMSC LAN8710/LAN8720 PHYs need (according to their
datasheet [1]) a continuous REF_CLK when configured to "REF_CLK In Mode".
Therefore set the PHY_RST_AFTER_CLK_EN flag for those PHYs to let the
ETH driver reset them after the REF_CLK is enabled.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/2165B.pdf

Signed-off-by: Richard Leitner 
Reviewed-by: Andrew Lunn 
---
 drivers/net/phy/smsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index a1961ba87e2b..be399d645224 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -312,7 +312,7 @@ static struct phy_driver smsc_phy_driver[] = {
.name   = "SMSC LAN8710/LAN8720",
 
.features   = PHY_BASIC_FEATURES,
-   .flags  = PHY_HAS_INTERRUPT,
+   .flags  = PHY_HAS_INTERRUPT | PHY_RST_AFTER_CLK_EN,
 
.probe  = smsc_phy_probe,
 
-- 
2.11.0

[PATCH net-next v5 1/4] phylib: Add device reset delay support

[Richard Leitner]

From: Richard Leitner 

Some PHYs need a minimum time after the reset gpio was asserted and/or
deasserted. To ensure we meet these timing requirements add two new
optional devicetree parameters for the phy: reset-delay-us and
reset-post-delay-us.

Signed-off-by: Richard Leitner 
Reviewed-by: Geert Uytterhoeven 
---
 Documentation/devicetree/bindings/net/phy.txt | 10 ++
 drivers/net/phy/mdio_device.c | 13 +++--
 drivers/of/of_mdio.c  |  4 
 include/linux/mdio.h  |  2 ++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt 
b/Documentation/devicetree/bindings/net/phy.txt
index c05479f5ac7c..72860ce7f610 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -55,6 +55,12 @@ Optional Properties:
 
 - reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
 
+- reset-delay-us: Delay after the reset was asserted in microseconds.
+  If this property is missing the delay will be skipped.
+
+- reset-post-delay-us: Delay after the reset was deasserted in microseconds.
+  If this property is missing the delay will be skipped.
+
 Example:
 
 ethernet-phy@0 {
@@ -62,4 +68,8 @@ ethernet-phy@0 {
interrupt-parent = <&PIC>;
interrupts = <35 IRQ_TYPE_EDGE_RISING>;
reg = <0>;
+
+   reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
+   reset-delay-us = <1000>;
+   reset-post-delay-us = <2000>;
 };
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 75d97dd9fb28..843c1dde93e4 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 void mdio_device_free(struct mdio_device *mdiodev)
 {
@@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
 
 void mdio_device_reset(struct mdio_device *mdiodev, int value)
 {
-   if (mdiodev->reset)
-   gpiod_set_value(mdiodev->reset, value);
+   unsigned int d;
+
+   if (!mdiodev->reset)
+   return;
+
+   gpiod_set_value(mdiodev->reset, value);
+
+   d = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
+   if (d)
+   usleep_range(d, d + max_t(unsigned int, d / 10, 100));
 }
 EXPORT_SYMBOL(mdio_device_reset);
 
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 98258583abb0..7c8767176315 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -77,6 +77,10 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
if (of_property_read_bool(child, "broken-turn-around"))
mdio->phy_ignore_ta_mask |= 1 << addr;
 
+   of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay);
+   of_property_read_u32(child, "reset-post-delay-us",
+&phy->mdio.reset_post_delay);
+
/* Associate the OF node with the device structure so it
 * can be looked up later */
of_node_get(child);
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 92d4e55ffe67..e37c21d8eb19 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -41,6 +41,8 @@ struct mdio_device {
int addr;
int flags;
struct gpio_desc *reset;
+   unsigned int reset_delay;
+   unsigned int reset_post_delay;
 };
 #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
 
-- 
2.11.0

Re: [PATCH net-next v4 1/4] phylib: Add device reset delay support

[Richard Leitner]

Hi Geert,

On 12/07/2017 03:52 PM, Geert Uytterhoeven wrote:
> Hi Richard,
> 
> On Thu, Dec 7, 2017 at 3:43 PM, Richard Leitner  wrote:
>> --- a/drivers/net/phy/mdio_device.c
>> +++ b/drivers/net/phy/mdio_device.c
>> @@ -24,6 +24,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  void mdio_device_free(struct mdio_device *mdiodev)
>>  {
>> @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
>>
>>  void mdio_device_reset(struct mdio_device *mdiodev, int value)
>>  {
>> -   if (mdiodev->reset)
>> -   gpiod_set_value(mdiodev->reset, value);
>> +   unsigned int d;
>> +
>> +   if (!mdiodev->reset)
>> +   return;
>> +
>> +   gpiod_set_value(mdiodev->reset, value);
>> +
>> +   d = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
>> +   if (d)
>> +   usleep_range(d, d + min_t(unsigned int, d / 10, 100));
> 
> Oops, I meant "max_t", not "min_t", else the upper limit can be "d + 0",
> which is not what we want.

You're right...

> Sorry, my fault.

I just copied it over from you suggestion without thinking about it...
So it's definitely my fault too ;-)

I'll wait for some more comments and send a new version next week.

regards;Richard.L

[PATCH net-next v4 3/4] net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag

[Richard Leitner]

From: Richard Leitner 

The Microchip/SMSC LAN8710/LAN8720 PHYs need (according to their
datasheet [1]) a continuous REF_CLK when configured to "REF_CLK In Mode".
Therefore set the PHY_RST_AFTER_CLK_EN flag for those PHYs to let the
ETH driver reset them after the REF_CLK is enabled.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/2165B.pdf

Signed-off-by: Richard Leitner 
Reviewed-by: Andrew Lunn 
---
 drivers/net/phy/smsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index a1961ba87e2b..be399d645224 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -312,7 +312,7 @@ static struct phy_driver smsc_phy_driver[] = {
.name   = "SMSC LAN8710/LAN8720",
 
.features   = PHY_BASIC_FEATURES,
-   .flags  = PHY_HAS_INTERRUPT,
+   .flags  = PHY_HAS_INTERRUPT | PHY_RST_AFTER_CLK_EN,
 
.probe  = smsc_phy_probe,
 
-- 
2.11.0

[PATCH net-next v4 0/4] net: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

From: Richard Leitner 

This patch series fixes the use of the SMSC LAN8710/20 with a Freescale ETH
when the refclk is generated by the FSL.

This patchset depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov, which was merged to
net-next as commit bafbdd527d56 ("phylib: Add device reset GPIO support").

Changes v4:
- simplify dts parsing
- simplify reset delay evaluation and execution
- fec: ensure to only reset once during fec_enet_open()
- remove dependency notes from commit message
- add reviews and acks

Changes v3:
- use phylib to hard-reset the PHY
- implement reset delays in phylib
- add new phylib API & flag (PHY_RST_AFTER_CLK_EN) to determine if
  a PHY is affected

Changes v2:
- simplify and fix fec_reset_phy function to support multiple calls
- include: linux: phy: harmonize phy_id{,_mask} type
- reset the phy instead of not turning the clock on and off
  (which would have caused a power consumption regression)

Richard Leitner (4):
  phylib: Add device reset delay support
  phylib: add reset after clk enable support
  net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag
  net: fec: add phy_reset_after_clk_enable() support

 Documentation/devicetree/bindings/net/phy.txt | 10 ++
 drivers/net/ethernet/freescale/fec_main.c | 20 
 drivers/net/phy/mdio_device.c | 13 +++--
 drivers/net/phy/phy_device.c  | 24 
 drivers/net/phy/smsc.c|  2 +-
 drivers/of/of_mdio.c  |  4 
 include/linux/mdio.h  |  2 ++
 include/linux/phy.h   |  2 ++
 8 files changed, 74 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH net-next v4 4/4] net: fec: add phy_reset_after_clk_enable() support

[Richard Leitner]

From: Richard Leitner 

Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
the refclk on and off again during operation (according to their
datasheet). Nonetheless exactly this behaviour was introduced for power
saving reasons by commit e8fcfcd5684a ("net: fec: optimize the clock management 
to save power").
Therefore add support for the phy_reset_after_clk_enable function from
phylib to mitigate this issue.

Generally speaking this issue is only relevant if the ref clk for the
PHY is generated by the SoC and therefore the PHY is configured to
"REF_CLK In Mode". In our specific case (PCB) this problem does occur at
about every 10th to 50th POR of an LAN8710 connected to an i.MX6SOLO
SoC. The typical symptom of this problem is a "swinging" ethernet link.
Similar issues were reported by users of the NXP forum:
https://community.nxp.com/thread/389902
https://community.nxp.com/message/309354
With this patch applied the issue didn't occur for at least a few
hundret PORs of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
Signed-off-by: Richard Leitner 
---
 drivers/net/ethernet/freescale/fec_main.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 610573855213..2d1b06579c1a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
ret = clk_prepare_enable(fep->clk_ref);
if (ret)
goto failed_clk_ref;
+
+   phy_reset_after_clk_enable(ndev->phydev);
} else {
clk_disable_unprepare(fep->clk_ahb);
clk_disable_unprepare(fep->clk_enet_out);
@@ -2834,6 +2836,7 @@ fec_enet_open(struct net_device *ndev)
 {
struct fec_enet_private *fep = netdev_priv(ndev);
int ret;
+   bool reset_again;
 
ret = pm_runtime_get_sync(&fep->pdev->dev);
if (ret < 0)
@@ -2844,6 +2847,17 @@ fec_enet_open(struct net_device *ndev)
if (ret)
goto clk_enable;
 
+   /* During the first fec_enet_open call the PHY isn't probed at this
+* point. Therefore the phy_reset_after_clk_enable() call within
+* fec_enet_clk_enable() fails. As we need this reset in order to be
+* sure the PHY is working correctly we check if we need to reset again
+* later when the PHY is probed
+*/
+   if (ndev->phydev && ndev->phydev->drv)
+   reset_again = false;
+   else
+   reset_again = true;
+
/* I should reset the ring buffers here, but I don't yet know
 * a simple way to do that.
 */
@@ -2860,6 +2874,12 @@ fec_enet_open(struct net_device *ndev)
if (ret)
goto err_enet_mii_probe;
 
+   /* Call phy_reset_after_clk_enable() again if it failed during
+* phy_reset_after_clk_enable() before because the PHY wasn't probed.
+*/
+   if (reset_again)
+   phy_reset_after_clk_enable(ndev->phydev);
+
if (fep->quirks & FEC_QUIRK_ERR006687)
imx6q_cpuidle_fec_irqs_used();
 
-- 
2.11.0

[PATCH net-next v4 2/4] phylib: add reset after clk enable support

[Richard Leitner]

From: Richard Leitner 

Some PHYs need the refclk to be a continuous clock. Therefore they don't
allow turning it off and on again during operation. Nonetheless such a
clock switching is performed by some ETH drivers (namely FEC [1]) for
power saving reasons. An example for an affected PHY is the
SMSC/Microchip LAN8720 in "REF_CLK In Mode".

In order to provide a uniform method to overcome this problem this patch
adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
function phy_reset_after_clk_enable() to the phylib. These should be
used to trigger reset of the PHY after the refclk is switched on again.

[1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save 
power")

Signed-off-by: Richard Leitner 
Reviewed-by: Andrew Lunn 
---
 drivers/net/phy/phy_device.c | 24 
 include/linux/phy.h  |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1de5e242b8b4..462c17ed87b8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1218,6 +1218,30 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+/**
+ * phy_reset_after_clk_enable - perform a PHY reset if needed
+ * @phydev: target phy_device struct
+ *
+ * Description: Some PHYs are known to need a reset after their refclk was
+ *   enabled. This function evaluates the flags and perform the reset if it's
+ *   needed. Returns < 0 on error, 0 if the phy wasn't reset and 1 if the phy
+ *   was reset.
+ */
+int phy_reset_after_clk_enable(struct phy_device *phydev)
+{
+   if (!phydev || !phydev->drv)
+   return -ENODEV;
+
+   if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
+   phy_device_reset(phydev, 1);
+   phy_device_reset(phydev, 0);
+   return 1;
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL(phy_reset_after_clk_enable);
+
 /* Generic PHY support and helper functions */
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index d3037e2ffbc4..c4b4715caa21 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -59,6 +59,7 @@
 
 #define PHY_HAS_INTERRUPT  0x0001
 #define PHY_IS_INTERNAL0x0002
+#define PHY_RST_AFTER_CLK_EN   0x0004
 #define MDIO_DEVICE_IS_PHY 0x8000
 
 /* Interface Mode definitions */
@@ -853,6 +854,7 @@ int phy_aneg_done(struct phy_device *phydev);
 
 int phy_stop_interrupts(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
+int phy_reset_after_clk_enable(struct phy_device *phydev);
 
 static inline void phy_device_reset(struct phy_device *phydev, int value)
 {
-- 
2.11.0

[PATCH net-next v4 1/4] phylib: Add device reset delay support

[Richard Leitner]

From: Richard Leitner 

Some PHYs need a minimum time after the reset gpio was asserted and/or
deasserted. To ensure we meet these timing requirements add two new
optional devicetree parameters for the phy: reset-delay-us and
reset-post-delay-us.

Signed-off-by: Richard Leitner 
Reviewed-by: Geert Uytterhoeven 
---
 Documentation/devicetree/bindings/net/phy.txt | 10 ++
 drivers/net/phy/mdio_device.c | 13 +++--
 drivers/of/of_mdio.c  |  4 
 include/linux/mdio.h  |  2 ++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt 
b/Documentation/devicetree/bindings/net/phy.txt
index c05479f5ac7c..72860ce7f610 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -55,6 +55,12 @@ Optional Properties:
 
 - reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
 
+- reset-delay-us: Delay after the reset was asserted in microseconds.
+  If this property is missing the delay will be skipped.
+
+- reset-post-delay-us: Delay after the reset was deasserted in microseconds.
+  If this property is missing the delay will be skipped.
+
 Example:
 
 ethernet-phy@0 {
@@ -62,4 +68,8 @@ ethernet-phy@0 {
interrupt-parent = <&PIC>;
interrupts = <35 IRQ_TYPE_EDGE_RISING>;
reg = <0>;
+
+   reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
+   reset-delay-us = <1000>;
+   reset-post-delay-us = <2000>;
 };
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 75d97dd9fb28..0423280c88fe 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 void mdio_device_free(struct mdio_device *mdiodev)
 {
@@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
 
 void mdio_device_reset(struct mdio_device *mdiodev, int value)
 {
-   if (mdiodev->reset)
-   gpiod_set_value(mdiodev->reset, value);
+   unsigned int d;
+
+   if (!mdiodev->reset)
+   return;
+
+   gpiod_set_value(mdiodev->reset, value);
+
+   d = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
+   if (d)
+   usleep_range(d, d + min_t(unsigned int, d / 10, 100));
 }
 EXPORT_SYMBOL(mdio_device_reset);
 
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 98258583abb0..7c8767176315 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -77,6 +77,10 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
if (of_property_read_bool(child, "broken-turn-around"))
mdio->phy_ignore_ta_mask |= 1 << addr;
 
+   of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay);
+   of_property_read_u32(child, "reset-post-delay-us",
+&phy->mdio.reset_post_delay);
+
/* Associate the OF node with the device structure so it
 * can be looked up later */
of_node_get(child);
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 92d4e55ffe67..e37c21d8eb19 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -41,6 +41,8 @@ struct mdio_device {
int addr;
int flags;
struct gpio_desc *reset;
+   unsigned int reset_delay;
+   unsigned int reset_post_delay;
 };
 #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
 
-- 
2.11.0

Re: [PATCH net-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support

[Richard Leitner]

Hi Andy,

On 12/06/2017 02:50 AM, Andy Duan wrote:
> From: Richard Leitner  Sent: Tuesday, December 05, 2017 9:26 
> PM
>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>> the refclk on and off again during operation (according to their datasheet).
>> Nonetheless exactly this behaviour was introduced for power saving reasons
>> by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>> power").
>> Therefore add support for the phy_reset_after_clk_enable function from
>> phylib to mitigate this issue.

...

>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 610573855213..8c3d0fb7db20 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device
>> *ndev, bool enable)
>>  ret = clk_prepare_enable(fep->clk_ref);
>>  if (ret)
>>  goto failed_clk_ref;
>> +
>> +phy_reset_after_clk_enable(ndev->phydev);
>>  } else {
>>  clk_disable_unprepare(fep->clk_ahb);
>>  clk_disable_unprepare(fep->clk_enet_out);
>> @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
>>  if (ret)
>>  goto err_enet_mii_probe;
>>
>> +/* reset phy if needed here, due to the fact this is the first time we
>> + * have the net_device to phy_driver link
>> + */
>> +phy_reset_after_clk_enable(ndev->phydev);
>> +
> 
> The patch series look better.
> But why does it need to reset phy here since phy already is hard reset after 
> clock enable.

The problem here is that in fec_enet_open() the fec_enet_clk_enable()
call is done before the phy is probed. Therefore (as mentioned in the
comment) there's no link from the net_device (ndev) to the phy_driver
(which holds the flags).

Any suggestions for a better solution are highly appreciated here! Thanks!

regards;Richard.L

Re: [PATCH net-next v3 2/4] phylib: add reset after clk enable support

[Richard Leitner]

Hi Andrew,

On 12/05/2017 06:34 PM, Andrew Lunn wrote:

On Tue, Dec 05, 2017 at 02:25:58PM +0100, Richard Leitner wrote:

From: Richard Leitner 

Some PHYs need the refclk to be a continuous clock. Therefore they don't
allow turning it off and on again during operation. Nonetheless such a
clock switching is performed by some ETH drivers (namely FEC [1]) for
power saving reasons. An example for an affected PHY is the
SMSC/Microchip LAN8720 in "REF_CLK In Mode".

In order to provide a uniform method to overcome this problem this patch
adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
function phy_reset_after_clk_enable() to the phylib. These should be
used to trigger reset of the PHY after the refclk is switched on again.

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov [2].

[1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save 
power")
[2] https://patchwork.kernel.org/patch/10090149/

Signed-off-by: Richard Leitner 


Hi Richard

Same comment about moving text below the ---


Ok. Thanks for your feedback and review.



Reviewed-by: Andrew Lunn 



regards;Richard.L


 Andrew

Re: [PATCH net-next v3 1/4] phylib: Add device reset delay support

[Richard Leitner]

Hi Andrew,

On 12/05/2017 06:28 PM, Andrew Lunn wrote:

Hi Richard


+++ b/drivers/of/of_mdio.c
@@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
if (of_property_read_bool(child, "broken-turn-around"))
mdio->phy_ignore_ta_mask |= 1 << addr;
  
+	if (of_property_read_u32(child, "reset-delay-us",

+&phy->mdio.reset_delay))
+   phy->mdio.reset_delay = 0;
+
+   if (of_property_read_u32(child, "reset-post-delay-us",
+&phy->mdio.reset_post_delay))
+   phy->mdio.reset_post_delay = 0;


of_property_read_u32() should not change the variable you pass to it,
if it does not find the property. So you can change this to:

phy->mdio.reset_delay = 0;
phy->mdio.reset_post_delay = 0;

of_property_read_u32(child, "reset-delay-us",
 &phy->mdio.reset_delay);

of_property_read_u32(child, "reset-post-delay-us",
 &phy->mdio.reset_post_delay);


Geert already pointed this out, but he said it's possible to omit also 
the zeroing of the variables.


> On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote:
>> If of_property_read_u32() fails, it doesn't write to its output
>> parameter.
>> As the structure should be zeroed during allocation, you can just
>> write:
>>
>> of_property_read_u32(child, "reset-delay-us",&phy->mdio.reset_delay);
>> of_property_read_u32(child, "reset-post-delay-us",
>>  &phy->mdio.reset_post_delay);

If that's ok I'll take the shorter (Geerts) suggestion for v4.

Nonetheless thanks for your quick feedback!

regards;Richard.L

Re: [PATCH net-next v3 1/4] phylib: Add device reset delay support

[Richard Leitner]

Hi Geert,

On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote:
> Hi Richard,
> 
> On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner  wrote:
>> From: Richard Leitner 
>>
>> Some PHYs need a minimum time after the reset gpio was asserted and/or
>> deasserted. To ensure we meet these timing requirements add two new
>> optional devicetree parameters for the phy: reset-delay-us and
>> reset-post-delay-us.
> 
> Thanks for your patch!
> 
>> This patch depends on the "phylib: Add device reset GPIO support" patch
>> submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
>> https://patchwork.kernel.org/patch/10090149/
> 
> The above paragraph belongs under the "---" line below, as it is not intended
> to be preserved in the eternal git history.

Ok. Thanks. That makes sense. I'll take it into account for v4.

> 
>> Signed-off-by: Richard Leitner 
> 
> Reviewed-by: Geert Uytterhoeven 
> 
> Although I have a few suggestions below:

Thank you for your review and suggestions (they make the code look way
more neater). I'll adapt v4 accordingly.

> 
>> --- a/drivers/net/phy/mdio_device.c
>> +++ b/drivers/net/phy/mdio_device.c
>> @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
>>
>>  void mdio_device_reset(struct mdio_device *mdiodev, int value)
>>  {
>> -   if (mdiodev->reset)
>> -   gpiod_set_value(mdiodev->reset, value);
>> +   if (!mdiodev->reset)
>> +   return;
>> +
>> +   gpiod_set_value(mdiodev->reset, value);
>> +
>> +   if (value && mdiodev->reset_delay)
>> +   usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 
>> 100);
>> +   else if (!value && mdiodev->reset_post_delay)
>> +   usleep_range(mdiodev->reset_post_delay,
>> +mdiodev->reset_post_delay + 100);
> 
> I think this can be written simpler using e.g.:
> 
> unsigned int delay;
> 
> ...
> delay = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
> if (delay)
> usleep_range(delay, delay + 100);
> 
> Perhaps the range extension should be relative, e.g.
> "delay + min(delay / 10, 100)"?
> 
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>> if (of_property_read_bool(child, "broken-turn-around"))
>> mdio->phy_ignore_ta_mask |= 1 << addr;
>>
>> +   if (of_property_read_u32(child, "reset-delay-us",
>> +&phy->mdio.reset_delay))
>> +   phy->mdio.reset_delay = 0;
>> +
>> +   if (of_property_read_u32(child, "reset-post-delay-us",
>> +&phy->mdio.reset_post_delay))
>> +   phy->mdio.reset_post_delay = 0;
> 
> If of_property_read_u32() fails, it doesn't write to its output parameter.
> As the structure should be zeroed during allocation, you can just write:
> 
> of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay);
> of_property_read_u32(child, "reset-post-delay-us",
>  &phy->mdio.reset_post_delay);

[PATCH net-next v3 0/4] net: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

From: Richard Leitner 

This patch series fixes the use of the SMSC LAN8710/20 with a Freescale ETH
when the refclk is generated by the FSL.

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
https://patchwork.kernel.org/patch/10090149/

Changes v3:
- use phylib to hard-reset the PHY
- implement reset delays in phylib
- add new phylib API & flag (PHY_RST_AFTER_CLK_EN) to determine if a PHY
  is affected

Changes v2:
- simplify and fix fec_reset_phy function to support multiple calls
- include: linux: phy: harmonize phy_id{,_mask} type
- reset the phy instead of not turning the clock on and off
  (which would have caused a power consumption regression)

Richard Leitner (4):
  phylib: Add device reset delay support
  phylib: add reset after clk enable support
  net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag
  net: fec: add phy_reset_after_clk_enable() support

 Documentation/devicetree/bindings/net/phy.txt | 10 ++
 drivers/net/ethernet/freescale/fec_main.c |  7 +++
 drivers/net/phy/mdio_device.c | 13 +++--
 drivers/net/phy/phy_device.c  | 24 
 drivers/net/phy/smsc.c|  2 +-
 drivers/of/of_mdio.c  |  8 
 include/linux/mdio.h  |  2 ++
 include/linux/phy.h   |  2 ++
 8 files changed, 65 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH net-next v3 2/4] phylib: add reset after clk enable support

[Richard Leitner]

From: Richard Leitner 

Some PHYs need the refclk to be a continuous clock. Therefore they don't
allow turning it off and on again during operation. Nonetheless such a
clock switching is performed by some ETH drivers (namely FEC [1]) for
power saving reasons. An example for an affected PHY is the
SMSC/Microchip LAN8720 in "REF_CLK In Mode".

In order to provide a uniform method to overcome this problem this patch
adds a new phy_driver flag (PHY_RST_AFTER_CLK_EN) and corresponding
function phy_reset_after_clk_enable() to the phylib. These should be
used to trigger reset of the PHY after the refclk is switched on again.

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov [2].

[1] commit e8fcfcd5684a ("net: fec: optimize the clock management to save 
power")
[2] https://patchwork.kernel.org/patch/10090149/

Signed-off-by: Richard Leitner 
---
 drivers/net/phy/phy_device.c | 24 
 include/linux/phy.h  |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1de5e242b8b4..462c17ed87b8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1218,6 +1218,30 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+/**
+ * phy_reset_after_clk_enable - perform a PHY reset if needed
+ * @phydev: target phy_device struct
+ *
+ * Description: Some PHYs are known to need a reset after their refclk was
+ *   enabled. This function evaluates the flags and perform the reset if it's
+ *   needed. Returns < 0 on error, 0 if the phy wasn't reset and 1 if the phy
+ *   was reset.
+ */
+int phy_reset_after_clk_enable(struct phy_device *phydev)
+{
+   if (!phydev || !phydev->drv)
+   return -ENODEV;
+
+   if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
+   phy_device_reset(phydev, 1);
+   phy_device_reset(phydev, 0);
+   return 1;
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL(phy_reset_after_clk_enable);
+
 /* Generic PHY support and helper functions */
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2bcbe894eb10..5c05fc73af70 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -59,6 +59,7 @@
 
 #define PHY_HAS_INTERRUPT  0x0001
 #define PHY_IS_INTERNAL0x0002
+#define PHY_RST_AFTER_CLK_EN   0x0004
 #define MDIO_DEVICE_IS_PHY 0x8000
 
 /* Interface Mode definitions */
@@ -839,6 +840,7 @@ int phy_aneg_done(struct phy_device *phydev);
 
 int phy_stop_interrupts(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
+int phy_reset_after_clk_enable(struct phy_device *phydev);
 
 static inline void phy_device_reset(struct phy_device *phydev, int value)
 {
-- 
2.11.0

[PATCH net-next v3 1/4] phylib: Add device reset delay support

[Richard Leitner]

From: Richard Leitner 

Some PHYs need a minimum time after the reset gpio was asserted and/or
deasserted. To ensure we meet these timing requirements add two new
optional devicetree parameters for the phy: reset-delay-us and
reset-post-delay-us.

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
https://patchwork.kernel.org/patch/10090149/

Signed-off-by: Richard Leitner 
---
 Documentation/devicetree/bindings/net/phy.txt | 10 ++
 drivers/net/phy/mdio_device.c | 13 +++--
 drivers/of/of_mdio.c  |  8 
 include/linux/mdio.h  |  2 ++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt 
b/Documentation/devicetree/bindings/net/phy.txt
index c05479f5ac7c..72860ce7f610 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -55,6 +55,12 @@ Optional Properties:
 
 - reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
 
+- reset-delay-us: Delay after the reset was asserted in microseconds.
+  If this property is missing the delay will be skipped.
+
+- reset-post-delay-us: Delay after the reset was deasserted in microseconds.
+  If this property is missing the delay will be skipped.
+
 Example:
 
 ethernet-phy@0 {
@@ -62,4 +68,8 @@ ethernet-phy@0 {
interrupt-parent = <&PIC>;
interrupts = <35 IRQ_TYPE_EDGE_RISING>;
reg = <0>;
+
+   reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
+   reset-delay-us = <1000>;
+   reset-post-delay-us = <2000>;
 };
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index 75d97dd9fb28..ca3ff43f8ee8 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 void mdio_device_free(struct mdio_device *mdiodev)
 {
@@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);
 
 void mdio_device_reset(struct mdio_device *mdiodev, int value)
 {
-   if (mdiodev->reset)
-   gpiod_set_value(mdiodev->reset, value);
+   if (!mdiodev->reset)
+   return;
+
+   gpiod_set_value(mdiodev->reset, value);
+
+   if (value && mdiodev->reset_delay)
+   usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100);
+   else if (!value && mdiodev->reset_post_delay)
+   usleep_range(mdiodev->reset_post_delay,
+mdiodev->reset_post_delay + 100);
 }
 EXPORT_SYMBOL(mdio_device_reset);
 
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 98258583abb0..fb56486dfaa0 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
if (of_property_read_bool(child, "broken-turn-around"))
mdio->phy_ignore_ta_mask |= 1 << addr;
 
+   if (of_property_read_u32(child, "reset-delay-us",
+&phy->mdio.reset_delay))
+   phy->mdio.reset_delay = 0;
+
+   if (of_property_read_u32(child, "reset-post-delay-us",
+&phy->mdio.reset_post_delay))
+   phy->mdio.reset_post_delay = 0;
+
/* Associate the OF node with the device structure so it
 * can be looked up later */
of_node_get(child);
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 92d4e55ffe67..e37c21d8eb19 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -41,6 +41,8 @@ struct mdio_device {
int addr;
int flags;
struct gpio_desc *reset;
+   unsigned int reset_delay;
+   unsigned int reset_post_delay;
 };
 #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
 
-- 
2.11.0

[PATCH net-next v3 4/4] net: fec: add phy_reset_after_clk_enable() support

[Richard Leitner]

From: Richard Leitner 

Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
the refclk on and off again during operation (according to their
datasheet). Nonetheless exactly this behaviour was introduced for power
saving reasons by commit e8fcfcd5684a ("net: fec: optimize the clock management 
to save power").
Therefore add support for the phy_reset_after_clk_enable function from
phylib to mitigate this issue.

Generally speaking this issue is only relevant if the ref clk for the
PHY is generated by the SoC and therefore the PHY is configured to
"REF_CLK In Mode". In our specific case (PCB) this problem does occur at
about every 10th to 50th POR of an LAN8710 connected to an i.MX6SOLO
SoC. The typical symptom of this problem is a "swinging" ethernet link.
Similar issues were reported by users of the NXP forum:
https://community.nxp.com/thread/389902
https://community.nxp.com/message/309354
With this patch applied the issue didn't occur for at least a few
hundret PORs of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
Signed-off-by: Richard Leitner 
---
 drivers/net/ethernet/freescale/fec_main.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 610573855213..8c3d0fb7db20 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1862,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
ret = clk_prepare_enable(fep->clk_ref);
if (ret)
goto failed_clk_ref;
+
+   phy_reset_after_clk_enable(ndev->phydev);
} else {
clk_disable_unprepare(fep->clk_ahb);
clk_disable_unprepare(fep->clk_enet_out);
@@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
if (ret)
goto err_enet_mii_probe;
 
+   /* reset phy if needed here, due to the fact this is the first time we
+* have the net_device to phy_driver link
+*/
+   phy_reset_after_clk_enable(ndev->phydev);
+
if (fep->quirks & FEC_QUIRK_ERR006687)
imx6q_cpuidle_fec_irqs_used();
 
-- 
2.11.0

[PATCH net-next v3 3/4] net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag

[Richard Leitner]

From: Richard Leitner 

The Microchip/SMSC LAN8710/LAN8720 PHYs need (according to their
datasheet [1]) a continuous REF_CLK when configured to "REF_CLK In Mode".
Therefore set the PHY_RST_AFTER_CLK_EN flag for those PHYs to let the
ETH driver reset them after the REF_CLK is enabled.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/2165B.pdf

Signed-off-by: Richard Leitner 
---
 drivers/net/phy/smsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index a1961ba87e2b..be399d645224 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -312,7 +312,7 @@ static struct phy_driver smsc_phy_driver[] = {
.name   = "SMSC LAN8710/LAN8720",
 
.features   = PHY_BASIC_FEATURES,
-   .flags  = PHY_HAS_INTERRUPT,
+   .flags  = PHY_HAS_INTERRUPT | PHY_RST_AFTER_CLK_EN,
 
.probe  = smsc_phy_probe,
 
-- 
2.11.0

Re: [PATCH v4.1] phylib: Add device reset GPIO support

[Richard Leitner]

On 12/04/2017 01:35 PM, Geert Uytterhoeven wrote:
> From: Sergei Shtylyov 
> 
> The PHY devices sometimes do have their reset signal (maybe even power
> supply?) tied to some GPIO and sometimes it also does happen that a boot
> loader does not leave it deasserted. So far this issue has been attacked
> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
> the GPIO in question; that solution, when applied to the device trees, led
> to adding the PHY reset GPIO properties to the MAC device node, with one
> exception: Cadence MACB driver which could handle the "reset-gpios" prop
> in a PHY device subnode. I believe that the correct approach is to teach
> the 'phylib' to get the MDIO device reset GPIO from the device tree node
> corresponding to this device -- which this patch is doing...
> 
> Note that I had to modify the AT803x PHY driver as it would stop working
> otherwise -- it made use of the reset GPIO for its own purposes...
> 
> Signed-off-by: Sergei Shtylyov 
> Acked-by: Rob Herring 
> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
> [geert: Avoid destroying initial setup]
> [geert: Consolidate GPIO descriptor acquiring code]
> Signed-off-by: Geert Uytterhoeven 
> ---

Successfully tested this patch on a i.MX6SOLO based board containing a
LAN8710 PHY:

Tested-by: Richard Leitner 

Re: [PATCH RESEND] net: phy: harmonize phy_id{,_mask} data type

[Richard Leitner]

On 11/27/2017 02:50 PM, Andrew Lunn wrote:
> On Mon, Nov 27, 2017 at 08:16:45AM +0100, Richard Leitner wrote:
>> From: Richard Leitner 
>>
>> Previously phy_id was u32 and phy_id_mask was unsigned int. As the
>> phy_id_mask defines the important bits of the phy_id (and is therefore
>> the same size) these two variables should be the same data type.
> 
> Hi Richard
> 
> It is normal to put inside the [] of the subject line which of the two
> trees this patch is for. In this case net-next.
> 
> No need to resend, but please try to do this for your the next
> networking patch.

Ok. Thank you for the information. Just saw that it's described in
netdev-FAQ.txt. Sorry for not reading that in advance!

> 
>  Andrew
> 

[PATCH RESEND] net: phy: harmonize phy_id{,_mask} data type

[Richard Leitner]

From: Richard Leitner 

Previously phy_id was u32 and phy_id_mask was unsigned int. As the
phy_id_mask defines the important bits of the phy_id (and is therefore
the same size) these two variables should be the same data type.

Signed-off-by: Richard Leitner 
Reviewed-by: Florian Fainelli 
Reviewed-by: Andrew Lunn 
---
RESEND as suggested by Andrew Lunn
---
 include/linux/phy.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07cb4fd..e00fd9ce3bce 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -509,7 +509,7 @@ struct phy_driver {
struct mdio_driver_common mdiodrv;
u32 phy_id;
char *name;
-   unsigned int phy_id_mask;
+   u32 phy_id_mask;
u32 features;
u32 flags;
const void *driver_data;
-- 
2.11.0

[PATCH] net: phy: harmonize phy_id{,_mask} data type

[Richard Leitner]

From: Richard Leitner 

Previously phy_id was u32 and phy_id_mask was unsigned int. As the
phy_id_mask defines the important bits of the phy_id (and is therefore
the same size) these two variables should be the same data type.

Signed-off-by: Richard Leitner 
---
This patch is extracted from the "net: ethernet: fec: fix refclk enable for 
SMSC LAN8710/20"
patch series. This was done because this series will be reworked and
rebased on not yet merged feature later on. For more details see:
https://patchwork.ozlabs.org/cover/839468/
---
 include/linux/phy.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07cb4fd..e00fd9ce3bce 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -509,7 +509,7 @@ struct phy_driver {
struct mdio_driver_common mdiodrv;
u32 phy_id;
char *name;
-   unsigned int phy_id_mask;
+   u32 phy_id_mask;
u32 features;
u32 flags;
const void *driver_data;
-- 
2.11.0

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

On 11/20/2017 02:13 PM, Geert Uytterhoeven wrote:
> Hi Richard,
> 
> On Mon, Nov 20, 2017 at 1:55 PM, Richard Leitner
>  wrote:
>> On 11/20/2017 11:35 AM, Andy Duan wrote:
>>> 3. add reset gpio descriptor for common phy device driver.
>>
>> ... if I understood it correctly the patch called "Teach phylib
>> hard-resetting devices" by Geert and Sergei is exactly doing this:
>> https://patchwork.ozlabs.org/cover/828503/
>> https://lkml.org/lkml/2017/10/20/166
>>
>> So I'll implement the phy_reset_after_clk_enable function atop of this
>> patch-set and add a note that my patch-series depends on it. Would that
>> be OK?
> 
> I will update and respin that patch series after the merge window has closed.

Ok. Thank you for the quick response an this information.

For the Freescale Fast Ethernet Controller (FEC) there are currently (in
addition to the reset gpio) two additional optional dt properties for
the reset:
 - phy-reset-duration : Reset duration in milliseconds.
 - phy-reset-post-delay : Post reset delay in milliseconds.

IMHO it would make sense to include them also in the phylib
implementation. What do you think about it? Should I include it in my
patch-series?

kind regards;
Richard.L

> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

On 11/20/2017 11:35 AM, Andy Duan wrote:
> From: Richard Leitner  Sent: Monday, November 
> 20, 2017 5:57 PM
>> To: Andy Duan ; f.faine...@gmail.com;
>> and...@lunn.ch
>> Cc: Richard Leitner ; net...@vger.kernel.org; linux-
>> ker...@vger.kernel.org
>> Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>> LAN8710/20
>>
>>
>> On 11/20/2017 10:47 AM, Andy Duan wrote:
>>> From: Richard Leitner  Sent: Monday, November 20, 2017
>>> 4:34 PM
>>>> To: f.faine...@gmail.com; Andy Duan ;
>>>> and...@lunn.ch
>>>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>> richard.leit...@skidata.com
>>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>>> SMSC
>>>> LAN8710/20
>>>>
>>>> From: Richard Leitner 
>>>>
>>>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow
>>>> turning the refclk on and off again during operation (according to their
>> datasheet).
>>>> Nonetheless exactly this behaviour was introduced for power saving
>>>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock
>>>> management to save power").
>>>> Therefore after enabling the refclk we detect if an affected PHY is
>>>> attached. If so reset and initialize it again.
>> ...
>>
>>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>>>> +*ndev) {
>>>> +  struct phy_device *phy_dev = ndev->phydev;
>>>> +  u32 real_phy_id;
>>>> +  int ret;
>>>> +
>>>> +  /* some PHYs need a reset after the refclk was enabled, so we
>>>> +   * reset them here
>>>> +   */
>>>> +  if (!phy_dev)
>>>> +  return 0;
>>>> +  if (!phy_dev->drv)
>>>> +  return 0;
>>>> +  real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>>>> +  switch (real_phy_id) {
>>>> +  case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
>>> Don't hard code here...
>>> I believe there have many other phys also do such operation, hardcode is
>> unacceptable...
>>> And these code can be put into phy_device.c as common interface.
>> Ok. Thank you for the feedback.
>> So it would be fine to hardcode the affected phy_id's in a common function in
>> phy_device.c?
>>
>>
>> Another possible solution that came to my mind is to add a flag called
>> something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
>> phy_driver. This flag could then be set in the smsc PHY driver for affected
>> PHYs.
>>
>> Then instead of comparing the phy_id in the MAC driver this flag could be
>> checked:
>>
>> if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
>>ret = fec_reset_phy(ndev);
>>...
>> }
>>
>> Would checking the flag be OK in fec_main.c?
> Yes, it is better than previous solution.
> But add new common API in phy_device.c is much better like: 
> 1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct 
> phy_driver,  all phy driver that need reset can set the flag.

OK.

> 2. add new common api interface phy_reset_after_clk_enable() in phy_device.c 
> driver

OK. But see below...

> 3. add reset gpio descriptor for common phy device driver. 

... if I understood it correctly the patch called "Teach phylib
hard-resetting devices" by Geert and Sergei is exactly doing this:
https://patchwork.ozlabs.org/cover/828503/
https://lkml.org/lkml/2017/10/20/166

So I'll implement the phy_reset_after_clk_enable function atop of this
patch-set and add a note that my patch-series depends on it. Would that
be OK?

> 4. then any mac driver can directly call the common interface 
> .phy_reset_after_clk_enable().

Sounds reasonable :-)

> 
> That is only my suggestion, maybe there have better idea.
> Thanks.
> 

Thanks for your quick feedback.

regards
Richard.L

Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

On 11/20/2017 10:47 AM, Andy Duan wrote:
> From: Richard Leitner  Sent: Monday, November 20, 2017 4:34 
> PM
>> To: f.faine...@gmail.com; Andy Duan ;
>> and...@lunn.ch
>> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
>> richard.leit...@skidata.com
>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>> LAN8710/20
>>
>> From: Richard Leitner 
>>
>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>> the refclk on and off again during operation (according to their datasheet).
>> Nonetheless exactly this behaviour was introduced for power saving reasons
>> by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>> power").
>> Therefore after enabling the refclk we detect if an affected PHY is 
>> attached. If
>> so reset and initialize it again.

...

>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>> +*ndev) {
>> +struct phy_device *phy_dev = ndev->phydev;
>> +u32 real_phy_id;
>> +int ret;
>> +
>> +/* some PHYs need a reset after the refclk was enabled, so we
>> + * reset them here
>> + */
>> +if (!phy_dev)
>> +return 0;
>> +if (!phy_dev->drv)
>> +return 0;
>> +real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>> +switch (real_phy_id) {
>> +case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
> 
> Don't hard code here...
> I believe there have many other phys also do such operation, hardcode is 
> unacceptable...
> 
> And these code can be put into phy_device.c as common interface.

Ok. Thank you for the feedback.
So it would be fine to hardcode the affected phy_id's in a common
function in phy_device.c?


Another possible solution that came to my mind is to add a flag called
something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct
phy_driver. This flag could then be set in the smsc PHY driver for
affected PHYs.

Then instead of comparing the phy_id in the MAC driver this flag could
be checked:

if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) {
ret = fec_reset_phy(ndev);
...
}

Would checking the flag be OK in fec_main.c?

What would be the "better" approach?

> 
>> +ret = fec_reset_phy(ndev);
>> +if (ret)
>> +return ret;
>> +ret = phy_init_hw(phy_dev);
>> +if (ret)
>> +return ret;
>> +}
>> +return 0;
>> +}
>> +
>> static int fec_enet_clk_enable(struct net_device *ndev, bool enable)  {
>>  struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6
>> +1889,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool
>> enable)
>>  ret = clk_prepare_enable(fep->clk_ref);
>>  if (ret)
>>  goto failed_clk_ref;
>> +
>> +ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>> +if (ret)
>> +netdev_warn(ndev, "Resetting PHY failed, connection
>> may be
>> +unstable\n");
>>  } else {
>>  clk_disable_unprepare(fep->clk_ahb);
>>  clk_disable_unprepare(fep->clk_enet_out);
>> @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
>>  if (ret)
>>  goto err_enet_mii_probe;
>>
>> +/* as the PHY is connected now, trigger the reset quirk again */
>> +ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>> +if (ret)
>> +netdev_warn(ndev, "Resetting PHY failed, connection may be
>> +unstable\n");
>> +
>>  if (fep->quirks & FEC_QUIRK_ERR006687)
>>  imx6q_cpuidle_fec_irqs_used();
>>
>>  napi_enable(&fep->napi);
>>  phy_start(ndev->phydev);
>> +
> 
> No need blank line here...
>>  netif_tx_start_all_queues(ndev);
>>
>>  device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
>> --
>> 2.11.0

[PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Richard Leitner]

From: Richard Leitner 

Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
the refclk on and off again during operation (according to their
datasheet). Nonetheless exactly this behaviour was introduced for power
saving reasons by commit e8fcfcd5684a ("net: fec: optimize the clock management 
to save power").
Therefore after enabling the refclk we detect if an affected PHY is
attached. If so reset and initialize it again.

For a better understanding here's a outline of the time response of the
clock and reset lines before and after this patch:

  v--fec_probe()  v--fec_enet_open()
  v   v
w/o patch eCLK: ___|
w/o patch nRST: __--
w/o patch CONF: ___XX___

w/  patch eCLK: ___|
w/  patch nRST: __-__---
w/  patch CONF: ___XX_XX
  ^   ^
  ^--fec_probe()  ^--fec_enet_open()

Generally speaking this issue is only relevant if the ref clk for the
PHY is generated by the SoC. In our specific case (PCB) this problem
does occur at about every 10th to 50th POR of an LAN8710 connected to an
i.MX6DL SoC. The typical symptom of this problem is a "swinging"
ethernet link. Similar issues were reported by users of the NXP forum:
https://community.nxp.com/thread/389902
https://community.nxp.com/message/309354
With this patch applied the issue didn't occur for at least a few
hundret PORs of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
Signed-off-by: Richard Leitner 
---
 drivers/net/ethernet/freescale/fec_main.c | 37 +++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 06a7caca0cee..52ec9b29a70e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -68,6 +68,7 @@
 
 static void set_multicast_list(struct net_device *ndev);
 static void fec_enet_itr_coal_init(struct net_device *ndev);
+static int fec_reset_phy(struct net_device *ndev);
 
 #define DRIVER_NAME"fec"
 
@@ -1833,6 +1834,32 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int 
mii_id, int regnum,
return ret;
 }
 
+static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device *ndev)
+{
+   struct phy_device *phy_dev = ndev->phydev;
+   u32 real_phy_id;
+   int ret;
+
+   /* some PHYs need a reset after the refclk was enabled, so we
+* reset them here
+*/
+   if (!phy_dev)
+   return 0;
+   if (!phy_dev->drv)
+   return 0;
+   real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
+   switch (real_phy_id) {
+   case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */
+   ret = fec_reset_phy(ndev);
+   if (ret)
+   return ret;
+   ret = phy_init_hw(phy_dev);
+   if (ret)
+   return ret;
+   }
+   return 0;
+}
+
 static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 {
struct fec_enet_private *fep = netdev_priv(ndev);
@@ -1862,6 +1889,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, 
bool enable)
ret = clk_prepare_enable(fep->clk_ref);
if (ret)
goto failed_clk_ref;
+
+   ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
+   if (ret)
+   netdev_warn(ndev, "Resetting PHY failed, connection may 
be unstable\n");
} else {
clk_disable_unprepare(fep->clk_ahb);
clk_disable_unprepare(fep->clk_enet_out);
@@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
if (ret)
goto err_enet_mii_probe;
 
+   /* as the PHY is connected now, trigger the reset quirk again */
+   ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
+   if (ret)
+   netdev_warn(ndev, "Resetting PHY failed, connection may be 
unstable\n");
+
if (fep->quirks & FEC_QUIRK_ERR006687)
imx6q_cpuidle_fec_irqs_used();
 
napi_enable(&fep->napi);
phy_start(ndev->phydev);
+
netif_tx_start_all_queues(ndev);
 
device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
-- 
2.11.0

[PATCH v2 1/3] net: ethernet: freescale: simplify fec_reset_phy

[Richard Leitner]

From: Richard Leitner 

The fec_reset_phy function allowed only one execution during probeing.
To make it more usable move the dt parsing and gpio allocation to the
probe function. The parameters of the phy reset are added to the
fec_enet_private struct. As a result the fec_reset_phy function may be
called anytime after probe.

One checkpatch.pl warning (too long line) is ignored. This is due to the
fact a string (dt property name) otherwise needs to be split over
multiple lines, which is counterproductive for the readability.

Signed-off-by: Richard Leitner 
---
 drivers/net/ethernet/freescale/fec.h  |  4 ++
 drivers/net/ethernet/freescale/fec_main.c | 88 ---
 2 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h 
b/drivers/net/ethernet/freescale/fec.h
index 5385074b3b7d..401c4eabf08a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -539,6 +539,10 @@ struct fec_enet_private {
int pause_flag;
int wol_flag;
u32 quirks;
+   int phy_reset;
+   int phy_reset_duration;
+   int phy_reset_post_delay;
+   boolphy_reset_active_high;
 
struct  napi_struct napi;
int csum_flags;
diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 610573855213..06a7caca0cee 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3212,62 +3212,36 @@ static int fec_enet_init(struct net_device *ndev)
 }
 
 #ifdef CONFIG_OF
-static int fec_reset_phy(struct platform_device *pdev)
+static int fec_reset_phy(struct net_device *ndev)
 {
-   int err, phy_reset;
-   bool active_high = false;
-   int msec = 1, phy_post_delay = 0;
-   struct device_node *np = pdev->dev.of_node;
-
-   if (!np)
-   return 0;
-
-   err = of_property_read_u32(np, "phy-reset-duration", &msec);
-   /* A sane reset duration should not be longer than 1s */
-   if (!err && msec > 1000)
-   msec = 1;
+   struct fec_enet_private *fep = netdev_priv(ndev);
 
-   phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
-   if (phy_reset == -EPROBE_DEFER)
-   return phy_reset;
-   else if (!gpio_is_valid(phy_reset))
+   if (!fep->phy_reset)
return 0;
 
-   err = of_property_read_u32(np, "phy-reset-post-delay", &phy_post_delay);
-   /* valid reset duration should be less than 1s */
-   if (!err && phy_post_delay > 1000)
-   return -EINVAL;
-
-   active_high = of_property_read_bool(np, "phy-reset-active-high");
+   gpio_set_value_cansleep(fep->phy_reset, fep->phy_reset_active_high);
 
-   err = devm_gpio_request_one(&pdev->dev, phy_reset,
-   active_high ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
-   "phy-reset");
-   if (err) {
-   dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
-   return err;
-   }
-
-   if (msec > 20)
-   msleep(msec);
+   if (fep->phy_reset_duration > 20)
+   msleep(fep->phy_reset_duration);
else
-   usleep_range(msec * 1000, msec * 1000 + 1000);
+   usleep_range(fep->phy_reset_duration * 1000,
+fep->phy_reset_duration * 1000 + 1000);
 
-   gpio_set_value_cansleep(phy_reset, !active_high);
+   gpio_set_value_cansleep(fep->phy_reset, !fep->phy_reset_active_high);
 
-   if (!phy_post_delay)
+   if (!fep->phy_reset_post_delay)
return 0;
 
-   if (phy_post_delay > 20)
-   msleep(phy_post_delay);
+   if (fep->phy_reset_post_delay > 20)
+   msleep(fep->phy_reset_post_delay);
else
-   usleep_range(phy_post_delay * 1000,
-phy_post_delay * 1000 + 1000);
+   usleep_range(fep->phy_reset_post_delay * 1000,
+fep->phy_reset_post_delay * 1000 + 1000);
 
return 0;
 }
 #else /* CONFIG_OF */
-static int fec_reset_phy(struct platform_device *pdev)
+static int fec_reset_phy(struct net_device *ndev)
 {
/*
 * In case of platform probe, the reset has been done
@@ -3400,6 +3374,36 @@ fec_probe(struct platform_device *pdev)
}
fep->phy_node = phy_node;
 
+   fep->phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+   if (gpio_is_valid(fep->phy_reset)) {
+   ret = of_property_read_u32(np, "phy-reset-duration",
+  &fep->phy_reset_duration);
+   /* A sane reset duration should not be longer t

[PATCH v2 2/3] include: linux: phy: harmonize phy_id{,_mask} type

[Richard Leitner]

From: Richard Leitner 

Previously phy_id was u32 and phy_id_mask was unsigned int. As the
phy_id_mask defines the important bits of the phy_id (and is therefore the
same size) these two variables should be the same datatype.

Signed-off-by: Richard Leitner 
---
 include/linux/phy.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07cb4fd..e00fd9ce3bce 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -509,7 +509,7 @@ struct phy_driver {
struct mdio_driver_common mdiodrv;
u32 phy_id;
char *name;
-   unsigned int phy_id_mask;
+   u32 phy_id_mask;
u32 features;
u32 flags;
const void *driver_data;
-- 
2.11.0