[1/2] input: clps711x-keypad: Switch to use syscon_regmap_lookup_by_phandle()
diff mbox series

Message ID 20181222155434.8104-1-shc_work@mail.ru
State Under Review
Headers show
Series
  • [1/2] input: clps711x-keypad: Switch to use syscon_regmap_lookup_by_phandle()
Related show

Commit Message

Alexander Shiyan Dec. 22, 2018, 3:54 p.m. UTC
As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
interface from platform devices"), we need to switch to using the
syscon_regmap_lookup_by_phandle() function. This patch makes this change.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/input/keyboard/clps711x-keypad.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Dmitry Torokhov Dec. 23, 2018, 6:15 p.m. UTC | #1
On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
> interface from platform devices"), we need to switch to using the
> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
> index e319f74..f7f49da 100644
> --- a/drivers/input/keyboard/clps711x-keypad.c
> +++ b/drivers/input/keyboard/clps711x-keypad.c
> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	priv->syscon =
> -		syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
> +	priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");

Do we care about compatibility with old DTSes that do not have syscon
phandle in the keypad descriptor?

Thanks.
Alexander Shiyan Dec. 23, 2018, 7:28 p.m. UTC | #2
>Воскресенье, 23 декабря 2018, 21:15 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>
>On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
>> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
>> interface from platform devices"), we need to switch to using the
>> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
>> 
>> Signed-off-by: Alexander Shiyan < shc_work@mail.ru >
>> ---
>>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
>> index e319f74..f7f49da 100644
>> --- a/drivers/input/keyboard/clps711x-keypad.c
>> +++ b/drivers/input/keyboard/clps711x-keypad.c
>> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
>>  if (!priv)
>>  return -ENOMEM;
>> 
>> -priv->syscon =
>> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
>> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
>
>Do we care about compatibility with old DTSes that do not have syscon
>phandle in the keypad descriptor?

Hello.

There are no users for this driver in current kernel.
The patch for adding bindings (with syscon phandle) is submitted by me recently.

---
Dmitry Torokhov Dec. 23, 2018, 8:10 p.m. UTC | #3
On Sun, Dec 23, 2018 at 10:28:50PM +0300, Alexander Shiyan wrote:
> >Воскресенье, 23 декабря 2018, 21:15 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> >
> >On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
> >> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
> >> interface from platform devices"), we need to switch to using the
> >> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
> >> 
> >> Signed-off-by: Alexander Shiyan < shc_work@mail.ru >
> >> ---
> >>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
> >> index e319f74..f7f49da 100644
> >> --- a/drivers/input/keyboard/clps711x-keypad.c
> >> +++ b/drivers/input/keyboard/clps711x-keypad.c
> >> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
> >>  if (!priv)
> >>  return -ENOMEM;
> >> 
> >> -priv->syscon =
> >> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
> >> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
> >
> >Do we care about compatibility with old DTSes that do not have syscon
> >phandle in the keypad descriptor?
> 
> Hello.
> 
> There are no users for this driver in current kernel.
> The patch for adding bindings (with syscon phandle) is submitted by me recently.

Alexander,

The argument that there are no users in kernel works well for adjusting
or removing platform data, but does not work for DTS, as it is supposed
to be separate from the kernel. The original binding is from 2014 and
there might be users of the driver that will not be aware of the change.
If anything, if there were users of the driver in DTSes shipped with the
kernel, we could make argument that we adjusted them to the new schema
and all should be well, but will all users being external that is not
the case.

I would love to just apply the patch as is, but I am afraid we need to
keep compatibility with older DTSes.

