Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 29, 2013 at 11:36 AM, Sonic Zhang wrote: > On Thu, Aug 29, 2013 at 4:19 PM, Linus Walleij > wrote: >> So splitting each block into a separate pin control device is definately >> one way to skin the cat. >> >> The ux500 would then have 9 pin controller instances (after a >> big fat refactoring, but whatever) instead of 9 GPIO instances >> and one pinctrl instance referencing them. Also this solves >> the problem of registering GPIO ranges from the wrong end >> of the pin controller. (...) > The pin controller device can be defined as a logic device to cover > many gpio devices, which are mapped into the same pin id space without > collision. All overhead in the soc data file can be removed in this > way. GPIO devices with pin id region collision should be put into > different pin controller devices. This is true for your device and if that works for you then go ahead with this. In the Nomadik case the registers for GPIO and pin control are mingled in the same memory range so things are more complex. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 29, 2013 at 11:31 AM, Sonic Zhang wrote: > On Thu, Aug 29, 2013 at 4:12 PM, Linus Walleij > wrote: >> This is similar to the situation in the pinctrl-nomadik.c driver, >> where the pinctrl portions wait for the GPIO devices to instantiate >> before proceeding to probe "on top" of the GPIO blocks, using >> the latter to get to the registers. >> >> I am not sure we have found the best way to sort out this >> type of system, let's see what we can come up with. > > In the blackfin pinctrol-adi2 driver, I probe all gpio devices > independently after all logic pinctrl devices. When one gpio device is > probed, it can get its pinctrl device name from its platform data and > add its gpio range into the pinctrl device via > gpiochip_add_pin_range(). The gpio device don't need to know anything > else about its pinctrl device. This is ideal in the situation when there is a clear separation between the GPIO and pin control (muxing, biasing) registers. So you're doing the right thing. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 29, 2013 at 11:31 AM, Sonic Zhang sonic@gmail.com wrote: On Thu, Aug 29, 2013 at 4:12 PM, Linus Walleij linus.wall...@linaro.org wrote: This is similar to the situation in the pinctrl-nomadik.c driver, where the pinctrl portions wait for the GPIO devices to instantiate before proceeding to probe on top of the GPIO blocks, using the latter to get to the registers. I am not sure we have found the best way to sort out this type of system, let's see what we can come up with. In the blackfin pinctrol-adi2 driver, I probe all gpio devices independently after all logic pinctrl devices. When one gpio device is probed, it can get its pinctrl device name from its platform data and add its gpio range into the pinctrl device via gpiochip_add_pin_range(). The gpio device don't need to know anything else about its pinctrl device. This is ideal in the situation when there is a clear separation between the GPIO and pin control (muxing, biasing) registers. So you're doing the right thing. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 29, 2013 at 11:36 AM, Sonic Zhang sonic@gmail.com wrote: On Thu, Aug 29, 2013 at 4:19 PM, Linus Walleij linus.wall...@linaro.org wrote: So splitting each block into a separate pin control device is definately one way to skin the cat. The ux500 would then have 9 pin controller instances (after a big fat refactoring, but whatever) instead of 9 GPIO instances and one pinctrl instance referencing them. Also this solves the problem of registering GPIO ranges from the wrong end of the pin controller. (...) The pin controller device can be defined as a logic device to cover many gpio devices, which are mapped into the same pin id space without collision. All overhead in the soc data file can be removed in this way. GPIO devices with pin id region collision should be put into different pin controller devices. This is true for your device and if that works for you then go ahead with this. In the Nomadik case the registers for GPIO and pin control are mingled in the same memory range so things are more complex. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Linus, On Thu, Aug 29, 2013 at 4:19 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 10:48 PM, Stephen Warren > wrote: >> On 08/22/2013 01:07 AM, Sonic Zhang wrote: >>> >>> There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function >>> pinmux_enable_setting() in current pinctrl framework assumes the >>> function mux setting of one peripheral pin group is configured in one >>> pinctrl device. But, the function mux setting of one blackfin >>> peripheral may be done among different GPIO HW blocks. So, I have to >>> separate the pinctrl driver from the GPIO block driver add the ranges >>> of all GPIO blocks into one pinctrl device for Blackfin. >> >> I don't think you need separate device; the pin control mapping table >> entries for a particular state simply needs to include entries for >> multiple pin controllers. > > So splitting each block into a separate pin control device is definately > one way to skin the cat. > > The ux500 would then have 9 pin controller instances (after a > big fat refactoring, but whatever) instead of 9 GPIO instances > and one pinctrl instance referencing them. Also this solves > the problem of registering GPIO ranges from the wrong end > of the pin controller. > > Hm, I should try this and see where it goes... What do you > think about this Sonic? As I discussed with Stephen: To separate the pinctrl_settings of one peripheral to multiple pinctrl devices, multiple pinctrl group arrays and function arrays should be defined in the soc data file. This change increases the code size of the soc data file a lot without get any real benefits. The pin controller device can be defined as a logic device to cover many gpio devices, which are mapped into the same pin id space without collision. All overhead in the soc data file can be removed in this way. GPIO devices with pin id region collision should be put into different pin controller devices. So, I think it is fine to either binding pinctrl device to gpio device or leave it as a logic device. Regards, Sonic -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Linus, On Thu, Aug 29, 2013 at 4:12 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 9:07 AM, Sonic Zhang wrote: > >> There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function >> pinmux_enable_setting() in current pinctrl framework assumes the >> function mux setting of one peripheral pin group is configured in one >> pinctrl device. But, the function mux setting of one blackfin >> peripheral may be done among different GPIO HW blocks. So, I have to >> separate the pinctrl driver from the GPIO block driver add the ranges >> of all GPIO blocks into one pinctrl device for Blackfin. > > This is similar to the situation in the pinctrl-nomadik.c driver, > where the pinctrl portions wait for the GPIO devices to instantiate > before proceeding to probe "on top" of the GPIO blocks, using > the latter to get to the registers. > > I am not sure we have found the best way to sort out this > type of system, let's see what we can come up with. In the blackfin pinctrol-adi2 driver, I probe all gpio devices independently after all logic pinctrl devices. When one gpio device is probed, it can get its pinctrl device name from its platform data and add its gpio range into the pinctrl device via gpiochip_add_pin_range(). The gpio device don't need to know anything else about its pinctrl device. Regards, Sonic > > One way I was contemplating was to have just one fat node > in the device tree containing all the register ranges, and one > fat probe function, so all these ranges associated with a > single struct device, but that does not well work with > clocking and interrupts (the GPIO ranges needed one > clock and interrupt each) so I gave up on that idea. > > My latest idea was to to to break the subsystems apart a > bit by letting GPIO devices probe without the underlying > pin controller being in place, so I queued up the operations > until the pin controller arrived, but unfortunately this creates > other problems :-( > > Still this creates a fuzz when trying to refactor stuff so we > need to find a solution. > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Wed, Aug 28, 2013 at 10:23 PM, Stephen Warren wrote:
> On 08/27/2013 09:56 PM, Sonic Zhang wrote:
>> Hi Stephen,
>>
>> On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren
>> wrote:
>>> On 08/27/2013 03:30 AM, Sonic Zhang wrote:
Hi Stephen,
On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren
wrote:
> On 08/22/2013 01:07 AM, Sonic Zhang wrote:
>> Hi Stephen,
>>
>> On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren
>> wrote:
>>> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
>>>
>> The
>> pinctrl_id field in platform data is to make the driver flexible for
>> future SoCs. If the later case is true, I can just fix the pinctrl
>> device name to "pinctrl-adi2.0".
>
> I was talking about pdata->port_pin_base being passed to
> gpiochip_add_pin_range(), not the device name.
>
>> The GPIO device's HW regsiter base, pin base, pin number and the
>> relationship with the PINT device are defined in the platform data.
>
> Hmmm. I suppose with a platform-data-based driver, there isn't a good
> opportunity to encode which HW the code is running on, and then derive
> those parameters from the SoC type and/or put that information into
> device tree. Perhaps platform data is the only way, although isn't there
> some kind of "device ID -> data" mapping table option, so that probe()
> can be told which SoC is in use based on the device name, and use that
> to look up SoC-specific data?
The soc data driver is use to describe the pin group and function
information of peripherals managed by a pin controller. It is more
traditional and simpler to put the device specific parameters into the
platform data.
>>>
>>> Sure, that's the way things have been done historically. However, if
>>> there's a better way, one may as well use it.
>>>
>
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = "adi-gpio-pint",
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe = adi_gpio_probe,
+ .remove = adi_gpio_remove,
+ .driver = {
+ .name = "adi-gpio",
+ },
+};
>>>
>>> Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
>>> there separate HW blocks?
>>>
>>> If there's one HW block, why not have just one driver?
>>>
>>> If there are separate HW blocks, then having separate GPIO and pinctrl
>>> drivers seems like it would make sense.
>>
>> There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
>> pinmux_enable_setting() in current pinctrl framework assumes the
>> function mux setting of one peripheral pin group is configured in one
>> pinctrl device. But, the function mux setting of one blackfin
>> peripheral may be done among different GPIO HW blocks. So, I have to
>> separate the pinctrl driver from the GPIO block driver add the ranges
>> of all GPIO blocks into one pinctrl device for Blackfin.
>
> I don't think you need separate device; the pin control mapping table
> entries for a particular state simply needs to include entries for
> multiple pin controllers.
Calling pinctrl_select_state() multiple times with different pin
controllers is not an atomic operation. If the second call fails, the
pins requested successfully in the first call won't be freed
automatically.
>>>
>>> Drivers should only call pinctrl_select_state() once. The state that
>>> gets selected can affect multiple pin controllers at once. This should
>>> be an atomic operation as far as the client driver is concerned. If any
>>> of that isn't true, it's a bug in pinctrl.
>>
>> /**
>> * pinctrl_select_state() - select/activate/program a pinctrl state to HW
>> * @p: the pinctrl handle for the device that requests configuration
>> * @state: the state handle to select/activate/program
>> */
>> int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>>
>> If drivers should still call pinctrl_select_state() once in case of
>> multiple pin controllers, the first parameter of
>>
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Linus,
On Thu, Aug 29, 2013 at 4:02 PM, Linus Walleij wrote:
> On Wed, Aug 21, 2013 at 8:30 AM, Sonic Zhang wrote:
>
>> From: Sonic Zhang
>>
>> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
>> processors. It differs a lot from the old one on BF5xx processors. So,
>> create a pinctrl driver under the pinctrl framework.
> (...)
>
> The v3 is a huge improvement! Keep going.
>
> I will not repeat any of Stephens review comments, I just
> saw this:
>
>> +static const unsigned uart1_pins[] = {
>> + GPIO_PH0, GPIO_PH1,
>> +#ifdef CONFIG_BFIN_UART1_CTSRTS
>> + GPIO_PE9, GPIO_PE10,
>> +#endif
> (...)
>> +static const unsigned atapi_pins[] = {
>> + GPIO_PH2, GPIO_PJ3, GPIO_PJ4, GPIO_PJ5, GPIO_PJ6,
>> + GPIO_PJ7, GPIO_PJ8, GPIO_PJ9, GPIO_PJ10,
>> + GPIO_PG5, GPIO_PG6, GPIO_PG7,
>> +#ifdef CONFIG_BF548_ATAPI_ALTERNATIVE_PORT
>> + GPIO_PF0, GPIO_PF1, GPIO_PF2, GPIO_PF3, GPIO_PF4, GPIO_PF5, GPIO_PF6,
>> + GPIO_PF7, GPIO_PF8, GPIO_PF9, GPIO_PF10, GPIO_PF11, GPIO_PF12,
>> + GPIO_PF13, GPIO_PF14, GPIO_PF15, GPIO_PG2, GPIO_PG3, GPIO_PG4,
>> +#endif
>
> (...)
>> +static const struct adi_pin_group adi_pin_groups[] = {
> (...)
>> + ADI_PIN_GROUP("uart1grp", uart1_pins),
> (...)
>> + ADI_PIN_GROUP("atapigrp", atapi_pins),
>> +};
>
> This is not how we do it. Do not use Kconfig to spefify how the SoC
> is utilized. This shall be done at runtime.
>
> What you want to do for UART0 is specify one group for the TX/RX
> pair and another group for the CTS/RTS pair like this:
>
> static const unsigned uart1_rxtx_pins[] = {
>GPIO_PH0, GPIO_PH1,
> };
>
> static const unsigned uart1_rtscts_pins[] = {
>GPIO_PE9, GPIO_PE10,
> };
>
> static const struct adi_pin_group adi_pin_groups[] = {
> (...)
> ADI_PIN_GROUP("uart1txrxgrp", uart1_rxtx_pins),
> ADI_PIN_GROUP("uart1rtsctsgrp", uart1_ctsrts_pins),
>(...)
> };
>
> (And vice versa for the atapi pins, create two groups.)
>
> If you want to use all four pins on UART1 in a system, you
> specify this in you map (in platform data or DTS), so that
> e.g. the state "default" will be activate *both* uart1txrxgrp
> and uart1rtscts groups.
>
> This is typically done with two entries in the map.
OK. I will separate the pins into 2 groups.
Regards,
Sonic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 22, 2013 at 10:48 PM, Stephen Warren wrote: > On 08/22/2013 01:07 AM, Sonic Zhang wrote: >> >> There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function >> pinmux_enable_setting() in current pinctrl framework assumes the >> function mux setting of one peripheral pin group is configured in one >> pinctrl device. But, the function mux setting of one blackfin >> peripheral may be done among different GPIO HW blocks. So, I have to >> separate the pinctrl driver from the GPIO block driver add the ranges >> of all GPIO blocks into one pinctrl device for Blackfin. > > I don't think you need separate device; the pin control mapping table > entries for a particular state simply needs to include entries for > multiple pin controllers. So splitting each block into a separate pin control device is definately one way to skin the cat. The ux500 would then have 9 pin controller instances (after a big fat refactoring, but whatever) instead of 9 GPIO instances and one pinctrl instance referencing them. Also this solves the problem of registering GPIO ranges from the wrong end of the pin controller. Hm, I should try this and see where it goes... What do you think about this Sonic? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 22, 2013 at 9:07 AM, Sonic Zhang wrote: > There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function > pinmux_enable_setting() in current pinctrl framework assumes the > function mux setting of one peripheral pin group is configured in one > pinctrl device. But, the function mux setting of one blackfin > peripheral may be done among different GPIO HW blocks. So, I have to > separate the pinctrl driver from the GPIO block driver add the ranges > of all GPIO blocks into one pinctrl device for Blackfin. This is similar to the situation in the pinctrl-nomadik.c driver, where the pinctrl portions wait for the GPIO devices to instantiate before proceeding to probe "on top" of the GPIO blocks, using the latter to get to the registers. I am not sure we have found the best way to sort out this type of system, let's see what we can come up with. One way I was contemplating was to have just one fat node in the device tree containing all the register ranges, and one fat probe function, so all these ranges associated with a single struct device, but that does not well work with clocking and interrupts (the GPIO ranges needed one clock and interrupt each) so I gave up on that idea. My latest idea was to to to break the subsystems apart a bit by letting GPIO devices probe without the underlying pin controller being in place, so I queued up the operations until the pin controller arrived, but unfortunately this creates other problems :-( Still this creates a fuzz when trying to refactor stuff so we need to find a solution. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 22, 2013 at 9:07 AM, Sonic Zhang wrote: > Is it possible that 2 and more pinctrl devices are on the same SoC? Or > do we always assume there is only one pinctrl device on one SoC? It is possible to have N pinctrl devices on the same SoC, not to mention the whole system. The subsystem does not care, just line them up. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Wed, Aug 21, 2013 at 8:30 AM, Sonic Zhang wrote:
> From: Sonic Zhang
>
> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
> processors. It differs a lot from the old one on BF5xx processors. So,
> create a pinctrl driver under the pinctrl framework.
(...)
The v3 is a huge improvement! Keep going.
I will not repeat any of Stephens review comments, I just
saw this:
> +static const unsigned uart1_pins[] = {
> + GPIO_PH0, GPIO_PH1,
> +#ifdef CONFIG_BFIN_UART1_CTSRTS
> + GPIO_PE9, GPIO_PE10,
> +#endif
(...)
> +static const unsigned atapi_pins[] = {
> + GPIO_PH2, GPIO_PJ3, GPIO_PJ4, GPIO_PJ5, GPIO_PJ6,
> + GPIO_PJ7, GPIO_PJ8, GPIO_PJ9, GPIO_PJ10,
> + GPIO_PG5, GPIO_PG6, GPIO_PG7,
> +#ifdef CONFIG_BF548_ATAPI_ALTERNATIVE_PORT
> + GPIO_PF0, GPIO_PF1, GPIO_PF2, GPIO_PF3, GPIO_PF4, GPIO_PF5, GPIO_PF6,
> + GPIO_PF7, GPIO_PF8, GPIO_PF9, GPIO_PF10, GPIO_PF11, GPIO_PF12,
> + GPIO_PF13, GPIO_PF14, GPIO_PF15, GPIO_PG2, GPIO_PG3, GPIO_PG4,
> +#endif
(...)
> +static const struct adi_pin_group adi_pin_groups[] = {
(...)
> + ADI_PIN_GROUP("uart1grp", uart1_pins),
(...)
> + ADI_PIN_GROUP("atapigrp", atapi_pins),
> +};
This is not how we do it. Do not use Kconfig to spefify how the SoC
is utilized. This shall be done at runtime.
What you want to do for UART0 is specify one group for the TX/RX
pair and another group for the CTS/RTS pair like this:
static const unsigned uart1_rxtx_pins[] = {
GPIO_PH0, GPIO_PH1,
};
static const unsigned uart1_rtscts_pins[] = {
GPIO_PE9, GPIO_PE10,
};
static const struct adi_pin_group adi_pin_groups[] = {
(...)
ADI_PIN_GROUP("uart1txrxgrp", uart1_rxtx_pins),
ADI_PIN_GROUP("uart1rtsctsgrp", uart1_ctsrts_pins),
(...)
};
(And vice versa for the atapi pins, create two groups.)
If you want to use all four pins on UART1 in a system, you
specify this in you map (in platform data or DTS), so that
e.g. the state "default" will be activate *both* uart1txrxgrp
and uart1rtscts groups.
This is typically done with two entries in the map.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Wed, Aug 21, 2013 at 8:30 AM, Sonic Zhang sonic@gmail.com wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
(...)
The v3 is a huge improvement! Keep going.
I will not repeat any of Stephens review comments, I just
saw this:
+static const unsigned uart1_pins[] = {
+ GPIO_PH0, GPIO_PH1,
+#ifdef CONFIG_BFIN_UART1_CTSRTS
+ GPIO_PE9, GPIO_PE10,
+#endif
(...)
+static const unsigned atapi_pins[] = {
+ GPIO_PH2, GPIO_PJ3, GPIO_PJ4, GPIO_PJ5, GPIO_PJ6,
+ GPIO_PJ7, GPIO_PJ8, GPIO_PJ9, GPIO_PJ10,
+ GPIO_PG5, GPIO_PG6, GPIO_PG7,
+#ifdef CONFIG_BF548_ATAPI_ALTERNATIVE_PORT
+ GPIO_PF0, GPIO_PF1, GPIO_PF2, GPIO_PF3, GPIO_PF4, GPIO_PF5, GPIO_PF6,
+ GPIO_PF7, GPIO_PF8, GPIO_PF9, GPIO_PF10, GPIO_PF11, GPIO_PF12,
+ GPIO_PF13, GPIO_PF14, GPIO_PF15, GPIO_PG2, GPIO_PG3, GPIO_PG4,
+#endif
(...)
+static const struct adi_pin_group adi_pin_groups[] = {
(...)
+ ADI_PIN_GROUP(uart1grp, uart1_pins),
(...)
+ ADI_PIN_GROUP(atapigrp, atapi_pins),
+};
This is not how we do it. Do not use Kconfig to spefify how the SoC
is utilized. This shall be done at runtime.
What you want to do for UART0 is specify one group for the TX/RX
pair and another group for the CTS/RTS pair like this:
static const unsigned uart1_rxtx_pins[] = {
GPIO_PH0, GPIO_PH1,
};
static const unsigned uart1_rtscts_pins[] = {
GPIO_PE9, GPIO_PE10,
};
static const struct adi_pin_group adi_pin_groups[] = {
(...)
ADI_PIN_GROUP(uart1txrxgrp, uart1_rxtx_pins),
ADI_PIN_GROUP(uart1rtsctsgrp, uart1_ctsrts_pins),
(...)
};
(And vice versa for the atapi pins, create two groups.)
If you want to use all four pins on UART1 in a system, you
specify this in you map (in platform data or DTS), so that
e.g. the state default will be activate *both* uart1txrxgrp
and uart1rtscts groups.
This is typically done with two entries in the map.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 22, 2013 at 9:07 AM, Sonic Zhang sonic@gmail.com wrote: Is it possible that 2 and more pinctrl devices are on the same SoC? Or do we always assume there is only one pinctrl device on one SoC? It is possible to have N pinctrl devices on the same SoC, not to mention the whole system. The subsystem does not care, just line them up. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 22, 2013 at 9:07 AM, Sonic Zhang sonic@gmail.com wrote: There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function pinmux_enable_setting() in current pinctrl framework assumes the function mux setting of one peripheral pin group is configured in one pinctrl device. But, the function mux setting of one blackfin peripheral may be done among different GPIO HW blocks. So, I have to separate the pinctrl driver from the GPIO block driver add the ranges of all GPIO blocks into one pinctrl device for Blackfin. This is similar to the situation in the pinctrl-nomadik.c driver, where the pinctrl portions wait for the GPIO devices to instantiate before proceeding to probe on top of the GPIO blocks, using the latter to get to the registers. I am not sure we have found the best way to sort out this type of system, let's see what we can come up with. One way I was contemplating was to have just one fat node in the device tree containing all the register ranges, and one fat probe function, so all these ranges associated with a single struct device, but that does not well work with clocking and interrupts (the GPIO ranges needed one clock and interrupt each) so I gave up on that idea. My latest idea was to to to break the subsystems apart a bit by letting GPIO devices probe without the underlying pin controller being in place, so I queued up the operations until the pin controller arrived, but unfortunately this creates other problems :-( Still this creates a fuzz when trying to refactor stuff so we need to find a solution. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On Thu, Aug 22, 2013 at 10:48 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/22/2013 01:07 AM, Sonic Zhang wrote: There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function pinmux_enable_setting() in current pinctrl framework assumes the function mux setting of one peripheral pin group is configured in one pinctrl device. But, the function mux setting of one blackfin peripheral may be done among different GPIO HW blocks. So, I have to separate the pinctrl driver from the GPIO block driver add the ranges of all GPIO blocks into one pinctrl device for Blackfin. I don't think you need separate device; the pin control mapping table entries for a particular state simply needs to include entries for multiple pin controllers. So splitting each block into a separate pin control device is definately one way to skin the cat. The ux500 would then have 9 pin controller instances (after a big fat refactoring, but whatever) instead of 9 GPIO instances and one pinctrl instance referencing them. Also this solves the problem of registering GPIO ranges from the wrong end of the pin controller. Hm, I should try this and see where it goes... What do you think about this Sonic? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Linus,
On Thu, Aug 29, 2013 at 4:02 PM, Linus Walleij linus.wall...@linaro.org wrote:
On Wed, Aug 21, 2013 at 8:30 AM, Sonic Zhang sonic@gmail.com wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
(...)
The v3 is a huge improvement! Keep going.
I will not repeat any of Stephens review comments, I just
saw this:
+static const unsigned uart1_pins[] = {
+ GPIO_PH0, GPIO_PH1,
+#ifdef CONFIG_BFIN_UART1_CTSRTS
+ GPIO_PE9, GPIO_PE10,
+#endif
(...)
+static const unsigned atapi_pins[] = {
+ GPIO_PH2, GPIO_PJ3, GPIO_PJ4, GPIO_PJ5, GPIO_PJ6,
+ GPIO_PJ7, GPIO_PJ8, GPIO_PJ9, GPIO_PJ10,
+ GPIO_PG5, GPIO_PG6, GPIO_PG7,
+#ifdef CONFIG_BF548_ATAPI_ALTERNATIVE_PORT
+ GPIO_PF0, GPIO_PF1, GPIO_PF2, GPIO_PF3, GPIO_PF4, GPIO_PF5, GPIO_PF6,
+ GPIO_PF7, GPIO_PF8, GPIO_PF9, GPIO_PF10, GPIO_PF11, GPIO_PF12,
+ GPIO_PF13, GPIO_PF14, GPIO_PF15, GPIO_PG2, GPIO_PG3, GPIO_PG4,
+#endif
(...)
+static const struct adi_pin_group adi_pin_groups[] = {
(...)
+ ADI_PIN_GROUP(uart1grp, uart1_pins),
(...)
+ ADI_PIN_GROUP(atapigrp, atapi_pins),
+};
This is not how we do it. Do not use Kconfig to spefify how the SoC
is utilized. This shall be done at runtime.
What you want to do for UART0 is specify one group for the TX/RX
pair and another group for the CTS/RTS pair like this:
static const unsigned uart1_rxtx_pins[] = {
GPIO_PH0, GPIO_PH1,
};
static const unsigned uart1_rtscts_pins[] = {
GPIO_PE9, GPIO_PE10,
};
static const struct adi_pin_group adi_pin_groups[] = {
(...)
ADI_PIN_GROUP(uart1txrxgrp, uart1_rxtx_pins),
ADI_PIN_GROUP(uart1rtsctsgrp, uart1_ctsrts_pins),
(...)
};
(And vice versa for the atapi pins, create two groups.)
If you want to use all four pins on UART1 in a system, you
specify this in you map (in platform data or DTS), so that
e.g. the state default will be activate *both* uart1txrxgrp
and uart1rtscts groups.
This is typically done with two entries in the map.
OK. I will separate the pins into 2 groups.
Regards,
Sonic
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Wed, Aug 28, 2013 at 10:23 PM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/27/2013 09:56 PM, Sonic Zhang wrote:
Hi Stephen,
On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/27/2013 03:30 AM, Sonic Zhang wrote:
Hi Stephen,
On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to pinctrl-adi2.0.
I was talking about pdata-port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of device ID - data mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
The soc data driver is use to describe the pin group and function
information of peripherals managed by a pin controller. It is more
traditional and simpler to put the device specific parameters into the
platform data.
Sure, that's the way things have been done historically. However, if
there's a better way, one may as well use it.
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe = adi_gpio_probe,
+ .remove = adi_gpio_remove,
+ .driver = {
+ .name = adi-gpio,
+ },
+};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
pinmux_enable_setting() in current pinctrl framework assumes the
function mux setting of one peripheral pin group is configured in one
pinctrl device. But, the function mux setting of one blackfin
peripheral may be done among different GPIO HW blocks. So, I have to
separate the pinctrl driver from the GPIO block driver add the ranges
of all GPIO blocks into one pinctrl device for Blackfin.
I don't think you need separate device; the pin control mapping table
entries for a particular state simply needs to include entries for
multiple pin controllers.
Calling pinctrl_select_state() multiple times with different pin
controllers is not an atomic operation. If the second call fails, the
pins requested successfully in the first call won't be freed
automatically.
Drivers should only call pinctrl_select_state() once. The state that
gets selected can affect multiple pin controllers at once. This should
be an atomic operation as far as the client driver is concerned. If any
of that isn't true, it's a bug in pinctrl.
/**
* pinctrl_select_state() - select/activate/program a pinctrl state to HW
* @p: the pinctrl handle for the device that requests configuration
* @state: the state handle to select/activate/program
*/
int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
If drivers should still call pinctrl_select_state() once in case of
multiple pin controllers, the first parameter of
pinctrl_select_state() is wrong. Which pinctrl device among all
affected pin controllers should the driver use? Or whatever pinctrl
device?
The function prototype is not wrong. struct pinctrl *p is not a
pinctrl device, but rather it's the result of calling pinctrl_get().
This value encompasses all information required to program all pinctrl
HW devices that need to be programmed.
Thanks to explain. I didn't dig into struct pinctrl much.
Regards,
Sonic
To separate the pinctrl_settings of one peripheral to
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Linus, On Thu, Aug 29, 2013 at 4:12 PM, Linus Walleij linus.wall...@linaro.org wrote: On Thu, Aug 22, 2013 at 9:07 AM, Sonic Zhang sonic@gmail.com wrote: There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function pinmux_enable_setting() in current pinctrl framework assumes the function mux setting of one peripheral pin group is configured in one pinctrl device. But, the function mux setting of one blackfin peripheral may be done among different GPIO HW blocks. So, I have to separate the pinctrl driver from the GPIO block driver add the ranges of all GPIO blocks into one pinctrl device for Blackfin. This is similar to the situation in the pinctrl-nomadik.c driver, where the pinctrl portions wait for the GPIO devices to instantiate before proceeding to probe on top of the GPIO blocks, using the latter to get to the registers. I am not sure we have found the best way to sort out this type of system, let's see what we can come up with. In the blackfin pinctrol-adi2 driver, I probe all gpio devices independently after all logic pinctrl devices. When one gpio device is probed, it can get its pinctrl device name from its platform data and add its gpio range into the pinctrl device via gpiochip_add_pin_range(). The gpio device don't need to know anything else about its pinctrl device. Regards, Sonic One way I was contemplating was to have just one fat node in the device tree containing all the register ranges, and one fat probe function, so all these ranges associated with a single struct device, but that does not well work with clocking and interrupts (the GPIO ranges needed one clock and interrupt each) so I gave up on that idea. My latest idea was to to to break the subsystems apart a bit by letting GPIO devices probe without the underlying pin controller being in place, so I queued up the operations until the pin controller arrived, but unfortunately this creates other problems :-( Still this creates a fuzz when trying to refactor stuff so we need to find a solution. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Linus, On Thu, Aug 29, 2013 at 4:19 PM, Linus Walleij linus.wall...@linaro.org wrote: On Thu, Aug 22, 2013 at 10:48 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/22/2013 01:07 AM, Sonic Zhang wrote: There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function pinmux_enable_setting() in current pinctrl framework assumes the function mux setting of one peripheral pin group is configured in one pinctrl device. But, the function mux setting of one blackfin peripheral may be done among different GPIO HW blocks. So, I have to separate the pinctrl driver from the GPIO block driver add the ranges of all GPIO blocks into one pinctrl device for Blackfin. I don't think you need separate device; the pin control mapping table entries for a particular state simply needs to include entries for multiple pin controllers. So splitting each block into a separate pin control device is definately one way to skin the cat. The ux500 would then have 9 pin controller instances (after a big fat refactoring, but whatever) instead of 9 GPIO instances and one pinctrl instance referencing them. Also this solves the problem of registering GPIO ranges from the wrong end of the pin controller. Hm, I should try this and see where it goes... What do you think about this Sonic? As I discussed with Stephen: To separate the pinctrl_settings of one peripheral to multiple pinctrl devices, multiple pinctrl group arrays and function arrays should be defined in the soc data file. This change increases the code size of the soc data file a lot without get any real benefits. The pin controller device can be defined as a logic device to cover many gpio devices, which are mapped into the same pin id space without collision. All overhead in the soc data file can be removed in this way. GPIO devices with pin id region collision should be put into different pin controller devices. So, I think it is fine to either binding pinctrl device to gpio device or leave it as a logic device. Regards, Sonic -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/27/2013 09:56 PM, Sonic Zhang wrote:
> Hi Stephen,
>
> On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren wrote:
>> On 08/27/2013 03:30 AM, Sonic Zhang wrote:
>>> Hi Stephen,
>>>
>>> On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren
>>> wrote:
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
> Hi Stephen,
>
> On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren
> wrote:
>> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
>>> From: Sonic Zhang
>>>
>>> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
>>> processors. It differs a lot from the old one on BF5xx processors. So,
>>> create a pinctrl driver under the pinctrl framework.
>>
> The
> pinctrl_id field in platform data is to make the driver flexible for
> future SoCs. If the later case is true, I can just fix the pinctrl
> device name to "pinctrl-adi2.0".
I was talking about pdata->port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
> The GPIO device's HW regsiter base, pin base, pin number and the
> relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of "device ID -> data" mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
>>>
>>> The soc data driver is use to describe the pin group and function
>>> information of peripherals managed by a pin controller. It is more
>>> traditional and simpler to put the device specific parameters into the
>>> platform data.
>>
>> Sure, that's the way things have been done historically. However, if
>> there's a better way, one may as well use it.
>>
>>>
>>>
>>> +static struct platform_driver adi_pinctrl_driver = {
>>> + .probe = adi_pinctrl_probe,
>>> + .remove = adi_pinctrl_remove,
>>> + .driver = {
>>> + .name = DRIVER_NAME,
>>> + },
>>> +};
>>> +
>>> +static struct platform_driver adi_gpio_pint_driver = {
>>> + .probe = adi_gpio_pint_probe,
>>> + .remove = adi_gpio_pint_remove,
>>> + .driver = {
>>> + .name = "adi-gpio-pint",
>>> + },
>>> +};
>>> +
>>> +static struct platform_driver adi_gpio_driver = {
>>> + .probe = adi_gpio_probe,
>>> + .remove = adi_gpio_remove,
>>> + .driver = {
>>> + .name = "adi-gpio",
>>> + },
>>> +};
>>
>> Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
>> there separate HW blocks?
>>
>> If there's one HW block, why not have just one driver?
>>
>> If there are separate HW blocks, then having separate GPIO and pinctrl
>> drivers seems like it would make sense.
>
> There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
> pinmux_enable_setting() in current pinctrl framework assumes the
> function mux setting of one peripheral pin group is configured in one
> pinctrl device. But, the function mux setting of one blackfin
> peripheral may be done among different GPIO HW blocks. So, I have to
> separate the pinctrl driver from the GPIO block driver add the ranges
> of all GPIO blocks into one pinctrl device for Blackfin.
I don't think you need separate device; the pin control mapping table
entries for a particular state simply needs to include entries for
multiple pin controllers.
>>>
>>> Calling pinctrl_select_state() multiple times with different pin
>>> controllers is not an atomic operation. If the second call fails, the
>>> pins requested successfully in the first call won't be freed
>>> automatically.
>>
>> Drivers should only call pinctrl_select_state() once. The state that
>> gets selected can affect multiple pin controllers at once. This should
>> be an atomic operation as far as the client driver is concerned. If any
>> of that isn't true, it's a bug in pinctrl.
>
> /**
> * pinctrl_select_state() - select/activate/program a pinctrl state to HW
> * @p: the pinctrl handle for the device that requests configuration
> * @state: the state handle to select/activate/program
> */
> int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>
> If drivers should still call pinctrl_select_state() once in case of
> multiple pin controllers, the first parameter of
> pinctrl_select_state() is wrong. Which pinctrl device among all
> affected pin controllers should the driver use? Or whatever pinctrl
> device?
The function prototype is not wrong. "struct pinctrl
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/27/2013 09:56 PM, Sonic Zhang wrote:
Hi Stephen,
On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/27/2013 03:30 AM, Sonic Zhang wrote:
Hi Stephen,
On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to pinctrl-adi2.0.
I was talking about pdata-port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of device ID - data mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
The soc data driver is use to describe the pin group and function
information of peripherals managed by a pin controller. It is more
traditional and simpler to put the device specific parameters into the
platform data.
Sure, that's the way things have been done historically. However, if
there's a better way, one may as well use it.
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe = adi_gpio_probe,
+ .remove = adi_gpio_remove,
+ .driver = {
+ .name = adi-gpio,
+ },
+};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
pinmux_enable_setting() in current pinctrl framework assumes the
function mux setting of one peripheral pin group is configured in one
pinctrl device. But, the function mux setting of one blackfin
peripheral may be done among different GPIO HW blocks. So, I have to
separate the pinctrl driver from the GPIO block driver add the ranges
of all GPIO blocks into one pinctrl device for Blackfin.
I don't think you need separate device; the pin control mapping table
entries for a particular state simply needs to include entries for
multiple pin controllers.
Calling pinctrl_select_state() multiple times with different pin
controllers is not an atomic operation. If the second call fails, the
pins requested successfully in the first call won't be freed
automatically.
Drivers should only call pinctrl_select_state() once. The state that
gets selected can affect multiple pin controllers at once. This should
be an atomic operation as far as the client driver is concerned. If any
of that isn't true, it's a bug in pinctrl.
/**
* pinctrl_select_state() - select/activate/program a pinctrl state to HW
* @p: the pinctrl handle for the device that requests configuration
* @state: the state handle to select/activate/program
*/
int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
If drivers should still call pinctrl_select_state() once in case of
multiple pin controllers, the first parameter of
pinctrl_select_state() is wrong. Which pinctrl device among all
affected pin controllers should the driver use? Or whatever pinctrl
device?
The function prototype is not wrong. struct pinctrl *p is not a
pinctrl device, but rather it's the result of calling pinctrl_get().
This value encompasses all information required to program all pinctrl
HW devices that need to be programmed.
To separate the pinctrl_settings of one peripheral to multiple pinctrl
devices, multiple pinctrl group arrays and function arrays should be
defined in the soc data file. This change increases the code size of
the soc data file a
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren wrote:
> On 08/27/2013 03:30 AM, Sonic Zhang wrote:
>> Hi Stephen,
>>
>> On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren
>> wrote:
>>> On 08/22/2013 01:07 AM, Sonic Zhang wrote:
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren
wrote:
> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
>> From: Sonic Zhang
>>
>> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
>> processors. It differs a lot from the old one on BF5xx processors. So,
>> create a pinctrl driver under the pinctrl framework.
>
The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to "pinctrl-adi2.0".
>>>
>>> I was talking about pdata->port_pin_base being passed to
>>> gpiochip_add_pin_range(), not the device name.
>>>
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
>>>
>>> Hmmm. I suppose with a platform-data-based driver, there isn't a good
>>> opportunity to encode which HW the code is running on, and then derive
>>> those parameters from the SoC type and/or put that information into
>>> device tree. Perhaps platform data is the only way, although isn't there
>>> some kind of "device ID -> data" mapping table option, so that probe()
>>> can be told which SoC is in use based on the device name, and use that
>>> to look up SoC-specific data?
>>
>> The soc data driver is use to describe the pin group and function
>> information of peripherals managed by a pin controller. It is more
>> traditional and simpler to put the device specific parameters into the
>> platform data.
>
> Sure, that's the way things have been done historically. However, if
> there's a better way, one may as well use it.
>
>>
>>
>>>
>> +static struct platform_driver adi_pinctrl_driver = {
>> + .probe = adi_pinctrl_probe,
>> + .remove = adi_pinctrl_remove,
>> + .driver = {
>> + .name = DRIVER_NAME,
>> + },
>> +};
>> +
>> +static struct platform_driver adi_gpio_pint_driver = {
>> + .probe = adi_gpio_pint_probe,
>> + .remove = adi_gpio_pint_remove,
>> + .driver = {
>> + .name = "adi-gpio-pint",
>> + },
>> +};
>> +
>> +static struct platform_driver adi_gpio_driver = {
>> + .probe = adi_gpio_probe,
>> + .remove = adi_gpio_remove,
>> + .driver = {
>> + .name = "adi-gpio",
>> + },
>> +};
>
> Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
> there separate HW blocks?
>
> If there's one HW block, why not have just one driver?
>
> If there are separate HW blocks, then having separate GPIO and pinctrl
> drivers seems like it would make sense.
There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
pinmux_enable_setting() in current pinctrl framework assumes the
function mux setting of one peripheral pin group is configured in one
pinctrl device. But, the function mux setting of one blackfin
peripheral may be done among different GPIO HW blocks. So, I have to
separate the pinctrl driver from the GPIO block driver add the ranges
of all GPIO blocks into one pinctrl device for Blackfin.
>>>
>>> I don't think you need separate device; the pin control mapping table
>>> entries for a particular state simply needs to include entries for
>>> multiple pin controllers.
>>
>> Calling pinctrl_select_state() multiple times with different pin
>> controllers is not an atomic operation. If the second call fails, the
>> pins requested successfully in the first call won't be freed
>> automatically.
>
> Drivers should only call pinctrl_select_state() once. The state that
> gets selected can affect multiple pin controllers at once. This should
> be an atomic operation as far as the client driver is concerned. If any
> of that isn't true, it's a bug in pinctrl.
/**
* pinctrl_select_state() - select/activate/program a pinctrl state to HW
* @p: the pinctrl handle for the device that requests configuration
* @state: the state handle to select/activate/program
*/
int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
If drivers should still call pinctrl_select_state() once in case of
multiple pin controllers, the first parameter of
pinctrl_select_state() is wrong. Which pinctrl device among all
affected pin controllers should the driver use? Or whatever pinctrl
device?
To separate the pinctrl_settings of one peripheral to multiple pinctrl
devices, multiple pinctrl group arrays and function arrays should be
defined in the soc data file. This change increases the code size of
the soc data file a
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/27/2013 03:30 AM, Sonic Zhang wrote:
> Hi Stephen,
>
> On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren wrote:
>> On 08/22/2013 01:07 AM, Sonic Zhang wrote:
>>> Hi Stephen,
>>>
>>> On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren
>>> wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
> From: Sonic Zhang
>
> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
> processors. It differs a lot from the old one on BF5xx processors. So,
> create a pinctrl driver under the pinctrl framework.
>>> The
>>> pinctrl_id field in platform data is to make the driver flexible for
>>> future SoCs. If the later case is true, I can just fix the pinctrl
>>> device name to "pinctrl-adi2.0".
>>
>> I was talking about pdata->port_pin_base being passed to
>> gpiochip_add_pin_range(), not the device name.
>>
>>> The GPIO device's HW regsiter base, pin base, pin number and the
>>> relationship with the PINT device are defined in the platform data.
>>
>> Hmmm. I suppose with a platform-data-based driver, there isn't a good
>> opportunity to encode which HW the code is running on, and then derive
>> those parameters from the SoC type and/or put that information into
>> device tree. Perhaps platform data is the only way, although isn't there
>> some kind of "device ID -> data" mapping table option, so that probe()
>> can be told which SoC is in use based on the device name, and use that
>> to look up SoC-specific data?
>
> The soc data driver is use to describe the pin group and function
> information of peripherals managed by a pin controller. It is more
> traditional and simpler to put the device specific parameters into the
> platform data.
Sure, that's the way things have been done historically. However, if
there's a better way, one may as well use it.
>
>
>>
> +static struct platform_driver adi_pinctrl_driver = {
> + .probe = adi_pinctrl_probe,
> + .remove = adi_pinctrl_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + },
> +};
> +
> +static struct platform_driver adi_gpio_pint_driver = {
> + .probe = adi_gpio_pint_probe,
> + .remove = adi_gpio_pint_remove,
> + .driver = {
> + .name = "adi-gpio-pint",
> + },
> +};
> +
> +static struct platform_driver adi_gpio_driver = {
> + .probe = adi_gpio_probe,
> + .remove = adi_gpio_remove,
> + .driver = {
> + .name = "adi-gpio",
> + },
> +};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
>>>
>>> There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
>>> pinmux_enable_setting() in current pinctrl framework assumes the
>>> function mux setting of one peripheral pin group is configured in one
>>> pinctrl device. But, the function mux setting of one blackfin
>>> peripheral may be done among different GPIO HW blocks. So, I have to
>>> separate the pinctrl driver from the GPIO block driver add the ranges
>>> of all GPIO blocks into one pinctrl device for Blackfin.
>>
>> I don't think you need separate device; the pin control mapping table
>> entries for a particular state simply needs to include entries for
>> multiple pin controllers.
>
> Calling pinctrl_select_state() multiple times with different pin
> controllers is not an atomic operation. If the second call fails, the
> pins requested successfully in the first call won't be freed
> automatically.
Drivers should only call pinctrl_select_state() once. The state that
gets selected can affect multiple pin controllers at once. This should
be an atomic operation as far as the client driver is concerned. If any
of that isn't true, it's a bug in pinctrl.
> And it is more complicated for peripheral driver to handle pin group
> and function in multiple pin controllers. The peripheral driver has to
> know explicit which pinctrl devices to select other than calling the
> default devm_pinctrl_get_select_default().
No; see above.
> So, I think if multiple gpio devices should be set up together for one
> peripheral, having one pin controller device to handle all of them is
> a more reasonable and clear solution.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren wrote:
> On 08/22/2013 01:07 AM, Sonic Zhang wrote:
>> Hi Stephen,
>>
>> On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren
>> wrote:
>>> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
>>>
drivers/pinctrl/Kconfig| 17 +
drivers/pinctrl/Makefile |3 +
drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
>>>
>>> Those files look reasonable.
>>>
drivers/pinctrl/pinctrl-adi2.c | 1501
drivers/pinctrl/pinctrl-adi2.h | 75 ++
include/linux/platform_data/pinctrl-adi2.h | 40 +
>>>
diff --git a/drivers/pinctrl/pinctrl-adi2.c
b/drivers/pinctrl/pinctrl-adi2.c
>>>
+/**
+ * struct gpio_reserve_map - a GPIO map structure containing the
+ * reservation status of each PIN.
+ *
+ * @owner: who request the reservation
+ * @rsv_gpio: if this pin is reserved as GPIO
+ * @rsv_int: if this pin is reserved as interrupt
+ * @rsv_peri: if this pin is reserved as part of a peripheral device
+ */
+struct gpio_reserve_map {
+ unsigned char owner[RESOURCE_LABEL_SIZE];
+ bool rsv_gpio;
+ bool rsv_int;
+ bool rsv_peri;
+};
>>>
>>> Why is that needed; don't the pinctrl/GPIO cores already know which
>>> pinctrl pins and which GPIOs are used, and for what?
>>
>> The interrupt pin is requested and reserved in irq_chip operation
>> irq_set_type() other than gpio_request(). In Blackfin, one gpio pin is
>> allowed to be set up as both gpio interrupt and gpio input concurrently.
>> So, we need bits to differentiate them.
>
> Does the HW need to be programmed differently in those cases?
>
> If not, I still don't see why the driver cares; if the HW allows both
> usages concurrently, there's nothing to check and hence no need to
> record any state.
>
> If the HW must be programmed differently, can't you read the current
> state out of the HW?
After checking the blackfin HRM again, I think the GPIO interrupt mode
is not mutually exclusive with the peripheral mode as well as the gpio
mode. So, it is not necessary to check the reservation mode of the
requested pin in IRQ chip operation. I will remove the reservation
map.
>
+static int adi_gpio_probe(struct platform_device *pdev)
>>> ...
+ /* Add gpio pin range */
+ snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, "pinctrl-adi2.%d",
+ pdata->pinctrl_id);
+ pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
+ ret = gpiochip_add_pin_range(>chip, pinctrl_devname,
+ 0, pdata->port_pin_base, port->width);
>>>
>>> This looks like platform data is providing the GPIO <-> pinctrl pin ID
>>> mapping, or at least part of it. Surely that mapping is fixed by the HW
>>> design, and hence isn't something platform data should influence. Do the
>>> files pinctrl-adi2-bf*.c not contain complete information about each HW
>>> configuration for some reason?
>>
>> Is it possible that 2 and more pinctrl devices are on the same SoC?
>
> Yes.
>
>> Or do we always assume there is only one pinctrl device on one SoC
>
> No.
>
>> The
>> pinctrl_id field in platform data is to make the driver flexible for
>> future SoCs. If the later case is true, I can just fix the pinctrl
>> device name to "pinctrl-adi2.0".
>
> I was talking about pdata->port_pin_base being passed to
> gpiochip_add_pin_range(), not the device name.
>
>> The GPIO device's HW regsiter base, pin base, pin number and the
>> relationship with the PINT device are defined in the platform data.
>
> Hmmm. I suppose with a platform-data-based driver, there isn't a good
> opportunity to encode which HW the code is running on, and then derive
> those parameters from the SoC type and/or put that information into
> device tree. Perhaps platform data is the only way, although isn't there
> some kind of "device ID -> data" mapping table option, so that probe()
> can be told which SoC is in use based on the device name, and use that
> to look up SoC-specific data?
The soc data driver is use to describe the pin group and function
information of peripherals managed by a pin controller. It is more
traditional and simpler to put the device specific parameters into the
platform data.
>
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
drivers/pinctrl/Kconfig| 17 +
drivers/pinctrl/Makefile |3 +
drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
Those files look reasonable.
drivers/pinctrl/pinctrl-adi2.c | 1501
drivers/pinctrl/pinctrl-adi2.h | 75 ++
include/linux/platform_data/pinctrl-adi2.h | 40 +
diff --git a/drivers/pinctrl/pinctrl-adi2.c
b/drivers/pinctrl/pinctrl-adi2.c
+/**
+ * struct gpio_reserve_map - a GPIO map structure containing the
+ * reservation status of each PIN.
+ *
+ * @owner: who request the reservation
+ * @rsv_gpio: if this pin is reserved as GPIO
+ * @rsv_int: if this pin is reserved as interrupt
+ * @rsv_peri: if this pin is reserved as part of a peripheral device
+ */
+struct gpio_reserve_map {
+ unsigned char owner[RESOURCE_LABEL_SIZE];
+ bool rsv_gpio;
+ bool rsv_int;
+ bool rsv_peri;
+};
Why is that needed; don't the pinctrl/GPIO cores already know which
pinctrl pins and which GPIOs are used, and for what?
The interrupt pin is requested and reserved in irq_chip operation
irq_set_type() other than gpio_request(). In Blackfin, one gpio pin is
allowed to be set up as both gpio interrupt and gpio input concurrently.
So, we need bits to differentiate them.
Does the HW need to be programmed differently in those cases?
If not, I still don't see why the driver cares; if the HW allows both
usages concurrently, there's nothing to check and hence no need to
record any state.
If the HW must be programmed differently, can't you read the current
state out of the HW?
After checking the blackfin HRM again, I think the GPIO interrupt mode
is not mutually exclusive with the peripheral mode as well as the gpio
mode. So, it is not necessary to check the reservation mode of the
requested pin in IRQ chip operation. I will remove the reservation
map.
+static int adi_gpio_probe(struct platform_device *pdev)
...
+ /* Add gpio pin range */
+ snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, pinctrl-adi2.%d,
+ pdata-pinctrl_id);
+ pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
+ ret = gpiochip_add_pin_range(port-chip, pinctrl_devname,
+ 0, pdata-port_pin_base, port-width);
This looks like platform data is providing the GPIO - pinctrl pin ID
mapping, or at least part of it. Surely that mapping is fixed by the HW
design, and hence isn't something platform data should influence. Do the
files pinctrl-adi2-bf*.c not contain complete information about each HW
configuration for some reason?
Is it possible that 2 and more pinctrl devices are on the same SoC?
Yes.
Or do we always assume there is only one pinctrl device on one SoC
No.
The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to pinctrl-adi2.0.
I was talking about pdata-port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of device ID - data mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
The soc data driver is use to describe the pin group and function
information of peripherals managed by a pin controller. It is more
traditional and simpler to put the device specific parameters into the
platform data.
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe =
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/27/2013 03:30 AM, Sonic Zhang wrote:
Hi Stephen,
On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to pinctrl-adi2.0.
I was talking about pdata-port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of device ID - data mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
The soc data driver is use to describe the pin group and function
information of peripherals managed by a pin controller. It is more
traditional and simpler to put the device specific parameters into the
platform data.
Sure, that's the way things have been done historically. However, if
there's a better way, one may as well use it.
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe = adi_gpio_probe,
+ .remove = adi_gpio_remove,
+ .driver = {
+ .name = adi-gpio,
+ },
+};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
pinmux_enable_setting() in current pinctrl framework assumes the
function mux setting of one peripheral pin group is configured in one
pinctrl device. But, the function mux setting of one blackfin
peripheral may be done among different GPIO HW blocks. So, I have to
separate the pinctrl driver from the GPIO block driver add the ranges
of all GPIO blocks into one pinctrl device for Blackfin.
I don't think you need separate device; the pin control mapping table
entries for a particular state simply needs to include entries for
multiple pin controllers.
Calling pinctrl_select_state() multiple times with different pin
controllers is not an atomic operation. If the second call fails, the
pins requested successfully in the first call won't be freed
automatically.
Drivers should only call pinctrl_select_state() once. The state that
gets selected can affect multiple pin controllers at once. This should
be an atomic operation as far as the client driver is concerned. If any
of that isn't true, it's a bug in pinctrl.
And it is more complicated for peripheral driver to handle pin group
and function in multiple pin controllers. The peripheral driver has to
know explicit which pinctrl devices to select other than calling the
default devm_pinctrl_get_select_default().
No; see above.
So, I think if multiple gpio devices should be set up together for one
peripheral, having one pin controller device to handle all of them is
a more reasonable and clear solution.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Wed, Aug 28, 2013 at 5:39 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/27/2013 03:30 AM, Sonic Zhang wrote:
Hi Stephen,
On Fri, Aug 23, 2013 at 4:48 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to pinctrl-adi2.0.
I was talking about pdata-port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of device ID - data mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
The soc data driver is use to describe the pin group and function
information of peripherals managed by a pin controller. It is more
traditional and simpler to put the device specific parameters into the
platform data.
Sure, that's the way things have been done historically. However, if
there's a better way, one may as well use it.
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe = adi_gpio_probe,
+ .remove = adi_gpio_remove,
+ .driver = {
+ .name = adi-gpio,
+ },
+};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
pinmux_enable_setting() in current pinctrl framework assumes the
function mux setting of one peripheral pin group is configured in one
pinctrl device. But, the function mux setting of one blackfin
peripheral may be done among different GPIO HW blocks. So, I have to
separate the pinctrl driver from the GPIO block driver add the ranges
of all GPIO blocks into one pinctrl device for Blackfin.
I don't think you need separate device; the pin control mapping table
entries for a particular state simply needs to include entries for
multiple pin controllers.
Calling pinctrl_select_state() multiple times with different pin
controllers is not an atomic operation. If the second call fails, the
pins requested successfully in the first call won't be freed
automatically.
Drivers should only call pinctrl_select_state() once. The state that
gets selected can affect multiple pin controllers at once. This should
be an atomic operation as far as the client driver is concerned. If any
of that isn't true, it's a bug in pinctrl.
/**
* pinctrl_select_state() - select/activate/program a pinctrl state to HW
* @p: the pinctrl handle for the device that requests configuration
* @state: the state handle to select/activate/program
*/
int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
If drivers should still call pinctrl_select_state() once in case of
multiple pin controllers, the first parameter of
pinctrl_select_state() is wrong. Which pinctrl device among all
affected pin controllers should the driver use? Or whatever pinctrl
device?
To separate the pinctrl_settings of one peripheral to multiple pinctrl
devices, multiple pinctrl group arrays and function arrays should be
defined in the soc data file. This change increases the code size of
the soc data file a lot without get any real benefits. The pin
controller device can be defined as a logic device to cover all gpio
devices, which are mapped into one peripheral pin id space without
collision. All overhead in the soc data file can be removed in this
way.
GPIO devices with peripheral pin id collision have to
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
> Hi Stephen,
>
> On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren wrote:
>> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
>>> From: Sonic Zhang
>>>
>>> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
>>> processors. It differs a lot from the old one on BF5xx processors. So,
>>> create a pinctrl driver under the pinctrl framework.
>>
>>> drivers/pinctrl/Kconfig| 17 +
>>> drivers/pinctrl/Makefile |3 +
>>> drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
>>> drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
>>
>> Those files look reasonable.
>>
>>> drivers/pinctrl/pinctrl-adi2.c | 1501
>>>
>>> drivers/pinctrl/pinctrl-adi2.h | 75 ++
>>> include/linux/platform_data/pinctrl-adi2.h | 40 +
>>
>>> diff --git a/drivers/pinctrl/pinctrl-adi2.c b/drivers/pinctrl/pinctrl-adi2.c
>>
>>> +/**
>>> + * struct gpio_reserve_map - a GPIO map structure containing the
>>> + * reservation status of each PIN.
>>> + *
>>> + * @owner: who request the reservation
>>> + * @rsv_gpio: if this pin is reserved as GPIO
>>> + * @rsv_int: if this pin is reserved as interrupt
>>> + * @rsv_peri: if this pin is reserved as part of a peripheral device
>>> + */
>>> +struct gpio_reserve_map {
>>> + unsigned char owner[RESOURCE_LABEL_SIZE];
>>> + bool rsv_gpio;
>>> + bool rsv_int;
>>> + bool rsv_peri;
>>> +};
>>
>> Why is that needed; don't the pinctrl/GPIO cores already know which
>> pinctrl pins and which GPIOs are used, and for what?
>
> The interrupt pin is requested and reserved in irq_chip operation
> irq_set_type() other than gpio_request(). In Blackfin, one gpio pin is
> allowed to be set up as both gpio interrupt and gpio input concurrently.
> So, we need bits to differentiate them.
Does the HW need to be programmed differently in those cases?
If not, I still don't see why the driver cares; if the HW allows both
usages concurrently, there's nothing to check and hence no need to
record any state.
If the HW must be programmed differently, can't you read the current
state out of the HW?
>>> +static int adi_gpio_probe(struct platform_device *pdev)
>> ...
>>> + /* Add gpio pin range */
>>> + snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, "pinctrl-adi2.%d",
>>> + pdata->pinctrl_id);
>>> + pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
>>> + ret = gpiochip_add_pin_range(>chip, pinctrl_devname,
>>> + 0, pdata->port_pin_base, port->width);
>>
>> This looks like platform data is providing the GPIO <-> pinctrl pin ID
>> mapping, or at least part of it. Surely that mapping is fixed by the HW
>> design, and hence isn't something platform data should influence. Do the
>> files pinctrl-adi2-bf*.c not contain complete information about each HW
>> configuration for some reason?
>
> Is it possible that 2 and more pinctrl devices are on the same SoC?
Yes.
> Or do we always assume there is only one pinctrl device on one SoC
No.
> The
> pinctrl_id field in platform data is to make the driver flexible for
> future SoCs. If the later case is true, I can just fix the pinctrl
> device name to "pinctrl-adi2.0".
I was talking about pdata->port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
> The GPIO device's HW regsiter base, pin base, pin number and the
> relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of "device ID -> data" mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
>>> +static struct platform_driver adi_pinctrl_driver = {
>>> + .probe = adi_pinctrl_probe,
>>> + .remove = adi_pinctrl_remove,
>>> + .driver = {
>>> + .name = DRIVER_NAME,
>>> + },
>>> +};
>>> +
>>> +static struct platform_driver adi_gpio_pint_driver = {
>>> + .probe = adi_gpio_pint_probe,
>>> + .remove = adi_gpio_pint_remove,
>>> + .driver = {
>>> + .name = "adi-gpio-pint",
>>> + },
>>> +};
>>> +
>>> +static struct platform_driver adi_gpio_driver = {
>>> + .probe = adi_gpio_probe,
>>> + .remove = adi_gpio_remove,
>>> + .driver = {
>>> + .name = "adi-gpio",
>>> + },
>>> +};
>>
>> Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
>> there separate HW blocks?
>>
>> If there's one HW block, why not have just one driver?
>>
>> If there are separate HW blocks, then having separate GPIO and pinctrl
>> drivers seems like it would make sense.
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren wrote:
> On 08/21/2013 12:30 AM, Sonic Zhang wrote:
>> From: Sonic Zhang
>>
>> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
>> processors. It differs a lot from the old one on BF5xx processors. So,
>> create a pinctrl driver under the pinctrl framework.
>
>> drivers/pinctrl/Kconfig| 17 +
>> drivers/pinctrl/Makefile |3 +
>> drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
>> drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
>
> Those files look reasonable.
>
>> drivers/pinctrl/pinctrl-adi2.c | 1501
>>
>> drivers/pinctrl/pinctrl-adi2.h | 75 ++
>> include/linux/platform_data/pinctrl-adi2.h | 40 +
>
>> diff --git a/drivers/pinctrl/pinctrl-adi2.c b/drivers/pinctrl/pinctrl-adi2.c
>
>> +/**
>> + * struct gpio_reserve_map - a GPIO map structure containing the
>> + * reservation status of each PIN.
>> + *
>> + * @owner: who request the reservation
>> + * @rsv_gpio: if this pin is reserved as GPIO
>> + * @rsv_int: if this pin is reserved as interrupt
>> + * @rsv_peri: if this pin is reserved as part of a peripheral device
>> + */
>> +struct gpio_reserve_map {
>> + unsigned char owner[RESOURCE_LABEL_SIZE];
>> + bool rsv_gpio;
>> + bool rsv_int;
>> + bool rsv_peri;
>> +};
>
> Why is that needed; don't the pinctrl/GPIO cores already know which
> pinctrl pins and which GPIOs are used, and for what?
The interrupt pin is requested and reserved in irq_chip operation
irq_set_type() other than gpio_request(). In Blackfin, one gpio pin is
allowed to be set up as both gpio interrupt and gpio input concurrently.
So, we need bits to differentiate them.
>
>> +#if defined(CONFIG_DEBUG_FS)
>> +static inline unsigned short get_gpio_dir(struct gpio_port *port,
>> ...
>
> Why aren't the existing GPIO/pinctrl subsystem debugfs files enough?
>
Yes, I do use the existing subsystem debugfs file.
port->chip.dbg_show = adi_gpio_dbg_show;
root:/> cat /sys/kernel/debug/gpio
GPIOs 0-15, adi-gpio:
GPIO_0: physmap-flash.0 Peripheral
GPIO_1: physmap-flash.0 Peripheral
GPIO_2: physmap-flash.0 Peripheral
GPIO_3: physmap-flash.0 Peripheral
GPIO_4: physmap-flash.0 Peripheral
..
>> +static int adi_pinmux_request(struct pinctrl_dev *pctldev, unsigned pin)
> ...
>> + /* If a pin can be muxed as either GPIO or peripheral, make
>> + * sure it is not already a GPIO pin when we request it.
>> + */
>> + if (port->rsvmap[offset].rsv_gpio) {
>> + if (system_state == SYSTEM_BOOTING)
>> + dump_stack();
>> + dev_err(pctldev->dev,
>> +"%s: Peripheral PIN %d is already reserved as GPIO by
>> %s!\n",
>> +__func__, pin, get_label(port, offset));
>> + spin_unlock_irqrestore(>lock, flags);
>> + return -EBUSY;
>> + }
>
> Yes, this definitely warrants some more explanation. It looks odd. What
> is "system_state"?
>
The system_state checking, which is from the legacy bf5xx gpio driver,
is to warn if any peripheral pin is requested before the kernel_init
is executed. It seems this is not necessary under the pinctrl
framework. I will remove all these checking.
>> +static int adi_gpio_probe(struct platform_device *pdev)
> ...
>> + /* Add gpio pin range */
>> + snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, "pinctrl-adi2.%d",
>> + pdata->pinctrl_id);
>> + pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
>> + ret = gpiochip_add_pin_range(>chip, pinctrl_devname,
>> + 0, pdata->port_pin_base, port->width);
>
> This looks like platform data is providing the GPIO <-> pinctrl pin ID
> mapping, or at least part of it. Surely that mapping is fixed by the HW
> design, and hence isn't something platform data should influence. Do the
> files pinctrl-adi2-bf*.c not contain complete information about each HW
> configuration for some reason?
Is it possible that 2 and more pinctrl devices are on the same SoC? Or
do we always assume there is only one pinctrl device on one SoC? The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to "pinctrl-adi2.0".
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
>
>> +static struct platform_driver adi_pinctrl_driver = {
>> + .probe = adi_pinctrl_probe,
>> + .remove = adi_pinctrl_remove,
>> + .driver = {
>> + .name = DRIVER_NAME,
>> + },
>> +};
>> +
>> +static struct platform_driver adi_gpio_pint_driver = {
>> + .probe = adi_gpio_pint_probe,
>> + .remove
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
drivers/pinctrl/Kconfig| 17 +
drivers/pinctrl/Makefile |3 +
drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
Those files look reasonable.
drivers/pinctrl/pinctrl-adi2.c | 1501
drivers/pinctrl/pinctrl-adi2.h | 75 ++
include/linux/platform_data/pinctrl-adi2.h | 40 +
diff --git a/drivers/pinctrl/pinctrl-adi2.c b/drivers/pinctrl/pinctrl-adi2.c
+/**
+ * struct gpio_reserve_map - a GPIO map structure containing the
+ * reservation status of each PIN.
+ *
+ * @owner: who request the reservation
+ * @rsv_gpio: if this pin is reserved as GPIO
+ * @rsv_int: if this pin is reserved as interrupt
+ * @rsv_peri: if this pin is reserved as part of a peripheral device
+ */
+struct gpio_reserve_map {
+ unsigned char owner[RESOURCE_LABEL_SIZE];
+ bool rsv_gpio;
+ bool rsv_int;
+ bool rsv_peri;
+};
Why is that needed; don't the pinctrl/GPIO cores already know which
pinctrl pins and which GPIOs are used, and for what?
The interrupt pin is requested and reserved in irq_chip operation
irq_set_type() other than gpio_request(). In Blackfin, one gpio pin is
allowed to be set up as both gpio interrupt and gpio input concurrently.
So, we need bits to differentiate them.
+#if defined(CONFIG_DEBUG_FS)
+static inline unsigned short get_gpio_dir(struct gpio_port *port,
...
Why aren't the existing GPIO/pinctrl subsystem debugfs files enough?
Yes, I do use the existing subsystem debugfs file.
port-chip.dbg_show = adi_gpio_dbg_show;
root:/ cat /sys/kernel/debug/gpio
GPIOs 0-15, adi-gpio:
GPIO_0: physmap-flash.0 Peripheral
GPIO_1: physmap-flash.0 Peripheral
GPIO_2: physmap-flash.0 Peripheral
GPIO_3: physmap-flash.0 Peripheral
GPIO_4: physmap-flash.0 Peripheral
..
+static int adi_pinmux_request(struct pinctrl_dev *pctldev, unsigned pin)
...
+ /* If a pin can be muxed as either GPIO or peripheral, make
+ * sure it is not already a GPIO pin when we request it.
+ */
+ if (port-rsvmap[offset].rsv_gpio) {
+ if (system_state == SYSTEM_BOOTING)
+ dump_stack();
+ dev_err(pctldev-dev,
+%s: Peripheral PIN %d is already reserved as GPIO by
%s!\n,
+__func__, pin, get_label(port, offset));
+ spin_unlock_irqrestore(port-lock, flags);
+ return -EBUSY;
+ }
Yes, this definitely warrants some more explanation. It looks odd. What
is system_state?
The system_state checking, which is from the legacy bf5xx gpio driver,
is to warn if any peripheral pin is requested before the kernel_init
is executed. It seems this is not necessary under the pinctrl
framework. I will remove all these checking.
+static int adi_gpio_probe(struct platform_device *pdev)
...
+ /* Add gpio pin range */
+ snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, pinctrl-adi2.%d,
+ pdata-pinctrl_id);
+ pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
+ ret = gpiochip_add_pin_range(port-chip, pinctrl_devname,
+ 0, pdata-port_pin_base, port-width);
This looks like platform data is providing the GPIO - pinctrl pin ID
mapping, or at least part of it. Surely that mapping is fixed by the HW
design, and hence isn't something platform data should influence. Do the
files pinctrl-adi2-bf*.c not contain complete information about each HW
configuration for some reason?
Is it possible that 2 and more pinctrl devices are on the same SoC? Or
do we always assume there is only one pinctrl device on one SoC? The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to pinctrl-adi2.0.
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/22/2013 01:07 AM, Sonic Zhang wrote:
Hi Stephen,
On Thu, Aug 22, 2013 at 2:45 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
drivers/pinctrl/Kconfig| 17 +
drivers/pinctrl/Makefile |3 +
drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
Those files look reasonable.
drivers/pinctrl/pinctrl-adi2.c | 1501
drivers/pinctrl/pinctrl-adi2.h | 75 ++
include/linux/platform_data/pinctrl-adi2.h | 40 +
diff --git a/drivers/pinctrl/pinctrl-adi2.c b/drivers/pinctrl/pinctrl-adi2.c
+/**
+ * struct gpio_reserve_map - a GPIO map structure containing the
+ * reservation status of each PIN.
+ *
+ * @owner: who request the reservation
+ * @rsv_gpio: if this pin is reserved as GPIO
+ * @rsv_int: if this pin is reserved as interrupt
+ * @rsv_peri: if this pin is reserved as part of a peripheral device
+ */
+struct gpio_reserve_map {
+ unsigned char owner[RESOURCE_LABEL_SIZE];
+ bool rsv_gpio;
+ bool rsv_int;
+ bool rsv_peri;
+};
Why is that needed; don't the pinctrl/GPIO cores already know which
pinctrl pins and which GPIOs are used, and for what?
The interrupt pin is requested and reserved in irq_chip operation
irq_set_type() other than gpio_request(). In Blackfin, one gpio pin is
allowed to be set up as both gpio interrupt and gpio input concurrently.
So, we need bits to differentiate them.
Does the HW need to be programmed differently in those cases?
If not, I still don't see why the driver cares; if the HW allows both
usages concurrently, there's nothing to check and hence no need to
record any state.
If the HW must be programmed differently, can't you read the current
state out of the HW?
+static int adi_gpio_probe(struct platform_device *pdev)
...
+ /* Add gpio pin range */
+ snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, pinctrl-adi2.%d,
+ pdata-pinctrl_id);
+ pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
+ ret = gpiochip_add_pin_range(port-chip, pinctrl_devname,
+ 0, pdata-port_pin_base, port-width);
This looks like platform data is providing the GPIO - pinctrl pin ID
mapping, or at least part of it. Surely that mapping is fixed by the HW
design, and hence isn't something platform data should influence. Do the
files pinctrl-adi2-bf*.c not contain complete information about each HW
configuration for some reason?
Is it possible that 2 and more pinctrl devices are on the same SoC?
Yes.
Or do we always assume there is only one pinctrl device on one SoC
No.
The
pinctrl_id field in platform data is to make the driver flexible for
future SoCs. If the later case is true, I can just fix the pinctrl
device name to pinctrl-adi2.0.
I was talking about pdata-port_pin_base being passed to
gpiochip_add_pin_range(), not the device name.
The GPIO device's HW regsiter base, pin base, pin number and the
relationship with the PINT device are defined in the platform data.
Hmmm. I suppose with a platform-data-based driver, there isn't a good
opportunity to encode which HW the code is running on, and then derive
those parameters from the SoC type and/or put that information into
device tree. Perhaps platform data is the only way, although isn't there
some kind of device ID - data mapping table option, so that probe()
can be told which SoC is in use based on the device name, and use that
to look up SoC-specific data?
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe = adi_gpio_probe,
+ .remove = adi_gpio_remove,
+ .driver = {
+ .name = adi-gpio,
+ },
+};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
There are 6 to 9 GPIO HW blocks in one Blackfin SoC. Function
pinmux_enable_setting() in current pinctrl framework assumes the
function mux setting of one peripheral pin group is configured in one
pinctrl
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
> From: Sonic Zhang
>
> The new ADI GPIO2 controller was introduced since the BF548 and BF60x
> processors. It differs a lot from the old one on BF5xx processors. So,
> create a pinctrl driver under the pinctrl framework.
> drivers/pinctrl/Kconfig| 17 +
> drivers/pinctrl/Makefile |3 +
> drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
> drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
Those files look reasonable.
> drivers/pinctrl/pinctrl-adi2.c | 1501
>
> drivers/pinctrl/pinctrl-adi2.h | 75 ++
> include/linux/platform_data/pinctrl-adi2.h | 40 +
> diff --git a/drivers/pinctrl/pinctrl-adi2.c b/drivers/pinctrl/pinctrl-adi2.c
> +/**
> + * struct gpio_reserve_map - a GPIO map structure containing the
> + * reservation status of each PIN.
> + *
> + * @owner: who request the reservation
> + * @rsv_gpio: if this pin is reserved as GPIO
> + * @rsv_int: if this pin is reserved as interrupt
> + * @rsv_peri: if this pin is reserved as part of a peripheral device
> + */
> +struct gpio_reserve_map {
> + unsigned char owner[RESOURCE_LABEL_SIZE];
> + bool rsv_gpio;
> + bool rsv_int;
> + bool rsv_peri;
> +};
Why is that needed; don't the pinctrl/GPIO cores already know which
pinctrl pins and which GPIOs are used, and for what?
> +#if defined(CONFIG_DEBUG_FS)
> +static inline unsigned short get_gpio_dir(struct gpio_port *port,
> ...
Why aren't the existing GPIO/pinctrl subsystem debugfs files enough?
> +static int adi_pinmux_request(struct pinctrl_dev *pctldev, unsigned pin)
...
> + /* If a pin can be muxed as either GPIO or peripheral, make
> + * sure it is not already a GPIO pin when we request it.
> + */
> + if (port->rsvmap[offset].rsv_gpio) {
> + if (system_state == SYSTEM_BOOTING)
> + dump_stack();
> + dev_err(pctldev->dev,
> +"%s: Peripheral PIN %d is already reserved as GPIO by
> %s!\n",
> +__func__, pin, get_label(port, offset));
> + spin_unlock_irqrestore(>lock, flags);
> + return -EBUSY;
> + }
Yes, this definitely warrants some more explanation. It looks odd. What
is "system_state"?
> +static int adi_gpio_probe(struct platform_device *pdev)
...
> + /* Add gpio pin range */
> + snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, "pinctrl-adi2.%d",
> + pdata->pinctrl_id);
> + pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
> + ret = gpiochip_add_pin_range(>chip, pinctrl_devname,
> + 0, pdata->port_pin_base, port->width);
This looks like platform data is providing the GPIO <-> pinctrl pin ID
mapping, or at least part of it. Surely that mapping is fixed by the HW
design, and hence isn't something platform data should influence. Do the
files pinctrl-adi2-bf*.c not contain complete information about each HW
configuration for some reason?
> +static struct platform_driver adi_pinctrl_driver = {
> + .probe = adi_pinctrl_probe,
> + .remove = adi_pinctrl_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + },
> +};
> +
> +static struct platform_driver adi_gpio_pint_driver = {
> + .probe = adi_gpio_pint_probe,
> + .remove = adi_gpio_pint_remove,
> + .driver = {
> + .name = "adi-gpio-pint",
> + },
> +};
> +
> +static struct platform_driver adi_gpio_driver = {
> + .probe = adi_gpio_probe,
> + .remove = adi_gpio_remove,
> + .driver = {
> + .name = "adi-gpio",
> + },
> +};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3 v3] pinctrl: ADI PIN control driver for the GPIO controller on bf54x and bf60x.
On 08/21/2013 12:30 AM, Sonic Zhang wrote:
From: Sonic Zhang sonic.zh...@analog.com
The new ADI GPIO2 controller was introduced since the BF548 and BF60x
processors. It differs a lot from the old one on BF5xx processors. So,
create a pinctrl driver under the pinctrl framework.
drivers/pinctrl/Kconfig| 17 +
drivers/pinctrl/Makefile |3 +
drivers/pinctrl/pinctrl-adi2-bf54x.c | 572 +++
drivers/pinctrl/pinctrl-adi2-bf60x.c | 454 +
Those files look reasonable.
drivers/pinctrl/pinctrl-adi2.c | 1501
drivers/pinctrl/pinctrl-adi2.h | 75 ++
include/linux/platform_data/pinctrl-adi2.h | 40 +
diff --git a/drivers/pinctrl/pinctrl-adi2.c b/drivers/pinctrl/pinctrl-adi2.c
+/**
+ * struct gpio_reserve_map - a GPIO map structure containing the
+ * reservation status of each PIN.
+ *
+ * @owner: who request the reservation
+ * @rsv_gpio: if this pin is reserved as GPIO
+ * @rsv_int: if this pin is reserved as interrupt
+ * @rsv_peri: if this pin is reserved as part of a peripheral device
+ */
+struct gpio_reserve_map {
+ unsigned char owner[RESOURCE_LABEL_SIZE];
+ bool rsv_gpio;
+ bool rsv_int;
+ bool rsv_peri;
+};
Why is that needed; don't the pinctrl/GPIO cores already know which
pinctrl pins and which GPIOs are used, and for what?
+#if defined(CONFIG_DEBUG_FS)
+static inline unsigned short get_gpio_dir(struct gpio_port *port,
...
Why aren't the existing GPIO/pinctrl subsystem debugfs files enough?
+static int adi_pinmux_request(struct pinctrl_dev *pctldev, unsigned pin)
...
+ /* If a pin can be muxed as either GPIO or peripheral, make
+ * sure it is not already a GPIO pin when we request it.
+ */
+ if (port-rsvmap[offset].rsv_gpio) {
+ if (system_state == SYSTEM_BOOTING)
+ dump_stack();
+ dev_err(pctldev-dev,
+%s: Peripheral PIN %d is already reserved as GPIO by
%s!\n,
+__func__, pin, get_label(port, offset));
+ spin_unlock_irqrestore(port-lock, flags);
+ return -EBUSY;
+ }
Yes, this definitely warrants some more explanation. It looks odd. What
is system_state?
+static int adi_gpio_probe(struct platform_device *pdev)
...
+ /* Add gpio pin range */
+ snprintf(pinctrl_devname, RESOURCE_LABEL_SIZE, pinctrl-adi2.%d,
+ pdata-pinctrl_id);
+ pinctrl_devname[RESOURCE_LABEL_SIZE - 1] = 0;
+ ret = gpiochip_add_pin_range(port-chip, pinctrl_devname,
+ 0, pdata-port_pin_base, port-width);
This looks like platform data is providing the GPIO - pinctrl pin ID
mapping, or at least part of it. Surely that mapping is fixed by the HW
design, and hence isn't something platform data should influence. Do the
files pinctrl-adi2-bf*.c not contain complete information about each HW
configuration for some reason?
+static struct platform_driver adi_pinctrl_driver = {
+ .probe = adi_pinctrl_probe,
+ .remove = adi_pinctrl_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_driver adi_gpio_pint_driver = {
+ .probe = adi_gpio_pint_probe,
+ .remove = adi_gpio_pint_remove,
+ .driver = {
+ .name = adi-gpio-pint,
+ },
+};
+
+static struct platform_driver adi_gpio_driver = {
+ .probe = adi_gpio_probe,
+ .remove = adi_gpio_remove,
+ .driver = {
+ .name = adi-gpio,
+ },
+};
Hmmm. Is there one HW block that controls GPIOs and pinctrl, or are
there separate HW blocks?
If there's one HW block, why not have just one driver?
If there are separate HW blocks, then having separate GPIO and pinctrl
drivers seems like it would make sense.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
