Re: [PATCH 4/6] OMAP: LDP: Port the display driver to new DSS2

[Igor Grinberg <grinberg@compulab.co.il>]

On 05/17/11 14:56, Tomi Valkeinen wrote:
> On Tue, 2011-05-17 at 14:49 +0300, Tomi Valkeinen wrote:
>> On Mon, 2011-05-16 at 14:13 +0300, Igor Grinberg wrote:
>>> On 05/16/11 11:53, Tomi Valkeinen wrote:
>>>> +static int ldp_twl_gpio_setup(struct device *dev, unsigned gpio, unsigned ngpio)
>>>> +{
>>>> +	int r;
>>>> +
>>>> +	struct gpio gpios[] = {
>>>> +		{gpio + 7 , GPIOF_OUT_INIT_LOW, "LCD ENABLE"},
>>>> +		{gpio + 15, GPIOF_OUT_INIT_LOW, "LCD BACKLIGHT"},
>>>> +	};
>>>> +
>>>> +	r = gpio_request_array(gpios, ARRAY_SIZE(gpios));
>>>> +	if (r)
>>>> +		pr_err("Cannot request LCD GPIOs, error %d\n", r);
>>>> +
>>>> +	ldp_backlight_gpio = gpio + 15;
>>>> +	ldp_lcd_enable_gpio = gpio + 7;
>>> If the gpio_request_array() fails (though it shouldn't),
>>> won't it be right to set both variables to -EINVAL?
>> Ah, yes. I'll fix that.
>>
>> Do you know what will happen if twl_gpio_setup fails? Just an error
>> print, or will TWL driver fail?

I'm currently based on Tony's devel-cleanup branch (which is v2.6.39-rc4-...)
this is what twl4030-gpio.c does:

status = pdata->setup(&pdev->dev,
                pdata->gpio_base, TWL4030_GPIO_MAX);
if (status)
            dev_dbg(&pdev->dev, "setup --> %d\n", status);

so yes it is just an error print.

> And I wonder if gpio_is_valid() works correctly for GPIOs from an gpio
> expander? With a quick search, gpio_is_valid returns true if gpio is
> between [0, 256[, and doesn't care if there are gpio expanders or not...

Right, a sanity check only...
I guess each time 256 is not enough it just gets bumped up...
This can be a problem some day...
May be it should be a part of struct gpio_chip so the responsibility for
gpio validity will be on the driver in charge of that gpio?
But that is totally different discussion.

Regarding:

> We still don't know if it works or not =).

I thought Mayuresh checked this and confirmed, no?



-- 
Regards,
Igor.