Thanks.
Alexander Shiyan Dec. 24, 2018, 6:02 a.m. UTC | #4
>Воскресенье, 23 декабря 2018, 23:10 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>
>On Sun, Dec 23, 2018 at 10:28:50PM +0300, Alexander Shiyan wrote:
>> >Воскресенье, 23 декабря 2018, 21:15 +03:00 от Dmitry Torokhov < dmitry.torokhov@gmail.com >:
>> >
>> >On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
>> >> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
>> >> interface from platform devices"), we need to switch to using the
>> >> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
>> >> 
>> >> Signed-off-by: Alexander Shiyan <  shc_work@mail.ru >
>> >> ---
>> >>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
>> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >> 
>> >> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
>> >> index e319f74..f7f49da 100644
>> >> --- a/drivers/input/keyboard/clps711x-keypad.c
>> >> +++ b/drivers/input/keyboard/clps711x-keypad.c
>> >> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
>> >>  if (!priv)
>> >>  return -ENOMEM;
>> >> 
>> >> -priv->syscon =
>> >> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
>> >> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
>> >
>> >Do we care about compatibility with old DTSes that do not have syscon
>> >phandle in the keypad descriptor?
>> 
>> Hello.
>> 
>> There are no users for this driver in current kernel.
>> The patch for adding bindings (with syscon phandle) is submitted by me recently.
>
>Alexander,
>
>The argument that there are no users in kernel works well for adjusting
>or removing platform data, but does not work for DTS, as it is supposed
>to be separate from the kernel. The original binding is from 2014 and
>there might be users of the driver that will not be aware of the change.
>If anything, if there were users of the driver in DTSes shipped with the
>kernel, we could make argument that we adjusted them to the new schema
>and all should be well, but will all users being external that is not
>the case.
>
>I would love to just apply the patch as is, but I am afraid we need to
>keep compatibility with older DTSes.

Ok, will it be better if we keep the syscon_regmap_lookup_by_compatible()
call as a fallback method for getting syscon phandle?

---
Dmitry Torokhov Dec. 24, 2018, 5:46 p.m. UTC | #5
On Mon, Dec 24, 2018 at 09:02:50AM +0300, Alexander Shiyan wrote:
> >Воскресенье, 23 декабря 2018, 23:10 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> >
> >On Sun, Dec 23, 2018 at 10:28:50PM +0300, Alexander Shiyan wrote:
> >> >Воскресенье, 23 декабря 2018, 21:15 +03:00 от Dmitry Torokhov < dmitry.torokhov@gmail.com >:
> >> >
> >> >On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
> >> >> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
> >> >> interface from platform devices"), we need to switch to using the
> >> >> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
> >> >> 
> >> >> Signed-off-by: Alexander Shiyan <  shc_work@mail.ru >
> >> >> ---
> >> >>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
> >> >> index e319f74..f7f49da 100644
> >> >> --- a/drivers/input/keyboard/clps711x-keypad.c
> >> >> +++ b/drivers/input/keyboard/clps711x-keypad.c
> >> >> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
> >> >>  if (!priv)
> >> >>  return -ENOMEM;
> >> >> 
> >> >> -priv->syscon =
> >> >> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
> >> >> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
> >> >
> >> >Do we care about compatibility with old DTSes that do not have syscon
> >> >phandle in the keypad descriptor?
> >> 
> >> Hello.
> >> 
> >> There are no users for this driver in current kernel.
> >> The patch for adding bindings (with syscon phandle) is submitted by me recently.
> >
> >Alexander,
> >
> >The argument that there are no users in kernel works well for adjusting
> >or removing platform data, but does not work for DTS, as it is supposed
> >to be separate from the kernel. The original binding is from 2014 and
> >there might be users of the driver that will not be aware of the change.
> >If anything, if there were users of the driver in DTSes shipped with the
> >kernel, we could make argument that we adjusted them to the new schema
> >and all should be well, but will all users being external that is not
> >the case.
> >
> >I would love to just apply the patch as is, but I am afraid we need to
> >keep compatibility with older DTSes.
> 
> Ok, will it be better if we keep the syscon_regmap_lookup_by_compatible()
> call as a fallback method for getting syscon phandle?
> 

Yes, please. Just make sure you do not try to fall back if
syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER, but for all
other errors we should try old syscon_regmap_lookup_by_compatible().

