| Message ID | 20200529213808.2815-1-p.yadav@ti.com |
|---|---|
| Headers | show |
| Series | gpio: Add a managed API | expand |
Hi Pratyush, On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote: > > Hi, > > This is a re-submission of Jean-Jacques' earlier work in October last > year. It can be found at [0]. The goal is to facilitate porting drivers > from the linux kernel. Most of the series will be about adding managed > API to existing infrastructure (GPIO, reset, regmap (already > submitted)). > > This particular series is about GPIOs. It adds a managed API using the > API as Linux. To make it 100% compatible with linux, there is a small > deviation from u-boot's way of naming the gpio lists: the managed > equivalent of gpio_request_by_name(..,"blabla-gpios", ...) is > devm_gpiod_get_index(..., "blabla", ...) > > Changes in v2: > - The original series had a patch that checked for NULL pointers in the > core GPIO functions. The checks were needed because of the addition of > devm_gpiod_get_index_optional() which would return NULL when when no > GPIO was assigned to the requested function. This is convenient for > drivers that need to handle optional GPIOs. > > Simon argued that those should be behind a Kconfig option because of > code size concerns. He also argued against implicit return in the > macro that checked for the optional GPIOs. > > This submission removes the controversial patch so that base > functionality can get unblocked. > > We still need to take a stance on who is responsible for the NULL > check: the driver or the GPIO core? Do we want to trust drivers to > take care of the NULL checks, or do we want to distrust them and make > sure they don't send us anything bogus in the GPIO core. For now the > responsibility lies on the drivers by default. I will send a separate > RFC of the NULL check patch and we can probably discuss the issue > there. > > [0] https://patchwork.ozlabs.org/project/uboot/cover/20191001115130.18886-1-jjhiblot@ti.com/ > > Jean-Jacques Hiblot (2): > drivers: gpio: Add a managed API to get a GPIO from the device-tree > test: gpio: Add tests for the managed API > > arch/sandbox/dts/test.dts | 10 ++++ > drivers/gpio/gpio-uclass.c | 70 +++++++++++++++++++++++++ > include/asm-generic/gpio.h | 47 +++++++++++++++++ > test/dm/gpio.c | 102 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 229 insertions(+) > > -- > 2.26.2 > The first question I have is why do you want to allocate the gpio_desc and return it? Doesn't the caller have a place for that in its private struct? Regards, Simon
On 31/05/20 08:08AM, Simon Glass wrote: > Hi Pratyush, > > On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > Hi, > > > > This is a re-submission of Jean-Jacques' earlier work in October last > > year. It can be found at [0]. The goal is to facilitate porting drivers > > from the linux kernel. Most of the series will be about adding managed > > API to existing infrastructure (GPIO, reset, regmap (already > > submitted)). > > > > This particular series is about GPIOs. It adds a managed API using the > > API as Linux. To make it 100% compatible with linux, there is a small > > deviation from u-boot's way of naming the gpio lists: the managed > > equivalent of gpio_request_by_name(..,"blabla-gpios", ...) is > > devm_gpiod_get_index(..., "blabla", ...) > > > > Changes in v2: > > - The original series had a patch that checked for NULL pointers in the > > core GPIO functions. The checks were needed because of the addition of > > devm_gpiod_get_index_optional() which would return NULL when when no > > GPIO was assigned to the requested function. This is convenient for > > drivers that need to handle optional GPIOs. > > > > Simon argued that those should be behind a Kconfig option because of > > code size concerns. He also argued against implicit return in the > > macro that checked for the optional GPIOs. > > > > This submission removes the controversial patch so that base > > functionality can get unblocked. > > > > We still need to take a stance on who is responsible for the NULL > > check: the driver or the GPIO core? Do we want to trust drivers to > > take care of the NULL checks, or do we want to distrust them and make > > sure they don't send us anything bogus in the GPIO core. For now the > > responsibility lies on the drivers by default. I will send a separate > > RFC of the NULL check patch and we can probably discuss the issue > > there. > > > > [0] https://patchwork.ozlabs.org/project/uboot/cover/20191001115130.18886-1-jjhiblot@ti.com/ > > > > Jean-Jacques Hiblot (2): > > drivers: gpio: Add a managed API to get a GPIO from the device-tree > > test: gpio: Add tests for the managed API > > > > arch/sandbox/dts/test.dts | 10 ++++ > > drivers/gpio/gpio-uclass.c | 70 +++++++++++++++++++++++++ > > include/asm-generic/gpio.h | 47 +++++++++++++++++ > > test/dm/gpio.c | 102 +++++++++++++++++++++++++++++++++++++ > > 4 files changed, 229 insertions(+) > > > > -- > > 2.26.2 > > > > The first question I have is why do you want to allocate the gpio_desc > and return it? Doesn't the caller have a place for that in its private > struct? Ask the Linux folks that ;-) The main aim of this series is to make it easier to port and maintain drivers from Linux. The less changes we have to make when porting a driver, the easier it is to port future fixes and features. Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly being made) use these APIs. FWIW, the docs in Linux say the optional wrappers to the functions are added as a convenience for drivers that need to handle optional GPIOs. [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/ti/phy-j721e-wiz.c
Hi Pratyush, On Mon, 1 Jun 2020 at 05:22, Pratyush Yadav <p.yadav@ti.com> wrote: > > On 31/05/20 08:08AM, Simon Glass wrote: > > Hi Pratyush, > > > > On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > Hi, > > > > > > This is a re-submission of Jean-Jacques' earlier work in October last > > > year. It can be found at [0]. The goal is to facilitate porting drivers > > > from the linux kernel. Most of the series will be about adding managed > > > API to existing infrastructure (GPIO, reset, regmap (already > > > submitted)). > > > > > > This particular series is about GPIOs. It adds a managed API using the > > > API as Linux. To make it 100% compatible with linux, there is a small > > > deviation from u-boot's way of naming the gpio lists: the managed > > > equivalent of gpio_request_by_name(..,"blabla-gpios", ...) is > > > devm_gpiod_get_index(..., "blabla", ...) > > > > > > Changes in v2: > > > - The original series had a patch that checked for NULL pointers in the > > > core GPIO functions. The checks were needed because of the addition of > > > devm_gpiod_get_index_optional() which would return NULL when when no > > > GPIO was assigned to the requested function. This is convenient for > > > drivers that need to handle optional GPIOs. > > > > > > Simon argued that those should be behind a Kconfig option because of > > > code size concerns. He also argued against implicit return in the > > > macro that checked for the optional GPIOs. > > > > > > This submission removes the controversial patch so that base > > > functionality can get unblocked. > > > > > > We still need to take a stance on who is responsible for the NULL > > > check: the driver or the GPIO core? Do we want to trust drivers to > > > take care of the NULL checks, or do we want to distrust them and make > > > sure they don't send us anything bogus in the GPIO core. For now the > > > responsibility lies on the drivers by default. I will send a separate > > > RFC of the NULL check patch and we can probably discuss the issue > > > there. > > > > > > [0] https://patchwork.ozlabs.org/project/uboot/cover/20191001115130.18886-1-jjhiblot@ti.com/ > > > > > > Jean-Jacques Hiblot (2): > > > drivers: gpio: Add a managed API to get a GPIO from the device-tree > > > test: gpio: Add tests for the managed API > > > > > > arch/sandbox/dts/test.dts | 10 ++++ > > > drivers/gpio/gpio-uclass.c | 70 +++++++++++++++++++++++++ > > > include/asm-generic/gpio.h | 47 +++++++++++++++++ > > > test/dm/gpio.c | 102 +++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 229 insertions(+) > > > > > > -- > > > 2.26.2 > > > > > > > The first question I have is why do you want to allocate the gpio_desc > > and return it? Doesn't the caller have a place for that in its private > > struct? > > Ask the Linux folks that ;-) > > The main aim of this series is to make it easier to port and maintain > drivers from Linux. The less changes we have to make when porting a > driver, the easier it is to port future fixes and features. > > Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly > being made) use these APIs. FWIW, the docs in Linux say the optional > wrappers to the functions are added as a convenience for drivers that > need to handle optional GPIOs. U-Boot already supports optional GPIOs. Can we put this behind a CONFIG_LINUX_COMPAT_GPIO flag perhaps, so people know they are trading off code / memory size for compatibility? > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/ti/phy-j721e-wiz.c > > -- > Regards, > Pratyush Yadav > Texas Instruments India Regards, Simon
Hi Simon, On 01/06/20 08:45AM, Simon Glass wrote: > Hi Pratyush, > > On Mon, 1 Jun 2020 at 05:22, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > On 31/05/20 08:08AM, Simon Glass wrote: > > > Hi Pratyush, > > > > > > On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > > > Hi, > > > > > > > > This is a re-submission of Jean-Jacques' earlier work in October last > > > > year. It can be found at [0]. The goal is to facilitate porting drivers > > > > from the linux kernel. Most of the series will be about adding managed > > > > API to existing infrastructure (GPIO, reset, regmap (already > > > > submitted)). > > > > > > > > This particular series is about GPIOs. It adds a managed API using the > > > > API as Linux. To make it 100% compatible with linux, there is a small > > > > deviation from u-boot's way of naming the gpio lists: the managed > > > > equivalent of gpio_request_by_name(..,"blabla-gpios", ...) is > > > > devm_gpiod_get_index(..., "blabla", ...) > > > > > > > > Changes in v2: > > > > - The original series had a patch that checked for NULL pointers in the > > > > core GPIO functions. The checks were needed because of the addition of > > > > devm_gpiod_get_index_optional() which would return NULL when when no > > > > GPIO was assigned to the requested function. This is convenient for > > > > drivers that need to handle optional GPIOs. > > > > > > > > Simon argued that those should be behind a Kconfig option because of > > > > code size concerns. He also argued against implicit return in the > > > > macro that checked for the optional GPIOs. > > > > > > > > This submission removes the controversial patch so that base > > > > functionality can get unblocked. > > > > > > > > We still need to take a stance on who is responsible for the NULL > > > > check: the driver or the GPIO core? Do we want to trust drivers to > > > > take care of the NULL checks, or do we want to distrust them and make > > > > sure they don't send us anything bogus in the GPIO core. For now the > > > > responsibility lies on the drivers by default. I will send a separate > > > > RFC of the NULL check patch and we can probably discuss the issue > > > > there. > > > > > > > > [0] https://patchwork.ozlabs.org/project/uboot/cover/20191001115130.18886-1-jjhiblot@ti.com/ > > > > > > > > Jean-Jacques Hiblot (2): > > > > drivers: gpio: Add a managed API to get a GPIO from the device-tree > > > > test: gpio: Add tests for the managed API > > > > > > > > arch/sandbox/dts/test.dts | 10 ++++ > > > > drivers/gpio/gpio-uclass.c | 70 +++++++++++++++++++++++++ > > > > include/asm-generic/gpio.h | 47 +++++++++++++++++ > > > > test/dm/gpio.c | 102 +++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 229 insertions(+) > > > > > > > > -- > > > > 2.26.2 > > > > > > > > > > The first question I have is why do you want to allocate the gpio_desc > > > and return it? Doesn't the caller have a place for that in its private > > > struct? > > > > Ask the Linux folks that ;-) > > > > The main aim of this series is to make it easier to port and maintain > > drivers from Linux. The less changes we have to make when porting a > > driver, the easier it is to port future fixes and features. > > > > Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly > > being made) use these APIs. FWIW, the docs in Linux say the optional > > wrappers to the functions are added as a convenience for drivers that > > need to handle optional GPIOs. > > U-Boot already supports optional GPIOs. I seem to have mixed up some things when sending the reply. Sorry. This patchset doesn't really have all that much to do with optional GPIOs. It is about adding managed counterparts to the GPIO APIs. > Can we put this behind a CONFIG_LINUX_COMPAT_GPIO flag perhaps, so > people know they are trading off code / memory size for compatibility? The decision to put these behind a config depends on what U-Boot's stance is on the "devm_*" functions in general. I see that there are devm_clk_*() functions in mainline already and they aren't beind a Kconfig option. A quick search tells me as of now no other subsystem has devm_* functions in mainline, though you did drop your Reviewed-by on the regmap patch that adds devm_regmap_init() [0] and it isn't guarded by a Kconfig option. So as of now I see the stance is to add devm_* functions without wrapping them in a compile time switch. If we don't want to make building them mandatory (at the added cognitive cost of code that harder to reason about because of the multiple ways you can build it), that can be done but IMO we should be consistent about what we put behind a config. One note I have about devm_gpiod_get_index() (which is the main source of new code) is that I don't think it is possible to have a dummy function in its stead since it is not a direct call to a lower level function and instead it plays around with the propname suffix. So if we do make it optional, boards with drivers using it should always make sure the config is enabled or their build will fail. [0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-2-p.yadav@ti.com/
Hi Pratyush, On Mon, 8 Jun 2020 at 12:05, Pratyush Yadav <p.yadav@ti.com> wrote: > > Hi Simon, > > On 01/06/20 08:45AM, Simon Glass wrote: > > Hi Pratyush, > > > > On Mon, 1 Jun 2020 at 05:22, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > On 31/05/20 08:08AM, Simon Glass wrote: > > > > Hi Pratyush, > > > > > > > > On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > > > > > Hi, > > > > > > > > > > This is a re-submission of Jean-Jacques' earlier work in October last > > > > > year. It can be found at [0]. The goal is to facilitate porting drivers > > > > > from the linux kernel. Most of the series will be about adding managed > > > > > API to existing infrastructure (GPIO, reset, regmap (already > > > > > submitted)). > > > > > > > > > > This particular series is about GPIOs. It adds a managed API using the > > > > > API as Linux. To make it 100% compatible with linux, there is a small > > > > > deviation from u-boot's way of naming the gpio lists: the managed > > > > > equivalent of gpio_request_by_name(..,"blabla-gpios", ...) is > > > > > devm_gpiod_get_index(..., "blabla", ...) > > > > > > > > > > Changes in v2: > > > > > - The original series had a patch that checked for NULL pointers in the > > > > > core GPIO functions. The checks were needed because of the addition of > > > > > devm_gpiod_get_index_optional() which would return NULL when when no > > > > > GPIO was assigned to the requested function. This is convenient for > > > > > drivers that need to handle optional GPIOs. > > > > > > > > > > Simon argued that those should be behind a Kconfig option because of > > > > > code size concerns. He also argued against implicit return in the > > > > > macro that checked for the optional GPIOs. > > > > > > > > > > This submission removes the controversial patch so that base > > > > > functionality can get unblocked. > > > > > > > > > > We still need to take a stance on who is responsible for the NULL > > > > > check: the driver or the GPIO core? Do we want to trust drivers to > > > > > take care of the NULL checks, or do we want to distrust them and make > > > > > sure they don't send us anything bogus in the GPIO core. For now the > > > > > responsibility lies on the drivers by default. I will send a separate > > > > > RFC of the NULL check patch and we can probably discuss the issue > > > > > there. > > > > > > > > > > [0] https://patchwork.ozlabs.org/project/uboot/cover/20191001115130.18886-1-jjhiblot@ti.com/ > > > > > > > > > > Jean-Jacques Hiblot (2): > > > > > drivers: gpio: Add a managed API to get a GPIO from the device-tree > > > > > test: gpio: Add tests for the managed API > > > > > > > > > > arch/sandbox/dts/test.dts | 10 ++++ > > > > > drivers/gpio/gpio-uclass.c | 70 +++++++++++++++++++++++++ > > > > > include/asm-generic/gpio.h | 47 +++++++++++++++++ > > > > > test/dm/gpio.c | 102 +++++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 229 insertions(+) > > > > > > > > > > -- > > > > > 2.26.2 > > > > > > > > > > > > > The first question I have is why do you want to allocate the gpio_desc > > > > and return it? Doesn't the caller have a place for that in its private > > > > struct? > > > > > > Ask the Linux folks that ;-) > > > > > > The main aim of this series is to make it easier to port and maintain > > > drivers from Linux. The less changes we have to make when porting a > > > driver, the easier it is to port future fixes and features. > > > > > > Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly > > > being made) use these APIs. FWIW, the docs in Linux say the optional > > > wrappers to the functions are added as a convenience for drivers that > > > need to handle optional GPIOs. > > > > U-Boot already supports optional GPIOs. > > I seem to have mixed up some things when sending the reply. Sorry. This > patchset doesn't really have all that much to do with optional GPIOs. It > is about adding managed counterparts to the GPIO APIs. > > > Can we put this behind a CONFIG_LINUX_COMPAT_GPIO flag perhaps, so > > people know they are trading off code / memory size for compatibility? > > The decision to put these behind a config depends on what U-Boot's > stance is on the "devm_*" functions in general. I see that there are > devm_clk_*() functions in mainline already and they aren't beind a > Kconfig option. A quick search tells me as of now no other subsystem has > devm_* functions in mainline, though you did drop your Reviewed-by on > the regmap patch that adds devm_regmap_init() [0] and it isn't guarded > by a Kconfig option. It is OK to add new functionality that doesn't increase size if it isn't used. But I worry with this API that we are changing the semantics of functions and adding more checks, which does increase code size, and sow confusion. > > So as of now I see the stance is to add devm_* functions without > wrapping them in a compile time switch. If we don't want to make > building them mandatory (at the added cognitive cost of code that harder > to reason about because of the multiple ways you can build it), that can > be done but IMO we should be consistent about what we put behind a > config. > > One note I have about devm_gpiod_get_index() (which is the main source > of new code) is that I don't think it is possible to have a dummy > function in its stead since it is not a direct call to a lower level > function and instead it plays around with the propname suffix. So if we > do make it optional, boards with drivers using it should always make > sure the config is enabled or their build will fail. Yes indeed. Boards need to choose to use this API and enable it. But that is the case with lots of U-Boot features so I think it makes sense. > > [0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-2-p.yadav@ti.com/ > > -- > Regards, > Pratyush Yadav > Texas Instruments India Regards, Simon