Hello Linus!
Thank you for your review. I will try to address your concerns, as my
time and knowledge permits ;)
On 11/29/2013 02:40 PM, Linus Walleij wrote:
>> + depends on X86
>
> Really? What stops me from soldering this onto one of my ARM or
> MIPS boards?
I suppose nothing, but I don't know. Will remove it.
> ISA-style probing, yeah I had to accept this last time ...
> So no discovery on this system, no ACPI?
The system has ACPI, but GPIOs were not working... I don't know enough
about ACPI, but if someone tells me what to look for I can try to debug it.
>> +/* internal variables */
>> +static struct platform_device *sch311x_gpio_pdev;
>> +static struct platform_device *i2c_gpio_pdev;
>> +
>> +static struct {
>> + unsigned short runtime_reg; /* Runtime Register base address */
>> + spinlock_t lock; /* lock for io operations */
>> +} sch311x_gpio_data;
>
> This is a singleton. What happens the day someone mounts two
> of these chips on a board?
Right, this part comes from the watchdog driver for the same chip
(watchdog/sch311x_wdt.c), so I thought I can do the same. That's not a
good excuse, I know...
> Please devm_kzalloc() a state container
> instead, look in other GPIO drivers for examples of this.
Do you have a good example?
>> +static inline void sch311x_sio_enter(int sio_config_port)
>> +{
>> + outb(0x55, sio_config_port);
>> +}
>
> 0x55?
>
>> +static inline void sch311x_sio_exit(int sio_config_port)
>> +{
>> + outb(0xaa, sio_config_port);
>> +}
>
> 0xaa?
> Please #define thise magic values.
Again, comes from watchdog/sch311x_wdt.c... will #define.
>> +static int sch311x_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> + unsigned char data = inb(sch311x_gpio_data.runtime_reg + GP1);
>> + return ((data >> offset) & 1);
>
> Use this:
>
> #include <linux/bitops.h>
>
> return !!(data & BIT(offset));
Ah, thanks!!!
>> +#if 0
>> + /* configure as "push/pull": output voltage is 3.3V */
>> + outb(SCH311X_GPIO_CONF_OUT, sch311x_gpio_data.runtime_reg +
>> sch311x_gpio_conf[offset]);
>
> No #if 0 code! Delete this.
OK, sorry.
>> +#else
>> + /* configure as "open drain": output voltage is 5V on an unconnected
>> PIN */
>> + outb(SCH311X_GPIO_CONF_OUT | SCH311X_GPIO_CONF_OPEN_DRAIN,
>> + sch311x_gpio_data.runtime_reg + sch311x_gpio_conf[offset]);
>
> So if you really only support push/pull, open drain and maybe even
> open source, then it's OK to use the GPIO subsystem. If you start
> doing more things, this becomes a matter of pin control.
It can just be configured as just push/pull or open drain. Is there any
way this can be configured within the GPIO subsystem?
> At the very least I want the driver split up in two files: one that
> adds the GPIO driver for an arbitrary number of pins, and one
> that adds this boards' configuration. You can put the latter in
> drivers/platform/x86 or wherever, just not in drivers/gpio.
OK, that makes sense.
bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