Thanks.
Alexander Shiyan Jan. 17, 2019, 7:44 a.m. UTC | #6
>Понедельник, 24 декабря 2018, 20:46 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
...
>> >> >On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
>> >> >> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
>> >> >> interface from platform devices"), we need to switch to using the
>> >> >> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
>> >> >> 
>> >> >> Signed-off-by: Alexander Shiyan <  shc_work@mail.ru >
>> >> >> ---
>> >> >>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
>> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >> >> 
>> >> >> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
>> >> >> index e319f74..f7f49da 100644
>> >> >> --- a/drivers/input/keyboard/clps711x-keypad.c
>> >> >> +++ b/drivers/input/keyboard/clps711x-keypad.c
>> >> >> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
>> >> >>  if (!priv)
>> >> >>  return -ENOMEM;
>> >> >> 
>> >> >> -priv->syscon =
>> >> >> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
>> >> >> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
...
>> >I would love to just apply the patch as is, but I am afraid we need to
>> >keep compatibility with older DTSes.
>> 
>> Ok, will it be better if we keep the syscon_regmap_lookup_by_compatible()
>> call as a fallback method for getting syscon phandle?
>
>Yes, please. Just make sure you do not try to fall back if
>syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER, but for all
>other errors we should try old syscon_regmap_lookup_by_compatible().

This check is no longer needed after the patch mentioned in the description.

---
Dmitry Torokhov Jan. 17, 2019, 7:49 a.m. UTC | #7
On Thu, Jan 17, 2019 at 10:44:08AM +0300, Alexander Shiyan wrote:
> >Понедельник, 24 декабря 2018, 20:46 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> ...
> >> >> >On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
> >> >> >> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
> >> >> >> interface from platform devices"), we need to switch to using the
> >> >> >> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
> >> >> >> 
> >> >> >> Signed-off-by: Alexander Shiyan <  shc_work@mail.ru >
> >> >> >> ---
> >> >> >>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
> >> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >> >> 
> >> >> >> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
> >> >> >> index e319f74..f7f49da 100644
> >> >> >> --- a/drivers/input/keyboard/clps711x-keypad.c
> >> >> >> +++ b/drivers/input/keyboard/clps711x-keypad.c
> >> >> >> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
> >> >> >>  if (!priv)
> >> >> >>  return -ENOMEM;
> >> >> >> 
> >> >> >> -priv->syscon =
> >> >> >> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
> >> >> >> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
> ...
> >> >I would love to just apply the patch as is, but I am afraid we need to
> >> >keep compatibility with older DTSes.
> >> 
> >> Ok, will it be better if we keep the syscon_regmap_lookup_by_compatible()
> >> call as a fallback method for getting syscon phandle?
> >
> >Yes, please. Just make sure you do not try to fall back if
> >syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER, but for all
> >other errors we should try old syscon_regmap_lookup_by_compatible().
> 
> This check is no longer needed after the patch mentioned in the description.

Why exactly?
Alexander Shiyan Jan. 17, 2019, 8:09 a.m. UTC | #8
>Четверг, 17 января 2019, 10:49 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
...
>> >> >> >On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
>> >> >> >> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
>> >> >> >> interface from platform devices"), we need to switch to using the
>> >> >> >> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
>> >> >> >> 
>> >> >> >> Signed-off-by: Alexander Shiyan <  shc_work@mail.ru >
>> >> >> >> ---
>> >> >> >>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
>> >> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
>> >> >> >> 
>> >> >> >> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
>> >> >> >> index e319f74..f7f49da 100644
>> >> >> >> --- a/drivers/input/keyboard/clps711x-keypad.c
>> >> >> >> +++ b/drivers/input/keyboard/clps711x-keypad.c
>> >> >> >> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
>> >> >> >>  if (!priv)
>> >> >> >>  return -ENOMEM;
>> >> >> >> 
>> >> >> >> -priv->syscon =
>> >> >> >> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
>> >> >> >> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
>> ...
>> >> >I would love to just apply the patch as is, but I am afraid we need to
>> >> >keep compatibility with older DTSes.
>> >> 
>> >> Ok, will it be better if we keep the syscon_regmap_lookup_by_compatible()
>> >> call as a fallback method for getting syscon phandle?
>> >
>> >Yes, please. Just make sure you do not try to fall back if
>> >syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER, but for all
>> >other errors we should try old syscon_regmap_lookup_by_compatible().
>> 
>> This check is no longer needed after the patch mentioned in the description.
>
>Why exactly?

As far as I understand, syscon_regmap_lookup_by_phandle() calls syscon_node_to_regmap(),
where it tries to find already registered syscon devices, then, if absent, of_syscon_register()
is called, so we always get a syscon device or get an -ENODEV error.

