From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Herbrechtsmeier Date: Wed, 25 Jul 2018 21:17:58 +0200 Subject: [U-Boot] [RFC PATCH] gpio: zynq: Setup bank_name to dev->name In-Reply-To: <7c732bfa-ee2c-af2c-1cdc-d155a3b5a186@xilinx.com> References: <3a5a2fbe9d0aad4fdbbbf197c39dc0f973e5045e.1531404282.git.michal.simek@xilinx.com> <589cfccf-dc79-c84f-f147-36a2ac7b4aee@herbrechtsmeier.net> <9ea0f292-f1e4-bc31-b4d4-fa2b68c737e1@herbrechtsmeier.net> <7c732bfa-ee2c-af2c-1cdc-d155a3b5a186@xilinx.com> Message-ID: <6c5495e1-ad56-8c7a-9d99-2855f0a52a46@herbrechtsmeier.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de Hi Michal, Am 25.07.2018 um 08:07 schrieb Michal Simek: > On 24.7.2018 21:39, Stefan Herbrechtsmeier wrote: >> Am 24.07.2018 um 10:37 schrieb Michal Simek: >>> On 23.7.2018 20:29, Stefan Herbrechtsmeier wrote: >>>> Am 23.07.2018 um 11:08 schrieb Michal Simek: >>>>> On 20.7.2018 21:31, Stefan Herbrechtsmeier wrote: >>>>>> Am 12.07.2018 um 16:04 schrieb Michal Simek: >>>>>>> There should be proper bank name setup to distiguish between >>>>>>> different >>>>>>> gpio drivers. Use dev->name for it. >>>>>>> >>>>>>> Signed-off-by: Michal Simek >>>>>>> --- >>>>>>> >>>>>>>     drivers/gpio/zynq_gpio.c | 2 ++ >>>>>>>     1 file changed, 2 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/gpio/zynq_gpio.c b/drivers/gpio/zynq_gpio.c >>>>>>> index 26f69b1a713f..f793ee5754a8 100644 >>>>>>> --- a/drivers/gpio/zynq_gpio.c >>>>>>> +++ b/drivers/gpio/zynq_gpio.c >>>>>>> @@ -337,6 +337,8 @@ static int zynq_gpio_probe(struct udevice *dev) >>>>>>>         struct zynq_gpio_privdata *priv = dev_get_priv(dev); >>>>>>>         struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); >>>>>>>     +    uc_priv->bank_name = dev->name; >>>>>>> + >>>>>>>         if (priv->p_data) >>>>>>>             uc_priv->gpio_count = priv->p_data->ngpio; >>>>>>> >>>>>> Does this not lead to ugly names because the gpio number is append to >>>>>> the bank_name? Have you check the "gpio status -a" output? >>>>> Yes I was checking it. Names are composed together but also just >>>>> numbers >>>>> works as before. >>>>> >>>>> gpio at ff0a00000: input: 0 [ ] >>>>> gpio at ff0a00001: input: 0 [ ] >>>>> gpio at ff0a00002: input: 0 [ ] >>>>> gpio at ff0a00003: input: 0 [ ] >>>>> gpio at ff0a00004: input: 0 [ ] >>>>> gpio at ff0a00005: input: 0 [ ] >>>>> gpio at ff0a00006: input: 0 [ ] >>>>> gpio at ff0a00007: input: 0 [ ] >>>>> gpio at ff0a00008: input: 0 [ ] >>>>> gpio at ff0a00009: input: 0 [ ] >>>> Do you think that this are meaningful names? It isn't possible to >>>> separate the device and pin number as well as it mix hex and decimal >>>> numbers. >>>> >>>>> If you know better way how to setup a bank name please let me know >>>>> but I >>>>> need to distinguish ps gpio from pl one and for pl we need to know the >>>>> address. >>>> I know the use case. >>>> >>>> A lot of drivers use the bank_name from the device tree, some drivers >>>> append an underscore to the bank name and others add the req_seq of the >>>> device to an alphabetic character. >>>> >>>>>> Other drivers use the gpio-bank-name from the device tree. >>>>> I can't see this property inside Linux kernel. If this has been >>>>> reviewed >>>>> by dt guys please let me know. >>>> This property is only used by u-boot. I think it isn't needed by the >>>> Linux kernel. >>> I am happy to use consistent solution but what's that? >> Consistent solution between what? > all drivers. Name should be composed consistently among all drivers. > It means drivers shouldn't add additional "_" in driver code for example. > >>> Mixing name with hex and int is not nice but adding "_" or something >>> else is just a pain in driver code. If this is done in core I am fine >>> with that but adding this code to all drivers don't look like generic >>> solution at all. >> Normally the bank name is an alphabetic character or string. Maybe we >> could add the device name to the gpio_lookup_name function and add an >> additional optional device name parameter to the gpio command. >> >>> Using additional u-boot property is not good too. >>> >>> I have mentioned in "gpio: xilinx: Convert driver to DM" >>> (sha1:10441ec9224d0d269dc512819a32c0785a6338d3) >>> that uc-priv->name is completely unused. Maybe this should be dev->name >>> and bank_name should be really used for banks. >> Isn't the uc-priv->name used for the label of the request? > Last time when I looked at it and I didn't see this name listed anywhere > in output. > > >>> Then in gpio status -a can be >>> >>> Device gpio at a0001000: >>> Bank: >>> ... >>> >>> but not sure how gpio commands will work to address exact pin from >>> prompt. Because this is normally working >>> gpio toggle gpio at a00010001 >> With an optional device name this would be: >> gpio toggle gpio at a0001000 1 >> >> Alternative the gpio command could support the requested labels: >> gpio toggle second-gpio > I am open to see this solution. Feel free to invest your time support > this but I have no time for that. What does this mean? I understand that you don't have the time to develop a new common solution. But the discussion disappoints me. You misuse the bank name in a poor way and decline alternative solutions with additional requirements even if they are already used in u-boot. The name "gpio at a000100011" for pin 11 of the device gpio at a0001000 isn't consistent between the u-boot drivers nor is the name used in Linux. A bank-name from the device tree is used by several u-boot drivers even if it isn't consistent between the drivers. Regards   Stefan