| Message ID | 20240519211346.30323-1-egyszeregy@freemail.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] spidev: Introduce "linux,spidev-name" property for device tree of spidev. | expand |
On Sun, May 19, 2024 at 11:13:46PM +0200, egyszeregy@freemail.hu wrote: > From: Benjamin Szőke <egyszeregy@freemail.hu> > > Optionally, spidev may have a "linux,spidev-name" property. > This is a string which is defining a custom suffix name for spi device in > /dev/spidev-<name> format. It helps to improve software portability between > various SoCs and reduce complexities of hardware related codes in SWs. This seems like what udev rules are for?
2024. 05. 20. 15:20 keltezéssel, Mark Brown írta: > On Sun, May 19, 2024 at 11:13:46PM +0200, egyszeregy@freemail.hu wrote: >> From: Benjamin Szőke <egyszeregy@freemail.hu> >> >> Optionally, spidev may have a "linux,spidev-name" property. >> This is a string which is defining a custom suffix name for spi device in >> /dev/spidev-<name> format. It helps to improve software portability between >> various SoCs and reduce complexities of hardware related codes in SWs. > > This seems like what udev rules are for? Hi, Goal of this patch is to introduce this new mode to assign a custom name from lowlevel device tree to a spidev device. As i know udev can do it, but to do it from device tree is the best and easier way for this feature in my opinion. It is more maintainable then use udev in userspace for it. For example there are three different SoCs: i.MX7, i.MX9, ZynqMP. In Yocto project, the Linux image's SW environment is nicely configurable independently from what is the target MACHNIE. But if i like to deploy a SW which uses peripheries like gpiobanks, i2c-dev, spidev these /dev/... name will be totally different on each SoCs, more over in ZynqMP and any other Adaptive SoC platform, the index number for the spidev, gpiobanks or other can be not deterministic if it probed in run-time. Goal is to easily make a Linux OS image which can support multiple SoCs in SW point of view easily. So, in Yocto project build system point of view the best, if any Machine specific settings is stored in the device tree files of the target machine in driver levels/config, because it will be deterministic in 100% sure and it will be nicely separated from the SW meta layers which may not contains any machine specific hacking with udev and so on. So this way to assign a custom name for a spidev from device tree is more efficient and more maintainable in SW developing point of view in embedded Linux and Yocto/buildroot world because i need to just define a name like linux,spidev-name = "sensor"; then use it with a fixed name in my generic SW under /dev/spidev-sensor name. And there are no need to care about what will be the index number of this spidev randomly after boot and how need to make an ugly append layer for udev config and make it for all of machine variants separately. My opinion udev is ugly to use for it, and no longer beneficial for new Adaptive SoCs where they can be not deterministic what kind of index number they got in driver probing for many gpio, spidev, i2c-dev peripheries (you do not have info about that which need to mapping for what custom name, it can be different in many time based on PL FW). It is much better, safe and easier to assign this custom suffix/name explicitly from device tree, moreover it is a driver related things, i think the best place is in device tree for it not in a sys config file for udev. DT binding would need to be documented later in a separated patch as a guideline mentioned it in Linux repo.
On Mon, May 20, 2024 at 07:20:12PM +0200, Szőke Benjamin wrote: > So, in Yocto project build system point of view the best, if any Machine > specific settings is stored in the device tree files of the target machine > in driver levels/config, because it will be deterministic in 100% sure and > it will be nicely separated from the SW meta layers which may not contains > any machine specific hacking with udev and so on. Given that with Yocto you're building a full system image it's not super obvious to me that it is particularly harder to ship udev rules in the image as opposed to modifying the DT. It's a little more annoying but not drastically so and it's not creating a burden on the ABI for something that's mainly used within a vertically integrated software stack. > DT binding would need to be documented later in a separated patch as a > guideline mentioned it in Linux repo. No, that needs to happen along with the code change.
2024. 05. 20. 22:14 keltezéssel, Mark Brown írta: > On Mon, May 20, 2024 at 07:20:12PM +0200, Szőke Benjamin wrote: > >> So, in Yocto project build system point of view the best, if any Machine >> specific settings is stored in the device tree files of the target machine >> in driver levels/config, because it will be deterministic in 100% sure and >> it will be nicely separated from the SW meta layers which may not contains >> any machine specific hacking with udev and so on. > > Given that with Yocto you're building a full system image it's not > super obvious to me that it is particularly harder to ship udev rules in > the image as opposed to modifying the DT. It's a little more annoying > but not drastically so and it's not creating a burden on the ABI for > something that's mainly used within a vertically integrated software > stack. > In Yocto and Buildroot it is harder and more ugly to provide MACHINE specific settings in a rootfs config files than define it in the machine specific .dts and .dtsi files because they are separated in meta-layers for SW recipes and HW related machine recipes. As i know udev is much older than device-tree in Linux kernel history. For embedded Linux image maintaining/developing for ARM, RISC-V etc. to solve this kind of features/issues is more elegant to do in device-tree than with udev, moreover for an embedded Linux developer it is more familiar to do in device-tree then udev. I spent 3-4 days to understand udev rules files and i tried to do it via udev, but i gave up it due to it complexity and incomplete documentation about it. axi_quad_spi_0: axi_quad_spi@a00a0000 { bits-per-word = <8>; clock-names = "ext_spi_clk", "s_axi_aclk"; clocks = <&zynqmp_clk 71>, <&zynqmp_clk 71>; compatible = "xlnx,axi-quad-spi-3.2", "xlnx,xps-spi-2.00.a"; fifo-size = <16>; interrupt-names = "ip2intc_irpt"; interrupt-parent = <&gic>; interrupts = <0 106 1>; num-cs = <0x1>; reg = <0x0 0xa00a0000 0x0 0x10000>; xlnx,num-ss-bits = <0x1>; xlnx,spi-mode = <0>; #address-cells = <1>; #size-cells = <0>; spidev@0 { reg = <0>; compatible = "rohm,dh2228fv"; spi-max-frequency = <1000000>; // via my kernel patch -> /dev/spidev-mysensor // linux,spidev-name = "mysensor"; }; }; As i understand "axi_quad_spi@a00a0000" can be mapped via udev to a custom symlink name but in a new adaptive SoC HWs like AMD ZynqMP, Intel Stratix, Microchip PolarFire Soc etc. it is not possible and not good solution because this axi reg address can be different and become to non-deterministic in day to next when there is a new PL FW update for their FPGA part in the silicon. What udev rules have to use for it if you say it can be perfectly done via udev and "axi_quad_spi@a00a0000" cannot be used for making this rule? >> DT binding would need to be documented later in a separated patch as a >> guideline mentioned it in Linux repo. > > No, that needs to happen along with the code change. The official documentation says totally different: "The Documentation/ and include/dt-bindings/ portion of the patch should be a separate patch. ..." https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/submitting-patches.rst By the way where can i find .yml or .txt dt-bindings documentation of spidev driver?
On Sun, Jun 02, 2024 at 05:31:10PM +0200, Szőke Benjamin wrote: > As i understand "axi_quad_spi@a00a0000" can be mapped via udev to a custom > symlink name but in a new adaptive SoC HWs like AMD ZynqMP, Intel Stratix, > Microchip PolarFire Soc etc. it is not possible and not good solution > because this axi reg address can be different and become to > non-deterministic in day to next when there is a new PL FW update for their > FPGA part in the silicon. > What udev rules have to use for it if you say it can be perfectly done via > udev and "axi_quad_spi@a00a0000" cannot be used for making this rule? This feels like something I'd expect the FPGA tools to help with, having to run around adding random properties to individual bindings to figure out what the IPs that the tooling has decided to instantiate doesn't seem scalable. > > > DT binding would need to be documented later in a separated patch as a > > > guideline mentioned it in Linux repo. > > No, that needs to happen along with the code change. > The official documentation says totally different: > "The Documentation/ and include/dt-bindings/ portion of the patch should be > a separate patch. ..." In the same series. We can document bindings without code but we don't take code without bindings. > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/submitting-patches.rst > By the way where can i find .yml or .txt dt-bindings documentation of spidev driver? The binding documentation describes the hardware, spidev is an implementation detail of Linux so should not have a binding. A bunch of the devices should be in the trivial bindings document.
On Sun, Jun 02, 2024 at 05:31:10PM +0200, Szőke Benjamin wrote: > spidev@0 { > reg = <0>; > compatible = "rohm,dh2228fv"; > spi-max-frequency = <1000000>; > > // via my kernel patch -> /dev/spidev-mysensor > // linux,spidev-name = "mysensor"; Given you are describing a DAC as a "sensor", I doubt you actually even have a dh2228fv*. You're looking to have a consistent name here, but you can't even rely on Linux even continuing to bind the spidev driver against this compatible if somebody comes along and writes an IIO driver for this DAC! That said, google seems to return no results for a dh2228fv, only for a bh2228fv. Makes me wonder if this device actually even exists...
Fri, Jun 07, 2024 at 05:07:23PM +0100, Conor Dooley kirjoitti: > On Sun, Jun 02, 2024 at 05:31:10PM +0200, Szőke Benjamin wrote: > > > spidev@0 { > > reg = <0>; > > compatible = "rohm,dh2228fv"; > > spi-max-frequency = <1000000>; > > > > // via my kernel patch -> /dev/spidev-mysensor > > // linux,spidev-name = "mysensor"; > > Given you are describing a DAC as a "sensor", I doubt you actually even > have a dh2228fv*. You're looking to have a consistent name here, but you > can't even rely on Linux even continuing to bind the spidev driver > against this compatible if somebody comes along and writes an IIO driver > for this DAC! > > That said, google seems to return no results for a dh2228fv, only for a > bh2228fv. Makes me wonder if this device actually even exists... Why not summon Matti?
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 95fb5f1c91c1..389baaf38e99 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -767,6 +767,8 @@ MODULE_DEVICE_TABLE(acpi, spidev_acpi_ids); static int spidev_probe(struct spi_device *spi) { + int ret; + const char *name; int (*match)(struct device *dev); struct spidev_data *spidev; int status; @@ -800,9 +802,20 @@ static int spidev_probe(struct spi_device *spi) struct device *dev; spidev->devt = MKDEV(SPIDEV_MAJOR, minor); - dev = device_create(&spidev_class, &spi->dev, spidev->devt, - spidev, "spidev%d.%d", - spi->controller->bus_num, spi_get_chipselect(spi, 0)); + + /* + * If "linux,spidev-name" is specified in device tree, use /dev/spidev-<name> + * in Linux userspace, otherwise use /dev/spidev<bus_num>.<cs_num>. + */ + ret = device_property_read_string(&spi->dev, "linux,spidev-name", &name); + if (ret < 0) + dev = device_create(&spidev_class, &spi->dev, spidev->devt, + spidev, "spidev%d.%d", + spi->controller->bus_num, spi_get_chipselect(spi, 0)); + else + dev = device_create(&spidev_class, &spi->dev, spidev->devt, + spidev, "spidev-%s", name); + status = PTR_ERR_OR_ZERO(dev); } else { dev_dbg(&spi->dev, "no minor number available!\n");