Hi,

On 08-03-17 11:30, Andy Shevchenko wrote:

On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote:

quoted

Hi,

On 07-03-17 14:55, Hans de Goede wrote:

quoted

Hi,

<more snip>

Thanks for looking into this!
My comments below.

quoted

quoted

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.

quoted

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.

quoted

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).

quoted

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.

quoted

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