Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: allow to retrieve device structure by the name prefix #21709

Closed

Conversation

jfischer-no
Copy link
Collaborator

This patch allows to retrieve available drivers for certain
types of devices based on the prefix in the name.

If the name is formed according to the rule TYPE_DEVICE then
a subsystem can find a appropriate drivers/devices
without additional configuration.

For example: a display could have the name DISPLAY_SSD1306,
a IEEE802154 transceiver the name 802154_MCR20A,
and a temperature sensor TSENSOR_HDC1010.

TODO: add test to tests/kernel/device

This patch allows to retrieve available drivers for certain
types of devices based on the prefix in the name.

If the name is formed according to the rule TYPE_DEVICE then
a subsystem can find a appropriate drivers/devices
without additional configuration.

For example: a display could have the name DISPLAY_SSD1306,
a IEEE802154 transceiver the name 802154_MCR20A,
and a temperature sensor TSENSOR_HDC1010.

Signed-off-by: Johann Fischer <j.fischer@phytec.de>
Remove LVGL_DISPLAY_DEV_NAME and unify configuration.
Use device type "DISPLAY" prefix for DT labels.

Signed-off-by: Johann Fischer <j.fischer@phytec.de>
@jfischer-no
Copy link
Collaborator Author

Shame, it fails with newlib, I guess because strnlen is a GNU extension. I will rework it.

@stephanosio
Copy link
Member

Shame, it fails with newlib, I guess because strnlen is a GNU extension. I will rework it.

Both minimal libc and newlib support strnlen.

https://github.com/bminor/newlib/blob/ad101bcb0f55f0eb1a9f60187f949c3decd855e4/newlib/libc/include/string.h#L118-L120

It looks like we will need to globally set _POSIX_C_SOURCE >= 200809L somewhere (posix arch already seems to be doing this in a somewhat hacky way).

Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the final purpose of this? (let me block the pr until then)

Note that the device name is something that should be removed (someday... ). And its only purpose is to get the device bindings. And it was introduced only because there were no other nice ways to do it. But it's useless besides this.

@erwango
Copy link
Member

erwango commented Jan 6, 2020

@jfischer-phytec-iot, can you check #21560 ?
Seems it would achieve same functionality w/o storing label strings in binary.

@jfischer-no
Copy link
Collaborator Author

@jfischer-phytec-iot, can you check #21560 ?
Seems it would achieve same functionality w/o storing label strings in binary.

That is what I am absolute against, we should not mix DT and Kconfig. Also #21560 do not resolve the issue with the subsystem, only one device/driver is configurable/can be selected. It is not possible to discover number of devices, e.g. number of displays or transceivers. Also see last commit as example, overhead of the prefix is minimal.

@jfischer-no
Copy link
Collaborator Author

What is the final purpose of this? (let me block the pr until then)

Note that the device name is something that should be removed (someday... ). And its only purpose is to get the device bindings. And it was introduced only because there were no other nice ways to do it. But it's useless besides this.

You can see the last commit as minimal example. Regardless what display controller driver is selected, both subsystems CFB and LVGL can retrieve device structure by the prefix, also more then one display can be used, e.g. EPD connected to SPI and OLED connected to I2C. An other example is to use one or more temperature sensors in the application like samples/bluetooth/peripheral_esp or two 802154 transceivers (operating on different bands).
The purpose for the prefix is the same, to get get the device bindings. I know there are plans to replace it with an other tree obtained from (?) device tree, however as long we have the device name, the prefix provides functionality I need with minimal overhead and minimal change.

@tbursztyka
Copy link
Collaborator

@jfischer-phytec-iot I see. But your approach is creating another dependency on device name (bringing all the issues of using the name with it also: it's slow etc...), and I don't want to see that happen. Then it will be even more complicated to better integrate DTS and get rid of this name afterwards.

Both your PR and #21560 (I agree we must not mix DTS and Kconfig) are, imo, wrong solutions. They are q&d fix for a real issue. I think it's worth spending a bit more thinking on that, I am sure something better can be found.

@jfischer-no
Copy link
Collaborator Author

jfischer-no commented Jan 7, 2020

@jfischer-phytec-iot I see. But your approach is creating another dependency on device name (bringing all the issues of using the name with it also: it's slow etc...), and I don't want to see that happen. Then it will be even more complicated to better integrate DTS and get rid of this name afterwards.

Both your PR and #21560 (I agree we must not mix DTS and Kconfig) are, imo, wrong solutions. They are q&d fix for a real issue. I think it's worth spending a bit more thinking on that, I am sure something better can be found.

@tbursztyka I know about slowness and accept as it happens only once at start. Also thought about sections in the rom for particular device type, something like devconfig for each device type, but that is more invasive approach.
DTS do not provide any property (I did not found one in the specification, also not 'device-types') to identify types of the devices available in the system, it ends on the driver level. I do not see any other simple way at the moment and current "workarounds" with CONFIG_ * DEV_NAME or dts_fixup.h do not scale and too expensive to maintain.

Edit: device_type is deprecated, DT self looks like an accident.

@tbursztyka
Copy link
Collaborator

DTS do not provide any property (I did not found one in the specification, also not 'device-types') to identify types of the devices available in the system, it ends on the driver level. I do not see any other simple way at the moment and current "workarounds" with CONFIG_ * DEV_NAME or dts_fixup.h do not scale and too expensive to maintain.

I know, all of this need to be fixed. Let's wake up @galak

@jfischer-no
Copy link
Collaborator Author

DTS do not provide any property (I did not found one in the specification, also not 'device-types') to identify types of the devices available in the system, it ends on the driver level. I do not see any other simple way at the moment and current "workarounds" with CONFIG_ * DEV_NAME or dts_fixup.h do not scale and too expensive to maintain.

I know, all of this need to be fixed. Let's wake up @galak

Probably device_type could be used for that, see also discussion in #17976

@tbursztyka
Copy link
Collaborator

Probably device_type could be used for that, see also discussion in #17976

Yes, though it would need to be reworked against new dts script. I did not make it that time, but it does not mean it could not go now since we would have use case for it. But sounds like a good approach, I really support doing things statically as much as possible since we do have all the information statically via dts. Such types could be introduced in the various base yaml files (spi-controller.yaml, pwm-controller.yaml etc... there are some missing but it would be trivial to add).

@mbolivar
Copy link
Contributor

mbolivar commented Jan 9, 2020

@jfischer-phytec-iot this looks like it may be related to the work in #20253; have you looked at that?

@jfischer-no jfischer-no added the dev-review To be discussed in dev-review meeting label Jan 9, 2020
@jfischer-no
Copy link
Collaborator Author

@jfischer-phytec-iot this looks like it may be related to the work in #20253; have you looked at that?

@mbolivar Thanks for the point, had it in mind too but did not found the PR. Seems the PR will take a while, this PR makes minimal change and could be replaced later with the solution from #20253

@jfischer-no jfischer-no removed the dev-review To be discussed in dev-review meeting label Jan 9, 2020
@jfischer-no
Copy link
Collaborator Author

outdated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants