* [PATCH v2 71/77] sh-pfc: Add OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
@ 2012-11-27 0:03 ` Laurent Pinchart
2012-12-01 22:55 ` [PATCH v2 00/77] SH pin control and GPIO rework with " Linus Walleij
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2012-11-27 0:03 UTC (permalink / raw)
To: linux-sh
Cc: Paul Mundt, Magnus Damm, Simon Horman, Linus Walleij,
Kuninori Morimoto, Phil Edworthy, Nobuhiro Iwamatsu,
devicetree-discuss
Support device instantiation through the device tree. The compatible
property is used to select the SoC pinmux information.
Set the gpio_chip device field to the PFC device to enable automatic
GPIO OF support.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: devicetree-discuss@lists.ozlabs.org
---
.../bindings/pinctrl/renesas,pfc-pinctrl.txt | 43 ++++++++++++++
drivers/pinctrl/sh-pfc/core.c | 62 +++++++++++++++++++-
drivers/pinctrl/sh-pfc/gpio.c | 1 +
3 files changed, 104 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
Only GPIO DT bindings are currently implemented, pinmux bindings are still
work in progress and will be posted soon.
On the GPIO side, some DT bindings require aliases for the GPIO (and pinctrl)
nodes. What's the reason behind that ?
diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
new file mode 100644
index 0000000..f08d8c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
@@ -0,0 +1,43 @@
+* Renesas GPIO and Pin Mux/Config controller
+
+Required Properties:
+- compatible: should be one of the following.
+ - "renesas,pfc-r8a7740": for R8A7740 (R-Mobile A1) compatible pin-controller.
+ - "renesas,pfc-r8a7779": for R8A7779 (R-Car H1) compatible pin-controller.
+ - "renesas,pfc-sh7372": for SH7372 (SH-Mobile AP4) compatible pin-controller.
+ - "renesas,pfc-sh73a0": for SH73A0 (SH-Mobile AG5) compatible pin-controller.
+
+- reg: Base address and length of each memory resource used by the pin
+ controller hardware module.
+
+- gpio-controller: Marks the device node as a gpio controller.
+
+- #gpio-cells: Should be 2. The first cell is the pin number and the second cell
+ is used to specify optional parameters (currently unused).
+
+ The syntax of the gpio specifier used by client nodes should be the following
+ with values derived from the SoC user manual.
+
+ <[phandle of the gpio controller node]
+ [pin number within the gpio controller]
+ [flags and pull up/down]>
+
+
+Example 1: SH73A0 (SH-Mobile AG5) pin controller node
+
+ gpio: pfc@e6050000 {
+ compatible = "renesas,pfc-sh73a0";
+ reg = <0xe6050000 0x8000>,
+ <0xe605801c 0x1c>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+Example 2: A GPIO LED node that references a GPIO
+
+ leds {
+ compatible = "gpio-leds";
+ led1 {
+ gpios = <&gpio 20 1>; /* Active low */
+ };
+ };
diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 062308f..9cbe684 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -19,6 +19,7 @@
#include <linux/ioport.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of_device.h>
#include <linux/slab.h>
#include <linux/pinctrl/machine.h>
#include <linux/platform_device.h>
@@ -498,8 +499,55 @@ int sh_pfc_config_gpio(struct sh_pfc *pfc, unsigned gpio, int pinmux_type,
return -1;
}
+#ifdef CONFIG_OF
+static const struct of_device_id sh_pfc_of_table[] = {
+#ifdef CONFIG_PINCTRL_PFC_R8A7740
+ {
+ .compatible = "renesas,pfc-r8a7740",
+ .data = &r8a7740_pinmux_info,
+ },
+#endif
+#ifdef CONFIG_PINCTRL_PFC_R8A7779
+ {
+ .compatible = "renesas,pfc-r8a7779",
+ .data = &r8a7779_pinmux_info,
+ },
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH7367
+ {
+ .compatible = "renesas,pfc-sh7367",
+ .data = &sh7367_pinmux_info,
+ },
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH7372
+ {
+ .compatible = "renesas,pfc-sh7372",
+ .data = &sh7372_pinmux_info,
+ },
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH7377
+ {
+ .compatible = "renesas,pfc-sh7377",
+ .data = &sh7377_pinmux_info,
+ },
+#endif
+#ifdef CONFIG_PINCTRL_PFC_SH73A0
+ {
+ .compatible = "renesas,pfc-sh73a0",
+ .data = &sh73a0_pinmux_info,
+ },
+#endif
+ { },
+};
+MODULE_DEVICE_TABLE(of, sh_pfc_of_table);
+#endif
+
static int sh_pfc_probe(struct platform_device *pdev)
{
+ const struct platform_device_id *platid = platform_get_device_id(pdev);
+#ifdef CONFIG_OF
+ struct device_node *np = pdev->dev.of_node;
+#endif
struct sh_pfc_soc_info *info;
struct sh_pfc *pfc;
int ret;
@@ -509,8 +557,15 @@ static int sh_pfc_probe(struct platform_device *pdev)
*/
BUILD_BUG_ON(PINMUX_FLAG_TYPE > ((1 << PINMUX_FLAG_DBIT_SHIFT) - 1));
- info = pdev->id_entry->driver_data
- ? (void *)pdev->id_entry->driver_data : pdev->dev.platform_data;
+ if (platid)
+ info = (void *)platid->driver_data;
+#ifdef CONFIG_OF
+ else if (np)
+ info = (void *)of_match_device(sh_pfc_of_table, &pdev->dev)->data;
+#endif
+ else
+ info = pdev->dev.platform_data;
+
if (info = NULL)
return -ENODEV;
@@ -640,6 +695,9 @@ static struct platform_driver sh_pfc_driver = {
.driver = {
.name = DRV_NAME,
.owner = THIS_MODULE,
+#ifdef CONFIG_OF
+ .of_match_table = sh_pfc_of_table,
+#endif
},
};
diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index 22df3fe..1431c5e 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -132,6 +132,7 @@ static void sh_pfc_gpio_setup(struct sh_pfc_chip *chip)
WARN_ON(pfc->info->first_gpio != 0); /* needs testing */
gc->label = pfc->info->name;
+ gc->dev = pfc->dev;
gc->owner = THIS_MODULE;
gc->base = pfc->info->first_gpio;
gc->ngpio = (pfc->info->last_gpio - pfc->info->first_gpio) + 1;
--
1.7.8.6
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
2012-11-27 0:03 ` [PATCH v2 71/77] sh-pfc: Add " Laurent Pinchart
@ 2012-12-01 22:55 ` Linus Walleij
2012-12-06 1:34 ` Laurent Pinchart
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2012-12-01 22:55 UTC (permalink / raw)
To: linux-sh
On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> Here's the second version of the SH pin control and GPIO rework patches. I've
> added OF support for PFC instantiation and GPIO mappings that was missing from
> v1. PINCTRL bindings are still missing and will come soon.
So I've tried the only way I could to review this by cloning your tree
and actually inspecting the end result ... overall it's looking very good!
Here are assorted comments:
- Some use of __devicexit/__devinit, this will be deleted in the v3.8
merge window so just remove this everywhere.
- You're using the method to add ranges from the pinctrl side of
things. This is basically deprecated with the changes to gpiolib
I make in this merge window. If you study the way I changed
the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
from being done in the pin controller to being done in the
gpiolib part, you will get the picture. The big upside is that
(A) makes the pin and GPIO references to the local GPIO
chip and pin controller and (B) that this supports adding ranges
from the device tree, which is probably what you want in the
end...
- The above probably means you can get rid of
sh_pfc_register_gpiochip() and decouple the pinctrl
and gpiolib code like I did in my patch set
(commit 8604ac34e in the pinctrl tree) and just
initialize the GPIO with some module_init().
But I might be wrong!
- sh_pfc_register_pinctrl() in pinctrl.c is using kzalloc/kfree,
but since it has a struct sh_pfc, which contains a
struct device *, I suspect you can use devm_kzalloc()
and cut the kfree():s. This probably applies in more
places in the driver.
- core.c contains pfc_ioremap() and pfc_iounmap()
which actually seem to exist much due to the fact
that devm_kzalloc() and devm_ioremap_nocache() did not
exist at the time. By using devm_* helpers and
maybe also inlining the code I think it'll look way
smoother.
- sh_pfc.h contains this:
typedef unsigned short pinmux_enum_t;
typedef unsigned short pinmux_flag_t;
But custom typedefs in the kernel is discouraged unless
there is a good reason for. What about you search/replace
this thing with u16 everywhere. I really like u16...
I can see macros building up some gigantic bit pattern in
these but in the end it's still just 16 unsigned bits.
The name "pinmux_enum_t" is very dangerously
close to the builting "enum" so it scares me a bit
for that reason too.
- sf_pfc.h contains a number of structs which I just
have no chance of understanding unless they are
supplied with something real nice like kerneldoc,
struct pinmux_gpio, pinmux_cfg_reg, pinmux_data_reg,
pinmux_irq, pinmux_range and sh_pfc_soc_info all
need some doc I think, I just don't understand these
structs.
- Same goes for the helper macros.
- In core.h document struct pfc_window, i.e. what are
these windows? What do we mean by a window?
How many of them exist in a certain configuration
typically? etc, stuff you want to know when reading
the code.
- struct sh_pfc is however quite self-explanatory.
- The pfc-* drivers include things like:
#include <cpu/sh7785.h>
#include <mach/r8a7740.h>
#include <mach/irqs.h>
#include <mach/r8a7779.h>
#include <mach/sh73a0.h>
#include <mach/irqs.h>
Why? Especially <mach/irqs.h> would be nice to
get rid of, as it sort of defeats the idea of passing
IRQs as resources or allocating them dynamically.
In some cases it seems these includes are just
surplus, so please look over this...
- This stuff in setup_data_regs():
rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg,
drp->reg_width);
You know, I think shadow registers is just another name
for regmap-mmio. Please consult
drivers/base/regmap/regmap-mmio.c and tell me if I'm
wrong. It's not like I'm going to require you to convert this
to regmap from day 1 if this is legacy stuff but it's probably
the same thing.
- In core.c, gpio_read_raw_reg() and gpio_write_raw_reg()
are looking like marshalling functions to me. This is also
what regmap is about. Marshalling and caching/shadow.
If you keep the functions as they are, atleast rename them
to sh_pfc_* because the gpio_* namespace is for gpiolib.
(There are a few other functions that need to be prefixed
in that file by the way.)
- While keeping Magnus' and Paul's names as authors in the
files it would be proper if you atleast add your own name since
you've probably written most of the code in these files now.
Also for MODULE_AUTHOR().
Let's have this patchset finished for v3.9 say? :-)
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
2012-11-27 0:03 ` [PATCH v2 71/77] sh-pfc: Add " Laurent Pinchart
2012-12-01 22:55 ` [PATCH v2 00/77] SH pin control and GPIO rework with " Linus Walleij
@ 2012-12-06 1:34 ` Laurent Pinchart
2012-12-06 18:52 ` Linus Walleij
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2012-12-06 1:34 UTC (permalink / raw)
To: linux-sh
Hi Linus,
Thank you for the review.
On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > Here's the second version of the SH pin control and GPIO rework patches.
> > I've added OF support for PFC instantiation and GPIO mappings that was
> > missing from v1. PINCTRL bindings are still missing and will come soon.
>
> So I've tried the only way I could to review this by cloning your tree
> and actually inspecting the end result ... overall it's looking very good!
>
> Here are assorted comments:
>
> - Some use of __devicexit/__devinit, this will be deleted in the v3.8
> merge window so just remove this everywhere.
Will fix.
> - You're using the method to add ranges from the pinctrl side of
> things. This is basically deprecated with the changes to gpiolib
> I make in this merge window. If you study the way I changed
> the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
> from being done in the pin controller to being done in the
> gpiolib part, you will get the picture. The big upside is that
> (A) makes the pin and GPIO references to the local GPIO
> chip and pin controller and (B) that this supports adding ranges
> from the device tree, which is probably what you want in the
> end...
OK, I will have a look at the code.
> - The above probably means you can get rid of
> sh_pfc_register_gpiochip() and decouple the pinctrl
> and gpiolib code like I did in my patch set
> (commit 8604ac34e in the pinctrl tree) and just
> initialize the GPIO with some module_init().
> But I might be wrong!
GPIO and pinmuxing are controlled by the same hardware block, with shared
registers (there's one register by pin that controls the direction, pull-
ups/pull-downs and function selection). That's why I've implemented them in a
single driver.
> - sh_pfc_register_pinctrl() in pinctrl.c is using kzalloc/kfree,
> but since it has a struct sh_pfc, which contains a
> struct device *, I suspect you can use devm_kzalloc()
> and cut the kfree():s. This probably applies in more
> places in the driver.
Agreed, I'll fix that.
> - core.c contains pfc_ioremap() and pfc_iounmap()
> which actually seem to exist much due to the fact
> that devm_kzalloc() and devm_ioremap_nocache() did not
> exist at the time. By using devm_* helpers and
> maybe also inlining the code I think it'll look way
> smoother.
Will fix as well.
> - sh_pfc.h contains this:
> typedef unsigned short pinmux_enum_t;
> typedef unsigned short pinmux_flag_t;
> But custom typedefs in the kernel is discouraged unless
> there is a good reason for. What about you search/replace
> this thing with u16 everywhere. I really like u16...
> I can see macros building up some gigantic bit pattern in
> these but in the end it's still just 16 unsigned bits.
> The name "pinmux_enum_t" is very dangerously
> close to the builting "enum" so it scares me a bit
> for that reason too.
It's totally scary, and I want it to go away. It's old code that I still need
to fix, you can expect a v3 with more than 77 patches :-)
> - sf_pfc.h contains a number of structs which I just
> have no chance of understanding unless they are
> supplied with something real nice like kerneldoc,
> struct pinmux_gpio, pinmux_cfg_reg, pinmux_data_reg,
> pinmux_irq, pinmux_range and sh_pfc_soc_info all
> need some doc I think, I just don't understand these
> structs.
>
> - Same goes for the helper macros.
Those structures and macros come from the existing driver (drivers/sh/pfc). I
want to replace them with a cleaner pinctrl-aware implementation, that's still
work in progress that should be in v3.
> - In core.h document struct pfc_window, i.e. what are
> these windows? What do we mean by a window?
> How many of them exist in a certain configuration
> typically? etc, stuff you want to know when reading
> the code.
It's basically an ioremapped region. The driver gets register physical
addresses in the SoC info structures, and walks through the ioremapped regions
when it needs to access the registers to find the ioremapped address.
> - struct sh_pfc is however quite self-explanatory.
>
> - The pfc-* drivers include things like:
> #include <cpu/sh7785.h>
> #include <mach/r8a7740.h>
> #include <mach/irqs.h>
> #include <mach/r8a7779.h>
> #include <mach/sh73a0.h>
> #include <mach/irqs.h>
> Why?
Because the code isn't new but comes from existing mach-level code.
> Especially <mach/irqs.h> would be nice to
> get rid of, as it sort of defeats the idea of passing
> IRQs as resources or allocating them dynamically.
> In some cases it seems these includes are just
> surplus, so please look over this...
I'll have a look at it (maybe for v4 though ;-))
> - This stuff in setup_data_regs():
> rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg,
> drp->reg_width);
> You know, I think shadow registers is just another name
> for regmap-mmio. Please consult
> drivers/base/regmap/regmap-mmio.c and tell me if I'm
> wrong. It's not like I'm going to require you to convert this
> to regmap from day 1 if this is legacy stuff but it's probably
> the same thing.
I'll have a look at it.
> - In core.c, gpio_read_raw_reg() and gpio_write_raw_reg()
> are looking like marshalling functions to me. This is also
> what regmap is about. Marshalling and caching/shadow.
> If you keep the functions as they are, atleast rename them
> to sh_pfc_* because the gpio_* namespace is for gpiolib.
> (There are a few other functions that need to be prefixed
> in that file by the way.)
OK.
> - While keeping Magnus' and Paul's names as authors in the
> files it would be proper if you atleast add your own name since
> you've probably written most of the code in these files now.
> Also for MODULE_AUTHOR().
OK :-)
> Let's have this patchset finished for v3.9 say? :-)
I'll try to.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
` (2 preceding siblings ...)
2012-12-06 1:34 ` Laurent Pinchart
@ 2012-12-06 18:52 ` Linus Walleij
2012-12-07 18:35 ` Laurent Pinchart
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2012-12-06 18:52 UTC (permalink / raw)
To: linux-sh
On Thu, Dec 6, 2012 at 2:34 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> Let's have this patchset finished for v3.9 say? :-)
>
> I'll try to.
Any working condition along the way will basically be fine to merge
from my point of view. Doing so leaves the kernel in a way better
shape than before, and I fully trust you to go in and finalize it, so
I think we can move ahead quite swiftly.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
` (3 preceding siblings ...)
2012-12-06 18:52 ` Linus Walleij
@ 2012-12-07 18:35 ` Laurent Pinchart
2012-12-12 1:43 ` Laurent Pinchart
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2012-12-07 18:35 UTC (permalink / raw)
To: linux-sh
Hi Linus,
On Thursday 06 December 2012 02:34:39 Laurent Pinchart wrote:
> On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> > On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > > Here's the second version of the SH pin control and GPIO rework patches.
> > > I've added OF support for PFC instantiation and GPIO mappings that was
> > > missing from v1. PINCTRL bindings are still missing and will come soon.
> >
> > So I've tried the only way I could to review this by cloning your tree
> > and actually inspecting the end result ... overall it's looking very good!
> >
> > Here are assorted comments:
[snip]
> > - You're using the method to add ranges from the pinctrl side of
> > things. This is basically deprecated with the changes to gpiolib
> > I make in this merge window. If you study the way I changed
> > the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
> > from being done in the pin controller to being done in the
> > gpiolib part, you will get the picture. The big upside is that
> > (A) makes the pin and GPIO references to the local GPIO
> > chip and pin controller and (B) that this supports adding ranges
> > from the device tree, which is probably what you want in the
> > end...
>
> OK, I will have a look at the code.
Do you have a tree with those patches ?
[snip]
> > - This stuff in setup_data_regs():
> > rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg, drp->reg_width);
> >
> > You know, I think shadow registers is just another name
> > for regmap-mmio. Please consult
> > drivers/base/regmap/regmap-mmio.c and tell me if I'm
> > wrong. It's not like I'm going to require you to convert this
> > to regmap from day 1 if this is legacy stuff but it's probably
> > the same thing.
>
> I'll have a look at it.
I've considered regmap but I think it's a bit overkill. True, the reg_shadow
is a different name for regmap-mmio (or rather for a small subset of it), but
I already have a data structure instance for each register due to other
requirements of the driver, so storing the cached value there is pretty much
free.
I might end up reworking the data registers related code in which case I will
try to use regmap.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
` (4 preceding siblings ...)
2012-12-07 18:35 ` Laurent Pinchart
@ 2012-12-12 1:43 ` Laurent Pinchart
2012-12-13 0:55 ` Simon Horman
2012-12-14 15:48 ` Linus Walleij
7 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2012-12-12 1:43 UTC (permalink / raw)
To: linux-sh
Hi Linus,
On Friday 07 December 2012 19:35:33 Laurent Pinchart wrote:
> On Thursday 06 December 2012 02:34:39 Laurent Pinchart wrote:
> > On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> > > On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > > > Here's the second version of the SH pin control and GPIO rework
> > > > patches.
> > > > I've added OF support for PFC instantiation and GPIO mappings that was
> > > > missing from v1. PINCTRL bindings are still missing and will come
> > > > soon.
> > >
> > > So I've tried the only way I could to review this by cloning your tree
> > > and actually inspecting the end result ... overall it's looking very
> > > good!
>
> > > Here are assorted comments:
> [snip]
>
> > > - You're using the method to add ranges from the pinctrl side of
> > > things. This is basically deprecated with the changes to gpiolib
> > > I make in this merge window. If you study the way I changed
> > > the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
> > > from being done in the pin controller to being done in the
> > > gpiolib part, you will get the picture. The big upside is that
> > > (A) makes the pin and GPIO references to the local GPIO
> > > chip and pin controller and (B) that this supports adding ranges
> > > from the device tree, which is probably what you want in the
> > > end...
> >
> > OK, I will have a look at the code.
>
> Do you have a tree with those patches ?
I should have looked myself for the tree before asking, sorry. I'll have a
look at the changes you've added there and will rework the PFC driver
accordingly.
I will send a v3 with fixes based on your comments. I might omit the DT
patches this time and send a pull request, as the patch set is getting too big
for my taste. Even though the result won't be perfect (yet :-)), it's still an
improvement, and I'll send additional patches on top of that.
> > > - This stuff in setup_data_regs():
> > > rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg, drp->reg_width);
> > >
> > > You know, I think shadow registers is just another name for
> > > regmap-mmio. Please consult drivers/base/regmap/regmap-mmio.c and
> > > tell me if I'm wrong. It's not like I'm going to require you to
> > > convert this to regmap from day 1 if this is legacy stuff but it's
> > > probably the same thing.
> >
> > I'll have a look at it.
>
> I've considered regmap but I think it's a bit overkill. True, the reg_shadow
> is a different name for regmap-mmio (or rather for a small subset of it),
> but I already have a data structure instance for each register due to other
> requirements of the driver, so storing the cached value there is pretty
> much free.
>
> I might end up reworking the data registers related code in which case I
> will try to use regmap.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
` (5 preceding siblings ...)
2012-12-12 1:43 ` Laurent Pinchart
@ 2012-12-13 0:55 ` Simon Horman
2012-12-14 15:48 ` Linus Walleij
7 siblings, 0 replies; 9+ messages in thread
From: Simon Horman @ 2012-12-13 0:55 UTC (permalink / raw)
To: linux-sh
On Wed, Dec 12, 2012 at 02:43:24AM +0100, Laurent Pinchart wrote:
> Hi Linus,
>
> On Friday 07 December 2012 19:35:33 Laurent Pinchart wrote:
> > On Thursday 06 December 2012 02:34:39 Laurent Pinchart wrote:
> > > On Saturday 01 December 2012 23:55:35 Linus Walleij wrote:
> > > > On Tue, Nov 27, 2012 at 1:01 AM, Laurent Pinchart wrote:
> > > > > Here's the second version of the SH pin control and GPIO rework
> > > > > patches.
> > > > > I've added OF support for PFC instantiation and GPIO mappings that was
> > > > > missing from v1. PINCTRL bindings are still missing and will come
> > > > > soon.
> > > >
> > > > So I've tried the only way I could to review this by cloning your tree
> > > > and actually inspecting the end result ... overall it's looking very
> > > > good!
> >
> > > > Here are assorted comments:
> > [snip]
> >
> > > > - You're using the method to add ranges from the pinctrl side of
> > > > things. This is basically deprecated with the changes to gpiolib
> > > > I make in this merge window. If you study the way I changed
> > > > the pinctrl-u300.c and pinctrl-coh901.c to switch the registration
> > > > from being done in the pin controller to being done in the
> > > > gpiolib part, you will get the picture. The big upside is that
> > > > (A) makes the pin and GPIO references to the local GPIO
> > > > chip and pin controller and (B) that this supports adding ranges
> > > > from the device tree, which is probably what you want in the
> > > > end...
> > >
> > > OK, I will have a look at the code.
> >
> > Do you have a tree with those patches ?
>
> I should have looked myself for the tree before asking, sorry. I'll have a
> look at the changes you've added there and will rework the PFC driver
> accordingly.
>
> I will send a v3 with fixes based on your comments. I might omit the DT
> patches this time and send a pull request, as the patch set is getting too big
> for my taste. Even though the result won't be perfect (yet :-)), it's still an
> improvement, and I'll send additional patches on top of that.
FWIW, I am quite comfortable with this approach.
>
> > > > - This stuff in setup_data_regs():
> > > > rp->reg_shadow = gpio_read_raw_reg(drp->mapped_reg, drp->reg_width);
> > > >
> > > > You know, I think shadow registers is just another name for
> > > > regmap-mmio. Please consult drivers/base/regmap/regmap-mmio.c and
> > > > tell me if I'm wrong. It's not like I'm going to require you to
> > > > convert this to regmap from day 1 if this is legacy stuff but it's
> > > > probably the same thing.
> > >
> > > I'll have a look at it.
> >
> > I've considered regmap but I think it's a bit overkill. True, the reg_shadow
> > is a different name for regmap-mmio (or rather for a small subset of it),
> > but I already have a data structure instance for each register due to other
> > requirements of the driver, so storing the cached value there is pretty
> > much free.
> >
> > I might end up reworking the data registers related code in which case I
> > will try to use regmap.
>
> --
> Regards,
>
> Laurent Pinchart
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support
2012-11-27 0:01 [PATCH v2 00/77] SH pin control and GPIO rework with OF support Laurent Pinchart
` (6 preceding siblings ...)
2012-12-13 0:55 ` Simon Horman
@ 2012-12-14 15:48 ` Linus Walleij
7 siblings, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2012-12-14 15:48 UTC (permalink / raw)
To: linux-sh
On Wed, Dec 12, 2012 at 2:43 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> I will send a v3 with fixes based on your comments. I might omit the DT
> patches this time and send a pull request, as the patch set is getting too big
> for my taste. Even though the result won't be perfect (yet :-)), it's still an
> improvement, and I'll send additional patches on top of that.
Sure the SH pinctrl business is already looking severa magnitudes better
after this so I will certainly pull it in.
I think I'll create a pinctrl-sh branch based off -rc1 after the merge window,
and we'll work on the sh stuff there for a while.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 9+ messages in thread