Re: [PATCH] gpio/serial: revert "linux,first-pin" property handling

[Jan Kiszka <jan.kiszka@xxxxxxxxxxx>]

On 2017-07-10 16:23, Jan Kiszka wrote:
> On 2017-07-10 16:09, Linus Walleij wrote:
>> This is not a legal device tree property, because its binding has
>> not been reviewed and approved, nor does it exist in any device
>> tree binding document.
>>
>> It is further wrong, because it is added to the GPIO offset which
>> is by definition controller-local.

BTW, I don't get this second statement. The parameter passed in is
controlling which pin of the GPIO device can be controlled by the driver
at all. And that's also a very private interface between 8250_exar and
gpio-exar: the former tells the latter which pins are physically
available to drive, because the former operates the others already.

>>
>> Cc: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>> Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> Cc: devicetree@xxxxxxxxxxxxxxx
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> ---
>> This got in because I was stressed and was not paying attention.
>> Sorry about not noticing earlier, but I usually strongly nix any
>> such properties, please discuss this on the device tree list
>> first.
>> ---
>>  drivers/gpio/gpio-exar.c            | 25 +++++++++----------------
>>  drivers/tty/serial/8250/8250_exar.c |  2 --
>>  2 files changed, 9 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
>> index fb8d304cfa17..d02c5260525f 100644
>> --- a/drivers/gpio/gpio-exar.c
>> +++ b/drivers/gpio/gpio-exar.c
>> @@ -31,7 +31,6 @@ struct exar_gpio_chip {
>>  	int index;
>>  	void __iomem *regs;
>>  	char name[20];
>> -	unsigned int first_pin;
>>  };
>>  
>>  static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
>> @@ -53,9 +52,9 @@ static int exar_set_direction(struct gpio_chip *chip, int direction,
>>  			      unsigned int offset)
>>  {
>>  	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
>> -	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
>> +	unsigned int addr = offset / 8 ?
>>  		EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
>> -	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
>> +	unsigned int bit  = offset % 8;
>>  
>>  	exar_update(chip, addr, direction, bit);
>>  	return 0;
>> @@ -76,9 +75,9 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
>>  static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>>  {
>>  	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
>> -	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
>> +	unsigned int addr = offset / 8 ?
>>  		EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
>> -	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
>> +	unsigned int bit  = offset % 8;
>>  
>>  	return !!(exar_get(chip, addr) & BIT(bit));
>>  }
>> @@ -86,9 +85,9 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>>  static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
>>  {
>>  	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
>> -	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
>> +	unsigned int addr = offset / 8 ?
>>  		EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
>> -	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
>> +	unsigned int bit  = offset % 8;
>>  
>>  	return !!(exar_get(chip, addr) & BIT(bit));
>>  }
>> @@ -97,9 +96,9 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
>>  			   int value)
>>  {
>>  	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
>> -	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
>> +	unsigned int addr = offset / 8 ?
>>  		EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
>> -	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
>> +	unsigned int bit  = offset % 8;
>>  
>>  	exar_update(chip, addr, value, bit);
>>  }
>> @@ -120,7 +119,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
>>  {
>>  	struct pci_dev *pcidev = to_pci_dev(pdev->dev.parent);
>>  	struct exar_gpio_chip *exar_gpio;
>> -	u32 first_pin, ngpios;
>> +	u32 ngpios;
>>  	void __iomem *p;
>>  	int index, ret;
>>  
>> @@ -132,11 +131,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
>>  	if (!p)
>>  		return -ENOMEM;
>>  
>> -	ret = device_property_read_u32(&pdev->dev, "linux,first-pin",
>> -				       &first_pin);
>> -	if (ret)
>> -		return ret;
>> -
>>  	ret = device_property_read_u32(&pdev->dev, "ngpios", &ngpios);
>>  	if (ret)
>>  		return ret;
>> @@ -161,7 +155,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
>>  	exar_gpio->gpio_chip.ngpio = ngpios;
>>  	exar_gpio->regs = p;
>>  	exar_gpio->index = index;
>> -	exar_gpio->first_pin = first_pin;
>>  
>>  	ret = devm_gpiochip_add_data(&pdev->dev,
>>  				     &exar_gpio->gpio_chip, exar_gpio);
>> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
>> index b5c98e5bf524..4fb3a4ed4a1f 100644
>> --- a/drivers/tty/serial/8250/8250_exar.c
>> +++ b/drivers/tty/serial/8250/8250_exar.c
>> @@ -261,7 +261,6 @@ __xr17v35x_register_gpio(struct pci_dev *pcidev,
>>  }
>>  
>>  static const struct property_entry exar_gpio_properties[] = {
>> -	PROPERTY_ENTRY_U32("linux,first-pin", 0),
>>  	PROPERTY_ENTRY_U32("ngpios", 16),
>>  	{ }
>>  };
>> @@ -326,7 +325,6 @@ static int iot2040_rs485_config(struct uart_port *port,
>>  }
>>  
>>  static const struct property_entry iot2040_gpio_properties[] = {
>> -	PROPERTY_ENTRY_U32("linux,first-pin", 10),
>>  	PROPERTY_ENTRY_U32("ngpios", 1),
>>  	{ }
>>  };
>>
> 
> Sorry for the mess, I didn't know the proper process in details.
> 
> However, just reverting breaks the IOT2000, need to check if there are
> even physical risks. How can we quickly resolve this to proper mechanism?

Ping, before we half-break things with such a revert.

To summarize what we discussed during the patch development:

This is a private, non-device-tree interface between the creator,
8250_exar, and the GPIO device. The original proposal was to not use
device properties at all, rather some platform-data-like pattern [1].
Then we switched to properties [2], but that implicitly creates an
interfaces in the space that is under device tree control. But, for the
given use case, gpio-exar will never be parametrized via device trees or
ACPI.

So, how to proceed?

Jan

[1]
https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1399507.html
[2]
https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1407975.html

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html