linux-kernel - Re: [PATCH v4 2/4] gpio/xilinx: Convert the driver to platform device interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPybu_3=UiWgsO5gTeZeWEG1w7Qr3ZuU1C6vCw4zv+ysGC+j4Q@mail.gmail.com>
Date:	Wed, 17 Dec 2014 14:31:23 +0100
From:	Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To:	Alexandre Courbot <gnurou@...il.com>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	Michal Simek <michal.simek@...inx.com>,
	Sören Brinkmann <soren.brinkmann@...inx.com>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] gpio/xilinx: Convert the driver to platform device interface

On Wed, Dec 17, 2014 at 2:26 PM, Alexandre Courbot <gnurou@...il.com> wrote:
> On Wed, Dec 17, 2014 at 8:02 PM, Ricardo Ribalda Delgado
> <ricardo.ribalda@...il.com> wrote:
>> Hello Alexandre
>>
>>> This should not be here. The mapping and call to gpiochip_add() are
>>> performed by of_mm_gpiochip_add(). We should thus have a
>>> of_mm_gpiochip_remove() function that undoes what _add did instead of
>>> expected all users to do unmap themselves. Can you add a patch to your
>>> series that adds this function?
>>
>>>
>>> Also I am not sure I understand why the unmapping is done only once.
>>> Both chips are supposed to have been added (and thus mapped) at this
>>> stage. Oh right I see, so this driver ends up mapping the same area
>>> twice! Not only are you iomapping the same area twice, you are
>>> unmapping it only once, and only if the chip is dual. This looks very
>>> broken.
>>>
>>
>> If you look carefully you can see that it is unmapped twice if it is
>> called twice. iounmap is called inside the  for loop.
>
> D'oh, you are right of course. I don't know why, but I thought the
> iounmap() was part of the if (i == 1) conditional block. >_<
>
>>
>>
>>> Couldn't you redesign the driver the following way: only add one chip
>>> (since you have 1 DT node), with an extra member to track which GPIOs
>>> belong to the second chip (in case it is dual), and change the other
>>> functions to handle this.
>>
>> I do not mind rearranging the driver so there is only one gpio device,
>> even for dual chips, but I think this should be done in a separate
>> patch.
>>
>> What about?
>>
>> 1) Keep the current patchset
>>
>> and then
>>
>> 2) Add another patchset with
>>
>>   - xilinx-gpio: only one gpio device
>>   - add  of_mm_gpiochip_remove() to the api
>>   - xilinx gpio: use of_mm_gpiochip_remove
>>   - others: use of_mm_gpiochip_remove
>
> I think this would look better this way:
>
> - xilinx-gpio: remove offset property
> - xilinx-gpio: only one gpio device
> - add  of_mm_gpiochip_remove() to the api
> - xilinx-gpio: use of_mm_gpiochip_remove
> - xilinx-gpio: Convert the driver to platform device interface.
>
> (others: use of_mm_gpiochip_remove would be appreciated of course, but
> I won't ask you to go that far and fix everybody).
>
> The reason for this order is that your current patch would be shorter
> is the driver is turned to add one device only first. It's also
> generally better to work on cleaner code. But to switch the driver to
> single-device, you will first need to remove the offset property
> (IIUC, at least).


I totally see your point but I rather do not touch the first patchset,
two reasons.

One is that other people has already acked it and that it will make my
life easier and probably have it ready before the holidays :)


Anyway I could change your mind to:
 - xilinx-gpio: remove offset property
 - xilinx-gpio: Convert the driver to platform device interface.
 - xilinx-gpio: only one gpio device
 - add  of_mm_gpiochip_remove() to the api
 - xilinx-gpio: use of_mm_gpiochip_remove
?

Regards!



-- 
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/