Re: [PATCH] pwm: lpc18xx-sct: remove unneeded semicolon

[Vladimir Zapolskiy]

On 2/3/21 4:50 AM, Yang Li wrote:

Eliminate the following coccicheck warning:
./drivers/pwm/pwm-lpc18xx-sct.c:292:2-3: Unneeded semicolon

Reported-by: Abaci Robot 
Signed-off-by: Yang Li 
---


Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] pwm: fix semicolon.cocci warnings

[Vladimir Zapolskiy]

On 1/28/21 10:57 PM, Uwe Kleine-König wrote:

Hello,

On Thu, Jan 28, 2021 at 09:45:37PM +0800, kernel test robot wrote:

From: kernel test robot 

drivers/pwm/pwm-lpc18xx-sct.c:292:2-3: Unneeded semicolon


  Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: e96c0ff4b1e0 ("pwm: Enable compile testing for some of drivers")


This looks wrong. e96c0ff4b1e0 only touches drivers/pwm/Kconfig.

The ; was introduced by commit 841e6f90bb78 ("pwm: NXP LPC18xx PWM/SCT
driver")


Right, thank you for the correction, Uwe.

Since the patch has been composed by the robot, it has to be fixed
in the first place.

And regarding this particular change and in general fixes to this type
of issues detected by the robot, I don't think that it earns a Fixes tag.


CC: Krzysztof Kozlowski 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 


--
Best wishes,
Vladimir

Re: [PATCH] pwm: fix semicolon.cocci warnings

[Vladimir Zapolskiy]

On 1/28/21 3:45 PM, kernel test robot wrote:

From: kernel test robot 

drivers/pwm/pwm-lpc18xx-sct.c:292:2-3: Unneeded semicolon


  Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: e96c0ff4b1e0 ("pwm: Enable compile testing for some of drivers")
CC: Krzysztof Kozlowski 
Reported-by: kernel test robot 
Signed-off-by: kernel test robot 


Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH v3 4/5] amba: Make the remove callback return void

[Vladimir Zapolskiy]

On 1/26/21 6:58 PM, Uwe Kleine-König wrote:

All amba drivers return 0 in their remove callback. Together with the
driver core ignoring the return value anyhow, it doesn't make sense to
return a value here.

Change the remove prototype to return void, which makes it explicit that
returning an error value doesn't work as expected. This simplifies changing
the core remove callback to return void, too.

Reviewed-by: Ulf Hansson 
Reviewed-by: Arnd Bergmann 
Acked-by: Alexandre Belloni 
Acked-by: Dmitry Torokhov 
Acked-by: Krzysztof Kozlowski  # for drivers/memory
Acked-by: Mark Brown 
Acked-by: Dmitry Torokhov 
Acked-by: Linus Walleij 
Signed-off-by: Uwe Kleine-König 


For drivers/memory/pl172.c:

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] MAINTAINERS: crypto: s5p-sss: drop Kamil Konieczny

[Vladimir Zapolskiy]

On 12/7/20 6:55 PM, Krzysztof Kozlowski wrote:

E-mails to Kamil Konieczny to his Samsung address bounce with 550 (User
unknown).  Kamil no longer takes care about Samsung S5P SSS driver so
remove the invalid email address from:
  - mailmap,
  - bindings maintainer entries,
  - maintainers entry for S5P Security Subsystem crypto accelerator.

Signed-off-by: Krzysztof Kozlowski 


Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH 13/32] pwm: lpc32xx: convert to devm_platform_ioremap_resource

[Vladimir Zapolskiy]

On 12/29/19 10:05 AM, Yangtao Li wrote:

Use devm_platform_ioremap_resource() to simplify code.

Signed-off-by: Yangtao Li 


Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH 29/32] pwm: lpc18xx-sct: convert to devm_platform_ioremap_resource

[Vladimir Zapolskiy]

On 12/29/19 10:06 AM, Yangtao Li wrote:

Use devm_platform_ioremap_resource() to simplify code.

Signed-off-by: Yangtao Li 


Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH 19/36] tty: serial: lpc32xx_hs: Remove unused variable 'tmp'

[Vladimir Zapolskiy]

Hi Lee,

On 11/4/20 9:35 PM, Lee Jones wrote:

Fixes the following W=1 kernel build warning(s):

  drivers/tty/serial/lpc32xx_hs.c: In function ‘__serial_uart_flush’:
  drivers/tty/serial/lpc32xx_hs.c:244:6: warning: variable ‘tmp’ set but not 
used [-Wunused-but-set-variable]

Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: Vladimir Zapolskiy 
Cc: Sylvain Lemieux 
Cc: Kevin Wells 
Cc: Roland Stigge 
Cc: linux-ser...@vger.kernel.org
Signed-off-by: Lee Jones 
---
  drivers/tty/serial/lpc32xx_hs.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/lpc32xx_hs.c b/drivers/tty/serial/lpc32xx_hs.c
index b5898c9320361..1fa098d7aec4b 100644
--- a/drivers/tty/serial/lpc32xx_hs.c
+++ b/drivers/tty/serial/lpc32xx_hs.c
@@ -241,12 +241,11 @@ static unsigned int __serial_get_clock_div(unsigned long 
uartclk,
  
  static void __serial_uart_flush(struct uart_port *port)

  {
-   u32 tmp;
int cnt = 0;
  
  	while ((readl(LPC32XX_HSUART_LEVEL(port->membase)) > 0) &&

   (cnt++ < FIFO_READ_LIMIT))
-   tmp = readl(LPC32XX_HSUART_FIFO(port->membase));
+   readl(LPC32XX_HSUART_FIFO(port->membase));
  }
  
  static void __serial_lpc32xx_rx(struct uart_port *port)




Thank you for the change.

Acked-by: Vladimir Zapolskiy 

I'm sure the change is correct, likely the local variable was introduced
to prevent an unwanted probable optimization by some odd/ancient compiler.

--
Best wishes,
Vladimir

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Vladimir Zapolskiy]

Hi Gao Xiang,

On 10/30/20 2:47 PM, Gao Xiang wrote:

Hi Vladimir,

On Fri, Oct 30, 2020 at 02:20:31PM +0200, Vladimir Zapolskiy wrote:

Hello Gao Xiang,

On 10/22/20 5:57 PM, Gao Xiang via Linux-erofs wrote:

From: Gao Xiang 

pcluster should be only set up for all managed pages instead of
temporary pages. Since it currently uses page->mapping to identify,
the impact is minor for now.

Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
Cc:  # 5.5+
Signed-off-by: Gao Xiang 


I was looking exactly at this problem recently, my change is one-to-one
to your fix, thus I can provide a tag:

Tested-by: Vladimir Zapolskiy 


Many thanks for confirming this!
I found this when I was killing magical stagingpage page->mapping,
it's somewhat late :-)



sure, for me it was an exciting immersion into the filesystem code :)




The fixed problem is minor, but the kernel log becomes polluted, if
a page allocation debug option is enabled:

 % md5sum ~/erofs/testfile
 BUG: Bad page state in process kworker/u9:0  pfn:687de
 page:57b8bcb4 refcount:0 mapcount:0 mapping: 
index:0x0 pfn:0x687de
 flags: 0x40002000(private)
 raw: 40002000 dead0100 dead0122 
 raw:  888066758690  
 page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
 Modules linked in:
 CPU: 1 PID: 602 Comm: kworker/u9:0 Not tainted 5.9.1 #2
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 
04/01/2014
 Workqueue: erofs_unzipd z_erofs_decompressqueue_work
 Call Trace:
  dump_stack+0x84/0xba
  bad_page.cold+0xac/0xb1
  check_free_page_bad+0xb0/0xc0
  free_pcp_prepare+0x2c8/0x2d0
  free_unref_page+0x18/0xf0
  put_pages_list+0x11a/0x120
  z_erofs_decompressqueue_work+0xc9/0x110
  ? z_erofs_decompress_pcluster.isra.0+0xf10/0xf10
  ? read_word_at_a_time+0x12/0x20
  ? strscpy+0xc7/0x1a0
  process_one_work+0x30c/0x730
  worker_thread+0x91/0x640
  ? __kasan_check_read+0x11/0x20
  ? rescuer_thread+0x8a0/0x8a0
  kthread+0x1dd/0x200
  ? kthread_unpark+0xa0/0xa0
  ret_from_fork+0x1f/0x30
 Disabling lock debugging due to kernel taint


Yeah, I can make a pull-request to Linus if you need this to be in master
now, or I can post it for v5.11-rc1 since 5.4 LTS isn't effected (and it
would be only a print problem with debugging option.)



As for myself I don't utterly need this fix on the master branch ASAP, however
it might be reasonable to get it included right into the next v5.10 release,
because I believe it'll be an LTS. Eventually it's up to you to make a decision,
from my side I won't urge you, the fixed issue is obviously a non-critical one.

Thank you for the original fix and taking my opinion into consideration :)

--
Best wishes,
Vladimir

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Vladimir Zapolskiy]

Hello Gao Xiang,

On 10/22/20 5:57 PM, Gao Xiang via Linux-erofs wrote:

From: Gao Xiang 

pcluster should be only set up for all managed pages instead of
temporary pages. Since it currently uses page->mapping to identify,
the impact is minor for now.

Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
Cc:  # 5.5+
Signed-off-by: Gao Xiang 


I was looking exactly at this problem recently, my change is one-to-one
to your fix, thus I can provide a tag:

Tested-by: Vladimir Zapolskiy 


The fixed problem is minor, but the kernel log becomes polluted, if
a page allocation debug option is enabled:

% md5sum ~/erofs/testfile
BUG: Bad page state in process kworker/u9:0  pfn:687de
page:57b8bcb4 refcount:0 mapcount:0 mapping: 
index:0x0 pfn:0x687de
flags: 0x40002000(private)
raw: 40002000 dead0100 dead0122 
raw:  888066758690  
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
Modules linked in:
CPU: 1 PID: 602 Comm: kworker/u9:0 Not tainted 5.9.1 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 
04/01/2014
Workqueue: erofs_unzipd z_erofs_decompressqueue_work
Call Trace:
 dump_stack+0x84/0xba
 bad_page.cold+0xac/0xb1
 check_free_page_bad+0xb0/0xc0
 free_pcp_prepare+0x2c8/0x2d0
 free_unref_page+0x18/0xf0
 put_pages_list+0x11a/0x120
 z_erofs_decompressqueue_work+0xc9/0x110
 ? z_erofs_decompress_pcluster.isra.0+0xf10/0xf10
 ? read_word_at_a_time+0x12/0x20
 ? strscpy+0xc7/0x1a0
 process_one_work+0x30c/0x730
 worker_thread+0x91/0x640
 ? __kasan_check_read+0x11/0x20
 ? rescuer_thread+0x8a0/0x8a0
 kthread+0x1dd/0x200
 ? kthread_unpark+0xa0/0xa0
 ret_from_fork+0x1f/0x30
Disabling lock debugging due to kernel taint

--
Best wishes,
Vladimir

Re: [PATCH 10/29] arm: dts: lpc18xx: Harmonize EHCI/OHCI DT nodes name

[Vladimir Zapolskiy]

On 10/20/20 2:59 PM, Serge Semin wrote:

In accordance with the Generic EHCI/OHCI bindings the corresponding node
name is suppose to comply with the Generic USB HCD DT schema, which
requires the USB nodes to have the name acceptable by the regexp:
"^usb(@.*)?" . Make sure the "generic-ehci" and "generic-ohci"-compatible
nodes are correctly named.

Signed-off-by: Serge Semin 
---
  arch/arm/boot/dts/lpc18xx.dtsi | 4 ++--


Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH -next v2] usb: gadget: lpc32xx_udc: Convert to DEFINE_SHOW_ATTRIBUTE

[Vladimir Zapolskiy]

On 9/19/20 5:52 AM, Qinglang Miao wrote:
> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
> 
> Signed-off-by: Qinglang Miao 

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path

[Vladimir Zapolskiy]

On 8/12/20 5:09 PM, Michał Mirosław wrote:
> On Wed, Aug 12, 2020 at 09:29:12AM +0300, Vladimir Zapolskiy wrote:
>> On 8/12/20 4:31 AM, Michał Mirosław wrote:
> [...]
>>> config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
>>> if (config == NULL) {
>>> -   kfree(rdev);
>>> ret = -ENOMEM;
>>> -   goto rinse;
>>> +   goto clean;
> [...]
>>>  clean:
>>> if (dangling_of_gpiod)
>>> gpiod_put(config->ena_gpiod);
>>
>> And above 'config' NULL pointer could be dereferenced now, right?
> 
> If config is NULL, dangling_of_gpiod cannot be true.
> 

It's true, thanks. Probably to avoid the known if(false) it might be better
to add a new goto label.

Also it seems to me that it's safe to enter regulator_dev_release() before
doing an assignment to rdev->dev.of_node and incrementing an of_node counter.

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH v2 5/7] regulator: plug of_node leak in regulator_register()'s error path

[Vladimir Zapolskiy]

Hi Michał,

