Re: [PATCH v2 2/4] gpio: add viperboard gpio driver

From: Lars Poeschel
Date: Thu Oct 25 2012 - 06:02:22 EST


On Wednesday 24 October 2012 at 18:31:55, Mark Brown wrote:
> On Wed, Oct 24, 2012 at 09:53:06AM +0200, Linus Walleij wrote:
> > On Tue, Oct 23, 2012 at 5:24 PM, Lars Poeschel <poeschel@xxxxxxxxxxx>
wrote:
> > > Ok, I tried to implement it (at least for the gpioa part of the
> > > driver).
> >
> > > It does not work. I got stuck. There are three problems I could not
solve:
> > So I'm looping in Mark Brown to get some hints whether the following
> > can be resolved easily or needs to be given up on...
>
> Some context would've been enormously useful for working out what the
> discussion is all about... this is *very* verbose and not at all clear.

You can reread our conversation before Linus Walleij brought you in starting
with my first patch submission here:

https://lkml.org/lkml/2012/10/12/218

I'll try to sum up, what happend so far:
I'd submit a driver some strange device (called "viperboard") that besides
some other functionality is a USB interface that has 2x16 GPIO at the other
side. Some USB to GPIO converter.
In my driver I had two 16 bit variables that cache the values of the GPIO and
two 16 bit variables holding the direction of the GPIO pin (input or output).
The idea is that in case a pin is an output and there is a read on that pin,
that I wanted to save the USB ping pong for reading the value of the pin.
Linus Walleij pointed out that there is already a framework for marshalling
and caching register read/writes called regmap.
I gave it a try and used it for my driver but I had problems and this is where
you came in.
As there is no general regmap_bus for reading/writing registers over USB (and
probably will never be). I had to implement it in my driver for the special
case of this device. I also have a regmap_config, that provides a volatile_reg
function. I need this to decide if the pin the value is queried is an output
or an input. In the latter case I obviously can't use the cache, in the former
case I want to use it. Here the first problem arises. In the volatile_reg
function I can not do a read of a regmap register since regmap internally uses
a mutex lock. Thus I have my own cache in a device private variable. This is a
bit strange. To use the cache to have to implement my own cache ;-)
The other problem is, that the volatile_reg function is called with struct
device and I can not container_of up to my device vprbrd_gpio struct, since
the struct regmap seems to be regmap private - not for use in drivers.
The last problem is that I have 16 registers for setting/reading the first 16
pins value. And there is another 16 registers for setting the pins direction
(input/output). Setting the pin to output is an atomic operation of setting
the pins direction AND setting it's value. So if there is a set pin direction
to output operation in my driver I want to update the value of the
corresponding value register, since regmap does not know of this change. But
the regcache_write I would use for this seems also not intended for use by
drivers. It is not exported.
You can have a look at my current development code of the driver in question
in this mail:
https://lkml.org/lkml/2012/10/23/374

> > > Setting the direction to output is an atomic set direction and set
> > > value. The register number is different from setting the value only.
> > > So after a successful call, I want to update the cache of output value
> > > using regcache_write. This is also not publicy available api. The only
> > > thing I could do is invalidate the whole cache using
> > > regcache_mark_dirty. (3rd problem)
> >
> > Same thing.
>
> The above doesn't sound like marking the register as dirty is going to
> be helpful... at most you'd want to discard the cache which is already
> supported per register. Marking the register as dirty would leave the
> existing (presumably wrong if there's a problem) value in place in the
> cache. It's not at all clear to me that a cache is even useful for
> these registers.
>
> > > I attach my current working state here. This is NOT WORKING and only as
> > > reference.
> >
> > So if you want to do this with regmap, I suggest the way forward would
> > be to go solve it at the root by implementing something like
> > drivers/base/regmap/regmap-usb-ctrl.c with your desired semantics
> > and using that.
>
> So the issue is that we're trying to use regmap for some USB device?

This is not an issue, since I can implement my regmap_bus for this inside my
driver.

Thanks for your help!

Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/