On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote:
> On 08-03-17 11:30, Andy Shevchenko wrote:
> > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
> > > On 07-03-17 14:55, Hans de Goede wrote:
> > > I did not get around
> > > to actually test if the fix the silead issue (I believe they will)
> > > as
> > > I
> > > started testing on a cht device and looking if soc_button_array
> > > still
> > > works with your patches applied.
> > >
> > > Unfortunately it no longer works, there are 2 problems:
> > >
> > > 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should
> > > also replace:
> > >
> > > desc = gpiod_get_index(dev, info->name, info->acpi_index,
> > > GPIOD_ASIS);
> > >
> > > with:
> > >
> > > desc = gpiod_get(dev, info->name, GPIOD_ASIS);
> > >
> > > At which point we can also drop the acpi_index field from the
> > > buttoninfo struct
> > > altogether.
> > >
> >
> > I was thinking about passing NULL as connection ID there as it's
> > done
> > for surface3 button array driver. In current soc-button-array we
> > have
> > file name passed for all of the pins, which is slightly informative.
>
> No currently the button name, so "power", "volume-up", "volume-down"
> gets passed.
Because of my WIP patches? They have FIXME: in commit message where I'm
in doubt on the way to go.
> The only place where the file-name used to get passed
> is in the gpiod_count call, but you've replaced it with NULL there.
Yes.
> But passing NULL and removing the need for a mapping table is fine
> with me.
NULL sounds to me a bit clearer in this case, since the original name of
connection IDs with underscores, not dashes.
> > > I think that "extcon: int3496: Add GPIO ACPI mapping table" will
> > > need
> > > a similar change (I haven't tested it yet).
> >
> > The mapping table converts Linux index, which you pass via
> > gpiod_get_index(), and _CRS index pair (resource, index in a list).
> >
> > If it doesn't work that way, there is another bug then.
>
> Hmm, so maybe this:
>
> static const struct acpi_gpio_params power_gpios = { 0, 0, false };
> static const struct acpi_gpio_params home_gpios = { 1, 0, false };
> Needs to be like this then? :
>
> static const struct acpi_gpio_params power_gpios = { 0, 0, false };
> static const struct acpi_gpio_params home_gpios = { 1, 1, false };
Obviously not.
Just really small pseudo ASL to consider:
_CRS:
GpioIo(...) { pin #5 }
GpioIo(...) { pin #3, pin #4, pin #2 }
GpioIo(...) { pin #15 }
In Linux (for example) [index, connection ID]:
index 0 "reset" (pin #2)
index 1 "func1" (pin #4)
index 2 "func2" (pin #3)
index 3 "enable" (pin #5)
index 4 "ready" (pin #15)
Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table):
index 0 pin #2 to 1,2
index 1 pin #4 to 1,1
index 2 pin #3 to 1,0
index 3 pin #5 to 0,0
index 4 pin #15 to 2,0
> In order for gpiod_get_index() to work ? Note that if we are going
> to use the mapping table I believe we really should just use
> gpiod_get (instead of gpiod_get_index) as the map also provides a
> name so the index is not necessary (I've tested that using
> gpiod_get() works fine with your current code).
See above, I think it makes picture clearer to understand the problems
we have.
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
