[PATCH RFC] sh: pfc: Add ability to use separate read & write GPIO data regs

[PATCH RFC] sh: pfc: Add ability to use separate read & write GPIO data regs

[Phil Edworthy]

On several devices, there are separate registers for data input
and data output.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
 drivers/sh/pfc/core.c  |    5 +++--
 include/linux/sh_pfc.h |    8 +++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/sh/pfc/core.c b/drivers/sh/pfc/core.c
index 6816937..173126e 100644
--- a/drivers/sh/pfc/core.c
+++ b/drivers/sh/pfc/core.c
@@ -163,14 +163,14 @@ void sh_pfc_write_bit(struct pinmux_data_reg *dr, unsigned long in_pos,
 
 	pr_debug("write_bit addr = %lx, value = %d, pos = %ld, "
 		 "r_width = %ld\n",
-		 dr->reg, !!value, pos, dr->reg_width);
+		 dr->wreg, !!value, pos, dr->reg_width);
 
 	if (value)
 		set_bit(pos, &dr->reg_shadow);
 	else
 		clear_bit(pos, &dr->reg_shadow);
 
-	gpio_write_raw_reg(dr->mapped_reg, dr->reg_width, dr->reg_shadow);
+	gpio_write_raw_reg(dr->mapped_wreg, dr->reg_width, dr->reg_shadow);
 }
 EXPORT_SYMBOL_GPL(sh_pfc_write_bit);
 
@@ -256,6 +256,7 @@ static int setup_data_reg(struct sh_pfc *pfc, unsigned gpio)
 			break;
 
 		data_reg->mapped_reg = pfc_phys_to_virt(pfc, data_reg->reg);
+		data_reg->mapped_wreg = pfc_phys_to_virt(pfc, data_reg->wreg);
 
 		for (n = 0; n < data_reg->reg_width; n++) {
 			if (data_reg->enum_ids[n] = gpiop->enum_id) {
diff --git a/include/linux/sh_pfc.h b/include/linux/sh_pfc.h
index c19a092..bbf955b 100644
--- a/include/linux/sh_pfc.h
+++ b/include/linux/sh_pfc.h
@@ -68,10 +68,16 @@ struct pinmux_data_reg {
 	unsigned long reg, reg_width, reg_shadow;
 	pinmux_enum_t *enum_ids;
 	void __iomem *mapped_reg;
+	unsigned long wreg;	/* separate write reg */
+	void __iomem *mapped_wreg;
 };
 
 #define PINMUX_DATA_REG(name, r, r_width) \
-	.reg = r, .reg_width = r_width,	\
+	.reg = r, .wreg = r, .reg_width = r_width,	\
+	.enum_ids = (pinmux_enum_t [r_width]) \
+
+#define PINMUX_DATA_REG2(name, r, r2, r_width) \
+	.reg = r, .wreg = r2, .reg_width = r_width,	\
 	.enum_ids = (pinmux_enum_t [r_width]) \
 
 struct pinmux_irq {
-- 
1.7.5.4

Re: [PATCH RFC] sh: pfc: Add ability to use separate read & write GPIO data regs

[Paul Mundt]

On Fri, Feb 08, 2013 at 03:02:51PM +0000, Phil Edworthy wrote:
> On several devices, there are separate registers for data input
> and data output.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Can you provide a bit more information out how these actually look? As
you are reusing the reg_width it seems the write register must be
relatively close to the read register for these parts, in which case you
could simply increase the size of the mapping and provide a write offset
(which would be 0 for parts where read_reg = write_reg).

> +#define PINMUX_DATA_REG2(name, r, r2, r_width) \
> +	.reg = r, .wreg = r2, .reg_width = r_width,	\
>  	.enum_ids = (pinmux_enum_t [r_width]) \
>  
As imaginative as r2 and reg2 are, how about something at least
masquerading as informative? PINMUX_DATA_RWREG() or something perhaps?

Re: [PATCH RFC] sh: pfc: Add ability to use separate read & write GPIO data regs

[phil.edworthy]

Hi Paul,

> On Fri, Feb 08, 2013 at 03:02:51PM +0000, Phil Edworthy wrote:
> > On several devices, there are separate registers for data input
> > and data output.
> > 
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Can you provide a bit more information out how these actually look? As
> you are reusing the reg_width it seems the write register must be
> relatively close to the read register for these parts, in which case you
> could simply increase the size of the mapping and provide a write offset
> (which would be 0 for parts where read_reg = write_reg).

Sure, for r8a7779 the data write reg (OUTDTx) is at an offset of -8 from 
the data read reg (INDTx). I had thought about just using an offset, but 
thought it would be better to have two mappings rather than the code to 
handle negative offsets. I assumed that other devices might have a 
positive offset to OUTDTx.
 
> > +#define PINMUX_DATA_REG2(name, r, r2, r_width) \
> > +   .reg = r, .wreg = r2, .reg_width = r_width,   \
> >     .enum_ids = (pinmux_enum_t [r_width]) \
> > 
> As imaginative as r2 and reg2 are, how about something at least
> masquerading as informative? PINMUX_DATA_RWREG() or something perhaps?
Yeah, I was suffering from a complete lack of imagination at the time! 
Would this be better?

#define PINMUX_DATA_RWREG(name, read_reg, write_reg, r_width) \
   .reg = read_reg, .wreg = write_reg, .reg_width = r_width,   \
   .enum_ids = (pinmux_enum_t [r_width]) \

Thanks
Phil