Re: [PATCH v2 00/77] SH pin control and GPIO rework with OF support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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