Hi,
On 08-03-17 11:30, Andy Shevchenko wrote:
On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:
Hi,
On 07-03-17 14:55, Hans de Goede wrote:
Hi,
<more snip>
Thanks for looking into this!
My comments below.
Ok, since it seems clear that I'm not going to be able to change
your
mind on this, I will give your patches a try and see if they fix the
silead ts problems.
So I've cherry picked all the gpio related patches from your
topic/uart/rpm
branch into my wip branch and then ran some tests.
Some of them are WIP, so, they might break something as well.
Understood.
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. 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.
But passing NULL and removing the need for a mapping table is fine with me.
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 };
static const struct acpi_gpio_params volume_up_gpios = { 2, 0, false };
static const struct acpi_gpio_params volume_down_gpios = { 3, 0, false };
static const struct acpi_gpio_params rotation_lock_gpios = { 4, 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 };
static const struct acpi_gpio_params volume_up_gpios = { 2, 2, false };
static const struct acpi_gpio_params volume_down_gpios = { 3, 3, false };
static const struct acpi_gpio_params rotation_lock_gpios = { 4, 4, false };
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).
2) acpi_gpio_count() does not seem to work right in combination with
the
new patches. It returns -ENOENT rather then the number of gpios
specified
in the table passed to devm_acpi_dev_add_driver_gpios. It seems to
only
check for gpios actually in the acpi-properties without looking at
adev->driver_gpios, where as acpi_can_fallback_to_crs() does check for
that and disallows fallback to counting the gpios in the _CRS causing
acpi_gpio_count() to not find any gpios. I believe the right fix for
this is to make acpi_gpio_count() also count the number of entries
in the adev->driver_gpios table.
Thanks for catching this, it sounds indeed as a bug.
For now I've just removed the acpi_gpio_count() check from
soc_button_array,
with that removed and 1) fixed soc_button_array does work.
I will try to do some more testing later today, but all my cht work is
a side project and I first need to finish some stuff for my actual
main $dayjob project.
Understood.
Regards,
Hans
--
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