On 8/12/20 4:31 AM, Michał Mirosław wrote:
> By calling device_initialize() earlier and noting that kfree(NULL) is
> ok, we can save a bit of code in error handling and plug of_node leak.
> Fixed commit already did part of the work.
> 
> Cc: sta...@vger.kernel.org
> Fixes: 9177514ce349 ("regulator: fix memory leak on error path of 
> regulator_register()")
> Signed-off-by: Michał Mirosław 
> Acked-by: Vladimir Zapolskiy 
> ---
>  drivers/regulator/core.c | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 71749f48caee..448a267641df 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5137,6 +5137,7 @@ regulator_register(const struct regulator_desc 
> *regulator_desc,
>   ret = -ENOMEM;
>   goto rinse;
>   }
> + device_initialize(>dev);
>  
>   /*
>* Duplicate the config so the driver could override it after
> @@ -5144,9 +5145,8 @@ regulator_register(const struct regulator_desc 
> *regulator_desc,
>*/
>   config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
>   if (config == NULL) {
> - kfree(rdev);
>   ret = -ENOMEM;
> - goto rinse;
> + goto clean;

Here config == NULL

>   }
>  
>   init_data = regulator_of_get_init_data(dev, regulator_desc, config,
> @@ -5158,10 +5158,8 @@ regulator_register(const struct regulator_desc 
> *regulator_desc,
>* from a gpio extender or something else.
>*/
>   if (PTR_ERR(init_data) == -EPROBE_DEFER) {
> - kfree(config);
> - kfree(rdev);
>   ret = -EPROBE_DEFER;
> - goto rinse;
> + goto clean;
>   }
>  
>   /*
> @@ -5214,7 +5212,6 @@ regulator_register(const struct regulator_desc 
> *regulator_desc,
>   }
>  
>   /* register with sysfs */
> - device_initialize(>dev);
>   rdev->dev.class = _class;
>   rdev->dev.parent = dev;
>   dev_set_name(>dev, "regulator.%lu",
> @@ -5292,13 +5289,11 @@ regulator_register(const struct regulator_desc 
> *regulator_desc,
>   mutex_lock(_list_mutex);
>   regulator_ena_gpio_free(rdev);
>   mutex_unlock(_list_mutex);
> - put_device(>dev);
> - rdev = NULL;
>  clean:
>   if (dangling_of_gpiod)
>   gpiod_put(config->ena_gpiod);

And above 'config' NULL pointer could be dereferenced now, right?

> - kfree(rdev);
>   kfree(config);
> + put_device(>dev);
>  rinse:
>   if (dangling_cfg_gpiod)
>   gpiod_put(cfg->ena_gpiod);
> 

--
Best wishes,
Vladimir

Re: [PATCH 5/7] regulator: plug of_node leak in regulator_register()'s error path

[Vladimir Zapolskiy]

Hi Michał,

On 8/11/20 4:07 AM, Michał Mirosław wrote:
> By calling device_initialize() earlier and noting that kfree(NULL) is
> ok, we can save a bit of code in error handling and plug of_node leak.
> Fixed commit already did part of the work.
> 
> Cc: sta...@vger.kernel.org
> Fixes: 9177514ce349 ("regulator: fix memory leak on error path of 
> regulator_register()")
> Signed-off-by: Michał Mirosław 

thank you for the patch!

I was worried about a potentially remaining of_node reference leak,
but I was not able to reproduce it on practice without code fuzzing.

The change looks valid and it's a nice simplification.

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

[PATCH] regulator: fix memory leak on error path of regulator_register()

[Vladimir Zapolskiy]

The change corrects registration and deregistration on error path
of a regulator, the problem was manifested by a reported memory
leak on deferred probe:

as3722-regulator as3722-regulator: regulator 13 register failed -517

# cat /sys/kernel/debug/kmemleak
unreferenced object 0xecc43740 (size 64):
  comm "swapper/0", pid 1, jiffies 4294937640 (age 712.880s)
  hex dump (first 32 bytes):
72 65 67 75 6c 61 74 6f 72 2e 32 34 00 5a 5a 5a  regulator.24.ZZZ
5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  
  backtrace:
[<0c4c3d1c>] __kmalloc_track_caller+0x15c/0x2c0
[<40c0ad48>] kvasprintf+0x64/0xd4
[<109abd29>] kvasprintf_const+0x70/0x84
[] kobject_set_name_vargs+0x34/0xa8
[<62282ea2>] dev_set_name+0x40/0x64
[] regulator_register+0x3a4/0x1344
[<16a9543f>] devm_regulator_register+0x4c/0x84
[<51a4c6a1>] as3722_regulator_probe+0x294/0x754
...

The memory leak problem was introduced as a side ef another fix in
regulator_register() error path, I believe that the proper fix is
to decouple device_register() function into its two compounds and
initialize a struct device before assigning any values to its fields
and then using it before actual registration of a device happens.

This lets to call put_device() safely after initialization, and, since
now a release callback is called, kfree(rdev->constraints) shall be
removed to exclude a double free condition.

Fixes: a3cde9534ebd ("regulator: core: fix regulator_register() error paths to 
properly release rdev")
Cc: Wen Yang 
Signed-off-by: Vladimir Zapolskiy 
---
 drivers/regulator/core.c | 18 +++---
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 03154f5b939f..720f28844795 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5023,7 +5023,6 @@ regulator_register(const struct regulator_desc 
*regulator_desc,
struct regulator_dev *rdev;
bool dangling_cfg_gpiod = false;
bool dangling_of_gpiod = false;
-   bool reg_device_fail = false;
struct device *dev;
int ret, i;
 
@@ -5152,10 +5151,12 @@ regulator_register(const struct regulator_desc 
*regulator_desc,
}
 
/* register with sysfs */
+   device_initialize(>dev);
rdev->dev.class = _class;
rdev->dev.parent = dev;
dev_set_name(>dev, "regulator.%lu",
(unsigned long) atomic_inc_return(_no));
+   dev_set_drvdata(>dev, rdev);
 
/* set regulator constraints */
if (init_data)
@@ -5206,12 +5207,9 @@ regulator_register(const struct regulator_desc 
*regulator_desc,
!rdev->desc->fixed_uV)
rdev->is_switch = true;
 
-   dev_set_drvdata(>dev, rdev);
-   ret = device_register(>dev);
-   if (ret != 0) {
-   reg_device_fail = true;
+   ret = device_add(>dev);
+   if (ret != 0)
goto unset_supplies;
-   }
 
rdev_init_debugfs(rdev);
 
@@ -5233,17 +5231,15 @@ regulator_register(const struct regulator_desc 
*regulator_desc,
mutex_unlock(_list_mutex);
 wash:
kfree(rdev->coupling_desc.coupled_rdevs);
-   kfree(rdev->constraints);
mutex_lock(_list_mutex);
regulator_ena_gpio_free(rdev);
mutex_unlock(_list_mutex);
+   put_device(>dev);
+   rdev = NULL;
 clean:
if (dangling_of_gpiod)
gpiod_put(config->ena_gpiod);
-   if (reg_device_fail)
-   put_device(>dev);
-   else
-   kfree(rdev);
+   kfree(rdev);
kfree(config);
 rinse:
if (dangling_cfg_gpiod)
-- 
2.24.0

Re: [PATCH 12/32] usb: gadget: udc: lpc32xx_udc: Staticify 2 local functions

[Vladimir Zapolskiy]

On 7/6/20 4:33 PM, Lee Jones wrote:
> These are not used outside of this sourcefile, so make them static.
> 
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/usb/gadget/udc/lpc32xx_udc.c:1929:6: warning: no previous prototype 
> for ‘udc_send_in_zlp’ [-Wmissing-prototypes]
>  1929 | void udc_send_in_zlp(struct lpc32xx_udc *udc, struct lpc32xx_ep *ep)
>  | ^~~
>  drivers/usb/gadget/udc/lpc32xx_udc.c:1943:6: warning: no previous prototype 
> for ‘udc_handle_eps’ [-Wmissing-prototypes]
>  1943 | void udc_handle_eps(struct lpc32xx_udc *udc, struct lpc32xx_ep *ep)
>  | ^~~~~~
> 
> Cc: Felipe Balbi 
> Cc: Vladimir Zapolskiy 
> Cc: Sylvain Lemieux 
> Cc: Kevin Wells 
> Cc: Roland Stigge 
> Signed-off-by: Lee Jones 

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] udc: lpc32xx: mark local function static

[Vladimir Zapolskiy]

Hi Arnd, thank you for the patch.

On 6/30/20 2:58 PM, Arnd Bergmann wrote:
> The kernel test robot reports two functions that should be marked
> static:
> 
>>> drivers/usb/gadget/udc/lpc32xx_udc.c:1928:6: warning: no previous prototype 
>>> for 'udc_send_in_zlp' [-Wmissing-prototypes]
> 1928 | void udc_send_in_zlp(struct lpc32xx_udc *udc, struct lpc32xx_ep 
> *ep)
>>> drivers/usb/gadget/udc/lpc32xx_udc.c:1942:6: warning: no previous prototype 
>>> for 'udc_handle_eps' [-Wmissing-prototypes]
> 1942 | void udc_handle_eps(struct lpc32xx_udc *udc, struct lpc32xx_ep *ep)
> 
> This showed up after my commit 792e559e94bc ("udc: lpc32xx: fix 64-bit
> compiler warning") made it possible to build the driver on x86-64.
> 
> Fix the warning as suggested.
> 
> Reported-by: kernel test robot 
> Signed-off-by: Arnd Bergmann 

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Vladimir Zapolskiy]

Hi Wolfram,

On 09/09/2019 10:22 AM, Wolfram Sang wrote:
> Hi Vladimir,
> 
>> I won't attend the LPC, however I would appreciate if you book some
> 
> A pity. I would have liked to have you in the room. Let's see if we can
> get enough input from you via mail here.
> 

if it might help, I'll attend the Embedded Recipes and ELCE conferences
this year.

>> time to review my original / alternative implementation of the TI
>> DS90Ux9xx I2C bridge device driver.
> 
> We have only 45 minutes, this will not allow to review specific
> implementations. I want to give an overview of existing implementations
> with pros/cons...
> 

Sure! Any shared summary/opinions are greatly welcome.

>> The reasons why my driver is better/more flexible/more functional are
>> discussed earlier, please let me know, if you expect anything else
>> from me to add, also I would be happy to get a summary of your offline
>> discussion.
> 
> ... and I'd appreciate support here from you, like your summary of the
> back then discussion (from where I can dive deeper if needed).
> 
> Also, with Luca's new series we discussed some scenarios which can
> happen WRT to I2C address conflicts. Maybe you could comment on them,
> too?

I do remember I've commented on the Luca's suggestion of using dynamic
I2C addresses from a pool (the introduced "i2c-alias-pool" property).

I have to scrutinize it in Luca's v2, but then it might happen that the
userspace won't know to which IC on the remote side it communicates to.

> As I read the old thread, you have a hardcoded aliases using
> "ti,i2c-bridge-maps". This means you can't have own DTSI files for e.g.
> add-on modules, do I get this correctly?
> 

Basically hardcoding of aliases completely resolves the highlighted
above problem. Still it is possible to have own DTSI files for the FPD
link detachable PCBs, and this is an exceptionally important scenario,
however some limitations shall be applied:
* dt overlays for "local" derializer/deserializer ICs, it's a generic
  and universal solution, it is successfully used in the field,
* only "compatible" PCBs are supposed to be connected (same set of I2C
  devices/addresses on every such PCB), this is imperfect for sure,
* "ti,i2c-bridge-maps" list is excessive to cover "almost compatible"
  (in the sense from above) PCBs, some of the missed alias matches
  just won't instantiate a driver, this is of course also imperfect.

In general nothing critical should happen, if some I2C device on the
remote side is simply not probed in runtime, in opposite you can imagine
that writing for instance to another EEPROM of two on the remote side
could be harmful.

Any technically better solution to the two given approaches (from Luca
and from me) is more than appreciated. For non-dynamic/fixed local and
remote PCBs the fixed mapping is better, the dynamic case is covered
by the dt overlays, why not?

As a side note please do remember that the I2C bridging on Maxim GMSL
and TI DS90Ux9xx are different, the former one is a real mux, and the
latter one is not, I'm uncertain if it's reasonable to think of a
generalized solution which covers both IC series, so likely we
have to review the developed solutions for them separately instead
of trying to work out a combined one.

--
Best wishes,
Vladimir

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Vladimir Zapolskiy]

Hi Luca, Jacopo, Wolfram, Peter,

On 09/08/2019 11:45 PM, Vladimir Zapolskiy wrote:
> Hi Luca, Jacopo, Wolfram, Peter,
> 
> On 09/01/2019 05:31 PM, jacopo mondi wrote:
>> Hi Luca,
>>thanks for keep pushing this series! I hope we can use part of this
>> for the (long time) on-going GMSL work...
>>
>> I hope you will be patient enough to provide (another :) overview
>> of this work during the BoF Wolfram has organized at LPC for the next
>> week.
>>
>> In the meantime I would have some comments after having a read at the
>> series and trying to apply its concept to GMSL
>>
> 
> I won't attend the LPC, however I would appreciate if you book some
> time to review my original / alternative implementation of the TI
> DS90Ux9xx I2C bridge device driver.
> 
> For your convenience the links to the driver are given below:
> * dt bindings: 
> https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#mead5ea226550b
> * driver code: 
> https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m2fe3664c5f884
> * usage example: 
> https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m56c146f5decdc
> 
> The reasons why my driver is better/more flexible/more functional are
> discussed earlier, please let me know, if you expect anything else
> from me to add, also I would be happy to get a summary of your offline
> discussion.

I forgot to repeat my main objection against Luca's approach, the TI
DS90Ux9xx I2C bridge driver does not require to call i2c_add_adapter()
or register a new mux/bus and then do run select/deselect in runtime to
overcome the created handicap.

> The undeniable fact is that the device tree bindings in my I2C bridge
> implementation can be improved further, thanks to Luca for the comments.
> 
>> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>>> An ATR is a device that looks similar to an i2c-mux: it has an I2C
>>> slave "upstream" port and N master "downstream" ports, and forwards
>>> transactions from upstream to the appropriate downstream port. But is
>>> is different in that the forwarded transaction has a different slave
>>> address. The address used on the upstream bus is called the "alias"
>>> and is (potentially) different from the physical slave address of the
>>> downstream chip.
>>>
>>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
>>> implementing ATR features in a device driver. The helper takes care or
>>> adapter creation/destruction and translates addresses at each transaction.
>>>
>>> Signed-off-by: Luca Ceresoli 
>>>
> 

--
Best wishes,
Vladimir

Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

[Vladimir Zapolskiy]

Hi Luca, Jacopo, Wolfram, Peter,

On 09/01/2019 05:31 PM, jacopo mondi wrote:
> Hi Luca,
>thanks for keep pushing this series! I hope we can use part of this
> for the (long time) on-going GMSL work...
> 
> I hope you will be patient enough to provide (another :) overview
> of this work during the BoF Wolfram has organized at LPC for the next
> week.
> 
> In the meantime I would have some comments after having a read at the
> series and trying to apply its concept to GMSL
> 

I won't attend the LPC, however I would appreciate if you book some
time to review my original / alternative implementation of the TI
DS90Ux9xx I2C bridge device driver.

For your convenience the links to the driver are given below:
* dt bindings: 
https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#mead5ea226550b
* driver code: 
https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m2fe3664c5f884
* usage example: 
https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m56c146f5decdc

The reasons why my driver is better/more flexible/more functional are
discussed earlier, please let me know, if you expect anything else
from me to add, also I would be happy to get a summary of your offline
discussion.

The undeniable fact is that the device tree bindings in my I2C bridge
implementation can be improved further, thanks to Luca for the comments.

> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>> An ATR is a device that looks similar to an i2c-mux: it has an I2C
>> slave "upstream" port and N master "downstream" ports, and forwards
>> transactions from upstream to the appropriate downstream port. But is
>> is different in that the forwarded transaction has a different slave
>> address. The address used on the upstream bus is called the "alias"
>> and is (potentially) different from the physical slave address of the
>> downstream chip.
>>
>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
>> implementing ATR features in a device driver. The helper takes care or
>> adapter creation/destruction and translates addresses at each transaction.
>>
>> Signed-off-by: Luca Ceresoli 
>>

--
Best wishes,
Vladimir

Re: [PATCH] ARM: lpc32xx: stop overwriting TEST_CLK_SEL

[Vladimir Zapolskiy]

Hi Alexandre, Gregory,

On 04/15/2019 10:59 AM, gregory.clem...@bootlin.com wrote:
> On 2019-04-11 16:12, Alexandre Belloni wrote:
>> While the UDA1380 is described in some lpc3250 device trees, there is
>> currently no real user of that codec. Anyway, if the codec needs a 
>> clock,
>> it should take it explicitly.
>>
>> lpc3250_machine_init is called for all the lpc32xx machines and some 
>> are
>> using test1_clk (for example to strobe an HW watchdog). Overwriting
>> TEST_CLK_SEL prevents booting those platforms.
>>
>> Signed-off-by: Alexandre Belloni 
> 
> Tested-by: Gregory CLEMENT 

I've applied the change to my tree, hopefully it will be included into v5.2.

Thank you!

--
Best wishes,
Vladimir

Re: [PATCH] watchdog: Convert to use devm_platform_ioremap_resource

[Vladimir Zapolskiy]

On 04/02/2019 10:01 PM, Guenter Roeck wrote:
> Use devm_platform_ioremap_resource to reduce source code size,
> improve readability, and reduce the likelyhood of bugs.
> 
> The conversion was done automatically with coccinelle using the
> following semantic patch.
> 
> @r@
> identifier res, pdev;
> expression a;
> expression index;
> expression e;
> @@
> 
> <+...
> - res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> - a = devm_ioremap_resource(e, res);
> + a = devm_platform_ioremap_resource(pdev, index);
> ...+>
> 
> @depends on r@
> identifier r.res;
> @@
> - struct resource *res;
>   ... when != res
> 
> @@
> identifier res, pdev;
> expression index;
> expression a;
> @@
> - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> - a = devm_ioremap_resource(>dev, res);
> + a = devm_platform_ioremap_resource(pdev, index);
> 
> Cc: Joel Stanley 
> Cc: Nicolas Ferre 
> Cc: Alexandre Belloni 
> Cc: Florian Fainelli 
> Cc: Linus Walleij 
> Cc: Baruch Siach 
> Cc: Keguang Zhang 
> Cc: Vladimir Zapolskiy 
> Cc: Kevin Hilman 
> Cc: Matthias Brugger 
> Cc: Avi Fishman 
> Cc: Nancy Yuen 
> Cc: Brendan Higgins 
> Cc: Wan ZongShun 
> Cc: Michal Simek 
> Cc: Sylvain Lemieux 
> Cc: Kukjin Kim 
> Cc: Barry Song 
> Cc: Orson Zhai 
> Cc: Patrice Chotard 
> Cc: Maxime Coquelin 
> Cc: Maxime Ripard 
> Cc: Chen-Yu Tsai 
> Cc: Marc Gonzalez 
> Cc: Thierry Reding 
> Cc: Shawn Guo 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/watchdog/asm9260_wdt.c| 4 +---
>  drivers/watchdog/aspeed_wdt.c | 4 +---
>  drivers/watchdog/at91sam9_wdt.c   | 4 +---
>  drivers/watchdog/ath79_wdt.c  | 4 +---
>  drivers/watchdog/atlas7_wdt.c | 4 +---
>  drivers/watchdog/bcm7038_wdt.c| 4 +---
>  drivers/watchdog/bcm_kona_wdt.c   | 4 +---
>  drivers/watchdog/cadence_wdt.c| 4 +---
>  drivers/watchdog/coh901327_wdt.c  | 4 +---
>  drivers/watchdog/davinci_wdt.c| 4 +---
>  drivers/watchdog/digicolor_wdt.c  | 4 +---
>  drivers/watchdog/dw_wdt.c | 4 +---
>  drivers/watchdog/ep93xx_wdt.c | 4 +---
>  drivers/watchdog/ftwdt010_wdt.c   | 4 +---
>  drivers/watchdog/imgpdc_wdt.c | 4 +---
>  drivers/watchdog/jz4740_wdt.c | 4 +---
>  drivers/watchdog/lantiq_wdt.c | 4 +---
>  drivers/watchdog/loongson1_wdt.c  | 4 +---
>  drivers/watchdog/lpc18xx_wdt.c| 4 +---
>  drivers/watchdog/max63xx_wdt.c| 4 +---
>  drivers/watchdog/meson_gxbb_wdt.c | 4 +---
>  drivers/watchdog/meson_wdt.c  | 4 +---
>  drivers/watchdog/moxart_wdt.c | 4 +---
>  drivers/watchdog/mpc8xxx_wdt.c| 3 +--
>  drivers/watchdog/mt7621_wdt.c | 5 +
>  drivers/watchdog/mtk_wdt.c| 4 +---
>  drivers/watchdog/npcm_wdt.c   | 4 +---
>  drivers/watchdog/nuc900_wdt.c | 4 +---
>  drivers/watchdog/of_xilinx_wdt.c  | 4 +---
>  drivers/watchdog/omap_wdt.c   | 4 +---
>  drivers/watchdog/orion_wdt.c  | 6 ++
>  drivers/watchdog/pic32-dmt.c  | 4 +---
>  drivers/watchdog/pic32-wdt.c  | 4 +---
>  drivers/watchdog/pnx4008_wdt.c| 4 +---
>  drivers/watchdog/renesas_wdt.c| 4 +---
>  drivers/watchdog/rt2880_wdt.c | 4 +---
>  drivers/watchdog/rtd119x_wdt.c| 4 +---
>  drivers/watchdog/rza_wdt.c| 4 +---
>  drivers/watchdog/s3c2410_wdt.c| 4 +---
>  drivers/watchdog/sama5d4_wdt.c| 4 +---
>  drivers/watchdog/sbsa_gwdt.c  | 7 ++-
>  drivers/watchdog/shwdt.c  | 4 +---
>  drivers/watchdog/sirfsoc_wdt.c| 4 +---
>  drivers/watchdog/sprd_wdt.c   | 4 +---
>  drivers/watchdog/st_lpc_wdt.c | 4 +---
>  drivers/watchdog/stm32_iwdg.c | 4 +---
>  drivers/watchdog/sunxi_wdt.c  | 4 +---
>  drivers/watchdog/tangox_wdt.c | 4 +---
>  drivers/watchdog/tegra_wdt.c  | 4 +---
>  drivers/watchdog/ts72xx_wdt.c | 7 ++-
>  drivers/watchdog/txx9wdt.c| 4 +---
>  drivers/watchdog/zx2967_wdt.c | 4 +---
>  52 files changed, 55 insertions(+), 161 deletions(-)

[snip]

> diff --git a/drivers/watchdog/lpc18xx_wdt.c b/drivers/watchdog/lpc18xx_wdt.c
> index 331cadb459ac..f6f66634cedf 100644
> --- a/drivers/watchdog/lpc18xx_wdt.c
> +++ b/drivers/watchdog/lpc18xx_wdt.c
> @@ -204,15 +204,13 @@ static int lpc18xx_wdt_probe(struct platform_device 
> *pdev)
>  {
>   struct lpc18xx_wdt_dev *lpc18xx_wdt;
>   struct device *dev = >dev;
> - struct resource *res;
>   int ret;
>  
>   lpc18xx_wdt = devm_kzalloc(dev, sizeof(*lpc18xx_wdt), GFP_KERNEL);
>   if (!lpc18xx_wdt)
>   return -ENOMEM;
>  
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - lpc18xx_wdt->base = devm_iorem

Re: [PATCH 18/42] drivers: gpio: lpc18xx: use devm_platform_ioremap_resource()

[Vladimir Zapolskiy]

On 03/11/2019 08:54 PM, Enrico Weigelt, metux IT consult wrote:
> Use the new helper that wraps the calls to platform_get_resource()
> and devm_ioremap_resource() together.
> 
> Signed-off-by: Enrico Weigelt, metux IT consult 
> ---
>  drivers/gpio/gpio-lpc18xx.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-lpc18xx.c b/drivers/gpio/gpio-lpc18xx.c
> index d441dba..d711ae0 100644
> --- a/drivers/gpio/gpio-lpc18xx.c
> +++ b/drivers/gpio/gpio-lpc18xx.c
> @@ -340,10 +340,7 @@ static int lpc18xx_gpio_probe(struct platform_device 
> *pdev)
>   index = of_property_match_string(dev->of_node, "reg-names", "gpio");
>   if (index < 0) {
>   /* To support backward compatibility take the first resource */
> - struct resource *res;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - gc->base = devm_ioremap_resource(dev, res);
> + gc->base = devm_platform_ioremap_resource(pdev, 0);
>   } else {
>   struct resource res;
>  
> 

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH serial] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()

[Vladimir Zapolskiy]

On 03/08/2019 04:09 PM, Mao Wenan wrote:
> Add the missing uart_unregister_driver() and i2c_del_driver() before return
> from sc16is7xx_init() in the error handling case.
> 
> Signed-off-by: Mao Wenan 
> ---
>  drivers/tty/serial/sc16is7xx.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 268098681856..114e94f476c6 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1509,7 +1509,7 @@ static int __init sc16is7xx_init(void)
>   ret = i2c_add_driver(_i2c_uart_driver);
>   if (ret < 0) {
>   pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
> - return ret;
> + goto err_i2c;
>   }
>  #endif
>  
> @@ -1517,10 +1517,18 @@ static int __init sc16is7xx_init(void)
>   ret = spi_register_driver(_spi_uart_driver);
>   if (ret < 0) {
>   pr_err("failed to init sc16is7xx spi --> %d\n", ret);
> - return ret;
> + goto err_spi;
>   }
>  #endif
>   return ret;
> +
> +err_spi:
> +#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
> + i2c_del_driver(_i2c_uart_driver);
> +#endif
> +err_i2c:
> +     uart_unregister_driver(_uart);
> + return ret;
>  }
>  module_init(sc16is7xx_init);
>  
> 

Nice catch, thank you!

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] pinctrl: remove unused 'pinconf-config' debugfs interface

[Vladimir Zapolskiy]

Hi Linus,

On 01/28/2019 02:36 PM, Linus Walleij wrote:
> On Sun, Jan 20, 2019 at 4:14 PM Vladimir Zapolskiy  wrote:
> 
>> The main goal of the change is to remove .pin_config_dbg_parse_modify
>> callback before a driver with its support appears. So far the in-kernel
>> interface did not attract any users since its introduction 5 years ago.
>>
>> Originally .pin_config_dbg_parse_modify callback and the associated
>> 'pinconf-config' debugfs file were introduced in commit f07512e615dd
>> ("pinctrl/pinconfig: add debug interface"), a short description of
>> 'pinconf-config' usage for debugging can be expressed this way:
>>
>> Write to 'pinconf-config' (see pinconf_dbg_config_write() function):
>>
>> % echo -n modify $map_type $device_name $state_name $pin_name $config > \
>> /sys/kernel/debug/pinctrl/$pinctrl/pinconf-config
>>
>> It supposes to update a global (therefore single!) 'pinconf_dbg_conf'
>> variable with an alternative setting, the arguments should match
>> an existing pinconf device and some registered pinctrl mapping 'map':
>>
>> * $map_type is either 'config_pin' or 'config_group', it should match
>>   'map->type' value of PIN_MAP_TYPE_CONFIGS_PIN or
>>PIN_MAP_TYPE_CONFIGS_GROUP accordingly,
>> * $device_name should match 'map->dev_name' string value,
>> * $state_name should match 'map->name' string value,
>> * $pin_name should match 'map->data.configs.group_or_pin' string value,
>>
>> If all above has matched, then $config is a new value to be set by calling
>> pinconfops->pin_config_dbg_parse_modify(pctldev, config, matched_config).
>>
>> After a successful write into 'pinconf-config' a user can read the file
>> to get information about that single modified pin configuration.
>>
>> The fact is .pin_config_dbg_parse_modify callback has never been defined
>> in 'struct pinconf_ops' of any pinconf driver, thus an actual modification
>> of a pin or group state on any present pinconf controller does not happen,
>> and it declares that all related code is no more than dead code.
>>
>> I discovered the issue while attempting to add .pin_config_dbg_parse_modify
>> support in some drivers and found that too short 'MAX_NAME_LEN' set by
>>
>>   drivers/pinctrl/pinconf.c:372:#define MAX_NAME_LEN 15
>>
>> is practically insufficient to store a regular pinctrl device name,
>> which are like 'e606.pin-controller-sh-pfc' or pin names like
>> 'MX6QDL_PAD_ENET_REF_CLK', thus it is another indicator that the code
>> is barely usable, insufficiently tested and unprepossessing.
>>
>> Of course it might be possible to increase MAX_NAME_LEN, and then add
>> .pin_config_dbg_parse_modify callbacks to the drivers, but the whole
>> idea of such a limited debug option looks inviable. A more flexible
>> way to functionally substitute the original approach is to implicitly
>> or explicitly use pinctrl_select_state() function whenever needed.
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> Cc: Laurent Meunier 
>> Cc: Masahiro Yamada 
>> Cc: Russell King 
> 
> This is fine, the build robot is complaining of some missing
> prototype though, probably some implicit include that
> disappeared when you removed 
> from the  file.
> 
> Will you send a v2 with this fixed? (I think just leaving the
> include in place would be a quickfix.)
> 

Fortunately it has been already done, please check the sent v2 with one
earned Reviewed-by tag from Richard:

* https://patchwork.ozlabs.org/patch/1029536/
* https://patchwork.ozlabs.org/patch/1029537/

Thank you for your approval of the change in general!

--
Best wishes,
Vladimir

Re: [PATCH] tty: serial: lpc32xx_hs: fix missing console boot messages

[Vladimir Zapolskiy]

Hi Alexandre,

On 01/15/2019 07:18 PM, Alexandre Belloni wrote:
> When probing the HSUART, it is put in loopback mode in order to prevent a
> potential issue that may happen on RX (Errata HSUART.1).
> 
> serial_lpc32xx_startup() moves it out of loopback mode but this is too late
> to get the kernel boot messages before userspace opens the device.
> 
> Also get out of loopback mode in lpc32xx_hsuart_console_setup().
> 
> Signed-off-by: Alexandre Belloni 

the change looks good, thank you.

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

[PATCH v2 2/2] pinctrl: remove unused 'pinconf-config' debugfs interface

[Vladimir Zapolskiy]

The main goal of the change is to remove .pin_config_dbg_parse_modify
callback before a driver with its support appears. So far the in-kernel
interface did not attract any users since its introduction 5 years ago.

Originally .pin_config_dbg_parse_modify callback and the associated
'pinconf-config' debugfs file were introduced in commit f07512e615dd
("pinctrl/pinconfig: add debug interface"), a short description of
'pinconf-config' usage for debugging can be expressed this way:

Write to 'pinconf-config' (see pinconf_dbg_config_write() function):

% echo -n modify $map_type $device_name $state_name $pin_name $config > \
/sys/kernel/debug/pinctrl/$pinctrl/pinconf-config

It supposes to update a global (therefore single!) 'pinconf_dbg_conf'
variable with an alternative setting, the arguments should match
an existing pinconf device and some registered pinctrl mapping 'map':

* $map_type is either 'config_pin' or 'config_group', it should match
  'map->type' value of PIN_MAP_TYPE_CONFIGS_PIN or
   PIN_MAP_TYPE_CONFIGS_GROUP accordingly,
* $device_name should match 'map->dev_name' string value,
* $state_name should match 'map->name' string value,
* $pin_name should match 'map->data.configs.group_or_pin' string value,

If all above has matched, then $config is a new value to be set by calling
pinconfops->pin_config_dbg_parse_modify(pctldev, config, matched_config).

After a successful write into 'pinconf-config' a user can read the file
to get information about that single modified pin configuration.

The fact is .pin_config_dbg_parse_modify callback has never been defined
in 'struct pinconf_ops' of any pinconf driver, thus an actual modification
of a pin or group state on any present pinconf controller does not happen,
and it declares that all related code is no more than dead code.

I discovered the issue while attempting to add .pin_config_dbg_parse_modify
support in some drivers and found that too short 'MAX_NAME_LEN' set by

  drivers/pinctrl/pinconf.c:372:#define MAX_NAME_LEN 15

is practically insufficient to store a regular pinctrl device name,
which are like 'e606.pin-controller-sh-pfc' or pin names like
'MX6QDL_PAD_ENET_REF_CLK', thus it is another indicator that the code
is barely usable, insufficiently tested and unprepossessing.

Of course it might be possible to increase MAX_NAME_LEN, and then add
.pin_config_dbg_parse_modify callbacks to the drivers, but the whole
idea of such a limited debug option looks inviable. A more flexible
way to functionally substitute the original approach is to implicitly
or explicitly use pinctrl_select_state() function whenever needed.

Signed-off-by: Vladimir Zapolskiy 
Cc: Laurent Meunier 
Cc: Masahiro Yamada 
Cc: Russell King 
---
Changes from v1 to v2:
* moved header removal in include/linux/pinctrl/pinconf.h file to patch 1/2

 drivers/pinctrl/pinconf.c   | 222 
 include/linux/pinctrl/pinconf.h |   4 -
 2 files changed, 226 deletions(-)

diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 2c7229380f08..2678603df14b 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -17,7 +17,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -369,225 +368,6 @@ static int pinconf_groups_show(struct seq_file *s, void 
*what)
 DEFINE_SHOW_ATTRIBUTE(pinconf_pins);
 DEFINE_SHOW_ATTRIBUTE(pinconf_groups);
 
-#define MAX_NAME_LEN 15
-
-struct dbg_cfg {
-   enum pinctrl_map_type map_type;
-   char dev_name[MAX_NAME_LEN + 1];
-   char state_name[MAX_NAME_LEN + 1];
-   char pin_name[MAX_NAME_LEN + 1];
-};
-
-/*
- * Goal is to keep this structure as global in order to simply read the
- * pinconf-config file after a write to check config is as expected
- */
-static struct dbg_cfg pinconf_dbg_conf;
-
-/**
- * pinconf_dbg_config_print() - display the pinctrl config from the pinctrl
- * map, of the dev/pin/state that was last written to pinconf-config file.
- * @s: string filled in  with config description
- * @d: not used
- */
-static int pinconf_dbg_config_print(struct seq_file *s, void *d)
-{
-   struct pinctrl_maps *maps_node;
-   const struct pinctrl_map *map;
-   const struct pinctrl_map *found = NULL;
-   struct pinctrl_dev *pctldev;
-   struct dbg_cfg *dbg = _dbg_conf;
-   int i;
-
-   mutex_lock(_maps_mutex);
-
-   /* Parse the pinctrl map and look for the elected pin/state */
-   for_each_maps(maps_node, i, map) {
-   if (map->type != dbg->map_type)
-   continue;
-   if (strcmp(map->dev_name, dbg->dev_name))
-   continue;
-   if (strcmp(map->name, dbg->state_name))
-   continue;
-
-   if (!strcmp(map->data.configs.group_or_pin, dbg->pin_name)) {
-   /* We found the right pin */
- 

[PATCH v2 1/2] pinctrl: remove pinctrl/machine.h inclusion from pinctrl/pinconf.h

[Vladimir Zapolskiy]

The change adds explicit inclusion of linux/pinctrl/machine.h header
to the only needed pinctrl-madera-core.c file, and therefore inclusion
of pinctrl/machine.h header from pinctrl/pinconf.h can be removed.

The change is preparatory to a follow-up reversal of commit f07512e615dd
("pinctrl/pinconfig: add debug interface").

Signed-off-by: Vladimir Zapolskiy 
Cc: Charles Keepax 
Cc: Richard Fitzgerald 
---
Changes from v1 to v2:
* new change to mitigate a compile time warning caused by a removal of
  implicit header inclusion, the issue was reported against v1 change.

This change might be percepted as a bit awkward, but I leave a room to
extend it to other drivers, if it becomes needed. However apparently
only pinctrl/cirrus/pinctrl-madera-core.c misses explicit inclusion of
linux/pinctrl/machine.h header.

 drivers/pinctrl/cirrus/pinctrl-madera-core.c | 1 +
 include/linux/pinctrl/pinconf.h  | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pinctrl/cirrus/pinctrl-madera-core.c 
b/drivers/pinctrl/cirrus/pinctrl-madera-core.c
index a5dda832024a..7c9694593f79 100644
--- a/drivers/pinctrl/cirrus/pinctrl-madera-core.c
+++ b/drivers/pinctrl/cirrus/pinctrl-madera-core.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
index 8dd85d302b90..109468d9d849 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -14,8 +14,6 @@
 
 #ifdef CONFIG_PINCONF
 
-#include 
-
 struct pinctrl_dev;
 struct seq_file;
 
-- 
2.20.1

[PATCH v2 0/2] pinctrl: remove unused 'pinconf-config' debugfs interface

[Vladimir Zapolskiy]

The main goal of the change is to remove .pin_config_dbg_parse_modify
callback before a driver with its support appears. So far the in-kernel
interface did not attract any users since its introduction 5 years ago.

Originally .pin_config_dbg_parse_modify callback and the associated
'pinconf-config' debugfs file were introduced in commit f07512e615dd
("pinctrl/pinconfig: add debug interface"), a short description of
'pinconf-config' usage for debugging can be expressed this way:

Write to 'pinconf-config' (see pinconf_dbg_config_write() function):

% echo -n modify $map_type $device_name $state_name $pin_name $config > \
/sys/kernel/debug/pinctrl/$pinctrl/pinconf-config

It supposes to update a global (therefore single!) 'pinconf_dbg_conf'
variable with an alternative setting, the arguments should match
an existing pinconf device and some registered pinctrl mapping 'map':

* $map_type is either 'config_pin' or 'config_group', it should match
  'map->type' value of PIN_MAP_TYPE_CONFIGS_PIN or
   PIN_MAP_TYPE_CONFIGS_GROUP accordingly,
* $device_name should match 'map->dev_name' string value,
* $state_name should match 'map->name' string value,
* $pin_name should match 'map->data.configs.group_or_pin' string value,

If all above has matched, then $config is a new value to be set by calling
pinconfops->pin_config_dbg_parse_modify(pctldev, config, matched_config).

After a successful write into 'pinconf-config' a user can read the file
to get information about that single modified pin configuration.

The fact is .pin_config_dbg_parse_modify callback has never been defined
in 'struct pinconf_ops' of any pinconf driver, thus an actual modification
of a pin or group state on any present pinconf controller does not happen,
and it declares that all related code is no more than dead code.

I discovered the issue while attempting to add .pin_config_dbg_parse_modify
support in some drivers and found that too short 'MAX_NAME_LEN' set by

  drivers/pinctrl/pinconf.c:372:#define MAX_NAME_LEN 15

is practically insufficient to store a regular pinctrl device name,
which are like 'e606.pin-controller-sh-pfc' or pin names like
'MX6QDL_PAD_ENET_REF_CLK', thus it is another indicator that the code
is barely usable, insufficiently tested and unprepossessing.

Of course it might be possible to increase MAX_NAME_LEN, and then add
.pin_config_dbg_parse_modify callbacks to the drivers, but the whole
idea of such a limited debug option looks inviable. A more flexible
way to functionally substitute the original approach is to implicitly
or explicitly use pinctrl_select_state() function whenever needed.

The proposal is to remove the changes added by commit f07512e615dd
("pinctrl/pinconfig: add debug interface") for now, as described in
patch 2/2 commit message it may affect only non-vanilla kernels formally.

Note that still it is a debugfs UAPI change, which is supposed to be
unstable, however, since 'pinconf-config' file is plainly unusable
on vanilla, I won't expect any user reports or asks to restore it back,
this my assumption nudges me not to add an RFC tag to the published
change.

Changes from v1 to v2:
* added one patch against cirrus/pinctrl-madera-core.c to avoid a compile
  time warning due to a removed header inclusion

Vladimir Zapolskiy (2):
  pinctrl: remove pinctrl/machine.h inclusion from pinctrl/pinconf.h
  pinctrl: remove unused 'pinconf-config' debugfs interface

 drivers/pinctrl/cirrus/pinctrl-madera-core.c |   1 +
 drivers/pinctrl/pinconf.c| 222 ---
 include/linux/pinctrl/pinconf.h  |   6 -
 3 files changed, 1 insertion(+), 228 deletions(-)

-- 
2.20.1

[PATCH] pinctrl: remove unused 'pinconf-config' debugfs interface

[Vladimir Zapolskiy]

The main goal of the change is to remove .pin_config_dbg_parse_modify
callback before a driver with its support appears. So far the in-kernel
interface did not attract any users since its introduction 5 years ago.

Originally .pin_config_dbg_parse_modify callback and the associated
'pinconf-config' debugfs file were introduced in commit f07512e615dd
("pinctrl/pinconfig: add debug interface"), a short description of
'pinconf-config' usage for debugging can be expressed this way:

Write to 'pinconf-config' (see pinconf_dbg_config_write() function):

% echo -n modify $map_type $device_name $state_name $pin_name $config > \
/sys/kernel/debug/pinctrl/$pinctrl/pinconf-config

It supposes to update a global (therefore single!) 'pinconf_dbg_conf'
variable with an alternative setting, the arguments should match
an existing pinconf device and some registered pinctrl mapping 'map':

* $map_type is either 'config_pin' or 'config_group', it should match
  'map->type' value of PIN_MAP_TYPE_CONFIGS_PIN or
   PIN_MAP_TYPE_CONFIGS_GROUP accordingly,
* $device_name should match 'map->dev_name' string value,
* $state_name should match 'map->name' string value,
* $pin_name should match 'map->data.configs.group_or_pin' string value,

If all above has matched, then $config is a new value to be set by calling
pinconfops->pin_config_dbg_parse_modify(pctldev, config, matched_config).

After a successful write into 'pinconf-config' a user can read the file
to get information about that single modified pin configuration.

The fact is .pin_config_dbg_parse_modify callback has never been defined
in 'struct pinconf_ops' of any pinconf driver, thus an actual modification
of a pin or group state on any present pinconf controller does not happen,
and it declares that all related code is no more than dead code.

I discovered the issue while attempting to add .pin_config_dbg_parse_modify
support in some drivers and found that too short 'MAX_NAME_LEN' set by

  drivers/pinctrl/pinconf.c:372:#define MAX_NAME_LEN 15

is practically insufficient to store a regular pinctrl device name,
which are like 'e606.pin-controller-sh-pfc' or pin names like
'MX6QDL_PAD_ENET_REF_CLK', thus it is another indicator that the code
is barely usable, insufficiently tested and unprepossessing.

Of course it might be possible to increase MAX_NAME_LEN, and then add
.pin_config_dbg_parse_modify callbacks to the drivers, but the whole
idea of such a limited debug option looks inviable. A more flexible
way to functionally substitute the original approach is to implicitly
or explicitly use pinctrl_select_state() function whenever needed.

Signed-off-by: Vladimir Zapolskiy 
Cc: Laurent Meunier 
Cc: Masahiro Yamada 
Cc: Russell King 
---

The proposal is to remove the changes added by commit f07512e615dd
("pinctrl/pinconfig: add debug interface") for now, as described in
the commit message it may affect only non-vanilla kernels formally.

Note that still it is a debugfs UAPI change, however, since
'pinconf-config' file is plainly unusable on vanilla, I won't expect
any user reports or asks to restore it back, this my assumption
nudges me not to add an RFC tag to the published change.

 drivers/pinctrl/pinconf.c   | 222 
 include/linux/pinctrl/pinconf.h |   6 -
 2 files changed, 228 deletions(-)

diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 2c7229380f08..2678603df14b 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -17,7 +17,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -369,225 +368,6 @@ static int pinconf_groups_show(struct seq_file *s, void 
*what)
 DEFINE_SHOW_ATTRIBUTE(pinconf_pins);
 DEFINE_SHOW_ATTRIBUTE(pinconf_groups);
 
-#define MAX_NAME_LEN 15
-
-struct dbg_cfg {
-   enum pinctrl_map_type map_type;
-   char dev_name[MAX_NAME_LEN + 1];
-   char state_name[MAX_NAME_LEN + 1];
-   char pin_name[MAX_NAME_LEN + 1];
-};
-
-/*
- * Goal is to keep this structure as global in order to simply read the
- * pinconf-config file after a write to check config is as expected
- */
-static struct dbg_cfg pinconf_dbg_conf;
-
-/**
- * pinconf_dbg_config_print() - display the pinctrl config from the pinctrl
- * map, of the dev/pin/state that was last written to pinconf-config file.
- * @s: string filled in  with config description
- * @d: not used
- */
-static int pinconf_dbg_config_print(struct seq_file *s, void *d)
-{
-   struct pinctrl_maps *maps_node;
-   const struct pinctrl_map *map;
-   const struct pinctrl_map *found = NULL;
-   struct pinctrl_dev *pctldev;
-   struct dbg_cfg *dbg = _dbg_conf;
-   int i;
-
-   mutex_lock(_maps_mutex);
-
-   /* Parse the pinctrl map and look for the elected pin/state */
-   for_each_maps(maps_node, i, map) {
-   if (map->type != dbg->map_type)
-   continue;
-  

Re: RFC: gpio: mmio: add support for 3 direction regs

[Vladimir Zapolskiy]

On 01/03/2019 10:51 AM, Fried, Ramon wrote:
> 
> On 1/3/2019 10:07, Vladimir Zapolskiy wrote:
>> Hi Ramon,
>>
>> On 01/03/2019 09:36 AM, Fried, Ramon wrote:
>>> Hi.
>>>
>>> I'm working on a driver for STA2X11 GPIO controller who seems to fit
>>> best to the generic mmio driver,
>> I hope you have seen the existing driver drivers/gpio/gpio-sta2x11.c
> I surely did. we have the same IP in our soc but it was changed a lot
> internally, don't want to litter the original.
>>
>>> the only problem I have is with the dir register case. The STA2X11
>>> has 3 registers for dir, one for data, one for set and one for
>>> clear. The generic-mmio driver has support for this fashion for the
>>> dat & set & clear registers but not for dirout/dirin registers.
>>>
>>> I wonder if support for this is generic enough to deserve a patch, if
>>> so I'm willing to quickly add this support, if not, adding a flag
>>> such as below, will allow partly using the generic mmio driver only
>>> for set/get and the direction can be handled outside the driver.
>>>
>> If gpio-mmio fits well, then it might be simpler to set a flag
>> BGPIOF_UNREADABLE_REG_DIR, then call bgpio_init() and then overwrite
>> .direction_input, .direction_output and .get_direction callbacks,
>> as a reference you can take a look at gpio-74xx-mmio.c
> 
> Nice.
> That's an option I didn't think of, better than setting the flag.
> what about adding the generic support ?

Setting a GPIO direction over three different registers doesn't sound
as a suitable candidate for generalization, also your particular
implementation is partial, relies on external platform/driver code
to preset the directions and it does not allow to change direction
in runtime, so just renounce the idea.

--
Best wishes,
Vladimir

Re: RFC: gpio: mmio: add support for 3 direction regs

[Vladimir Zapolskiy]

Hi Ramon,

On 01/03/2019 09:36 AM, Fried, Ramon wrote:
> Hi.
> 
> I'm working on a driver for STA2X11 GPIO controller who seems to fit
> best to the generic mmio driver,

I hope you have seen the existing driver drivers/gpio/gpio-sta2x11.c

> the only problem I have is with the dir register case. The STA2X11
> has 3 registers for dir, one for data, one for set and one for
> clear. The generic-mmio driver has support for this fashion for the
> dat & set & clear registers but not for dirout/dirin registers.
> 
> I wonder if support for this is generic enough to deserve a patch, if
> so I'm willing to quickly add this support, if not, adding a flag
> such as below, will allow partly using the generic mmio driver only
> for set/get and the direction can be handled outside the driver.
> 

If gpio-mmio fits well, then it might be simpler to set a flag
BGPIOF_UNREADABLE_REG_DIR, then call bgpio_init() and then overwrite
.direction_input, .direction_output and .get_direction callbacks,
as a reference you can take a look at gpio-74xx-mmio.c

--
Best wishes,
Vladimir

Re: [PATCH] regmap: regmap-irq/gpio-max77620: add level-irq support

[Vladimir Zapolskiy]

On 12/10/2018 10:14 AM, Matti Vaittinen wrote:
> Add level active IRQ support to regmap-irq irqchip. Change breaks
> existing regmap-irq type setting. Convert the existing drivers which
> use regmap-irq with trigger type setting (gpio-max77620) to work
> with this new approach. So we do not magically support level-active
> IRQs on gpio-max77620 - but add support to the regmap-irq for chips
> which support them =)
> 
> We do not support distinguishing situation where HW supports rising
> and falling edge detection but not both. Separating this would require
> inventing yet another flags for IRQ types.
> 
> Signed-off-by: Matti Vaittinen 
> ---
> I did both the regmap-irq and max77620 changes in same commit because
> I'd rather not cause spot where max77620 breaks. Besides the changes in
> max77620 driver are trivial. Please let me know if this is not Ok.
> 
> Reason why I submit this patch now - even though my driver which would
> use level active type setting with regmap-irq is not yet ready for
> being submited - is that I'd like to minimize amount of existing drivers
> we need to patch. And if we add level active irq support like this then
> we must patch all existing drivers using type setting with regmap-irq.
> So doing this now when only max77620 uses type setting may be easier
> than postponing this to the future.
> 
> And finally - I don't have max77620 so I have only done _wery_ limited
> testing. So I would really appreciate if someone had time to review this
> thoroughly - and even happier if someone had possibility to try this out
> with the max77620.
> 

[snip]

> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index a367d59c301d..91c431ad98c3 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -1098,6 +1098,9 @@ int regmap_fields_update_bits_base(struct regmap_field 
> *field,  unsigned int id,
>   * @type_reg_offset: Offset register for the irq type setting.
>   * @type_rising_mask: Mask bit to configure RISING type irq.
>   * @type_falling_mask: Mask bit to configure FALLING type irq.
> + * @type_level_low_mask: Mask bit to configure LEVEL_LOW type irq.
> + * @type_level_high_mask: Mask bit to configure LEVEL_HIGH type irq.
> + * @types_supported: logical OR of IRQ_TYPE_* flags indicating supported 
> types.

While the existing interface is awful, you don't make it better.

.types_supported value is always derived from non-zero .type_*_mask values, so
it is simply not needed, as well as the whole change to gpio-max77620.c driver.

>   */
>  struct regmap_irq {
>   unsigned int reg_offset;
> @@ -1105,6 +1108,9 @@ struct regmap_irq {
>   unsigned int type_reg_offset;
>   unsigned int type_rising_mask;
>   unsigned int type_falling_mask;
> + unsigned int type_level_low_mask;
> + unsigned int type_level_high_mask;
> + unsigned int types_supported;
>  };
>  
>  #define REGMAP_IRQ_REG(_irq, _off, _mask)\
> 

--
Best wishes,
Vladimir

Re: [PATCH linux-next v2 6/6] ASoC: rsnd: add avb clocks

[Vladimir Zapolskiy]

Hi Jiada,

On 12/03/2018 01:24 PM, jiada_w...@mentor.com wrote:
> From: Jiada Wang 
> 
> There are AVB Counter Clocks in ADG, each clock has 12bits integral
> and 8 bits fractional dividers which operates with S0D1ϕ clock.
> 
> This patch registers 8 AVB Counter Clocks when clock-cells of
> rcar_sound node is 2,
> 
> Signed-off-by: Jiada Wang 
> ---
>  sound/soc/sh/rcar/adg.c  | 306 +--
>  sound/soc/sh/rcar/gen.c  |   9 ++
>  sound/soc/sh/rcar/rsnd.h |   9 ++
>  3 files changed, 315 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
> index 6768a66588eb..2c03d420ae76 100644
> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -5,6 +5,8 @@
>  //  Copyright (C) 2013  Kuninori Morimoto 
>  
>  #include 
> +#include 
> +#include 

Drop the inclusion of the header above, see my comment to patch 5/6.

>  #include "rsnd.h"
>  
>  #define CLKA 0
> @@ -21,13 +23,33 @@
>  
>  #define BRRx_MASK(x) (0x3FF & x)
>  
> +#define ADG_CLK_NAME "adg"
> +#define AVB_CLK_NAME "avb"

Can you please remove two macro above and replace their usage by values in 
clk_register_avb() function?

Also I don't think that it is good to hardcode parent clock name here,
likely you should get it in runtime, see __clk_get_name().

> +#define AVB_CLK_NUM  8
> +#define AVB_CLK_NAME_SIZE10

The one macro above also can be removed in my opinion.

> +#define AVB_MAX_RATE 2500
> +#define AVB_DIV_EN_COM   BIT(31)
> +#define AVB_DIV_MASK 0x3
> +#define AVB_MAX_DIV  0x3ffc0
> +
>  static struct rsnd_mod_ops adg_ops = {
>   .name = "adg",
>  };
>  
> +struct clk_avb {
> + struct clk_hw hw;
> + unsigned int idx;
> + struct rsnd_mod *mod;
> + /* lock reg access */
> + spinlock_t *lock;
> +};
> +
> +#define to_clk_avb(_hw) container_of(_hw, struct clk_avb, hw)
> +
>  struct rsnd_adg {
>   struct clk *clk[CLKMAX];
>   struct clk *clkout[CLKOUTMAX];
> + struct clk *clkavb[AVB_CLK_NUM];
>   struct clk_onecell_data onecell;
>   struct rsnd_mod mod;
>   u32 flags;
> @@ -37,6 +59,7 @@ struct rsnd_adg {
>  
>   int rbga_rate_for_441khz; /* RBGA */
>   int rbgb_rate_for_48khz;  /* RBGB */
> + spinlock_t avb_lock;
>  };
>  
>  #define LRCLK_ASYNC  (1 << 0)
> @@ -408,6 +431,239 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int 
> enable)
>   }
>  }
>  
> +static struct clk *rsnd_adg_clk_src_twocell_get(struct of_phandle_args 
> *clkspec,
> + void *data)
> +{
> + unsigned int clkidx = clkspec->args[1];
> + struct rsnd_adg *adg = data;
> + const char *type;
> + struct clk *clk;
> +
> + switch (clkspec->args[0]) {
> + case ADG_FIX:
> + type = "fixed";

Apparently you need 'type' local variable just to print an error message.

Please remove the variable and update format strings accordingly.

> + if (clkidx >= CLKOUTMAX) {
> + pr_err("Invalid %s clock index %u\n", type,
> +clkidx);
> + return ERR_PTR(-EINVAL);
> + }
> + clk = adg->clkout[clkidx];
> + break;
> + case ADG_AVB:
> + type = "avb";
> + if (clkidx >= AVB_CLK_NUM) {
> + pr_err("Invalid %s clock index %u\n", type,
> +clkidx);
> + return ERR_PTR(-EINVAL);
> + }
> + clk = adg->clkavb[clkidx];
> + break;
> + default:
> + pr_err("Invalid ADG clock type %u\n", clkspec->args[0]);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return clk;
> +}
> +
> +static void clk_avb_div_write(struct rsnd_mod *mod, u32 data, int idx)

unsigned int idx to match a type of 'struct clk_avb' field.

> +{
> + switch (idx) {
> + case 0:
> + rsnd_mod_write(mod, AVB_CLK_DIV0, data);
> + break;
> + case 1:
> + rsnd_mod_write(mod, AVB_CLK_DIV1, data);
> + break;
> + case 2:
> + rsnd_mod_write(mod, AVB_CLK_DIV2, data);
> + break;
> + case 3:
> + rsnd_mod_write(mod, AVB_CLK_DIV3, data);
> + break;
> + case 4:
> + rsnd_mod_write(mod, AVB_CLK_DIV4, data);
> + break;
> + case 5:
> + rsnd_mod_write(mod, AVB_CLK_DIV5, data);
> + break;
> + case 6:
> + rsnd_mod_write(mod, AVB_CLK_DIV6, data);
> + break;
> + case 7:
> + rsnd_mod_write(mod, AVB_CLK_DIV7, data);
> + break;
> + }
> +}
> +
> +static u32 clk_avb_div_read(struct rsnd_mod *mod, int idx)

unsigned int idx to match a type of 'struct clk_avb' field.

> +{
> + u32 val = 0;
> +
> + switch (idx) {
> + case 0:
> + val = rsnd_mod_read(mod, AVB_CLK_DIV0);
> 

Re: [PATCH linux-next v2 6/6] ASoC: rsnd: add avb clocks

[Vladimir Zapolskiy]

Hi Jiada,

On 12/03/2018 01:24 PM, jiada_w...@mentor.com wrote:
> From: Jiada Wang 
> 
> There are AVB Counter Clocks in ADG, each clock has 12bits integral
> and 8 bits fractional dividers which operates with S0D1ϕ clock.
> 
> This patch registers 8 AVB Counter Clocks when clock-cells of
> rcar_sound node is 2,
> 
> Signed-off-by: Jiada Wang 
> ---
>  sound/soc/sh/rcar/adg.c  | 306 +--
>  sound/soc/sh/rcar/gen.c  |   9 ++
>  sound/soc/sh/rcar/rsnd.h |   9 ++
>  3 files changed, 315 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
> index 6768a66588eb..2c03d420ae76 100644
> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -5,6 +5,8 @@
>  //  Copyright (C) 2013  Kuninori Morimoto 
>  
>  #include 
> +#include 
> +#include 

Drop the inclusion of the header above, see my comment to patch 5/6.

>  #include "rsnd.h"
>  
>  #define CLKA 0
> @@ -21,13 +23,33 @@
>  
>  #define BRRx_MASK(x) (0x3FF & x)
>  
> +#define ADG_CLK_NAME "adg"
> +#define AVB_CLK_NAME "avb"

Can you please remove two macro above and replace their usage by values in 
clk_register_avb() function?

Also I don't think that it is good to hardcode parent clock name here,
likely you should get it in runtime, see __clk_get_name().

> +#define AVB_CLK_NUM  8
> +#define AVB_CLK_NAME_SIZE10

The one macro above also can be removed in my opinion.

> +#define AVB_MAX_RATE 2500
> +#define AVB_DIV_EN_COM   BIT(31)
> +#define AVB_DIV_MASK 0x3
> +#define AVB_MAX_DIV  0x3ffc0
> +
>  static struct rsnd_mod_ops adg_ops = {
>   .name = "adg",
>  };
>  
> +struct clk_avb {
> + struct clk_hw hw;
> + unsigned int idx;
> + struct rsnd_mod *mod;
> + /* lock reg access */
> + spinlock_t *lock;
> +};
> +
> +#define to_clk_avb(_hw) container_of(_hw, struct clk_avb, hw)
> +
>  struct rsnd_adg {
>   struct clk *clk[CLKMAX];
>   struct clk *clkout[CLKOUTMAX];
> + struct clk *clkavb[AVB_CLK_NUM];
>   struct clk_onecell_data onecell;
>   struct rsnd_mod mod;
>   u32 flags;
> @@ -37,6 +59,7 @@ struct rsnd_adg {
>  
>   int rbga_rate_for_441khz; /* RBGA */
>   int rbgb_rate_for_48khz;  /* RBGB */
> + spinlock_t avb_lock;
>  };
>  
>  #define LRCLK_ASYNC  (1 << 0)
> @@ -408,6 +431,239 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int 
> enable)
>   }
>  }
>  
> +static struct clk *rsnd_adg_clk_src_twocell_get(struct of_phandle_args 
> *clkspec,
> + void *data)
> +{
> + unsigned int clkidx = clkspec->args[1];
> + struct rsnd_adg *adg = data;
> + const char *type;
> + struct clk *clk;
> +
> + switch (clkspec->args[0]) {
> + case ADG_FIX:
> + type = "fixed";

Apparently you need 'type' local variable just to print an error message.

Please remove the variable and update format strings accordingly.

> + if (clkidx >= CLKOUTMAX) {
> + pr_err("Invalid %s clock index %u\n", type,
> +clkidx);
> + return ERR_PTR(-EINVAL);
> + }
> + clk = adg->clkout[clkidx];
> + break;
> + case ADG_AVB:
> + type = "avb";
> + if (clkidx >= AVB_CLK_NUM) {
> + pr_err("Invalid %s clock index %u\n", type,
> +clkidx);
> + return ERR_PTR(-EINVAL);
> + }
> + clk = adg->clkavb[clkidx];
> + break;
> + default:
> + pr_err("Invalid ADG clock type %u\n", clkspec->args[0]);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return clk;
> +}
> +
> +static void clk_avb_div_write(struct rsnd_mod *mod, u32 data, int idx)

unsigned int idx to match a type of 'struct clk_avb' field.

> +{
> + switch (idx) {
> + case 0:
> + rsnd_mod_write(mod, AVB_CLK_DIV0, data);
> + break;
> + case 1:
> + rsnd_mod_write(mod, AVB_CLK_DIV1, data);
> + break;
> + case 2:
> + rsnd_mod_write(mod, AVB_CLK_DIV2, data);
> + break;
> + case 3:
> + rsnd_mod_write(mod, AVB_CLK_DIV3, data);
> + break;
> + case 4:
> + rsnd_mod_write(mod, AVB_CLK_DIV4, data);
> + break;
> + case 5:
> + rsnd_mod_write(mod, AVB_CLK_DIV5, data);
> + break;
> + case 6:
> + rsnd_mod_write(mod, AVB_CLK_DIV6, data);
> + break;
> + case 7:
> + rsnd_mod_write(mod, AVB_CLK_DIV7, data);
> + break;
> + }
> +}
> +
> +static u32 clk_avb_div_read(struct rsnd_mod *mod, int idx)

unsigned int idx to match a type of 'struct clk_avb' field.

> +{
> + u32 val = 0;
> +
> + switch (idx) {
> + case 0:
> + val = rsnd_mod_read(mod, AVB_CLK_DIV0);
> 

Re: [PATCH linux-next v2 5/6] dt-bindings: clock: add clock id for renesas adg clocks

[Vladimir Zapolskiy]

Hi Jiada,

On 12/03/2018 01:24 PM, jiada_w...@mentor.com wrote:
> From: Jiada Wang 
> 
> This patch adds clock ID for renesas adg clocks
> 
> Signed-off-by: Jiada Wang 
> ---
>  include/dt-bindings/clock/renesas-adg.h | 11 +++
>  1 file changed, 11 insertions(+)
>  create mode 100644 include/dt-bindings/clock/renesas-adg.h
> 
> diff --git a/include/dt-bindings/clock/renesas-adg.h 
> b/include/dt-bindings/clock/renesas-adg.h
> new file mode 100644
> index ..fe30c186cef7
> --- /dev/null
> +++ b/include/dt-bindings/clock/renesas-adg.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 Mentor Graphics inc.
> + */
> +#ifndef __DT_BINDINGS_RENESAS_ADG_H__
> +#define __DT_BINDINGS_RENESAS_ADG_H__
> +
> +#define ADG_FIX  0
> +#define ADG_AVB  1
> +
> +#endif /* __DT_BINDINGS_RENESAS_ADG_H__ */
> 

instead of adding these trivial definitions in a new header file with no
potential for further extension, I would like to ask you to write a proper
device tree bindings documentation section and describe the clock ids as
a list of acceptable values for a cell.

--
Best wishes,
Vladimir

Re: [PATCH linux-next v2 5/6] dt-bindings: clock: add clock id for renesas adg clocks

[Vladimir Zapolskiy]

Hi Jiada,

On 12/03/2018 01:24 PM, jiada_w...@mentor.com wrote:
> From: Jiada Wang 
> 
> This patch adds clock ID for renesas adg clocks
> 
> Signed-off-by: Jiada Wang 
> ---
>  include/dt-bindings/clock/renesas-adg.h | 11 +++
>  1 file changed, 11 insertions(+)
>  create mode 100644 include/dt-bindings/clock/renesas-adg.h
> 
> diff --git a/include/dt-bindings/clock/renesas-adg.h 
> b/include/dt-bindings/clock/renesas-adg.h
> new file mode 100644
> index ..fe30c186cef7
> --- /dev/null
> +++ b/include/dt-bindings/clock/renesas-adg.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 Mentor Graphics inc.
> + */
> +#ifndef __DT_BINDINGS_RENESAS_ADG_H__
> +#define __DT_BINDINGS_RENESAS_ADG_H__
> +
> +#define ADG_FIX  0
> +#define ADG_AVB  1
> +
> +#endif /* __DT_BINDINGS_RENESAS_ADG_H__ */
> 

instead of adding these trivial definitions in a new header file with no
potential for further extension, I would like to ask you to write a proper
device tree bindings documentation section and describe the clock ids as
a list of acceptable values for a cell.

--
Best wishes,
Vladimir

Re: [PATCH linux-next v2 0/6] clk: renesas: adg: add AVB Clock

[Vladimir Zapolskiy]

Hi Jiada,

On 12/03/2018 01:21 PM, jiada_w...@mentor.com wrote:
> From: Jiada Wang 
> 
> on R-Car SoCs there are AVB Counter Clocks, each clock has 12bits integral
> and 8 bits fractional dividers which operates with S0D1ϕ clock.
> 
> This patch-set adds 'adg' clock to R-Car Soc, and changes adg driver to
> register avb clocks when clock-cells of rcar_sound node is 2.
> 
> ---
> v2:
> - expends adg register size and register avb clocks instead of
>   add new clk-avb driver
> - Add adg clock 
> 
> v1: initial version
> 
> Jiada Wang (2):
>   dt-bindings: clock: add clock id for renesas adg clocks
>   ASoC: rsnd: add avb clocks
> 
> Takeshi Kihara (4):
>   clk: renesas: r8a7795: Add ADG clock
>   clk: renesas: r8a7796: Add ADG clock
>   clk: renesas: r8a77990: Add ADG clocks
>   clk: renesas: r8a77995: Add ADG clock
> 

plural 'clocks' for r8a77990 vs. 'clock' in other cases, please unify subjects.

You can consider to add the ADG clock description for r8a77965 / M3-N as well.

>  drivers/clk/renesas/r8a7795-cpg-mssr.c  |   1 +
>  drivers/clk/renesas/r8a7796-cpg-mssr.c  |   1 +
>  drivers/clk/renesas/r8a77990-cpg-mssr.c |   1 +
>  drivers/clk/renesas/r8a77995-cpg-mssr.c |   1 +
>  include/dt-bindings/clock/renesas-adg.h |  11 +

The new header file added above is not needed in my opinion.

>  sound/soc/sh/rcar/adg.c | 306 +++-
>  sound/soc/sh/rcar/gen.c |   9 +
>  sound/soc/sh/rcar/rsnd.h|   9 +
>  8 files changed, 330 insertions(+), 9 deletions(-)
>  create mode 100644 include/dt-bindings/clock/renesas-adg.h
> 

--
Best wishes,
Vladimir

Re: [PATCH linux-next v2 0/6] clk: renesas: adg: add AVB Clock

[Vladimir Zapolskiy]

Hi Jiada,

On 12/03/2018 01:21 PM, jiada_w...@mentor.com wrote:
> From: Jiada Wang 
> 
> on R-Car SoCs there are AVB Counter Clocks, each clock has 12bits integral
> and 8 bits fractional dividers which operates with S0D1ϕ clock.
> 
> This patch-set adds 'adg' clock to R-Car Soc, and changes adg driver to
> register avb clocks when clock-cells of rcar_sound node is 2.
> 
> ---
> v2:
> - expends adg register size and register avb clocks instead of
>   add new clk-avb driver
> - Add adg clock 
> 
> v1: initial version
> 
> Jiada Wang (2):
>   dt-bindings: clock: add clock id for renesas adg clocks
>   ASoC: rsnd: add avb clocks
> 
> Takeshi Kihara (4):
>   clk: renesas: r8a7795: Add ADG clock
>   clk: renesas: r8a7796: Add ADG clock
>   clk: renesas: r8a77990: Add ADG clocks
>   clk: renesas: r8a77995: Add ADG clock
> 

plural 'clocks' for r8a77990 vs. 'clock' in other cases, please unify subjects.

You can consider to add the ADG clock description for r8a77965 / M3-N as well.

>  drivers/clk/renesas/r8a7795-cpg-mssr.c  |   1 +
>  drivers/clk/renesas/r8a7796-cpg-mssr.c  |   1 +
>  drivers/clk/renesas/r8a77990-cpg-mssr.c |   1 +
>  drivers/clk/renesas/r8a77995-cpg-mssr.c |   1 +
>  include/dt-bindings/clock/renesas-adg.h |  11 +

The new header file added above is not needed in my opinion.

>  sound/soc/sh/rcar/adg.c | 306 +++-
>  sound/soc/sh/rcar/gen.c |   9 +
>  sound/soc/sh/rcar/rsnd.h|   9 +
>  8 files changed, 330 insertions(+), 9 deletions(-)
>  create mode 100644 include/dt-bindings/clock/renesas-adg.h
> 

--
Best wishes,
Vladimir

Re: [PATCH] iio: adc: Replace license text w/ SPDX identifier

[Vladimir Zapolskiy]

On 11/28/2018 07:53 PM, Matheus Tavares wrote:
> From: Lucas Santos 
> 
> This patch removes all license boilerplate texts from the .c and .h
> files at drivers/iio/adc/ and, instead, adds the proper SPDX license
> identifiers.
> 
> Signed-off-by: Lucas Santos 
> Signed-off-by: Matheus Tavares 

[snip]

> diff --git a/drivers/iio/adc/lpc18xx_adc.c b/drivers/iio/adc/lpc18xx_adc.c
> index 041dc4a3f66c..00d39234143d 100644
> --- a/drivers/iio/adc/lpc18xx_adc.c
> +++ b/drivers/iio/adc/lpc18xx_adc.c
> @@ -1,12 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * IIO ADC driver for NXP LPC18xx ADC
>   *
>   * Copyright (C) 2016 Joachim Eastwood 
>   *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
>   * UNSUPPORTED hardware features:
>   *  - Hardware triggers
>   *  - Burst mode
> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> index 20b36690fa4f..e361c1532a75 100644
> --- a/drivers/iio/adc/lpc32xx_adc.c
> +++ b/drivers/iio/adc/lpc32xx_adc.c
> @@ -1,23 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
>   *  lpc32xx_adc.c - Support for ADC in LPC32XX
>   *
>   *  3-channel, 10-bit ADC
>   *
>   *  Copyright (C) 2011, 2012 Roland Stigge 
> - *
> - *  This program is free software; you can redistribute it and/or modify
> - *  it under the terms of the GNU General Public License as published by
> - *  the Free Software Foundation; either version 2 of the License, or
> - *  (at your option) any later version.
> - *
> - *  This program is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *  GNU General Public License for more details.
> - *
> - *  You should have received a copy of the GNU General Public License
> - *  along with this program; if not, write to the Free Software
> - *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
>  #include 

For lpc18xx/lpc32xx:

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] iio: adc: Replace license text w/ SPDX identifier

[Vladimir Zapolskiy]

On 11/28/2018 07:53 PM, Matheus Tavares wrote:
> From: Lucas Santos 
> 
> This patch removes all license boilerplate texts from the .c and .h
> files at drivers/iio/adc/ and, instead, adds the proper SPDX license
> identifiers.
> 
> Signed-off-by: Lucas Santos 
> Signed-off-by: Matheus Tavares 

[snip]

> diff --git a/drivers/iio/adc/lpc18xx_adc.c b/drivers/iio/adc/lpc18xx_adc.c
> index 041dc4a3f66c..00d39234143d 100644
> --- a/drivers/iio/adc/lpc18xx_adc.c
> +++ b/drivers/iio/adc/lpc18xx_adc.c
> @@ -1,12 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * IIO ADC driver for NXP LPC18xx ADC
>   *
>   * Copyright (C) 2016 Joachim Eastwood 
>   *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
>   * UNSUPPORTED hardware features:
>   *  - Hardware triggers
>   *  - Burst mode
> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> index 20b36690fa4f..e361c1532a75 100644
> --- a/drivers/iio/adc/lpc32xx_adc.c
> +++ b/drivers/iio/adc/lpc32xx_adc.c
> @@ -1,23 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
>   *  lpc32xx_adc.c - Support for ADC in LPC32XX
>   *
>   *  3-channel, 10-bit ADC
>   *
>   *  Copyright (C) 2011, 2012 Roland Stigge 
> - *
> - *  This program is free software; you can redistribute it and/or modify
> - *  it under the terms of the GNU General Public License as published by
> - *  the Free Software Foundation; either version 2 of the License, or
> - *  (at your option) any later version.
> - *
> - *  This program is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *  GNU General Public License for more details.
> - *
> - *  You should have received a copy of the GNU General Public License
> - *  along with this program; if not, write to the Free Software
> - *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
>  #include 

For lpc18xx/lpc32xx:

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

[PATCH] gpio: restore original GPLv2+ license of gpiolib-of.c sources

[Vladimir Zapolskiy]

It's easy to verify that the change of drivers/gpio/gpiolib-of.c license
header to SPDX standard changes the license from GPLv2+ to GPLv2, and
this change corrects it.

Fixes: dae5f0afcfc3 ("gpio: Use SPDX header for core library")
Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpio/gpiolib-of.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 7f1260c78270..59cb87325179 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * OF helpers for the GPIO API
  *
-- 
2.17.1

[PATCH] gpio: restore original GPLv2+ license of gpiolib-of.c sources

[Vladimir Zapolskiy]

It's easy to verify that the change of drivers/gpio/gpiolib-of.c license
header to SPDX standard changes the license from GPLv2+ to GPLv2, and
this change corrects it.

Fixes: dae5f0afcfc3 ("gpio: Use SPDX header for core library")
Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpio/gpiolib-of.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 7f1260c78270..59cb87325179 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * OF helpers for the GPIO API
  *
-- 
2.17.1

Re: [PATCH] ARM: dts: i.MX25: add the clocks for the EPIT blocks

[Vladimir Zapolskiy]

Adding Clément.

On 11/04/2018 04:46 PM, Shawn Guo wrote:
> On Thu, Nov 01, 2018 at 06:32:47PM +0100, Martin Kaiser wrote:
>> The i.MX25 contains two EPIT (Enhanced Periodic Interrupt Timer)
>> function blocks. Add their ipg and per clocks to the device tree.
>>
>> Signed-off-by: Martin Kaiser 
> 
> Are these EPIT devices actually used in upstream kernel, or just
> somewhere else?

Clément wrote a good driver, which has already passed a series of reviews,
but this clocks add-on is actually missing.

Clément, can you please add clock-names property and the corresponding
functional change to your driver? Thank you in advance.

--
Best wishes,
Vladimir

>> ---
>>
>> I tried to make this similar to gpt1 and gpt2.
>>
>>  arch/arm/boot/dts/imx25.dtsi | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
>> index b25309d26ea5..e80101847aff 100644
>> --- a/arch/arm/boot/dts/imx25.dtsi
>> +++ b/arch/arm/boot/dts/imx25.dtsi
>> @@ -388,12 +388,16 @@
>>  epit1: timer@53f94000 {
>>  compatible = "fsl,imx25-epit";
>>  reg = <0x53f94000 0x4000>;
>> +clocks = < 83>, < 43>;
>> +clock-names = "ipg", "per";
>>  interrupts = <28>;
>>  };
>>  
>>  epit2: timer@53f98000 {
>>  compatible = "fsl,imx25-epit";
>>  reg = <0x53f98000 0x4000>;
>> +clocks = < 84>, < 43>;
>> +clock-names = "ipg", "per";
>>  interrupts = <27>;
>>  };
>>  
>> -- 
>> 2.1.4
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: i.MX25: add the clocks for the EPIT blocks

[Vladimir Zapolskiy]

Adding Clément.

On 11/04/2018 04:46 PM, Shawn Guo wrote:
> On Thu, Nov 01, 2018 at 06:32:47PM +0100, Martin Kaiser wrote:
>> The i.MX25 contains two EPIT (Enhanced Periodic Interrupt Timer)
>> function blocks. Add their ipg and per clocks to the device tree.
>>
>> Signed-off-by: Martin Kaiser 
> 
> Are these EPIT devices actually used in upstream kernel, or just
> somewhere else?

Clément wrote a good driver, which has already passed a series of reviews,
but this clocks add-on is actually missing.

Clément, can you please add clock-names property and the corresponding
functional change to your driver? Thank you in advance.

--
Best wishes,
Vladimir

>> ---
>>
>> I tried to make this similar to gpt1 and gpt2.
>>
>>  arch/arm/boot/dts/imx25.dtsi | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
>> index b25309d26ea5..e80101847aff 100644
>> --- a/arch/arm/boot/dts/imx25.dtsi
>> +++ b/arch/arm/boot/dts/imx25.dtsi
>> @@ -388,12 +388,16 @@
>>  epit1: timer@53f94000 {
>>  compatible = "fsl,imx25-epit";
>>  reg = <0x53f94000 0x4000>;
>> +clocks = < 83>, < 43>;
>> +clock-names = "ipg", "per";
>>  interrupts = <28>;
>>  };
>>  
>>  epit2: timer@53f98000 {
>>  compatible = "fsl,imx25-epit";
>>  reg = <0x53f98000 0x4000>;
>> +clocks = < 84>, < 43>;
>> +clock-names = "ipg", "per";
>>  interrupts = <27>;
>>  };
>>  
>> -- 
>> 2.1.4
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: i.MX25: add the clocks for the EPIT blocks

[Vladimir Zapolskiy]

Hi Martin,

On 11/01/2018 07:32 PM, Martin Kaiser wrote:
> The i.MX25 contains two EPIT (Enhanced Periodic Interrupt Timer)
> function blocks. Add their ipg and per clocks to the device tree.
> 
> Signed-off-by: Martin Kaiser 

the change is obviously correct.

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] ARM: dts: i.MX25: add the clocks for the EPIT blocks

[Vladimir Zapolskiy]

Hi Martin,

On 11/01/2018 07:32 PM, Martin Kaiser wrote:
> The i.MX25 contains two EPIT (Enhanced Periodic Interrupt Timer)
> function blocks. Add their ipg and per clocks to the device tree.
> 
> Signed-off-by: Martin Kaiser 

the change is obviously correct.

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] pinctrl: lpc18xx: Use define directive for PIN_CONFIG_GPIO_PIN_INT

[Vladimir Zapolskiy]

Hi Nathan,

thank you for your patch.

On 11/01/2018 02:52 AM, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another:
> 
> drivers/pinctrl/pinctrl-lpc18xx.c:643:29: warning: implicit conversion
> from enumeration type 'enum lpc18xx_pin_config_param' to different
> enumeration type 'enum pin_config_param' [-Wenum-conversion]
> {"nxp,gpio-pin-interrupt", PIN_CONFIG_GPIO_PIN_INT, 0},
> ~  ^~~
> drivers/pinctrl/pinctrl-lpc18xx.c:648:12: warning: implicit conversion
> from enumeration type 'enum lpc18xx_pin_config_param' to different
> enumeration type 'enum pin_config_param' [-Wenum-conversion]
> PCONFDUMP(PIN_CONFIG_GPIO_PIN_INT, "gpio pin int", NULL, true),
> ~~^~~~
> ./include/linux/pinctrl/pinconf-generic.h:163:11: note: expanded from
> macro 'PCONFDUMP'
> .param = a, .display = b, .format = c, .has_arg = d \
>  ^
> 2 warnings generated.
> 
> It is expected that pinctrl drivers can extend pin_config_param because
> of the gap between PIN_CONFIG_END and PIN_CONFIG_MAX so this conversion
> isn't an issue. Most drivers that take advantage of this define the
> PIN_CONFIG variables as constants, rather than enumerated values. Do the
> same thing here so that Clang no longer warns.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/140
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/pinctrl/pinctrl-lpc18xx.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c 
> b/drivers/pinctrl/pinctrl-lpc18xx.c
> index a14bc5e5fc24..4bee606088e1 100644
> --- a/drivers/pinctrl/pinctrl-lpc18xx.c
> +++ b/drivers/pinctrl/pinctrl-lpc18xx.c
> @@ -631,13 +631,10 @@ static const struct pinctrl_pin_desc lpc18xx_pins[] = {
>  };
>  
>  /**
> - * enum lpc18xx_pin_config_param - possible pin configuration parameters
>   * @PIN_CONFIG_GPIO_PIN_INT: route gpio to the gpio pin interrupt
>   *   controller.
>   */
> -enum lpc18xx_pin_config_param {
> - PIN_CONFIG_GPIO_PIN_INT = PIN_CONFIG_END + 1,
> -};
> +#define PIN_CONFIG_GPIO_PIN_INT  (PIN_CONFIG_END + 1)
>  
>  static const struct pinconf_generic_params lpc18xx_params[] = {
>   {"nxp,gpio-pin-interrupt", PIN_CONFIG_GPIO_PIN_INT, 0},
> 

The change, if it is applied, starts to produce a W=1 warning:

drivers/pinctrl/pinctrl-lpc18xx.c:634: warning: Cannot understand  * 
@PIN_CONFIG_GPIO_PIN_INT: route gpio to the gpio pin interrupt on line 634 - I 
thought it was a doc line

Could you please take a look how to satisfy process_name() check from 
scripts/kernel-doc?

My proposals are:
1) change the first line of the comment block from '/**' to '/*',
2) remove '@' prefix symbol and place pinconf description on one line.

--
Best wishes,
Vladimir

Re: [PATCH] pinctrl: lpc18xx: Use define directive for PIN_CONFIG_GPIO_PIN_INT

[Vladimir Zapolskiy]

Hi Nathan,

thank you for your patch.

On 11/01/2018 02:52 AM, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another:
> 
> drivers/pinctrl/pinctrl-lpc18xx.c:643:29: warning: implicit conversion
> from enumeration type 'enum lpc18xx_pin_config_param' to different
> enumeration type 'enum pin_config_param' [-Wenum-conversion]
> {"nxp,gpio-pin-interrupt", PIN_CONFIG_GPIO_PIN_INT, 0},
> ~  ^~~
> drivers/pinctrl/pinctrl-lpc18xx.c:648:12: warning: implicit conversion
> from enumeration type 'enum lpc18xx_pin_config_param' to different
> enumeration type 'enum pin_config_param' [-Wenum-conversion]
> PCONFDUMP(PIN_CONFIG_GPIO_PIN_INT, "gpio pin int", NULL, true),
> ~~^~~~
> ./include/linux/pinctrl/pinconf-generic.h:163:11: note: expanded from
> macro 'PCONFDUMP'
> .param = a, .display = b, .format = c, .has_arg = d \
>  ^
> 2 warnings generated.
> 
> It is expected that pinctrl drivers can extend pin_config_param because
> of the gap between PIN_CONFIG_END and PIN_CONFIG_MAX so this conversion
> isn't an issue. Most drivers that take advantage of this define the
> PIN_CONFIG variables as constants, rather than enumerated values. Do the
> same thing here so that Clang no longer warns.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/140
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/pinctrl/pinctrl-lpc18xx.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c 
> b/drivers/pinctrl/pinctrl-lpc18xx.c
> index a14bc5e5fc24..4bee606088e1 100644
> --- a/drivers/pinctrl/pinctrl-lpc18xx.c
> +++ b/drivers/pinctrl/pinctrl-lpc18xx.c
> @@ -631,13 +631,10 @@ static const struct pinctrl_pin_desc lpc18xx_pins[] = {
>  };
>  
>  /**
> - * enum lpc18xx_pin_config_param - possible pin configuration parameters
>   * @PIN_CONFIG_GPIO_PIN_INT: route gpio to the gpio pin interrupt
>   *   controller.
>   */
> -enum lpc18xx_pin_config_param {
> - PIN_CONFIG_GPIO_PIN_INT = PIN_CONFIG_END + 1,
> -};
> +#define PIN_CONFIG_GPIO_PIN_INT  (PIN_CONFIG_END + 1)
>  
>  static const struct pinconf_generic_params lpc18xx_params[] = {
>   {"nxp,gpio-pin-interrupt", PIN_CONFIG_GPIO_PIN_INT, 0},
> 

The change, if it is applied, starts to produce a W=1 warning:

drivers/pinctrl/pinctrl-lpc18xx.c:634: warning: Cannot understand  * 
@PIN_CONFIG_GPIO_PIN_INT: route gpio to the gpio pin interrupt on line 634 - I 
thought it was a doc line

Could you please take a look how to satisfy process_name() check from 
scripts/kernel-doc?

My proposals are:
1) change the first line of the comment block from '/**' to '/*',
2) remove '@' prefix symbol and place pinconf description on one line.

--
Best wishes,
Vladimir

Re: [PATCH 3/7] dt-bindings: pinctrl: ds90ux9xx: add description of TI DS90Ux9xx pinmux

[Vladimir Zapolskiy]

Hi Luca,

On 10/30/2018 06:44 PM, Luca Ceresoli wrote:
> Hi Vladimir,
> 
> On 16/10/18 14:48, Laurent Pinchart wrote:
>> Hi Vladimir,
>>
>> On Saturday, 13 October 2018 16:47:48 EEST Vladimir Zapolskiy wrote:
>>> On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
>>>> On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
>>>>> From: Vladimir Zapolskiy 
>>>>>
>>>>> TI DS90Ux9xx de-/serializers have a capability to multiplex pin
>>>>> functions, in particular a pin may have selectable functions of GPIO,
>>>>> GPIO line transmitter, one of I2S lines, one of RGB24 video signal lines
>>>>> and so on.
>>>>>
>>>>> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
>>>>> controllers.
>>>>>
>>>>> Signed-off-by: Vladimir Zapolskiy 
> [...]
>>>>> +Available pins, groups and functions (reference to device datasheets):
>>>>> +
>>>>> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
>>>>> +   "gpio9" is on DS90Ux940 only)
>>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
>>>>> +  "gpio7", "gpio8", "gpio9"
>>>>> +
>>>>> +function: "gpio-remote"
>>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
>>>>> +
>>>>> +function: "pass" (DS90Ux940 specific only)
>>>>> + - pins: "gpio0", "gpio3"
>>>>
>>>> What do those functions mean ?
>>>
>>> "gpio" function should be already familiar to you.
>>
>> I assume this function is only available for the local device, not the 
>> remote 
>> one ?
>>
>>> "gpio-remote" function is the pin function for a GPIO line bridging.
>>>
>>> "pass" function sets a pin to a status pin function for detecting
>>> display timing issues, namely DE or Vsync length value mismatch.
>>
>> All this is not clear at all from the proposed DT bindings, it should be 
>> properly documented.
> 
> It's not clear to me as well. The "gpio-remote" can mean two different
> things (at least in the camera serdes TI chips):
> 
>  - a GPIO input on the the *local* chip, replicated as an output on the
>*remote* chip
>  - a GPIO input on the the *remote* chip, replicated as an output on the
>*local* chip
> 
> How to you differentiate them in DT?
> 

"gpio-remote" function is directly translated into "GPIOx Remote Enable"
bit setting, the documentation says:

1) Deserializer IC pin configuration:

Enable GPIO control from remote Serializer. The GPIO pin will
be an output, and the value is received from the remote Serializer.

2) Serializer IC pin configuration:

Enable GPIO control from remote Deserializer. The GPIO pin will
be an output, and the value is received from the remote Deserializer.

So, it is always an output signal, the line signal is "bridged" (repeated)
as a corresponding line signal on a remote IC, note that there is no
difference between serializer and deserializer ICs.

> The "pass" function is also not clear. A comprehensive example would
> help a lot.

As this devicetree documentation says, the "pass" pin function is specific
for DS90Ux940 deserializer, I would suggest to check its datasheet for
getting a comprehensive answer, but I've already copy-pasted information
from the datasheet into my previous answer to Laurent.

The reason why the "pass" pin function is listed is quite simple, the
pin function interferes other pin functions, see DS90Ux940 GPIO0 and
GPIO3 controls, I hope I've managed to describe it properly by
DS90UX940_GPIO(0, ...) and DS90UX940_GPIO(3, ...) pin descriptions in
the pinctrl driver.

--
Best wishes,
Vladimir

Re: [PATCH 3/7] dt-bindings: pinctrl: ds90ux9xx: add description of TI DS90Ux9xx pinmux

[Vladimir Zapolskiy]

Hi Luca,

On 10/30/2018 06:44 PM, Luca Ceresoli wrote:
> Hi Vladimir,
> 
> On 16/10/18 14:48, Laurent Pinchart wrote:
>> Hi Vladimir,
>>
>> On Saturday, 13 October 2018 16:47:48 EEST Vladimir Zapolskiy wrote:
>>> On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
>>>> On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
>>>>> From: Vladimir Zapolskiy 
>>>>>
>>>>> TI DS90Ux9xx de-/serializers have a capability to multiplex pin
>>>>> functions, in particular a pin may have selectable functions of GPIO,
>>>>> GPIO line transmitter, one of I2S lines, one of RGB24 video signal lines
>>>>> and so on.
>>>>>
>>>>> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
>>>>> controllers.
>>>>>
>>>>> Signed-off-by: Vladimir Zapolskiy 
> [...]
>>>>> +Available pins, groups and functions (reference to device datasheets):
>>>>> +
>>>>> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
>>>>> +   "gpio9" is on DS90Ux940 only)
>>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
>>>>> +  "gpio7", "gpio8", "gpio9"
>>>>> +
>>>>> +function: "gpio-remote"
>>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
>>>>> +
>>>>> +function: "pass" (DS90Ux940 specific only)
>>>>> + - pins: "gpio0", "gpio3"
>>>>
>>>> What do those functions mean ?
>>>
>>> "gpio" function should be already familiar to you.
>>
>> I assume this function is only available for the local device, not the 
>> remote 
>> one ?
>>
>>> "gpio-remote" function is the pin function for a GPIO line bridging.
>>>
>>> "pass" function sets a pin to a status pin function for detecting
>>> display timing issues, namely DE or Vsync length value mismatch.
>>
>> All this is not clear at all from the proposed DT bindings, it should be 
>> properly documented.
> 
> It's not clear to me as well. The "gpio-remote" can mean two different
> things (at least in the camera serdes TI chips):
> 
>  - a GPIO input on the the *local* chip, replicated as an output on the
>*remote* chip
>  - a GPIO input on the the *remote* chip, replicated as an output on the
>*local* chip
> 
> How to you differentiate them in DT?
> 

"gpio-remote" function is directly translated into "GPIOx Remote Enable"
bit setting, the documentation says:

1) Deserializer IC pin configuration:

Enable GPIO control from remote Serializer. The GPIO pin will
be an output, and the value is received from the remote Serializer.

2) Serializer IC pin configuration:

Enable GPIO control from remote Deserializer. The GPIO pin will
be an output, and the value is received from the remote Deserializer.

So, it is always an output signal, the line signal is "bridged" (repeated)
as a corresponding line signal on a remote IC, note that there is no
difference between serializer and deserializer ICs.

> The "pass" function is also not clear. A comprehensive example would
> help a lot.

As this devicetree documentation says, the "pass" pin function is specific
for DS90Ux940 deserializer, I would suggest to check its datasheet for
getting a comprehensive answer, but I've already copy-pasted information
from the datasheet into my previous answer to Laurent.

The reason why the "pass" pin function is listed is quite simple, the
pin function interferes other pin functions, see DS90Ux940 GPIO0 and
GPIO3 controls, I hope I've managed to describe it properly by
DS90UX940_GPIO(0, ...) and DS90UX940_GPIO(3, ...) pin descriptions in
the pinctrl driver.

--
Best wishes,
Vladimir

Re: [PATCH 4/7] mfd: ds90ux9xx: add TI DS90Ux9xx de-/serializer MFD driver

[Vladimir Zapolskiy]

Hi Laurent,

On 10/12/2018 02:59 PM, Vladimir Zapolskiy wrote:
> Hello Laurent.
> 
> On 10/12/2018 04:01 PM, Laurent Pinchart wrote:
>> Hello Vladimir,
>>

...

>> then move to the driver side. In that area I would like to have a full 
>> example 
>> of a system using these chips, as the "initial support" is too limited for a 
>> proper review. This won't come as a surprise, but I will expect the OF graph 
>> bindings to be used to model data connections.
>>
> 
> The leverage of "the initial support" to "the complete support" requires:
> * audio bridge cell driver -- trivial, just one mute/unmute control,
> * interrupt controller cell driver -- trivial, but for sake of perfection
>   it requires some minimal changes in drivers/base/regmap/regmap-irq.c
> * DS90Ux940 MIPI CSI-2 -- non-trivial one, but we have it ready, I just
>   don't want to add it to the pile at the moment, otherwise we'll continue
>   discussing cameras, and I'd like to postpone it :)
> 
> No more than that is needed to get absolutely complete support of 5 claimed
> DS90UB9xx ICs, really. 5 other DS90UH9xx will require HDCP support in 
> addition.
> 
> I'll try to roll out an example of DTS snippet soon.

Below I share an example of the serializer and deserializer equipped boards
described in DT.

The example naturally describes *two* simplistic boards in device tree
representation -- main board with an application SoC (ordinary i.MX6*)
and panel display module board. For demonstation I select a simple
FPD-Link III connection between two boards, note that significantly
more advanced configurations are also supported by the published
drivers, for example deliberately I skip audio bridging functionality.

The main board features:
* TI DS90UB927Q serializer (LVDS input) at 0xc, connected to SoC over I2C2,
  SoC GPIO5[10] signal is connected to the IC PDB pin,
* a status LED connected to DS90UB927Q GPIO2, it shall turn on,
  if FPD-Link III connection is established,
* TI DS90UB928Q GPIO0 line signal is pulled-up,
* TI DS90UB927Q GPIO3 line serves as generic GPIO, it is supposed to be
  controlled from userspace,
* TI DS90UB927Q INTB line is connected to SoC GPIO5[4], the line serves
  as an interrupt line routed from a touchscreen controller on a panel
  display module.

The panel display module board features:
* TI DS90UB928Q deserializer (LVDS output), *mapped* to have 0x3b address,
* AUO C070EAT01 panel,
* I2C EEPROM at 0x50, *mapped* to have 0x52 address on SoC I2C bus,
* Atmel MaxTouch touchscreen controller at 0x4b, *mapped* to have 0x60
  address on SoC I2C bus, power-up control signal is connected to DS90UB928Q 
GPIO4,
* a status LED connected to DS90UB928Q GPIO0, its on/off status shall
  repeat a user-defined status of DS90UB927Q GPIO0 on the main board,
* TI DS90UB928Q GPIO1 controls panel backlight, bridges DS90UB927Q
  GPIO1 signal level, which in turn is connected to a SoC controlled GPIO,
* TI DS90UB928Q GPIO2 line signal is pulled-up,
* TI DS90UB928Q GPIO3 line serves as generic GPIOs, it is supposed to be
  controlled from userspace.

All OF hard-coded controls like pinmuxing, I2C bridging of a remote
deserializer and I2C devices behind it, GPIO line state setting and so
forth must be applied with no interaction from a user -- and it just
works with the current / published versions of the drivers, in other
words a panel display module as a whole is truly hot-pluggable over
FPD-Link III connection.

The example is quite close to ones found in reality, if we put aside
production main boards from the real world, *the panel display modules*
or sensor modules (in case of a reverted serializer to deserializer link
connection to SoC) are even more complex, they host FPGAs, all kinds of
sensors, RF tuners, audio sources and sinks, and loads of other
incredible and fascinating stuff.

The published drivers allow to support very intricate and fine grained
control of "remote" PCBs, and reducing the complexity to "just a media
device" level could be done only if various IC functions are excluded
from the consideration. Here my purpose is to demonstrate that
* pinmux and GPIO controller functions are crucial and non-replaceable,
* I2C bridge function modeled as another cell device actually does not
  fit into the existing I2C host/mux device driver models, and it is
  a completely new abstraction with custom device tree properties,
* video bridge is absolutely transparent, thus trivial, and is modeled
  as another cell device, if needed it would be possible to write a
  DRM driver at no cost,
* reusing OF graph model fits naturally, bus vs. link discussion can
  be started separately, note that LVDS (a.k.a FPD-Link) is formally
  an electric bus, so please define the difference,
* to sum up I see no real objections to the given model of IC series
  support in Linux as an MFD parent dev

Re: [PATCH 4/7] mfd: ds90ux9xx: add TI DS90Ux9xx de-/serializer MFD driver

[Vladimir Zapolskiy]

Hi Laurent,

On 10/12/2018 02:59 PM, Vladimir Zapolskiy wrote:
> Hello Laurent.
> 
> On 10/12/2018 04:01 PM, Laurent Pinchart wrote:
>> Hello Vladimir,
>>

...

>> then move to the driver side. In that area I would like to have a full 
>> example 
>> of a system using these chips, as the "initial support" is too limited for a 
>> proper review. This won't come as a surprise, but I will expect the OF graph 
>> bindings to be used to model data connections.
>>
> 
> The leverage of "the initial support" to "the complete support" requires:
> * audio bridge cell driver -- trivial, just one mute/unmute control,
> * interrupt controller cell driver -- trivial, but for sake of perfection
>   it requires some minimal changes in drivers/base/regmap/regmap-irq.c
> * DS90Ux940 MIPI CSI-2 -- non-trivial one, but we have it ready, I just
>   don't want to add it to the pile at the moment, otherwise we'll continue
>   discussing cameras, and I'd like to postpone it :)
> 
> No more than that is needed to get absolutely complete support of 5 claimed
> DS90UB9xx ICs, really. 5 other DS90UH9xx will require HDCP support in 
> addition.
> 
> I'll try to roll out an example of DTS snippet soon.

Below I share an example of the serializer and deserializer equipped boards
described in DT.

The example naturally describes *two* simplistic boards in device tree
representation -- main board with an application SoC (ordinary i.MX6*)
and panel display module board. For demonstation I select a simple
FPD-Link III connection between two boards, note that significantly
more advanced configurations are also supported by the published
drivers, for example deliberately I skip audio bridging functionality.

The main board features:
* TI DS90UB927Q serializer (LVDS input) at 0xc, connected to SoC over I2C2,
  SoC GPIO5[10] signal is connected to the IC PDB pin,
* a status LED connected to DS90UB927Q GPIO2, it shall turn on,
  if FPD-Link III connection is established,
* TI DS90UB928Q GPIO0 line signal is pulled-up,
* TI DS90UB927Q GPIO3 line serves as generic GPIO, it is supposed to be
  controlled from userspace,
* TI DS90UB927Q INTB line is connected to SoC GPIO5[4], the line serves
  as an interrupt line routed from a touchscreen controller on a panel
  display module.

The panel display module board features:
* TI DS90UB928Q deserializer (LVDS output), *mapped* to have 0x3b address,
* AUO C070EAT01 panel,
* I2C EEPROM at 0x50, *mapped* to have 0x52 address on SoC I2C bus,
* Atmel MaxTouch touchscreen controller at 0x4b, *mapped* to have 0x60
  address on SoC I2C bus, power-up control signal is connected to DS90UB928Q 
GPIO4,
* a status LED connected to DS90UB928Q GPIO0, its on/off status shall
  repeat a user-defined status of DS90UB927Q GPIO0 on the main board,
* TI DS90UB928Q GPIO1 controls panel backlight, bridges DS90UB927Q
  GPIO1 signal level, which in turn is connected to a SoC controlled GPIO,
* TI DS90UB928Q GPIO2 line signal is pulled-up,
* TI DS90UB928Q GPIO3 line serves as generic GPIOs, it is supposed to be
  controlled from userspace.

All OF hard-coded controls like pinmuxing, I2C bridging of a remote
deserializer and I2C devices behind it, GPIO line state setting and so
forth must be applied with no interaction from a user -- and it just
works with the current / published versions of the drivers, in other
words a panel display module as a whole is truly hot-pluggable over
FPD-Link III connection.

The example is quite close to ones found in reality, if we put aside
production main boards from the real world, *the panel display modules*
or sensor modules (in case of a reverted serializer to deserializer link
connection to SoC) are even more complex, they host FPGAs, all kinds of
sensors, RF tuners, audio sources and sinks, and loads of other
incredible and fascinating stuff.

The published drivers allow to support very intricate and fine grained
control of "remote" PCBs, and reducing the complexity to "just a media
device" level could be done only if various IC functions are excluded
from the consideration. Here my purpose is to demonstrate that
* pinmux and GPIO controller functions are crucial and non-replaceable,
* I2C bridge function modeled as another cell device actually does not
  fit into the existing I2C host/mux device driver models, and it is
  a completely new abstraction with custom device tree properties,
* video bridge is absolutely transparent, thus trivial, and is modeled
  as another cell device, if needed it would be possible to write a
  DRM driver at no cost,
* reusing OF graph model fits naturally, bus vs. link discussion can
  be started separately, note that LVDS (a.k.a FPD-Link) is formally
  an electric bus, so please define the difference,
* to sum up I see no real objections to the given model of IC series
  support in Linux as an MFD parent dev

Re: [PATCH 3/7] dt-bindings: pinctrl: ds90ux9xx: add description of TI DS90Ux9xx pinmux

[Vladimir Zapolskiy]

Hi Laurent,

thank you for review, please find my comments below.

On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
> Hi Vladimir,
> 
> Thank you for the patch.
> 
> On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
>> From: Vladimir Zapolskiy 
>>
>> TI DS90Ux9xx de-/serializers have a capability to multiplex pin functions,
>> in particular a pin may have selectable functions of GPIO, GPIO line
>> transmitter, one of I2S lines, one of RGB24 video signal lines and so on.
>>
>> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
>> controllers.
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> ---
>>  .../bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt | 83 +++
>>  1 file changed, 83 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>> b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt new
>> file mode 100644
>> index ..fbfa1a3cdf9f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>> @@ -0,0 +1,83 @@
>> +TI DS90Ux9xx de-/serializer pinmux and GPIO subcontroller
>> +
>> +Required properties:
>> +- compatible: Must contain a generic "ti,ds90ux9xx-pinctrl" value and
>> +may contain one more specific value from the list:
>> +"ti,ds90ux925-pinctrl",
>> +"ti,ds90ux926-pinctrl",
>> +"ti,ds90ux927-pinctrl",
>> +"ti,ds90ux928-pinctrl",
>> +"ti,ds90ux940-pinctrl".
> 
> No need for a subnode, you can mark the main DT node with gpio-controller 
> directly.

If the IC is seen as an MFD, and you guess I highly prefer it and I object
the "overkill" argument, then the subnode is requred.

Also the more complicated part of the subcontroller and its device driver
is to provide pinmuxing function to consumers rather than to allow GPIO
line configuration.

The pinctrl/GPIO driver can not be alloyed with the base driver's code
to sustain maintainability, so it will reside in drivers/pinctrl as
a separate cell driver, and by the way that is the reason why it earns
its own very non-trivial DT binding description documentation.

>> +- gpio-controller: Marks the device node as a GPIO controller.
>> +
>> +- #gpio-cells: Must be set to 2,
>> +- the first cell is the GPIO offset number within the controller,
>> +- the second cell is used to specify the GPIO line polarity.
>> +
>> +- gpio-ranges: Mapping to pin controller pins (as described in
>> +Documentation/devicetree/bindings/gpio/gpio.txt)
>> +
>> +Optional properties:
>> +- ti,video-depth-18bit: Sets video bridge pins to RGB 18-bit mode.
> 
> Please use standard properties to configure bus width. There is one defined 
> in 
> Documentation/devicetree/bindings/media/video-interfaces.txt.

Here it is not a bus width description or property, but rather it is a custom
pinmux control.

It could make sense to reduce the scope of the property to "parallel" pin
function only though, like in

ds90ux927_0_pins: pinmux {
parallel {
groups = "parallel";
function = "parallel";
ti,video-depth-18bit;
};
};

Alternatively the removal of the property would be almost loseless, it is
needed just in one very specific case, please reference to the driver code
for details, there you'll find a comment in ds90ux9xx_parse_dt_properties()
function.

>> +Available pins, groups and functions (reference to device datasheets):
>> +
>> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
>> +  "gpio9" is on DS90Ux940 only)
>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
>> + "gpio7", "gpio8", "gpio9"
>> +
>> +function: "gpio-remote"
>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
>> +
>> +function: "pass" (DS90Ux940 specific only)
>> + - pins: "gpio0", "gpio3"
> 
> What do those functions mean ?
> 

"gpio" function should be already familiar to you.

"gpio-remote" function is the pin function for a GPIO line bridging.

"pass" function sets a pin to a status pin function for detecting
display timing issues, namely DE or Vsync length 

Re: [PATCH 3/7] dt-bindings: pinctrl: ds90ux9xx: add description of TI DS90Ux9xx pinmux

[Vladimir Zapolskiy]

Hi Laurent,

thank you for review, please find my comments below.

On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
> Hi Vladimir,
> 
> Thank you for the patch.
> 
> On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
>> From: Vladimir Zapolskiy 
>>
>> TI DS90Ux9xx de-/serializers have a capability to multiplex pin functions,
>> in particular a pin may have selectable functions of GPIO, GPIO line
>> transmitter, one of I2S lines, one of RGB24 video signal lines and so on.
>>
>> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
>> controllers.
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> ---
>>  .../bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt | 83 +++
>>  1 file changed, 83 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>> b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt new
>> file mode 100644
>> index ..fbfa1a3cdf9f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>> @@ -0,0 +1,83 @@
>> +TI DS90Ux9xx de-/serializer pinmux and GPIO subcontroller
>> +
>> +Required properties:
>> +- compatible: Must contain a generic "ti,ds90ux9xx-pinctrl" value and
>> +may contain one more specific value from the list:
>> +"ti,ds90ux925-pinctrl",
>> +"ti,ds90ux926-pinctrl",
>> +"ti,ds90ux927-pinctrl",
>> +"ti,ds90ux928-pinctrl",
>> +"ti,ds90ux940-pinctrl".
> 
> No need for a subnode, you can mark the main DT node with gpio-controller 
> directly.

If the IC is seen as an MFD, and you guess I highly prefer it and I object
the "overkill" argument, then the subnode is requred.

Also the more complicated part of the subcontroller and its device driver
is to provide pinmuxing function to consumers rather than to allow GPIO
line configuration.

The pinctrl/GPIO driver can not be alloyed with the base driver's code
to sustain maintainability, so it will reside in drivers/pinctrl as
a separate cell driver, and by the way that is the reason why it earns
its own very non-trivial DT binding description documentation.

>> +- gpio-controller: Marks the device node as a GPIO controller.
>> +
>> +- #gpio-cells: Must be set to 2,
>> +- the first cell is the GPIO offset number within the controller,
>> +- the second cell is used to specify the GPIO line polarity.
>> +
>> +- gpio-ranges: Mapping to pin controller pins (as described in
>> +Documentation/devicetree/bindings/gpio/gpio.txt)
>> +
>> +Optional properties:
>> +- ti,video-depth-18bit: Sets video bridge pins to RGB 18-bit mode.
> 
> Please use standard properties to configure bus width. There is one defined 
> in 
> Documentation/devicetree/bindings/media/video-interfaces.txt.

Here it is not a bus width description or property, but rather it is a custom
pinmux control.

It could make sense to reduce the scope of the property to "parallel" pin
function only though, like in

ds90ux927_0_pins: pinmux {
parallel {
groups = "parallel";
function = "parallel";
ti,video-depth-18bit;
};
};

Alternatively the removal of the property would be almost loseless, it is
needed just in one very specific case, please reference to the driver code
for details, there you'll find a comment in ds90ux9xx_parse_dt_properties()
function.

>> +Available pins, groups and functions (reference to device datasheets):
>> +
>> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
>> +  "gpio9" is on DS90Ux940 only)
>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
>> + "gpio7", "gpio8", "gpio9"
>> +
>> +function: "gpio-remote"
>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
>> +
>> +function: "pass" (DS90Ux940 specific only)
>> + - pins: "gpio0", "gpio3"
> 
> What do those functions mean ?
> 

"gpio" function should be already familiar to you.

"gpio-remote" function is the pin function for a GPIO line bridging.

"pass" function sets a pin to a status pin function for detecting
display timing issues, namely DE or Vsync length 

Re: [PATCH 4/7] mfd: ds90ux9xx: add TI DS90Ux9xx de-/serializer MFD driver

[Vladimir Zapolskiy]

On 10/12/2018 02:43 PM, Lee Jones wrote:
> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote:
> 
>> On 10/12/2018 11:39 AM, Lee Jones wrote:
>>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote:
>>>> On 10/12/2018 09:03 AM, Lee Jones wrote:
>>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote:
>>>>>
>>>>>> From: Vladimir Zapolskiy 
>>>>>>
>>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers,
>>>>>> support of subdevice controllers is done in separate drivers, because
>>>>>> not all IC functionality may be needed in particular situations, and
>>>>>> this can be fine grained controlled in device tree.
>>>>>>
>>>>>> The development of the driver was a collaborative work, the
>>>>>> contribution done by Balasubramani Vivekanandan includes:
>>>>>> * original implementation of the driver based on a reference driver,
>>>>>> * regmap powered interrupt controller support on serializers,
>>>>>> * support of implicitly or improperly specified in device tree ICs,
>>>>>> * support of device properties and attributes: backward compatible
>>>>>>   mode, low frequency operation mode, spread spectrum clock generator.
>>>>>>
>>>>>> Contribution by Steve Longerbeam:
>>>>>> * added ds90ux9xx_read_indirect() function,
>>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(),
>>>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver,
>>>>>> * added fpd_link_show device attribute.
>>>>>>
>>>>>> Sandeep Jain added support of pixel clock edge configuration.
>>>>>>
>>>>>> Signed-off-by: Vladimir Zapolskiy 
>>>>>> ---
>>>>>>  drivers/mfd/Kconfig   |  14 +
>>>>>>  drivers/mfd/Makefile  |   1 +
>>>>>>  drivers/mfd/ds90ux9xx-core.c  | 879 ++
>>>>>>  include/linux/mfd/ds90ux9xx.h |  42 ++
>>>>>>  4 files changed, 936 insertions(+)
>>>>>>  create mode 100644 drivers/mfd/ds90ux9xx-core.c
>>>>>>  create mode 100644 include/linux/mfd/ds90ux9xx.h
>>>>>>
>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>> index 8c5dfdce4326..a969fa123f64 100644
>>>>>> --- a/drivers/mfd/Kconfig
>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP
>>>>>>boards.  MSP430 firmware manages resets and power sequencing,
>>>>>>inputs from buttons and the IR remote, LEDs, an RTC, and more.
>>>>>>  
>>>>>> +config MFD_DS90UX9XX
>>>>>> +tristate "TI DS90Ux9xx FPD-Link de-/serializer driver"
>>>>>> +depends on I2C && OF
>>>>>> +select MFD_CORE
>>>>>> +select REGMAP_I2C
>>>>>> +help
>>>>>> +  Say yes here to enable support for TI DS90UX9XX 
>>>>>> de-/serializer ICs.
>>>>>> +
>>>>>> +  This driver provides basic support for setting up the 
>>>>>> de-/serializer
>>>>>> +  chips. Additional functionalities like connection handling to
>>>>>> +  remote de-/serializers, I2C bridging, pin multiplexing, GPIO
>>>>>> +  controller and so on are provided by separate drivers and 
>>>>>> should
>>>>>> +  enabled individually.
>>>>>
>>>>> This is not an MFD driver.
>>>>
>>>> Why do you think so? The representation of the ICs into device tree format
>>>> of hardware description shows that this is a truly MFD driver with multiple
>>>> IP subcomponents naturally mapped into MFD cells.
>>>
>>> This driver does too much real work ('stuff') to be an MFD driver.
>>> MFD drivers should not need to care of; links, gates, modes, pixels,
>>> frequencies maps or properties.  Nor should they contain elaborate
>>> sysfs structures to control the aforementioned 'stuff'.
>>>
>>
>> What is the reason why device drivers for sort of multimedia ICs like
>> WL1273, WM8994 and other numerous codecs are found

Re: [PATCH 4/7] mfd: ds90ux9xx: add TI DS90Ux9xx de-/serializer MFD driver

[Vladimir Zapolskiy]

On 10/12/2018 02:43 PM, Lee Jones wrote:
> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote:
> 
>> On 10/12/2018 11:39 AM, Lee Jones wrote:
>>> On Fri, 12 Oct 2018, Vladimir Zapolskiy wrote:
>>>> On 10/12/2018 09:03 AM, Lee Jones wrote:
>>>>> On Tue, 09 Oct 2018, Vladimir Zapolskiy wrote:
>>>>>
>>>>>> From: Vladimir Zapolskiy 
>>>>>>
>>>>>> The change adds I2C device driver for TI DS90Ux9xx de-/serializers,
>>>>>> support of subdevice controllers is done in separate drivers, because
>>>>>> not all IC functionality may be needed in particular situations, and
>>>>>> this can be fine grained controlled in device tree.
>>>>>>
>>>>>> The development of the driver was a collaborative work, the
>>>>>> contribution done by Balasubramani Vivekanandan includes:
>>>>>> * original implementation of the driver based on a reference driver,
>>>>>> * regmap powered interrupt controller support on serializers,
>>>>>> * support of implicitly or improperly specified in device tree ICs,
>>>>>> * support of device properties and attributes: backward compatible
>>>>>>   mode, low frequency operation mode, spread spectrum clock generator.
>>>>>>
>>>>>> Contribution by Steve Longerbeam:
>>>>>> * added ds90ux9xx_read_indirect() function,
>>>>>> * moved number of links property and added ds90ux9xx_num_fpd_links(),
>>>>>> * moved and updated ds90ux9xx_get_link_status() function to core driver,
>>>>>> * added fpd_link_show device attribute.
>>>>>>
>>>>>> Sandeep Jain added support of pixel clock edge configuration.
>>>>>>
>>>>>> Signed-off-by: Vladimir Zapolskiy 
>>>>>> ---
>>>>>>  drivers/mfd/Kconfig   |  14 +
>>>>>>  drivers/mfd/Makefile  |   1 +
>>>>>>  drivers/mfd/ds90ux9xx-core.c  | 879 ++
>>>>>>  include/linux/mfd/ds90ux9xx.h |  42 ++
>>>>>>  4 files changed, 936 insertions(+)
>>>>>>  create mode 100644 drivers/mfd/ds90ux9xx-core.c
>>>>>>  create mode 100644 include/linux/mfd/ds90ux9xx.h
>>>>>>
>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>> index 8c5dfdce4326..a969fa123f64 100644
>>>>>> --- a/drivers/mfd/Kconfig
>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>> @@ -1280,6 +1280,20 @@ config MFD_DM355EVM_MSP
>>>>>>boards.  MSP430 firmware manages resets and power sequencing,
>>>>>>inputs from buttons and the IR remote, LEDs, an RTC, and more.
>>>>>>  
>>>>>> +config MFD_DS90UX9XX
>>>>>> +tristate "TI DS90Ux9xx FPD-Link de-/serializer driver"
>>>>>> +depends on I2C && OF
>>>>>> +select MFD_CORE
>>>>>> +select REGMAP_I2C
>>>>>> +help
>>>>>> +  Say yes here to enable support for TI DS90UX9XX 
>>>>>> de-/serializer ICs.
>>>>>> +
>>>>>> +  This driver provides basic support for setting up the 
>>>>>> de-/serializer
>>>>>> +  chips. Additional functionalities like connection handling to
>>>>>> +  remote de-/serializers, I2C bridging, pin multiplexing, GPIO
>>>>>> +  controller and so on are provided by separate drivers and 
>>>>>> should
>>>>>> +  enabled individually.
>>>>>
>>>>> This is not an MFD driver.
>>>>
>>>> Why do you think so? The representation of the ICs into device tree format
>>>> of hardware description shows that this is a truly MFD driver with multiple
>>>> IP subcomponents naturally mapped into MFD cells.
>>>
>>> This driver does too much real work ('stuff') to be an MFD driver.
>>> MFD drivers should not need to care of; links, gates, modes, pixels,
>>> frequencies maps or properties.  Nor should they contain elaborate
>>> sysfs structures to control the aforementioned 'stuff'.
>>>
>>
>> What is the reason why device drivers for sort of multimedia ICs like
>> WL1273, WM8994 and other numerous codecs are found

[PATCH] MAINTAINERS: Assign myself as a maintainer of ARM/LPC18XX architecture

[Vladimir Zapolskiy]

To all appearance Joachim Eastwood abandoned the maintenance of NXP
LPC18xx/LPC43xx archtecture about two years ago, and for me it would be
possible to continue the support, fortunately the quality of platform
drivers written by Joachim is exceptionally high.

The change is based on https://lkml.org/lkml/2018/9/24/1398 discussion.

At the same time two redundant explicit driver file paths are dropped
from the list, clk-lpc18xx* drivers are covered by "lpc18xx" search
pattern and timer-lpc32xx driver is covered by "lpc32xx" pattern and it
goes into ARM/LPC32XX entry, which is also under my wing, in other words
LPC18xx/LPC43xx clocksource and CCF drivers will remain maintained.

Signed-off-by: Vladimir Zapolskiy 
Cc: Joachim Eastwood 
---
 MAINTAINERS | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 57e78e400fd3..15c4abb7bda6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1653,12 +1653,10 @@ L:  linux-arm-ker...@lists.infradead.org (moderated 
for non-subscribers)
 S: Maintained
 
 ARM/LPC18XX ARCHITECTURE
-M: Joachim Eastwood 
+M:     Vladimir Zapolskiy 
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 S: Maintained
 F: arch/arm/boot/dts/lpc43*
-F: drivers/clk/nxp/clk-lpc18xx*
-F: drivers/clocksource/timer-lpc32xx.c
 F: drivers/i2c/busses/i2c-lpc2k.c
 F: drivers/memory/pl172.c
 F: drivers/mtd/spi-nor/nxp-spifi.c
-- 
2.17.1

[PATCH] MAINTAINERS: Assign myself as a maintainer of ARM/LPC18XX architecture

[Vladimir Zapolskiy]

To all appearance Joachim Eastwood abandoned the maintenance of NXP
LPC18xx/LPC43xx archtecture about two years ago, and for me it would be
possible to continue the support, fortunately the quality of platform
drivers written by Joachim is exceptionally high.

The change is based on https://lkml.org/lkml/2018/9/24/1398 discussion.

At the same time two redundant explicit driver file paths are dropped
from the list, clk-lpc18xx* drivers are covered by "lpc18xx" search
pattern and timer-lpc32xx driver is covered by "lpc32xx" pattern and it
goes into ARM/LPC32XX entry, which is also under my wing, in other words
LPC18xx/LPC43xx clocksource and CCF drivers will remain maintained.

Signed-off-by: Vladimir Zapolskiy 
Cc: Joachim Eastwood 
---
 MAINTAINERS | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 57e78e400fd3..15c4abb7bda6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1653,12 +1653,10 @@ L:  linux-arm-ker...@lists.infradead.org (moderated 
for non-subscribers)
 S: Maintained
 
 ARM/LPC18XX ARCHITECTURE
-M: Joachim Eastwood 
+M:     Vladimir Zapolskiy 
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 S: Maintained
 F: arch/arm/boot/dts/lpc43*
-F: drivers/clk/nxp/clk-lpc18xx*
-F: drivers/clocksource/timer-lpc32xx.c
 F: drivers/i2c/busses/i2c-lpc2k.c
 F: drivers/memory/pl172.c
 F: drivers/mtd/spi-nor/nxp-spifi.c
-- 
2.17.1

Re: [PATCH] MAINTAINERS: Set ARM/LPC18XX architecture orphan

[Vladimir Zapolskiy]

Hi Arnd, Joachim,
 
On 09/24/2018 04:19 PM, Arnd Bergmann wrote:
> On Mon, Sep 24, 2018 at 6:43 AM Daniel Lezcano
>  wrote:
>>
>> While sending patches around, Joachim Eastwood was Cc'ed but I got an error
>> its mailbox was full and the mail can not be delivered which makes me think
>> there is no body at the other end of the line.
>>
>> After doing some statistics, it appears the latest commit as author/commiter
>> is:
>>
>> commit b16ebc017ebf52a81e5e88743ccd68fc25e99ba9
>> Author: Joachim Eastwood 
>> Date:   Mon Oct 31 14:45:17 2016 +
>>
>> Remove the maintainer entry and set the component as orphan.
>>
>> Signed-off-by: Daniel Lezcano 
>> Cc: Arnd Bergmann 
> 
> Acked-by: Arnd Bergmann 
> 
> I also checked the github account and any other mails I could
> find from Joachim, which stopped around the same time.
> 
> I've added the lpc32xx maintainers to Cc, as the two platforms are
> related. Maybe they are interested in picking up support for this
> as well, and/or merge the two directories into one (there is only
> one trivial file in mach-lpc18xx, the rest are drivers).
> 

I'm interested in maintenance of lpc18xx/lpc43xx, I have one board
powered by LPC4357, so it would make the maintenance easier for me,
also multiple controllers are equal on lpc18xx/lpc43xx and lpc32xx,
and thus familiar to me.

And I greatly appreciate Joachim's contribution, all platform drivers
written by Joachim are of perfect quality.

I'll send a patch to MAINTAINERS later on.

--
Best wishes,
Vladimir

Re: [PATCH] MAINTAINERS: Set ARM/LPC18XX architecture orphan

[Vladimir Zapolskiy]

Hi Arnd, Joachim,
 
On 09/24/2018 04:19 PM, Arnd Bergmann wrote:
> On Mon, Sep 24, 2018 at 6:43 AM Daniel Lezcano
>  wrote:
>>
>> While sending patches around, Joachim Eastwood was Cc'ed but I got an error
>> its mailbox was full and the mail can not be delivered which makes me think
>> there is no body at the other end of the line.
>>
>> After doing some statistics, it appears the latest commit as author/commiter
>> is:
>>
>> commit b16ebc017ebf52a81e5e88743ccd68fc25e99ba9
>> Author: Joachim Eastwood 
>> Date:   Mon Oct 31 14:45:17 2016 +
>>
>> Remove the maintainer entry and set the component as orphan.
>>
>> Signed-off-by: Daniel Lezcano 
>> Cc: Arnd Bergmann 
> 
> Acked-by: Arnd Bergmann 
> 
> I also checked the github account and any other mails I could
> find from Joachim, which stopped around the same time.
> 
> I've added the lpc32xx maintainers to Cc, as the two platforms are
> related. Maybe they are interested in picking up support for this
> as well, and/or merge the two directories into one (there is only
> one trivial file in mach-lpc18xx, the rest are drivers).
> 

I'm interested in maintenance of lpc18xx/lpc43xx, I have one board
powered by LPC4357, so it would make the maintenance easier for me,
also multiple controllers are equal on lpc18xx/lpc43xx and lpc32xx,
and thus familiar to me.

And I greatly appreciate Joachim's contribution, all platform drivers
written by Joachim are of perfect quality.

I'll send a patch to MAINTAINERS later on.

--
Best wishes,
Vladimir

Re: [PATCH] mmc: mxcmmc: replace spin_lock_irqsave with spin_lock in ISR

[Vladimir Zapolskiy]

On 09/11/2018 05:47 PM, jun qian wrote:
> As you are already in ISR, it is unnecessary to call spin_lock_irqsave.
> 
> Signed-off-by: jun qian 

Reviewed-by: Vladimir Zapolskiy 

---
Best wishes,
Vladimir

Re: [PATCH] mmc: mxcmmc: replace spin_lock_irqsave with spin_lock in ISR

[Vladimir Zapolskiy]

On 09/11/2018 05:47 PM, jun qian wrote:
> As you are already in ISR, it is unnecessary to call spin_lock_irqsave.
> 
> Signed-off-by: jun qian 

Reviewed-by: Vladimir Zapolskiy 

---
Best wishes,
Vladimir

Re: [PATCH] pinctrl: lpc18xx: mark expected switch fall-throughs

[Vladimir Zapolskiy]

On 08/15/2018 08:10 PM, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Addresses-Coverity-ID: 1292308 ("Missing break in switch")
> Addresses-Coverity-ID: 1292309 ("Missing break in switch")
> Addresses-Coverity-ID: 1309546 ("Missing break in switch")
> Addresses-Coverity-ID: 1357369 ("Missing break in switch")
> Addresses-Coverity-ID: 1357389 ("Missing break in switch")
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/pinctrl/pinctrl-lpc18xx.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c 
> b/drivers/pinctrl/pinctrl-lpc18xx.c
> index 190f17e..a14bc5e 100644
> --- a/drivers/pinctrl/pinctrl-lpc18xx.c
> +++ b/drivers/pinctrl/pinctrl-lpc18xx.c
> @@ -844,8 +844,11 @@ static int lpc18xx_pconf_get_pin(struct pinctrl_dev 
> *pctldev, unsigned param,
>   *arg = (reg & LPC18XX_SCU_PIN_EHD_MASK) >> 
> LPC18XX_SCU_PIN_EHD_POS;
>   switch (*arg) {
>   case 3: *arg += 5;
> + /* fall through */
>   case 2: *arg += 5;
> + /* fall through */
>   case 1: *arg += 3;
> + /* fall through */
>   case 0: *arg += 4;
>   }
>   break;
> @@ -1060,8 +1063,11 @@ static int lpc18xx_pconf_set_pin(struct pinctrl_dev 
> *pctldev, unsigned param,
>  
>   switch (param_val) {
>   case 20: param_val -= 5;
> +  /* fall through */
>   case 14: param_val -= 5;
> +  /* fall through */
>   case  8: param_val -= 3;
> +  /* fall through */
>   case  4: param_val -= 4;
>break;
>       default:
> 

The code snippets are about a mind-blowing hyper-optimization, but I took
it as a chance to verify the correctness, and there are no issues found.

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] pinctrl: lpc18xx: mark expected switch fall-throughs

[Vladimir Zapolskiy]

On 08/15/2018 08:10 PM, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Addresses-Coverity-ID: 1292308 ("Missing break in switch")
> Addresses-Coverity-ID: 1292309 ("Missing break in switch")
> Addresses-Coverity-ID: 1309546 ("Missing break in switch")
> Addresses-Coverity-ID: 1357369 ("Missing break in switch")
> Addresses-Coverity-ID: 1357389 ("Missing break in switch")
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/pinctrl/pinctrl-lpc18xx.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c 
> b/drivers/pinctrl/pinctrl-lpc18xx.c
> index 190f17e..a14bc5e 100644
> --- a/drivers/pinctrl/pinctrl-lpc18xx.c
> +++ b/drivers/pinctrl/pinctrl-lpc18xx.c
> @@ -844,8 +844,11 @@ static int lpc18xx_pconf_get_pin(struct pinctrl_dev 
> *pctldev, unsigned param,
>   *arg = (reg & LPC18XX_SCU_PIN_EHD_MASK) >> 
> LPC18XX_SCU_PIN_EHD_POS;
>   switch (*arg) {
>   case 3: *arg += 5;
> + /* fall through */
>   case 2: *arg += 5;
> + /* fall through */
>   case 1: *arg += 3;
> + /* fall through */
>   case 0: *arg += 4;
>   }
>   break;
> @@ -1060,8 +1063,11 @@ static int lpc18xx_pconf_set_pin(struct pinctrl_dev 
> *pctldev, unsigned param,
>  
>   switch (param_val) {
>   case 20: param_val -= 5;
> +  /* fall through */
>   case 14: param_val -= 5;
> +  /* fall through */
>   case  8: param_val -= 3;
> +  /* fall through */
>   case  4: param_val -= 4;
>break;
>       default:
> 

The code snippets are about a mind-blowing hyper-optimization, but I took
it as a chance to verify the correctness, and there are no issues found.

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH] arm:pm: Use kmemdup to replace duplicating its implementation

[Vladimir Zapolskiy]

Hi zhong jiang,

On 08/10/2018 05:40 AM, zhong jiang wrote:
> kmemdup is better than kmalloc() + memcpy(), and we do not like
> open code. So just use kmemdup instead.
> 
> Signed-off-by: zhong jiang 
> ---
>  arch/arm/mach-lpc32xx/pm.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-lpc32xx/pm.c b/arch/arm/mach-lpc32xx/pm.c
> index 6247157..6eefff8 100644
> --- a/arch/arm/mach-lpc32xx/pm.c
> +++ b/arch/arm/mach-lpc32xx/pm.c
> @@ -86,7 +86,8 @@ static int lpc32xx_pm_enter(suspend_state_t state)
>   void *iram_swap_area;
>  
>   /* Allocate some space for temporary IRAM storage */
> - iram_swap_area = kmalloc(lpc32xx_sys_suspend_sz, GFP_KERNEL);
> + iram_swap_area = kmemdup((void *)TEMP_IRAM_AREA,
> + lpc32xx_sys_suspend_sz, GFP_KERNEL);

there is a minor coding style issue, which is reported by checkpatch:

CHECK: Alignment should match open parenthesis
#53: FILE: arch/arm/mach-lpc32xx/pm.c:90:

Other than that the change is valid, I'll fix the indentation issue
myself and apply the change for v4.19.

--
Best wishes,
Vladimir

>   if (!iram_swap_area) {
>   printk(KERN_ERR
>  "PM Suspend: cannot allocate memory to save portion "
> @@ -94,10 +95,6 @@ static int lpc32xx_pm_enter(suspend_state_t state)
>   return -ENOMEM;
>   }
>  
> - /* Backup a small area of IRAM used for the suspend code */
> - memcpy(iram_swap_area, (void *) TEMP_IRAM_AREA,
> - lpc32xx_sys_suspend_sz);
> -
>   /*
>* Copy code to suspend system into IRAM. The suspend code
>* needs to run from IRAM as DRAM may no longer be available
> 

Re: [PATCH] arm:pm: Use kmemdup to replace duplicating its implementation

[Vladimir Zapolskiy]

Hi zhong jiang,

On 08/10/2018 05:40 AM, zhong jiang wrote:
> kmemdup is better than kmalloc() + memcpy(), and we do not like
> open code. So just use kmemdup instead.
> 
> Signed-off-by: zhong jiang 
> ---
>  arch/arm/mach-lpc32xx/pm.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-lpc32xx/pm.c b/arch/arm/mach-lpc32xx/pm.c
> index 6247157..6eefff8 100644
> --- a/arch/arm/mach-lpc32xx/pm.c
> +++ b/arch/arm/mach-lpc32xx/pm.c
> @@ -86,7 +86,8 @@ static int lpc32xx_pm_enter(suspend_state_t state)
>   void *iram_swap_area;
>  
>   /* Allocate some space for temporary IRAM storage */
> - iram_swap_area = kmalloc(lpc32xx_sys_suspend_sz, GFP_KERNEL);
> + iram_swap_area = kmemdup((void *)TEMP_IRAM_AREA,
> + lpc32xx_sys_suspend_sz, GFP_KERNEL);

there is a minor coding style issue, which is reported by checkpatch:

CHECK: Alignment should match open parenthesis
#53: FILE: arch/arm/mach-lpc32xx/pm.c:90:

Other than that the change is valid, I'll fix the indentation issue
myself and apply the change for v4.19.

--
Best wishes,
Vladimir

>   if (!iram_swap_area) {
>   printk(KERN_ERR
>  "PM Suspend: cannot allocate memory to save portion "
> @@ -94,10 +95,6 @@ static int lpc32xx_pm_enter(suspend_state_t state)
>   return -ENOMEM;
>   }
>  
> - /* Backup a small area of IRAM used for the suspend code */
> - memcpy(iram_swap_area, (void *) TEMP_IRAM_AREA,
> - lpc32xx_sys_suspend_sz);
> -
>   /*
>* Copy code to suspend system into IRAM. The suspend code
>* needs to run from IRAM as DRAM may no longer be available
> 

Re: [PATCH] dt-bindings: remove 'interrupt-parent' from bindings

[Vladimir Zapolskiy]

Hi Rob,

On 07/24/2018 01:13 AM, Rob Herring wrote:
> 'interrupt-parent' is often documented as part of define bindings, but
> it is really outside the scope of a device binding. It's never required
> in a given node as it is often inherited from a parent node. Or it can
> be implicit if a parent node is an 'interrupt-controller' node. So
> remove it from all the binding files.
> 
> Cc: Mark Rutland 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Rob Herring 
> ---

[snip]

> diff --git 
> a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt 
> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt
> index 38211f344dc8..0bfb3ba55f4c 100644
> --- 
> a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt
> +++ 
> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt
> @@ -14,8 +14,6 @@ Required properties:
>Reset value is IRQ_TYPE_LEVEL_LOW.
>  
>  Optional properties:
> -- interrupt-parent: empty for MIC interrupt controller, link to parent
> -  MIC interrupt controller for SIC1 and SIC2
>  - interrupts: empty for MIC interrupt controller, cascaded MIC
>hardware interrupts for SIC1 and SIC2
>  

I would rather ask you to keep the property here, it is optional in sense that
its presence distinguishes MIC and SIC types of interrupt controllers.

--
Best wishes,
Vladimir

Re: [PATCH] dt-bindings: remove 'interrupt-parent' from bindings

[Vladimir Zapolskiy]

Hi Rob,

On 07/24/2018 01:13 AM, Rob Herring wrote:
> 'interrupt-parent' is often documented as part of define bindings, but
> it is really outside the scope of a device binding. It's never required
> in a given node as it is often inherited from a parent node. Or it can
> be implicit if a parent node is an 'interrupt-controller' node. So
> remove it from all the binding files.
> 
> Cc: Mark Rutland 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Rob Herring 
> ---

[snip]

> diff --git 
> a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt 
> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt
> index 38211f344dc8..0bfb3ba55f4c 100644
> --- 
> a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt
> +++ 
> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc3220-mic.txt
> @@ -14,8 +14,6 @@ Required properties:
>Reset value is IRQ_TYPE_LEVEL_LOW.
>  
>  Optional properties:
> -- interrupt-parent: empty for MIC interrupt controller, link to parent
> -  MIC interrupt controller for SIC1 and SIC2
>  - interrupts: empty for MIC interrupt controller, cascaded MIC
>hardware interrupts for SIC1 and SIC2
>  

I would rather ask you to keep the property here, it is optional in sense that
its presence distinguishes MIC and SIC types of interrupt controllers.

--
Best wishes,
Vladimir

[PATCH] block: remove blkdev_entry_to_request() macro

[Vladimir Zapolskiy]

Remove blkdev_entry_to_request() macro, which remained unused through
the observable history, also note that it repeats list_entry_rq() macro
verbatim.

Signed-off-by: Vladimir Zapolskiy 
---
 include/linux/blkdev.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 79226ca..eb101fd 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1437,8 +1437,6 @@ enum blk_default_limits {
BLK_SEG_BOUNDARY_MASK   = 0xUL,
 };
 
-#define blkdev_entry_to_request(entry) list_entry((entry), struct request, 
queuelist)
-
 static inline unsigned long queue_segment_boundary(struct request_queue *q)
 {
return q->limits.seg_boundary_mask;
-- 
2.10.2

[PATCH] block: remove blkdev_entry_to_request() macro

[Vladimir Zapolskiy]

Remove blkdev_entry_to_request() macro, which remained unused through
the observable history, also note that it repeats list_entry_rq() macro
verbatim.

Signed-off-by: Vladimir Zapolskiy 
---
 include/linux/blkdev.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 79226ca..eb101fd 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1437,8 +1437,6 @@ enum blk_default_limits {
BLK_SEG_BOUNDARY_MASK   = 0xUL,
 };
 
-#define blkdev_entry_to_request(entry) list_entry((entry), struct request, 
queuelist)
-
 static inline unsigned long queue_segment_boundary(struct request_queue *q)
 {
return q->limits.seg_boundary_mask;
-- 
2.10.2

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Vladimir Zapolskiy]

On 07/03/2018 02:47 PM, Johan Hovold wrote:
> On Tue, Jul 03, 2018 at 01:23:30PM +0300, Vladimir Zapolskiy wrote:
>> Hi Johan,
>>
>> On 07/03/2018 01:05 PM, Johan Hovold wrote:
>>> Make sure to enable the clock before registering regions and exporting
>>> partitions to user space at which point we must be prepared for I/O.
>>>
>>> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
>>> Cc: Vladimir Zapolskiy 
>>> Signed-off-by: Johan Hovold 
>>
>> thank you for the change, however please note that the identified commit
>> for the fix is incorrect one apparently.
>>
>> In my opinion the proper tag contents would be
>>
>> Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")
>>
>> I hope you agree to it, also I would suggest to swap the changes in
>> the series.
> 
> No, I think I used the right commit in the Fixes tag as that was the
> commit which moved the clock enable to after the memory-region
> registration (at which point the memory could potentially be accessed).

I was confused by the moved sram_reserve_regions() call, which was added
way later.

Allright, if it is assumed that gen_pool_get() interface requires only
a registered memory pool provider device, and it does, then there is
another kind of a problem, a SRAM/genpool consumer may not get access
to a valid region in SRAM before the latter is added to the SRAM pool
in sram_probe().

Instantly I don't know how to solve the issue above, it may require
a change to lib/genalloc.c to request a registration of genpool device
driver, but then such a change solves the problem identified by you
as well.

For your change as a proper (partial?) fix:

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Vladimir Zapolskiy]

On 07/03/2018 02:47 PM, Johan Hovold wrote:
> On Tue, Jul 03, 2018 at 01:23:30PM +0300, Vladimir Zapolskiy wrote:
>> Hi Johan,
>>
>> On 07/03/2018 01:05 PM, Johan Hovold wrote:
>>> Make sure to enable the clock before registering regions and exporting
>>> partitions to user space at which point we must be prepared for I/O.
>>>
>>> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
>>> Cc: Vladimir Zapolskiy 
>>> Signed-off-by: Johan Hovold 
>>
>> thank you for the change, however please note that the identified commit
>> for the fix is incorrect one apparently.
>>
>> In my opinion the proper tag contents would be
>>
>> Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")
>>
>> I hope you agree to it, also I would suggest to swap the changes in
>> the series.
> 
> No, I think I used the right commit in the Fixes tag as that was the
> commit which moved the clock enable to after the memory-region
> registration (at which point the memory could potentially be accessed).

I was confused by the moved sram_reserve_regions() call, which was added
way later.

Allright, if it is assumed that gen_pool_get() interface requires only
a registered memory pool provider device, and it does, then there is
another kind of a problem, a SRAM/genpool consumer may not get access
to a valid region in SRAM before the latter is added to the SRAM pool
in sram_probe().

Instantly I don't know how to solve the issue above, it may require
a change to lib/genalloc.c to request a registration of genpool device
driver, but then such a change solves the problem identified by you
as well.

For your change as a proper (partial?) fix:

Reviewed-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Vladimir Zapolskiy]

Hi Johan,

On 07/03/2018 01:05 PM, Johan Hovold wrote:
> Make sure to enable the clock before registering regions and exporting
> partitions to user space at which point we must be prepared for I/O.
> 
> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
> Cc: Vladimir Zapolskiy 
> Signed-off-by: Johan Hovold 

thank you for the change, however please note that the identified commit
for the fix is incorrect one apparently.

In my opinion the proper tag contents would be

Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")

I hope you agree to it, also I would suggest to swap the changes in
the series.

--
Best wishes,
Vladimir

Re: [PATCH 2/2] misc: sram: enable clock before registering regions

[Vladimir Zapolskiy]

Hi Johan,

On 07/03/2018 01:05 PM, Johan Hovold wrote:
> Make sure to enable the clock before registering regions and exporting
> partitions to user space at which point we must be prepared for I/O.
> 
> Fixes: ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")
> Cc: Vladimir Zapolskiy 
> Signed-off-by: Johan Hovold 

thank you for the change, however please note that the identified commit
for the fix is incorrect one apparently.

In my opinion the proper tag contents would be

Fixes: b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")

I hope you agree to it, also I would suggest to swap the changes in
the series.

--
Best wishes,
Vladimir

Re: [PATCH v6 4/5] clocksource: add driver for i.MX EPIT timer

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add driver for NXP's EPIT timer used in i.MX SoC.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 

Reviewed-by: Vladimir Zapolskiy 
Tested-by: Vladimir Zapolskiy 

I tested the driver on i.MX31 only, and I didn't find any problems.

Please fix the indentation issue found by Stefan in v7.

Regarding utilization of timer-of.c it can be postponed IMHO,
but it's up to clocksource maintainers and you to decide, and if
you do such an update for v7, then please don't add my tags,
I'll review and test it again.

--
With best wishes,
Vladimir

Re: [PATCH v6 4/5] clocksource: add driver for i.MX EPIT timer

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add driver for NXP's EPIT timer used in i.MX SoC.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 

Reviewed-by: Vladimir Zapolskiy 
Tested-by: Vladimir Zapolskiy 

I tested the driver on i.MX31 only, and I didn't find any problems.

Please fix the indentation issue found by Stefan in v7.

Regarding utilization of timer-of.c it can be postponed IMHO,
but it's up to clocksource maintainers and you to decide, and if
you do such an update for v7, then please don't add my tags,
I'll review and test it again.

--
With best wishes,
Vladimir

Re: [PATCH v6 5/5] ARM: dts: imx: add missing compatible and clock properties for EPIT

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add missing compatible and clock properties for EPIT node.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 

Reviewed-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir

Re: [PATCH v6 5/5] ARM: dts: imx: add missing compatible and clock properties for EPIT

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add missing compatible and clock properties for EPIT node.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 

Reviewed-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir

Re: [PATCH v6 0/5] Reintroduce i.MX EPIT Timer

[Vladimir Zapolskiy]

Hi Clément,

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> As suggested in the commit message we have added the device tree support,
> proper bindings and we moved the driver into the correct folder.
> 
> Moreover we made some changes like use of relaxed IO accesor,
> implement sched_clock, delay_timer and reduce the clockevents min_delta.
> 

I reviewed and tested the driver on i.MX31, as expected it works fine,
and I'll give my tags per a commit, please add them to v7 changes.

--
Best wishes,
Vladimir

Re: [PATCH v6 2/5] ARM: imx: remove inexistant EPIT timer init

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> i.MX EPIT timer has been removed but not the init function declaration.
> 
> Signed-off-by: Clément Peron 
> Reviewed-by: Fabio Estevam 

Reviewed-by: Vladimir Zapolskiy 
Tested-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir

Re: [PATCH v6 0/5] Reintroduce i.MX EPIT Timer

[Vladimir Zapolskiy]

Hi Clément,

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> As suggested in the commit message we have added the device tree support,
> proper bindings and we moved the driver into the correct folder.
> 
> Moreover we made some changes like use of relaxed IO accesor,
> implement sched_clock, delay_timer and reduce the clockevents min_delta.
> 

I reviewed and tested the driver on i.MX31, as expected it works fine,
and I'll give my tags per a commit, please add them to v7 changes.

--
Best wishes,
Vladimir

Re: [PATCH v6 2/5] ARM: imx: remove inexistant EPIT timer init

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> i.MX EPIT timer has been removed but not the init function declaration.
> 
> Signed-off-by: Clément Peron 
> Reviewed-by: Fabio Estevam 

Reviewed-by: Vladimir Zapolskiy 
Tested-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir

Re: [PATCH v6 3/5] dt-bindings: timer: add i.MX EPIT timer binding

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> Add devicetree binding document for NXP's i.MX SoC specific
> EPIT timer driver.
> 
> Signed-off-by: Clément Peron 

Reviewed-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir

Re: [PATCH v6 3/5] dt-bindings: timer: add i.MX EPIT timer binding

[Vladimir Zapolskiy]

On 06/07/2018 05:05 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> Add devicetree binding document for NXP's i.MX SoC specific
> EPIT timer driver.
> 
> Signed-off-by: Clément Peron 

Reviewed-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir

Re: [PATCH v4 5/5] ARM: dts: imx6qdl: add missing compatible and clock properties for EPIT

[Vladimir Zapolskiy]

Hi Clément,

On 05/31/2018 11:41 AM, Clément Péron wrote:
> Hi Vladimir,
> 
> On Thu, 31 May 2018 at 10:33, Vladimir Zapolskiy
>  wrote:
>>
>> On 05/30/2018 03:03 PM, Clément Péron wrote:
>>> From: Colin Didier 
>>>
>>> Add missing compatible and clock properties for EPIT node.
>>>
>>> Signed-off-by: Colin Didier 
>>> Signed-off-by: Clément Peron 
>>> Reviewed-by: Fabio Estevam 
>>> ---
>>>  arch/arm/boot/dts/imx6qdl.dtsi | 10 ++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>>> index c003e62bf290..0feec516847a 100644
>>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>>> @@ -844,13 +844,23 @@
>>>   };
>>>
>>>   epit1: epit@20d { /* EPIT1 */
>>
>> epit1: timer@20d { ...
>>
>> And /* EPIT1 */ comment can be removed, it is quite clear from the same
>> line context.
>>
>> Formally it is a subject to another patch, but I think this can be
>> accepted as a part of this one.
> 
> Should I also update other boards ?
> I only did it for imx6qdl.dtsi, but the EPIT is present in other boards
> but i can't test it myself.
> 

Sure, please do it, why not, it is quite a safe modification.

One change per one dtsi file will suffice, and I see that imx25.dtsi
already contains the requested change, however probably you may
want to update its compatible = "fsl,imx25-epit" line.

Regarding compatibles for other imx6* SoCs, I think all of them should
be documented in fsl,imxepit.txt and then added to the correspondent
dtsi files one per SoC.

And I forgot the outcome of one former discussion with Uwe Kleine-König,
but if my bad memory serves me, we agreed that i.MX25 was released later
than i.MX31, so the most generic (the last value in the list) compatible
should be a compatible with i.MX31 like in

imx25.dtsi:367: compatible = "fsl,imx25-gpt", "fsl,imx31-gpt";

--
With best wishes,
Vladimir

Re: [PATCH v4 5/5] ARM: dts: imx6qdl: add missing compatible and clock properties for EPIT

[Vladimir Zapolskiy]

Hi Clément,

On 05/31/2018 11:41 AM, Clément Péron wrote:
> Hi Vladimir,
> 
> On Thu, 31 May 2018 at 10:33, Vladimir Zapolskiy
>  wrote:
>>
>> On 05/30/2018 03:03 PM, Clément Péron wrote:
>>> From: Colin Didier 
>>>
>>> Add missing compatible and clock properties for EPIT node.
>>>
>>> Signed-off-by: Colin Didier 
>>> Signed-off-by: Clément Peron 
>>> Reviewed-by: Fabio Estevam 
>>> ---
>>>  arch/arm/boot/dts/imx6qdl.dtsi | 10 ++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
>>> index c003e62bf290..0feec516847a 100644
>>> --- a/arch/arm/boot/dts/imx6qdl.dtsi
>>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
>>> @@ -844,13 +844,23 @@
>>>   };
>>>
>>>   epit1: epit@20d { /* EPIT1 */
>>
>> epit1: timer@20d { ...
>>
>> And /* EPIT1 */ comment can be removed, it is quite clear from the same
>> line context.
>>
>> Formally it is a subject to another patch, but I think this can be
>> accepted as a part of this one.
> 
> Should I also update other boards ?
> I only did it for imx6qdl.dtsi, but the EPIT is present in other boards
> but i can't test it myself.
> 

Sure, please do it, why not, it is quite a safe modification.

One change per one dtsi file will suffice, and I see that imx25.dtsi
already contains the requested change, however probably you may
want to update its compatible = "fsl,imx25-epit" line.

Regarding compatibles for other imx6* SoCs, I think all of them should
be documented in fsl,imxepit.txt and then added to the correspondent
dtsi files one per SoC.

And I forgot the outcome of one former discussion with Uwe Kleine-König,
but if my bad memory serves me, we agreed that i.MX25 was released later
than i.MX31, so the most generic (the last value in the list) compatible
should be a compatible with i.MX31 like in

imx25.dtsi:367: compatible = "fsl,imx25-gpt", "fsl,imx31-gpt";

--
With best wishes,
Vladimir

Re: [PATCH v4 4/5] clocksource: add driver for i.MX EPIT timer

[Vladimir Zapolskiy]

Hi Clément,

On 05/30/2018 03:03 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add driver for NXP's EPIT timer used in i.MX 6 family of SoC.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 
> ---

[snip]

> +++ b/drivers/clocksource/timer-imx-epit.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX EPIT Timer
> + *
> + * Copyright (C) 2010 Sascha Hauer 
> + * Copyright (C) 2018 Colin Didier 
> + * Copyright (C) 2018 Clément Péron 
> + */
> +
> +#include 
> +#include 
> +#include 

The included header above still can be removed.

I have no more comments about the code, I will try to find time to
test the driver, but please don't take it as a promise.

--
With best wishes,
Vladimir

Re: [PATCH v4 4/5] clocksource: add driver for i.MX EPIT timer

[Vladimir Zapolskiy]

Hi Clément,

On 05/30/2018 03:03 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add driver for NXP's EPIT timer used in i.MX 6 family of SoC.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 
> ---

[snip]

> +++ b/drivers/clocksource/timer-imx-epit.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX EPIT Timer
> + *
> + * Copyright (C) 2010 Sascha Hauer 
> + * Copyright (C) 2018 Colin Didier 
> + * Copyright (C) 2018 Clément Péron 
> + */
> +
> +#include 
> +#include 
> +#include 

The included header above still can be removed.

I have no more comments about the code, I will try to find time to
test the driver, but please don't take it as a promise.

--
With best wishes,
Vladimir

Re: [PATCH v4 5/5] ARM: dts: imx6qdl: add missing compatible and clock properties for EPIT

[Vladimir Zapolskiy]

On 05/30/2018 03:03 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add missing compatible and clock properties for EPIT node.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 
> Reviewed-by: Fabio Estevam 
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index c003e62bf290..0feec516847a 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -844,13 +844,23 @@
>   };
>  
>   epit1: epit@20d { /* EPIT1 */

epit1: timer@20d { ...

And /* EPIT1 */ comment can be removed, it is quite clear from the same
line context.

Formally it is a subject to another patch, but I think this can be
accepted as a part of this one.

> + compatible = "fsl,imx6q-epit", "fsl,imx31-epit";
>   reg = <0x020d 0x4000>;
>   interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = < IMX6QDL_CLK_IPG_PER>,
> +  < IMX6QDL_CLK_EPIT1>;
> + clock-names = "ipg", "per";
> + status = "disabled";
>   };
>  
>   epit2: epit@20d4000 { /* EPIT2 */

Same as above.

> + compatible = "fsl,imx6q-epit", "fsl,imx31-epit";
>   reg = <0x020d4000 0x4000>;
>   interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = < IMX6QDL_CLK_IPG_PER>,
> +  < IMX6QDL_CLK_EPIT2>;
> + clock-names = "ipg", "per";
> + status = "disabled";
>   };
>  
>   src: src@20d8000 {
> 

--
With best wishes,
Vladimir

Re: [PATCH v4 5/5] ARM: dts: imx6qdl: add missing compatible and clock properties for EPIT

[Vladimir Zapolskiy]

On 05/30/2018 03:03 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add missing compatible and clock properties for EPIT node.
> 
> Signed-off-by: Colin Didier 
> Signed-off-by: Clément Peron 
> Reviewed-by: Fabio Estevam 
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index c003e62bf290..0feec516847a 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -844,13 +844,23 @@
>   };
>  
>   epit1: epit@20d { /* EPIT1 */

epit1: timer@20d { ...

And /* EPIT1 */ comment can be removed, it is quite clear from the same
line context.

Formally it is a subject to another patch, but I think this can be
accepted as a part of this one.

> + compatible = "fsl,imx6q-epit", "fsl,imx31-epit";
>   reg = <0x020d 0x4000>;
>   interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = < IMX6QDL_CLK_IPG_PER>,
> +  < IMX6QDL_CLK_EPIT1>;
> + clock-names = "ipg", "per";
> + status = "disabled";
>   };
>  
>   epit2: epit@20d4000 { /* EPIT2 */

Same as above.

> + compatible = "fsl,imx6q-epit", "fsl,imx31-epit";
>   reg = <0x020d4000 0x4000>;
>   interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = < IMX6QDL_CLK_IPG_PER>,
> +  < IMX6QDL_CLK_EPIT2>;
> + clock-names = "ipg", "per";
> + status = "disabled";
>   };
>  
>   src: src@20d8000 {
> 

--
With best wishes,
Vladimir

Re: [PATCH v4 3/5] Documentation: DT: add i.MX EPIT timer binding

[Vladimir Zapolskiy]

Hi Clément,

On 05/30/2018 03:03 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> Add devicetree binding document for NXP's i.MX SoC specific
> EPIT timer driver.
> 
> Signed-off-by: Clément Peron 
> ---
>  .../devicetree/bindings/timer/fsl,imxepit.txt | 24 +++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/fsl,imxepit.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/fsl,imxepit.txt 
> b/Documentation/devicetree/bindings/timer/fsl,imxepit.txt
> new file mode 100644
> index ..90112d58af10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/fsl,imxepit.txt
> @@ -0,0 +1,24 @@
> +Binding for the i.MX EPIT timer
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +

no, this leftover reference to clock-bindings.txt is invalid, please remove it.

Instead you may add a simple description of the timer module.

> +Required properties:
> +- compatible: should be "fsl,imx31-epit"

To satisfy compatibles with multiple SoCs, apparently you may follow a model,
which is used with other Freescale controllers, for instance

gpio/fsl-imx-gpio.txt   - compatible : Should be "fsl,-gpio"
mmc/fsl-imx-esdhc.txt   - compatible : Should be "fsl,-esdhc"
serial/fsl-imx-uart.txt - compatible : Should be "fsl,-uart"
timer/fsl,imxgpt.txt- compatible : should be "fsl,-gpt"

and so on, I hope it would cover Rob's ask.

> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +- interrupts: Should contain EPIT controller interrupt
> +- clocks: list of clock specifiers, must contain an entry for each required
> +  entry in clock-names
> +- clock-names : should include entries "ipg", "per"
> +
> +Example for i.MX6QDL:
> + epit1: epit@20d {
> + compatible = "fsl,imx6q-epit", "fsl,imx31-epit";
> + reg = <0x020d 0x4000>;
> + interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = < IMX6QDL_CLK_IPG_PER>,
> + < IMX6QDL_CLK_EPIT1>;
> + clock-names = "ipg", "per";
> + };
> 

--
With best wishes,
Vladimir

Re: [PATCH v4 3/5] Documentation: DT: add i.MX EPIT timer binding

[Vladimir Zapolskiy]

Hi Clément,

On 05/30/2018 03:03 PM, Clément Péron wrote:
> From: Clément Peron 
> 
> Add devicetree binding document for NXP's i.MX SoC specific
> EPIT timer driver.
> 
> Signed-off-by: Clément Peron 
> ---
>  .../devicetree/bindings/timer/fsl,imxepit.txt | 24 +++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/fsl,imxepit.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/fsl,imxepit.txt 
> b/Documentation/devicetree/bindings/timer/fsl,imxepit.txt
> new file mode 100644
> index ..90112d58af10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/fsl,imxepit.txt
> @@ -0,0 +1,24 @@
> +Binding for the i.MX EPIT timer
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +

no, this leftover reference to clock-bindings.txt is invalid, please remove it.

Instead you may add a simple description of the timer module.

> +Required properties:
> +- compatible: should be "fsl,imx31-epit"

To satisfy compatibles with multiple SoCs, apparently you may follow a model,
which is used with other Freescale controllers, for instance

gpio/fsl-imx-gpio.txt   - compatible : Should be "fsl,-gpio"
mmc/fsl-imx-esdhc.txt   - compatible : Should be "fsl,-esdhc"
serial/fsl-imx-uart.txt - compatible : Should be "fsl,-uart"
timer/fsl,imxgpt.txt- compatible : should be "fsl,-gpt"

and so on, I hope it would cover Rob's ask.

> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +- interrupts: Should contain EPIT controller interrupt
> +- clocks: list of clock specifiers, must contain an entry for each required
> +  entry in clock-names
> +- clock-names : should include entries "ipg", "per"
> +
> +Example for i.MX6QDL:
> + epit1: epit@20d {
> + compatible = "fsl,imx6q-epit", "fsl,imx31-epit";
> + reg = <0x020d 0x4000>;
> + interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = < IMX6QDL_CLK_IPG_PER>,
> + < IMX6QDL_CLK_EPIT1>;
> + clock-names = "ipg", "per";
> + };
> 

--
With best wishes,
Vladimir

Re: [PATCH 4/5] clocksource: add driver for i.MX EPIT timer

[Vladimir Zapolskiy]

Hi Clément,

please find basic review comments below.

On 05/28/2018 08:34 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add driver for NXP's EPIT timer used in i.MX 6 family of SoC.
> 

The first author's signed-off-by tag is missing.

> Signed-off-by: Clément Peron 
> ---
>  drivers/clocksource/Kconfig  |  12 ++
>  drivers/clocksource/Makefile |   1 +
>  drivers/clocksource/timer-imx-epit.c | 254 +++
>  3 files changed, 267 insertions(+)
>  create mode 100644 drivers/clocksource/timer-imx-epit.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 8e8a09755d10..cc1ed592fa6f 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -576,6 +576,18 @@ config H8300_TPU
> This enables the clocksource for the H8300 platform with the
> H8S2678 cpu.
>  
> +config CLKSRC_IMX_EPIT
> + bool "Clocksource using i.MX EPIT"
> + depends on ARM && CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST)
> + select CLKSRC_OF if OF

The driver strictly depends on OF, and this has to be specified.

> + select CLKSRC_MMIO
> + help
> +   This enables EPIT support available on some i.MX platforms.
> +   Normally you don't have a reason to do so as the EPIT has
> +   the same features and uses the same clocks as the GPT.
> +   Anyway, on some systems the GPT may be in use for other
> +   purposes.
> +
>  config CLKSRC_IMX_GPT
>   bool "Clocksource using i.MX GPT" if COMPILE_TEST
>   depends on ARM && CLKDEV_LOOKUP
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 00caf37e52f9..d9426f69ec69 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_INTEGRATOR_AP_TIMER)   += timer-integrator-ap.o
>  obj-$(CONFIG_CLKSRC_VERSATILE)   += versatile.o
>  obj-$(CONFIG_CLKSRC_MIPS_GIC)+= mips-gic-timer.o
>  obj-$(CONFIG_CLKSRC_TANGO_XTAL)  += tango_xtal.o
> +obj-$(CONFIG_CLKSRC_IMX_EPIT)+= timer-imx-epit.o
>  obj-$(CONFIG_CLKSRC_IMX_GPT) += timer-imx-gpt.o
>  obj-$(CONFIG_CLKSRC_IMX_TPM) += timer-imx-tpm.o
>  obj-$(CONFIG_ASM9260_TIMER)  += asm9260_timer.o
> diff --git a/drivers/clocksource/timer-imx-epit.c 
> b/drivers/clocksource/timer-imx-epit.c
> new file mode 100644
> index ..96eb6435a9c3
> --- /dev/null
> +++ b/drivers/clocksource/timer-imx-epit.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX EPIT Timer
> + *
> + * Copyright (C) 2010 Sascha Hauer 
> + * Copyright (C) 2018 Colin Didier 
> + */
> +
> +#define EPITCR   0x00
> +#define EPITSR   0x04
> +#define EPITLR   0x08
> +#define EPITCMPR 0x0c
> +#define EPITCNR  0x10
> +
> +#define EPITCR_EN(1 << 0)
> +#define EPITCR_ENMOD (1 << 1)
> +#define EPITCR_OCIEN (1 << 2)
> +#define EPITCR_RLD   (1 << 3)
> +#define EPITCR_PRESC(x)  (((x) & 0xfff) << 4)
> +#define EPITCR_SWR   (1 << 16)
> +#define EPITCR_IOVW  (1 << 17)
> +#define EPITCR_DBGEN (1 << 18)
> +#define EPITCR_WAITEN(1 << 19)
> +#define EPITCR_RES   (1 << 20)
> +#define EPITCR_STOPEN(1 << 21)
> +#define EPITCR_OM_DISCON (0 << 22)
> +#define EPITCR_OM_TOGGLE (1 << 22)
> +#define EPITCR_OM_CLEAR  (2 << 22)
> +#define EPITCR_OM_SET(3 << 22)
> +#define EPITCR_CLKSRC_OFF(0 << 24)
> +#define EPITCR_CLKSRC_PERIPHERAL (1 << 24)
> +#define EPITCR_CLKSRC_REF_HIGH   (2 << 24)
> +#define EPITCR_CLKSRC_REF_LOW(3 << 24)
> +
> +#define EPITSR_OCIF  (1 << 0)
> +

Please place all macro definitions after the list of included header files.
Also for bit field definitions please use BIT() macro, and remove
(0 << x) macro, they are anyway unused in the code.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please sort out the list of headers alphabetically.

> +
> +

Surplus empty line, please remove it.

> +struct epit_timer {
> + void __iomem *base;
> + int irq;
> + struct clk *clk_per;
> + struct clock_event_device ced;
> + struct irqaction act;
> +};
> +
> +static inline struct epit_timer *to_epit_timer(struct clock_event_device 
> *ced)
> +{
> + return container_of(ced, struct epit_timer, ced);
> +}
> +
> +static inline void epit_irq_disable(struct epit_timer *epittm)
> +{
> + u32 val;
> +
> + val = readl_relaxed(epittm->base + EPITCR);
> + writel_relaxed(val & ~EPITCR_OCIEN, epittm->base + EPITCR);
> +}
> +
> +static inline void epit_irq_enable(struct epit_timer *epittm)
> +{
> + u32 val;
> 

Re: [PATCH 4/5] clocksource: add driver for i.MX EPIT timer

[Vladimir Zapolskiy]

Hi Clément,

please find basic review comments below.

On 05/28/2018 08:34 PM, Clément Péron wrote:
> From: Colin Didier 
> 
> Add driver for NXP's EPIT timer used in i.MX 6 family of SoC.
> 

The first author's signed-off-by tag is missing.

> Signed-off-by: Clément Peron 
> ---
>  drivers/clocksource/Kconfig  |  12 ++
>  drivers/clocksource/Makefile |   1 +
>  drivers/clocksource/timer-imx-epit.c | 254 +++
>  3 files changed, 267 insertions(+)
>  create mode 100644 drivers/clocksource/timer-imx-epit.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 8e8a09755d10..cc1ed592fa6f 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -576,6 +576,18 @@ config H8300_TPU
> This enables the clocksource for the H8300 platform with the
> H8S2678 cpu.
>  
> +config CLKSRC_IMX_EPIT
> + bool "Clocksource using i.MX EPIT"
> + depends on ARM && CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST)
> + select CLKSRC_OF if OF

The driver strictly depends on OF, and this has to be specified.

> + select CLKSRC_MMIO
> + help
> +   This enables EPIT support available on some i.MX platforms.
> +   Normally you don't have a reason to do so as the EPIT has
> +   the same features and uses the same clocks as the GPT.
> +   Anyway, on some systems the GPT may be in use for other
> +   purposes.
> +
>  config CLKSRC_IMX_GPT
>   bool "Clocksource using i.MX GPT" if COMPILE_TEST
>   depends on ARM && CLKDEV_LOOKUP
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 00caf37e52f9..d9426f69ec69 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_INTEGRATOR_AP_TIMER)   += timer-integrator-ap.o
>  obj-$(CONFIG_CLKSRC_VERSATILE)   += versatile.o
>  obj-$(CONFIG_CLKSRC_MIPS_GIC)+= mips-gic-timer.o
>  obj-$(CONFIG_CLKSRC_TANGO_XTAL)  += tango_xtal.o
> +obj-$(CONFIG_CLKSRC_IMX_EPIT)+= timer-imx-epit.o
>  obj-$(CONFIG_CLKSRC_IMX_GPT) += timer-imx-gpt.o
>  obj-$(CONFIG_CLKSRC_IMX_TPM) += timer-imx-tpm.o
>  obj-$(CONFIG_ASM9260_TIMER)  += asm9260_timer.o
> diff --git a/drivers/clocksource/timer-imx-epit.c 
> b/drivers/clocksource/timer-imx-epit.c
> new file mode 100644
> index ..96eb6435a9c3
> --- /dev/null
> +++ b/drivers/clocksource/timer-imx-epit.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX EPIT Timer
> + *
> + * Copyright (C) 2010 Sascha Hauer 
> + * Copyright (C) 2018 Colin Didier 
> + */
> +
> +#define EPITCR   0x00
> +#define EPITSR   0x04
> +#define EPITLR   0x08
> +#define EPITCMPR 0x0c
> +#define EPITCNR  0x10
> +
> +#define EPITCR_EN(1 << 0)
> +#define EPITCR_ENMOD (1 << 1)
> +#define EPITCR_OCIEN (1 << 2)
> +#define EPITCR_RLD   (1 << 3)
> +#define EPITCR_PRESC(x)  (((x) & 0xfff) << 4)
> +#define EPITCR_SWR   (1 << 16)
> +#define EPITCR_IOVW  (1 << 17)
> +#define EPITCR_DBGEN (1 << 18)
> +#define EPITCR_WAITEN(1 << 19)
> +#define EPITCR_RES   (1 << 20)
> +#define EPITCR_STOPEN(1 << 21)
> +#define EPITCR_OM_DISCON (0 << 22)
> +#define EPITCR_OM_TOGGLE (1 << 22)
> +#define EPITCR_OM_CLEAR  (2 << 22)
> +#define EPITCR_OM_SET(3 << 22)
> +#define EPITCR_CLKSRC_OFF(0 << 24)
> +#define EPITCR_CLKSRC_PERIPHERAL (1 << 24)
> +#define EPITCR_CLKSRC_REF_HIGH   (2 << 24)
> +#define EPITCR_CLKSRC_REF_LOW(3 << 24)
> +
> +#define EPITSR_OCIF  (1 << 0)
> +

Please place all macro definitions after the list of included header files.
Also for bit field definitions please use BIT() macro, and remove
(0 << x) macro, they are anyway unused in the code.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please sort out the list of headers alphabetically.

> +
> +

Surplus empty line, please remove it.

> +struct epit_timer {
> + void __iomem *base;
> + int irq;
> + struct clk *clk_per;
> + struct clock_event_device ced;
> + struct irqaction act;
> +};
> +
> +static inline struct epit_timer *to_epit_timer(struct clock_event_device 
> *ced)
> +{
> + return container_of(ced, struct epit_timer, ced);
> +}
> +
> +static inline void epit_irq_disable(struct epit_timer *epittm)
> +{
> + u32 val;
> +
> + val = readl_relaxed(epittm->base + EPITCR);
> + writel_relaxed(val & ~EPITCR_OCIEN, epittm->base + EPITCR);
> +}
> +
> +static inline void epit_irq_enable(struct epit_timer *epittm)
> +{
> + u32 val;
> 

Re: [PATCH v4 4/8] PCI: Replace dev_node parameter of of_pci_get_host_bridge_resources with device

[Vladimir Zapolskiy]

Hi Jan, Bjorn,

On 05/15/2018 12:07 PM, Jan Kiszka wrote:
> From: Jan Kiszka 
> 
> Another step towards a managed version of
> of_pci_get_host_bridge_resources(): Feed in the underlying device,
> rather than just the OF node. This will allow to use managed resource
> allocation internally later on.
> 
> CC: Jingoo Han 
> CC: Joao Pinto 
> CC: Lorenzo Pieralisi 
> Signed-off-by: Jan Kiszka 

[snip]

> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> index a6af62e0256d..61802e55a00c 100644
> --- a/drivers/pci/host/pcie-altera.c
> +++ b/drivers/pci/host/pcie-altera.c
> @@ -488,11 +488,10 @@ static int 
> altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
>  {
>   int err, res_valid = 0;
>   struct device *dev = >pdev->dev;
> - struct device_node *np = dev->of_node;
>   struct resource_entry *win;
>  
> - err = of_pci_get_host_bridge_resources(np, 0, 0xff, >resources,
> -NULL);
> + err = of_pci_get_host_bridge_resources(dev, 0, 0xff
> + >resources, NULL);
>   if (err)
>   return err;
>  

In case if it is an undiscovered issue, a comma was mistakenly removed,
which will result it compilation error.

The problem is also found in pci/next , see commit 88e3909aa125.

--
With best wishes,
Vladimir

Re: [PATCH v4 4/8] PCI: Replace dev_node parameter of of_pci_get_host_bridge_resources with device

[Vladimir Zapolskiy]

Hi Jan, Bjorn,

On 05/15/2018 12:07 PM, Jan Kiszka wrote:
> From: Jan Kiszka 
> 
> Another step towards a managed version of
> of_pci_get_host_bridge_resources(): Feed in the underlying device,
> rather than just the OF node. This will allow to use managed resource
> allocation internally later on.
> 
> CC: Jingoo Han 
> CC: Joao Pinto 
> CC: Lorenzo Pieralisi 
> Signed-off-by: Jan Kiszka 

[snip]

> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> index a6af62e0256d..61802e55a00c 100644
> --- a/drivers/pci/host/pcie-altera.c
> +++ b/drivers/pci/host/pcie-altera.c
> @@ -488,11 +488,10 @@ static int 
> altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
>  {
>   int err, res_valid = 0;
>   struct device *dev = >pdev->dev;
> - struct device_node *np = dev->of_node;
>   struct resource_entry *win;
>  
> - err = of_pci_get_host_bridge_resources(np, 0, 0xff, >resources,
> -NULL);
> + err = of_pci_get_host_bridge_resources(dev, 0, 0xff
> + >resources, NULL);
>   if (err)
>   return err;
>  

In case if it is an undiscovered issue, a comma was mistakenly removed,
which will result it compilation error.

The problem is also found in pci/next , see commit 88e3909aa125.

--
With best wishes,
Vladimir

Re: [PATCH] PCI: Clean up resource allocation in devm_of_pci_get_host_bridge_resources()

[Vladimir Zapolskiy]

On 05/16/2018 03:31 PM, Jan Kiszka wrote:
> Instead of first allocating and then freeing memory for struct resource
> in case we cannot parse a PCI resource from the device tree, work
> against a local struct and kmemdup it when we decide to go with it.
> 
> Suggested-by: Andy Shevchenko <andy.shevche...@gmail.com>
> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
> ---
>  drivers/pci/of.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index b06585a1da75..fc0f906c5c25 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -266,7 +266,7 @@ int devm_of_pci_get_host_bridge_resources(struct device 
> *dev,
>   struct list_head *resources, resource_size_t *io_base)
>  {
>   struct device_node *dev_node = dev->of_node;
> - struct resource *res;
> + struct resource *res, tmp_res;
>   struct resource *bus_range;
>   struct of_pci_range range;
>   struct of_pci_range_parser parser;
> @@ -320,18 +320,16 @@ int devm_of_pci_get_host_bridge_resources(struct device 
> *dev,
>   if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
>   continue;
>  
> - res = devm_kzalloc(dev, sizeof(struct resource), GFP_KERNEL);
> + err = of_pci_range_to_resource(, dev_node, _res);
> + if (err)
> + continue;
> +
> + res = devm_kmemdup(dev, _res, sizeof(tmp_res), GFP_KERNEL);

The change looks okay, apparently probable garbage in tmp_res.desc has no 
impact.

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir

Re: [PATCH] PCI: Clean up resource allocation in devm_of_pci_get_host_bridge_resources()

[Vladimir Zapolskiy]

On 05/16/2018 03:31 PM, Jan Kiszka wrote:
> Instead of first allocating and then freeing memory for struct resource
> in case we cannot parse a PCI resource from the device tree, work
> against a local struct and kmemdup it when we decide to go with it.
> 
> Suggested-by: Andy Shevchenko 
> Signed-off-by: Jan Kiszka 
> ---
>  drivers/pci/of.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index b06585a1da75..fc0f906c5c25 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -266,7 +266,7 @@ int devm_of_pci_get_host_bridge_resources(struct device 
> *dev,
>   struct list_head *resources, resource_size_t *io_base)
>  {
>   struct device_node *dev_node = dev->of_node;
> - struct resource *res;
> + struct resource *res, tmp_res;
>   struct resource *bus_range;
>   struct of_pci_range range;
>   struct of_pci_range_parser parser;
> @@ -320,18 +320,16 @@ int devm_of_pci_get_host_bridge_resources(struct device 
> *dev,
>   if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
>   continue;
>  
> - res = devm_kzalloc(dev, sizeof(struct resource), GFP_KERNEL);
> + err = of_pci_range_to_resource(, dev_node, _res);
> + if (err)
> + continue;
> +
> + res = devm_kmemdup(dev, _res, sizeof(tmp_res), GFP_KERNEL);

The change looks okay, apparently probable garbage in tmp_res.desc has no 
impact.

Reviewed-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir