| Message ID | 20181222155434.8104-1-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d238b8f68018f33f12253a63543a04712d2d6a5b |
| Headers | show |
| Series | [1/2] input: clps711x-keypad: Switch to use syscon_regmap_lookup_by_phandle() | expand |
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.
>Воскресенье, 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. ---
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.
>Воскресенье, 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? ---
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.
>Понедельник, 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. ---
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?
>Четверг, 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"); } ---
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.
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);
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(-)