The code below works fine for me.

	priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
	if (IS_ERR(priv->syscon)) {
		/* falling back to old bindings */
		priv->syscon =
			syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
		if (IS_ERR(priv->syscon))
			return PTR_ERR(priv->syscon);
		else
			dev_warn(dev, "Please migrate driver bindings"
				      " to use syscon phandle.\n");
	}

---
Dmitry Torokhov Jan. 19, 2019, 9:19 a.m. UTC | #9
On Thu, Jan 17, 2019 at 11:09:43AM +0300, Alexander Shiyan wrote:
> >Четверг, 17 января 2019, 10:49 +03:00 от Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> ...
> >> >> >> >On Sat, Dec 22, 2018 at 06:54:34PM +0300, Alexander Shiyan wrote:
> >> >> >> >> As mentioned in the patch bdb0066df96e ("mfd: syscon: Decouple syscon
> >> >> >> >> interface from platform devices"), we need to switch to using the
> >> >> >> >> syscon_regmap_lookup_by_phandle() function. This patch makes this change.
> >> >> >> >> 
> >> >> >> >> Signed-off-by: Alexander Shiyan <  shc_work@mail.ru >
> >> >> >> >> ---
> >> >> >> >>  drivers/input/keyboard/clps711x-keypad.c | 3 +--
> >> >> >> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >> >> >> 
> >> >> >> >> diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
> >> >> >> >> index e319f74..f7f49da 100644
> >> >> >> >> --- a/drivers/input/keyboard/clps711x-keypad.c
> >> >> >> >> +++ b/drivers/input/keyboard/clps711x-keypad.c
> >> >> >> >> @@ -100,8 +100,7 @@ static int clps711x_keypad_probe(struct platform_device *pdev)
> >> >> >> >>  if (!priv)
> >> >> >> >>  return -ENOMEM;
> >> >> >> >> 
> >> >> >> >> -priv->syscon =
> >> >> >> >> -syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
> >> >> >> >> +priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
> >> ...
> >> >> >I would love to just apply the patch as is, but I am afraid we need to
> >> >> >keep compatibility with older DTSes.
> >> >> 
> >> >> Ok, will it be better if we keep the syscon_regmap_lookup_by_compatible()
> >> >> call as a fallback method for getting syscon phandle?
> >> >
> >> >Yes, please. Just make sure you do not try to fall back if
> >> >syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER, but for all
> >> >other errors we should try old syscon_regmap_lookup_by_compatible().
> >> 
> >> This check is no longer needed after the patch mentioned in the description.
> >
> >Why exactly?
> 
> As far as I understand, syscon_regmap_lookup_by_phandle() calls syscon_node_to_regmap(),
> where it tries to find already registered syscon devices, then, if absent, of_syscon_register()
> is called, so we always get a syscon device or get an -ENODEV error.

of_syscon_register() may fail, and we do not know what error codes it
might return. Even if at this time it does not ever return
-EPROBE_DEFER, this can change in the future, and in this case we should
not execute fallback.

> 
> The code below works fine for me.
> 
> 	priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
> 	if (IS_ERR(priv->syscon)) {

Please change tp:

		if (PTR_ERR(priv->syscon) != -EPROBE_DEFER) {
			/* falling back to old bindings */
			priv->syscon =
				syscon_regmap_lookup_by_compatible(
						"cirrus,ep7209-syscon1");
		}

> 		/* falling back to old bindings */
> 		priv->syscon =
> 			syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
> 		if (IS_ERR(priv->syscon))
> 			return PTR_ERR(priv->syscon);
> 		else
> 			dev_warn(dev, "Please migrate driver bindings"
> 				      " to use syscon phandle.\n");
> 	}
> 
> ---

Thanks.

Patch
diff mbox series

diff --git a/drivers/input/keyboard/clps711x-keypad.c b/drivers/input/keyboard/clps711x-keypad.c
index e319f74..f7f49da 100644
--- a/drivers/input/keyboard/clps711x-keypad.c
+++ b/drivers/input/keyboard/clps711x-keypad.c
@@ -100,8 +100,7 @@  static int clps711x_keypad_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	priv->syscon =
-		syscon_regmap_lookup_by_compatible("cirrus,ep7209-syscon1");
+	priv->syscon = syscon_regmap_lookup_by_phandle(np, "syscon");
 	if (IS_ERR(priv->syscon))
 		return PTR_ERR(priv->syscon);