linux-kernel - Re: [PATCH v5 0/7] misc: Add mikroBUS driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 28 Jun 2024 15:52:40 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Ayush Singh <ayush@...gleboard.org>
Cc: Mark Brown <broonie@...nel.org>,
	Vaishnav M A <vaishnav@...gleboard.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Derek Kiernan <derek.kiernan@....com>,
	Dragan Cvetic <dragan.cvetic@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
	Tero Kristo <kristo@...nel.org>, Michael Walle <mwalle@...nel.org>,
	jkridner@...gleboard.org, robertcnelson@...gleboard.org,
	linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Ayush Singh <ayushdevel1325@...il.com>
Subject: Re: [PATCH v5 0/7] misc: Add mikroBUS driver

> 3. Allowing creation of sysfs entries `new_device` and `delete_device`
> similar to what already exists for I2C, etc.

On the I2C bus, these operate at the device level, you instantiate a
new I2C device.  I assume here you are actually talking about board
level operations? So they would be 'new_board', and 'delete_board'
files in sysfs?

> 
> 4. Allow using 1-wire-eeprom in a fashion that allows automatic board
> discovery.
> 
> 
> Let me now introduce the 2 architectures we will be discussing:
> 
> 1. mikrobus-connector has phandle to mikrobus-board:
> 
> ```
> 
> \ {
> 
>     connector1 {
> 
>         board = <&board1>;
> 
>     };
> 
> 
>     mikrobus_boards {
> 
>         board1 {
> 
>             ...
> 
>         };
> 
>     };
> 
> };
> 
> ```
> 
> 
> 2. mikrobus board is a child node of mikrobus-connector:
> 
> ```
> 
> \ {
> 
>     connector1 {
> 
>         ...
> 
>         spi {

So there would actually be multiple child nodes, one per bus, and then
maybe a simple-bus for nodes which do not correspond to a bus,
e.g. gpio-key, gpio-leds, etc.,

> 
>             board1 {
> 
>                 ...
> 
>             };
> 
>         };
> 
>     };
> 
> };
> 
> ```
> 
> 
> I will now go over how each of these goals might look like in both of the
> architecture.
> 
> 1. Keeping the device tree properties upstream in a system independent way:
> 
> a. mikrobus-connector has phandle to mikrobus-board
> 
> It is possible to create an overlay as follows which will work with any
> system that defines the `mikrobus_boards` node. This node is completely
> independent of mikroBUS connector and thus does not need to be rewritten (or
> generated) for each board. There are no problems for system with more than 1
> mikrobus connector.
> 
> ```
> 
> &mikrobus_boards {
> 
>     board2 {
> 
>         ...
> 
>     };
> 
> 
>     board3 {
> 
>         ...
> 
>     };
> 
> };

So by default, you have an empty mikrobus_boards node? You then use DT
overlay to load the needed board into this node, and then update the
phandle in the connection node to point to the newly loaded node?

> b. mikrobus board is a child node of mikrobus-connector:
> 
> Not sure how to do something similar here. The overlay needs to be rewritten
> (or generated) for each board.

It would be good to explain why...

> Systems with multiple mikrobus connectors
> will need multiple overlays adding the boards as child node of each
> connector (with status = "disabled").

Why? Just load the one overlay actually required.

> &connector1 {
> 
>     spi = {
> 
>         board 2 {
> 
>             ...
> 
>         };
> 
>         board 3 {
> 
>             ...
> 
>         };
> 
>     };
> 
> };

I don't actually understand this description. I was expecting more
like:

connector1: {

	spi =  {
	    /* Optional TI TSC2046 touchscreen controller */
            opt_touch: touchscreen@0 {
                    compatible = "ti,tsc2046";
                    spi-max-frequency = <2500000>;
                    reg = <0>;
                    pinctrl-0 = <&pmx_gpio_13>;
                    pinctrl-names = "default";
                    interrupts-extended = <&gpio0 13 IRQ_TYPE_EDGE_FALLING>;
            };
	};

	i2c = {
	        opt_audio: audio@1a {
                compatible = "ti,tlv320aic23";
                reg = <0x1a>;
        };

	the_rest = {
        	gpio_keys {
                    compatible = "gpio-keys";
                    #address-cells = <1>;
                    #size-cells = <0>;
                    pinctrl-0 = <&pmx_reset_button &pmx_USB_copy_button>;
                    pinctrl-names = "default";
    
                    copy {
                            label = "USB Copy";
                            linux,code = <KEY_COPY>;
                            gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
                    };
                    reset {
                            label = "Reset";
                            linux,code = <KEY_RESTART>;
                            gpios = <&gpio0 16 GPIO_ACTIVE_LOW>;
                    };
            };

This is completely made up. You probably should use an example of a
real complex board using multiple busses.

So for each actual bus on Mikrobus, you have a bus node, and then a
node for everything which is not bus orientated, like gpio-keys.

So the overlay would simply populate these child nodes.

> Maybe it is possible to have special behavior for mikrobus-connector nodes
> in dt overlay but that will break compatibility with exisiting
> infrastructure which isn't great.

You have not explain what special behaviour is actually needed.

	Andrew