Re: [PATCH] pinctrl: amd: Add support for additional GPIO

["Shah, Nehal-bakulchandra" <Nehal-Bakulchandra.shah@xxxxxxx>]

On 11/30/2016 6:40 PM, Linus Walleij wrote:
> On Tue, Nov 29, 2016 at 7:32 AM, Shah, Nehal-bakulchandra
> <Nehal-Bakulchandra.shah@xxxxxxx> wrote:
> 
>> This patch provides support for additional GPIO devices and also provides IRQ sharing for AMD's GPIO devices and set
>> IRQCHIP_SKIP_SET_WAKE flag.
>>
>> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx>
>> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@xxxxxxx>
> 
> (...)
>>         struct amd_gpio *gpio_dev;
>> +       struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> +
>> +       ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);
> 
> I guess this is the unique hardware instance number?
> 
>>         gpio_dev = devm_kzalloc(&pdev->dev,
>>                                 sizeof(struct amd_gpio), GFP_KERNEL);
>> @@ -763,16 +771,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
>>         gpio_dev->gc.set                        = amd_gpio_set_value;
>>         gpio_dev->gc.set_debounce       = amd_gpio_set_debounce;
>>         gpio_dev->gc.dbg_show           = amd_gpio_dbg_show;
>> +       gpio_dev->gc.base               = gpio_dev->gc.ngpio * hwnum;
>>
>> -       gpio_dev->gc.base                       = 0;
> 
> Please don't do this. Instead set .base to -1 so you get dynamic assignment
> of GPIO numbers.
> 
> Suggestion: maybe you could augment gc.label to reflect instance number
> instead?
> 
> That way the stuff from "lsgpio" (tools/gpio/lsgpio.c) will look nice and
> identifiable.
> 
> Apart from that it looks good.
> 
> Yours,
> Linus Walleij
> 



Hi Linus

Thanks for the review. Will resubmit the patch with making .base to -1.

Regarding suggestion for gc.label will take care when submitting patch for more than one gpio controller.

Thanks 
Nehal
--
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