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

[Simon Horman <horms@verge.net.au>]

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
>