[PATCH v5 0/7] misc: Add mikroBUS driver

[PATCH v5 0/7] misc: Add mikroBUS driver

[Ayush Singh]

MikroBUS is an open standard  developed by MikroElektronika for connecting
add-on boards to microcontrollers or microprocessors. It essentially
allows you to easily expand the functionality of your main boards using
these add-on boards.

This patchset adds mikroBUS as a Linux bus type and provides a driver to
parse and register the mikroBUS board using device tree infrastructure.

The patchset is based on work originally done by Vaishnav.

Link: https://www.mikroe.com/mikrobus
Link: https://docs.beagleboard.org/latest/boards/beagleplay/
Link: https://lore.kernel.org/all/20240317193714.403132-1-ayushdevel1325@gmail.com/ Patch v4

Changes v5
- Complete rewrite to use device tree instead of mikroBUS manifest.
- Only support for SPI.
- Adds `mikrobus,spi` compatible property.

Changes v4:
- Better commit messages
- Remove clickID, serdev, pwm, regulator, clocks etc. Just the basic
  mikroBUS driver.
- Fix a lot of memory leaks, unused variables, etc.
- Create accompanying PR in Greybus Spec repository
- Switch to 80 columns formatting
- Some other fixes pointed out in v3

Changes in v3:
- Use phandle instead of busname for spi
- Use spi board info for registering new device
- Convert dt bindings to yaml
- Add support for clickID
- Code cleanup and style changes
- Additions required to spi, serdev, w1 and regulator subsystems

Changes in v2:
- support for adding mikroBUS ports from DT overlays,
- remove debug sysFS interface for adding mikrobus ports,
- consider extended pin usage/deviations from mikrobus standard
  specifications
- use greybus CPort protocol enum instead of new protocol enums
- Fix cases of wrong indentation, ignoring return values, freeing allocated
  resources in case of errors and other style suggestions in v1 review.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
Ayush Singh (7):
      dt-bindings: connector: Add mikrobus-connector
      dt-bindings: mikrobus: Add mikrobus board base
      dt-bindings: mikrobus: Add mikrobus-spi binding
      spi: Make of_find_spi_controller_by_node() available
      spi: Make of_register_spi_device() available
      mikrobus: Add mikroBUS driver
      dts: ti: k3-am625-beagleplay: Add mikroBUS

 .../bindings/connector/mikrobus-connector.yaml     | 107 ++++++
 .../bindings/mikrobus/mikrobus-board.yaml          |  20 ++
 .../devicetree/bindings/mikrobus/mikrobus-spi.yaml |  37 +++
 MAINTAINERS                                        |   9 +
 arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts     |  94 +++++-
 drivers/misc/Kconfig                               |  16 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/mikrobus.c                            | 361 +++++++++++++++++++++
 drivers/spi/spi.c                                  | 209 ++++++------
 include/linux/spi/spi.h                            |   7 +
 10 files changed, 750 insertions(+), 111 deletions(-)
---
base-commit: f76698bd9a8ca01d3581236082d786e9a6b72bb7
change-id: 20240627-mikrobus-scratch-spi-ad8c98dcec98

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>

[PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Ayush Singh]

Add DT bindings for mikroBUS interface. MikroBUS is an open standard
developed by MikroElektronika for connecting add-on boards to
microcontrollers or microprocessors.

mikroBUS is a connector and does not have a controller. Instead the
software is responsible for identification of board and setting up uart,
spi, i2c, pwm and other buses. Additionally, some new mikroBUS boards
contain 1-wire EEPROM that contains a manifest to describe the addon
board to provide plug and play capabilities.

A mikroBUS addon board is free to leave some of the pins unused which
are marked as NC or Not Connected.

Some of the pins might need to be configured as GPIOs deviating from their
reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.

For some add-on boards the driver may not take care of some additional
signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
(RST pin on the mikrobus port) needs to be pulled high.

Some SPI addon boards use other pins like RST, AN etc as chipselect (eg.
SPI Extend Click). Thus, `spi-cs` and `spi-cs-names` property is added
to allow mikroBUS addon board to specify chipselect by name.

Here's the list of pins in mikroBUS connector:
AN - Analog
RST - Reset
CS - SPI Chip Select
SCK - SPI Clock
MISO - SPI Master Input Slave Output
MOSI - SPI Master Output Slave Input
+3.3V - VCC-3.3V power
GND - Reference Ground
PWM - PWM output
INT - Hardware Interrupt
RX - UART Receive
TX - UART Transmit
SCL - I2C Clock
SDA - I2C Data
+5V - VCC-5V power
GND - Reference Ground

Link: https://www.mikroe.com/mikrobus
Link:
https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
mikroBUS specification
Link: https://www.mikroe.com/sht1x-click SHT15 Click
Link: https://www.mikroe.com/eth-click ENC28J60 Click
Link: https://www.mikroe.com/spi-extend-click SPI Extend Click

Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org>
Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 .../bindings/connector/mikrobus-connector.yaml     | 107 +++++++++++++++++++++
 MAINTAINERS                                        |   6 ++
 2 files changed, 113 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
new file mode 100644
index 000000000000..033479f8604f
--- /dev/null
+++ b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/connector/mikrobus-connector.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: mikroBUS add-on board connector
+
+maintainers:
+  - Ayush Singh <ayush@beagleboard.org>
+
+properties:
+  compatible:
+    const: mikrobus-connector
+
+  pinctrl-0: true
+  pinctrl-1: true
+  pinctrl-2: true
+  pinctrl-3: true
+  pinctrl-4: true
+  pinctrl-5: true
+  pinctrl-6: true
+  pinctrl-7: true
+  pinctrl-8: true
+
+  pinctrl-names:
+    minItems: 1
+    maxItems: 9
+    items:
+      enum: [default, pwm_default, pwm_gpio, uart_default, uart_gpio, i2c_default, i2c_gpio, spi_default,
+             spi_gpio]
+
+  spi-controller:
+    description: spi-controller of mikroBUS SPI pins along with cs pins.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  spi-cs:
+    description: spi chip-select corresponding to the chip-selects on the mikrobus socket.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  spi-cs-names:
+    minItems: 1
+    maxItems: 12
+    items:
+      enum: [default, pwm, int, rx, tx, scl, sda, an, rst, sck, cipo, copi]
+
+  i2c-controller:
+    description: i2c controller attached to the mikrobus socket.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  uart-controller:
+    description: uart controller attached to the mikrobus socket
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  pwms:
+    description: the pwm-controller corresponding to the mikroBUS PWM pin.
+    maxItems: 1
+
+  mikrobus-gpios:
+    minItems: 1
+    maxItems: 12
+
+  mikrobus-gpio-names:
+    minItems: 1
+    maxItems: 12
+    items:
+      enum: [pwm, int, rx, tx, scl, sda, an, rst, cs, sck, cipo, copi]
+
+  board:
+    description: board attached to mikrobus connector
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+required:
+  - compatible
+  - pinctrl-0
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    mikrobus {
+      compatible = "mikrobus-connector";
+      pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",
+                      "i2c_gpio", "spi_default", "spi_gpio";
+      pinctrl-0 = <&P2_03_gpio_input_pin &P1_04_gpio_pin &P1_02_gpio_pin>;
+      pinctrl-1 = <&P2_01_pwm_pin>;
+      pinctrl-2 = <&P2_01_gpio_pin>;
+      pinctrl-3 = <&P2_05_uart_pin &P2_07_uart_pin>;
+      pinctrl-4 = <&P2_05_gpio_pin &P2_07_gpio_pin>;
+      pinctrl-5 = <&P2_09_i2c_pin &P2_11_i2c_pin>;
+      pinctrl-6 = <&P2_09_gpio_pin &P2_11_gpio_pin>;
+      pinctrl-7 = <&P1_12_spi_pin &P1_10_spi_pin &P1_08_spi_sclk_pin &P1_06_spi_cs_pin>;
+      pinctrl-8 = <&P1_12_gpio_pin &P1_10_gpio_pin &P1_08_gpio_pin &P1_06_gpio_pin>;
+      pwms = <&ehrpwm1 0 500000 0>;
+      i2c-controller = <&i2c1>;
+      uart-controller = <&uart1>;
+      spi-controller = <&spi1>;
+      spi-cs = <0 1>;
+      spi-cs-names = "default", "rst";
+      mikrobus-gpios = <&gpio1 18 GPIO_ACTIVE_HIGH>, <&gpio0 23 GPIO_ACTIVE_HIGH>,
+                       <&gpio0 30 GPIO_ACTIVE_HIGH>, <&gpio0 31 GPIO_ACTIVE_HIGH>,
+                       <&gpio0 15 GPIO_ACTIVE_HIGH>, <&gpio0 14 GPIO_ACTIVE_HIGH>,
+                       <&gpio0 4 GPIO_ACTIVE_HIGH>, <&gpio0 3 GPIO_ACTIVE_HIGH>,
+                       <&gpio0 2 GPIO_ACTIVE_HIGH>, <&gpio0 5 GPIO_ACTIVE_HIGH>,
+                       <&gpio2 25 GPIO_ACTIVE_HIGH>, <&gpio2 3 GPIO_ACTIVE_HIGH>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 807feae089c4..8e4115e93aeb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15108,6 +15108,12 @@ M:	Oliver Neukum <oliver@neukum.org>
 S:	Maintained
 F:	drivers/usb/image/microtek.*
 
+MIKROBUS
+M:	Ayush Singh <ayush@beagleboard.org>
+M:	Vaishnav M A <vaishnav@beagleboard.org>
+S:	Maintained
+F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
+
 MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
 M:	Luka Kovacic <luka.kovacic@sartura.hr>
 M:	Luka Perkov <luka.perkov@sartura.hr>

-- 
2.45.2

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Michael Walle]

Hi,

Could you give us a DT snippet of how this should look like with a
board?

On Thu Jun 27, 2024 at 6:26 PM CEST, Ayush Singh wrote:
> +  board:
> +    description: board attached to mikrobus connector
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

Shouldn't this be a subnode of the connector?

i.e.

connector {
	compatible = "mikrobus-connector";

	// phandles to the parent controllers

	spi {
		temp-sensor@0 {
			compatible = "maxim,max31855k";
			reg = <0>;
		};
	};

	i2c {
		..
	};
};

I don't think you can introduce a new
  compatible = "maxim,max31855k", "mikrobus,spi";
if there is already a binding for "maxim,max31855k". But I might be
wrong. Why is this compatible needed at all?

Also, the mikrobus-connector driver could translate the chipselects.

-michael

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Ayush Singh]

On 6/27/24 22:42, Michael Walle wrote:

> Hi,
>
> Could you give us a DT snippet of how this should look like with a
> board?
>
> On Thu Jun 27, 2024 at 6:26 PM CEST, Ayush Singh wrote:
>> +  board:
>> +    description: board attached to mikrobus connector
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> Shouldn't this be a subnode of the connector?
>
> i.e.
>
> connector {
> 	compatible = "mikrobus-connector";
>
> 	// phandles to the parent controllers
>
> 	spi {
> 		temp-sensor@0 {
> 			compatible = "maxim,max31855k";
> 			reg = <0>;
> 		};
> 	};
>
> 	i2c {
> 		..
> 	};
> };
>
> I don't think you can introduce a new
>    compatible = "maxim,max31855k", "mikrobus,spi";
> if there is already a binding for "maxim,max31855k". But I might be
> wrong. Why is this compatible needed at all?

So I did consider the design you just proposed, but I was not able to 
solve a few issues.

1. How to deal with say 2 mikrobus connectors in a single system?

My goal is to have only 1 overlay required for the board config at most. 
Ideally, I would actually like to add the dt for most mikroBUS boards to 
upstream and thus only the following overlay would be required:

```

&connector0 {

     board = <&temp-board>;

};

```


The problem with making it children is that each connector will require 
seperate overlays for board configs.


Additionally, there are boards with 1 wire eeprom available which can 
theselves store the overlay. In the current setup it will look as follows:

```

&mikrobus_board {

     thermo-sensor {

         ...

     };

};

```

Which is completely independent of the connector. If the same can be 
achieved with child-node as well, then I am open to doing that.


>
> Also, the mikrobus-connector driver could translate the chipselects.
>
> -michael

Yes, so it is currently doing that. Translating chip select name to the 
actual number. I am doing it the name way since some boards might use 
pins other than CS as chipselect and not all connectors will allow all 
pins to be used as chipselect.


Ayush Singh

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Michael Walle]

Hi,

On Thu Jun 27, 2024 at 7:29 PM CEST, Ayush Singh wrote:
> On 6/27/24 22:42, Michael Walle wrote:
>
> > Hi,
> >
> > Could you give us a DT snippet of how this should look like with a
> > board?
> >
> > On Thu Jun 27, 2024 at 6:26 PM CEST, Ayush Singh wrote:
> >> +  board:
> >> +    description: board attached to mikrobus connector
> >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > Shouldn't this be a subnode of the connector?
> >
> > i.e.
> >
> > connector {
> > 	compatible = "mikrobus-connector";
> >
> > 	// phandles to the parent controllers
> >
> > 	spi {
> > 		temp-sensor@0 {
> > 			compatible = "maxim,max31855k";
> > 			reg = <0>;
> > 		};
> > 	};
> >
> > 	i2c {
> > 		..
> > 	};
> > };
> >
> > I don't think you can introduce a new
> >    compatible = "maxim,max31855k", "mikrobus,spi";
> > if there is already a binding for "maxim,max31855k". But I might be
> > wrong. Why is this compatible needed at all?
>
> So I did consider the design you just proposed, but I was not able to 
> solve a few issues.
>
> 1. How to deal with say 2 mikrobus connectors in a single system?

Yes, interesting problem. That info should go into the cover letter.

> My goal is to have only 1 overlay required for the board config at most. 
> Ideally, I would actually like to add the dt for most mikroBUS boards to 
> upstream and thus only the following overlay would be required:
>
> ```
>
> &connector0 {
>
>      board = <&temp-board>;
>
> };

That's then per board, per click board. right?

>
> ```
>
>
> The problem with making it children is that each connector will require 
> seperate overlays for board configs.

Right.

> Additionally, there are boards with 1 wire eeprom available which can 
> theselves store the overlay. In the current setup it will look as follows:
>
> ```
>
> &mikrobus_board {

Where is that phandle pointing to? And what if there are two boards?

>
>      thermo-sensor {
>
>          ...
>
>      };
>
> };

But here you can have subnodes, no? These could then be just
enumerated as usual.

&mikrobus_board {
	mikrobus_gpio: gpio {
		gpio-controller;
		#gpio-cells = <1>;
	};

	spi {
		cs-gpios = <&mikrobus_gpio 1>;

		spi@0 {
			compatible = "mydevice";
			reg = <0>;
		};
	};
};


> ```

Not sure what this is, but my mail reader doesn't render RST? ;)

-michael

>
> Which is completely independent of the connector. If the same can be 
> achieved with child-node as well, then I am open to doing that.
>
>
> >
> > Also, the mikrobus-connector driver could translate the chipselects.
> >
> > -michael
>
> Yes, so it is currently doing that. Translating chip select name to the 
> actual number. I am doing it the name way since some boards might use 
> pins other than CS as chipselect and not all connectors will allow all 
> pins to be used as chipselect.
>
>
> Ayush Singh

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Andrew Lunn]

> That's then per board, per click board. right?
> 
> >
> > ```
> >
> >
> > The problem with making it children is that each connector will require 
> > seperate overlays for board configs.
> 
> Right.

For somebody who has not used overlays, could you expand on that.

Is it not possible to say "Overlay this DT fragment on point X of the
tree". So you populate children on a node. It should not matter if you
have the same children somewhere else, they have different parents?

     Andrew

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Ayush Singh]

>> But here you can have subnodes, no? These could then be just
>> enumerated as usual.
>>
>> &mikrobus_board {
>> 	mikrobus_gpio: gpio {
>> 		gpio-controller;
>> 		#gpio-cells = <1>;
>> 	};
>>
>> 	spi {
>> 		cs-gpios = <&mikrobus_gpio 1>;
>>
>> 		spi@0 {
>> 			compatible = "mydevice";
>> 			reg = <0>;
>> 		};
>> 	};
>> };
>>

Hi, I am now working on an approach for mikroBUS based on the apprach 
described here: [1]


I am thinking of the gpio-controller approach you seem to have used 
here. So I wanted to inquire if there already exists a gpio-controller 
driver that can create a proxy controller that forwards stuff to the 
underlying actual controller. So something like the following:


&mikrobus_gpio: gpio {

     gpio-controller;

     #gpio-cells = <2>;

     gpios = <&gpio1 0>, <&gpi2 1>;

};


spi {

     cs-gpios = <&mikrobus_gpio 1 GPIO_ACTIVE_HIGH>;

};


There does exist gpio0-virtio, but that seems to be for vm context.


[1]: 
https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/


Ayush Singh

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Rob Herring]

On Sat, Aug 31, 2024 at 1:11 PM Ayush Singh <ayush@beagleboard.org> wrote:
>
> >> But here you can have subnodes, no? These could then be just
> >> enumerated as usual.
> >>
> >> &mikrobus_board {
> >>      mikrobus_gpio: gpio {
> >>              gpio-controller;
> >>              #gpio-cells = <1>;
> >>      };
> >>
> >>      spi {
> >>              cs-gpios = <&mikrobus_gpio 1>;
> >>
> >>              spi@0 {
> >>                      compatible = "mydevice";
> >>                      reg = <0>;
> >>              };
> >>      };
> >> };
> >>
>
> Hi, I am now working on an approach for mikroBUS based on the apprach
> described here: [1]
>
>
> I am thinking of the gpio-controller approach you seem to have used
> here. So I wanted to inquire if there already exists a gpio-controller
> driver that can create a proxy controller that forwards stuff to the
> underlying actual controller.

gpio-map is what you are looking for. It's documented in the DT spec.
It was created exactly for this purpose of remapping GPIO lines on a
connector.

Rob

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Ayush Singh]

>> gpio-map is what you are looking for. It's documented in the DT spec.
>> It was created exactly for this purpose of remapping GPIO lines on a
>> connector.
>>
>> Rob


Hi. I found docs on nexus nodes [1] and tried using it for mikroBUS, but 
it does not seem to be working. Here is my connector:

```

     mikrobus_gpio0: mikrobus-gpio0 {
         #gpio-cells = <2>;
         gpio-map =
         <0 0 &main_gpio1 11 0>, <1 0 &main_gpio1 9 0>,
         <2 0 &main_gpio1 24 0>, <3 0 &main_gpio1 25 0>,
         <4 0 &main_gpio1 22 0>, <5 0 &main_gpio1 23 0>,
         <6 0 &main_gpio1 7 0>, <7 0 &main_gpio1 8 0>,
         <8 0 &main_gpio1 14 0>, <9 0 &main_gpio1 13 0>,
         <10 0 &main_gpio1 12 0>, <11 0 &main_gpio1 10 0>;
         gpio-map-mask = <0xf 0x0>;
         gpio-map-pass-thru = <0x0 0x1>;
     };

...

&main_uart5 {
     status = "okay";
     pinctrl-names = "default";
     pinctrl-0 = <&mikrobus_uart_pins_default>;

     gnss {
         compatible = "u-blox,neo-8";
         reset-gpios = <&mikrobus_gpio0 10 GPIO_ACTIVE_LOW>;
     };
};

```


After some fdtdump, I can see that at least the dtc compiler does not 
seem to do the forwarding at dt compile time. Here is the dump:

```

mikrobus-gpio0 {
         #gpio-cells = <0x00000002>;
         gpio-map = <0x00000000 0x00000000 0x00000025 0x0000000b 
0x00000000 0x00000001 0x00000000 0x00000025 0x00000009 0x00000000 
0x00000002 0x00000000 0x00000025 0x00000018 0x00000000 0x00000003 
0x00000000 0x00000025 0x00000019 0x00000000 0x00000004 0x00000000 
0x00000025 0x00000016 0x00000000 0x00000005 0x00000000 0x00000025 
0x00000017 0x00000000 0x00000006 0x00000000 0x00000025 0x00000007 
0x00000000 0x00000007 0x00000000 0x00000025 0x00000008 0x00000000 
0x00000008 0x00000000 0x00000025 0x0000000e 0x00000000 0x00000009 
0x00000000 0x00000025 0x0000000d 0x00000000 0x0000000a 0x00000000 
0x00000025 0x0000000c 0x00000000 0x0000000b 0x00000000 0x00000025 
0x0000000a 0x00000000>;
         gpio-map-mask = <0x0000000f 0x00000000>;
         gpio-map-pass-thru = <0x00000000 0x00000001>;
         phandle = <0x0000000e>;
     };

...

serial@2850000 {
             compatible = "ti,am64-uart", "ti,am654-uart";
             reg = <0x00000000 0x02850000 0x00000000 0x00000100>;
             interrupts = <0x00000000 0x000000b7 0x00000004>;
             power-domains = <0x00000003 0x0000009c 0x00000001>;
             clocks = <0x00000002 0x0000009c 0x00000000>;
             clock-names = "fclk";
             status = "okay";
             pinctrl-names = "default";
             pinctrl-0 = <0x0000000d>;
             phandle = <0x00000081>;
             gnss {
                 compatible = "u-blox,neo-8";
                 reset-gpios = <0x0000000e 0x0000000a 0x00000001>;
             };
  };

```


So I am a bit unsure. Is the dtc parser in the kernel supposed to do the 
mapping, or is it supposed to be done by `dtc` at compile time? Maybe we 
do not have support for it in upstream kernel yet? Or maybe I am missing 
something?


[1]: 
https://devicetree-specification.readthedocs.io/en/v0.3/devicetree-basics.html#nexus-nodes-and-specifier-mapping


Ayush Singh

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Rob Herring]

On Wed, Sep 4, 2024 at 12:09 PM Ayush Singh <ayush@beagleboard.org> wrote:
>
> >> gpio-map is what you are looking for. It's documented in the DT spec.
> >> It was created exactly for this purpose of remapping GPIO lines on a
> >> connector.
> >>
> >> Rob
>
>
> Hi. I found docs on nexus nodes [1] and tried using it for mikroBUS, but
> it does not seem to be working. Here is my connector:
>
> ```
>
>      mikrobus_gpio0: mikrobus-gpio0 {
>          #gpio-cells = <2>;
>          gpio-map =
>          <0 0 &main_gpio1 11 0>, <1 0 &main_gpio1 9 0>,
>          <2 0 &main_gpio1 24 0>, <3 0 &main_gpio1 25 0>,
>          <4 0 &main_gpio1 22 0>, <5 0 &main_gpio1 23 0>,
>          <6 0 &main_gpio1 7 0>, <7 0 &main_gpio1 8 0>,
>          <8 0 &main_gpio1 14 0>, <9 0 &main_gpio1 13 0>,
>          <10 0 &main_gpio1 12 0>, <11 0 &main_gpio1 10 0>;
>          gpio-map-mask = <0xf 0x0>;
>          gpio-map-pass-thru = <0x0 0x1>;
>      };
>
> ...
>
> &main_uart5 {
>      status = "okay";
>      pinctrl-names = "default";
>      pinctrl-0 = <&mikrobus_uart_pins_default>;
>
>      gnss {
>          compatible = "u-blox,neo-8";
>          reset-gpios = <&mikrobus_gpio0 10 GPIO_ACTIVE_LOW>;
>      };
> };
>
> ```
>
>
> After some fdtdump, I can see that at least the dtc compiler does not
> seem to do the forwarding at dt compile time. Here is the dump:

dtc knows nothing about it.

> ```
>
> mikrobus-gpio0 {
>          #gpio-cells = <0x00000002>;
>          gpio-map = <0x00000000 0x00000000 0x00000025 0x0000000b
> 0x00000000 0x00000001 0x00000000 0x00000025 0x00000009 0x00000000
> 0x00000002 0x00000000 0x00000025 0x00000018 0x00000000 0x00000003
> 0x00000000 0x00000025 0x00000019 0x00000000 0x00000004 0x00000000
> 0x00000025 0x00000016 0x00000000 0x00000005 0x00000000 0x00000025
> 0x00000017 0x00000000 0x00000006 0x00000000 0x00000025 0x00000007
> 0x00000000 0x00000007 0x00000000 0x00000025 0x00000008 0x00000000
> 0x00000008 0x00000000 0x00000025 0x0000000e 0x00000000 0x00000009
> 0x00000000 0x00000025 0x0000000d 0x00000000 0x0000000a 0x00000000
> 0x00000025 0x0000000c 0x00000000 0x0000000b 0x00000000 0x00000025
> 0x0000000a 0x00000000>;
>          gpio-map-mask = <0x0000000f 0x00000000>;
>          gpio-map-pass-thru = <0x00000000 0x00000001>;
>          phandle = <0x0000000e>;
>      };

You might need "gpio-controller" here. Though if you do, I think
that's a mistake in the kernel. It should work like interrupt-map and
generally you have either interrupt-controller or interrupt-map, but
not both (though that is allowed now too).

> ...
>
> serial@2850000 {
>              compatible = "ti,am64-uart", "ti,am654-uart";
>              reg = <0x00000000 0x02850000 0x00000000 0x00000100>;
>              interrupts = <0x00000000 0x000000b7 0x00000004>;
>              power-domains = <0x00000003 0x0000009c 0x00000001>;
>              clocks = <0x00000002 0x0000009c 0x00000000>;
>              clock-names = "fclk";
>              status = "okay";
>              pinctrl-names = "default";
>              pinctrl-0 = <0x0000000d>;
>              phandle = <0x00000081>;
>              gnss {
>                  compatible = "u-blox,neo-8";
>                  reset-gpios = <0x0000000e 0x0000000a 0x00000001>;
>              };
>   };
>
> ```
>
>
> So I am a bit unsure. Is the dtc parser in the kernel supposed to do the

No such thing as "dtc parser in the kernel".

> mapping, or is it supposed to be done by `dtc` at compile time?

No.

> Maybe we
> do not have support for it in upstream kernel yet?

Yes, there is upstream support. Grep for of_parse_phandle_with_args_map.

Rob

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Ayush Singh]

On 9/4/24 23:19, Rob Herring wrote:

> On Wed, Sep 4, 2024 at 12:09 PM Ayush Singh <ayush@beagleboard.org> wrote:
>>>> gpio-map is what you are looking for. It's documented in the DT spec.
>>>> It was created exactly for this purpose of remapping GPIO lines on a
>>>> connector.
>>>>
>>>> Rob
>>
>> Hi. I found docs on nexus nodes [1] and tried using it for mikroBUS, but
>> it does not seem to be working. Here is my connector:
>>
>> ```
>>
>>       mikrobus_gpio0: mikrobus-gpio0 {
>>           #gpio-cells = <2>;
>>           gpio-map =
>>           <0 0 &main_gpio1 11 0>, <1 0 &main_gpio1 9 0>,
>>           <2 0 &main_gpio1 24 0>, <3 0 &main_gpio1 25 0>,
>>           <4 0 &main_gpio1 22 0>, <5 0 &main_gpio1 23 0>,
>>           <6 0 &main_gpio1 7 0>, <7 0 &main_gpio1 8 0>,
>>           <8 0 &main_gpio1 14 0>, <9 0 &main_gpio1 13 0>,
>>           <10 0 &main_gpio1 12 0>, <11 0 &main_gpio1 10 0>;
>>           gpio-map-mask = <0xf 0x0>;
>>           gpio-map-pass-thru = <0x0 0x1>;
>>       };
>>
>> ...
>>
>> &main_uart5 {
>>       status = "okay";
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&mikrobus_uart_pins_default>;
>>
>>       gnss {
>>           compatible = "u-blox,neo-8";
>>           reset-gpios = <&mikrobus_gpio0 10 GPIO_ACTIVE_LOW>;
>>       };
>> };
>>
>> ```
>>
>>
>> After some fdtdump, I can see that at least the dtc compiler does not
>> seem to do the forwarding at dt compile time. Here is the dump:
> dtc knows nothing about it.
>
>> ```
>>
>> mikrobus-gpio0 {
>>           #gpio-cells = <0x00000002>;
>>           gpio-map = <0x00000000 0x00000000 0x00000025 0x0000000b
>> 0x00000000 0x00000001 0x00000000 0x00000025 0x00000009 0x00000000
>> 0x00000002 0x00000000 0x00000025 0x00000018 0x00000000 0x00000003
>> 0x00000000 0x00000025 0x00000019 0x00000000 0x00000004 0x00000000
>> 0x00000025 0x00000016 0x00000000 0x00000005 0x00000000 0x00000025
>> 0x00000017 0x00000000 0x00000006 0x00000000 0x00000025 0x00000007
>> 0x00000000 0x00000007 0x00000000 0x00000025 0x00000008 0x00000000
>> 0x00000008 0x00000000 0x00000025 0x0000000e 0x00000000 0x00000009
>> 0x00000000 0x00000025 0x0000000d 0x00000000 0x0000000a 0x00000000
>> 0x00000025 0x0000000c 0x00000000 0x0000000b 0x00000000 0x00000025
>> 0x0000000a 0x00000000>;
>>           gpio-map-mask = <0x0000000f 0x00000000>;
>>           gpio-map-pass-thru = <0x00000000 0x00000001>;
>>           phandle = <0x0000000e>;
>>       };
> You might need "gpio-controller" here. Though if you do, I think
> that's a mistake in the kernel. It should work like interrupt-map and
> generally you have either interrupt-controller or interrupt-map, but
> not both (though that is allowed now too).
>
>> ...
>>
>> serial@2850000 {
>>               compatible = "ti,am64-uart", "ti,am654-uart";
>>               reg = <0x00000000 0x02850000 0x00000000 0x00000100>;
>>               interrupts = <0x00000000 0x000000b7 0x00000004>;
>>               power-domains = <0x00000003 0x0000009c 0x00000001>;
>>               clocks = <0x00000002 0x0000009c 0x00000000>;
>>               clock-names = "fclk";
>>               status = "okay";
>>               pinctrl-names = "default";
>>               pinctrl-0 = <0x0000000d>;
>>               phandle = <0x00000081>;
>>               gnss {
>>                   compatible = "u-blox,neo-8";
>>                   reset-gpios = <0x0000000e 0x0000000a 0x00000001>;
>>               };
>>    };
>>
>> ```
>>
>>
>> So I am a bit unsure. Is the dtc parser in the kernel supposed to do the
> No such thing as "dtc parser in the kernel".
>
>> mapping, or is it supposed to be done by `dtc` at compile time?
> No.
>
>> Maybe we
>> do not have support for it in upstream kernel yet?
> Yes, there is upstream support. Grep for of_parse_phandle_with_args_map.
>
> Rob


So, after a bit of troubleshooting, it seems that a nexus node should 
not be present at root level (unless it also has an actual driver). If 
the nexus node is a root node without an actual driver, anything 
referring to the node is deferred.


I am still a bit unsure if I should make the `mikrobus-connector` itself 
a nexus node or if I should have a subnode named `mikrobus_gpio`, but at 
least things seem to be working now. So, thanks for your help.


Ayush Singh

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Rob Herring]

On Thu, Jun 27, 2024 at 10:59:46PM +0530, Ayush Singh wrote:
> On 6/27/24 22:42, Michael Walle wrote:
> 
> > Hi,
> > 
> > Could you give us a DT snippet of how this should look like with a
> > board?
> > 
> > On Thu Jun 27, 2024 at 6:26 PM CEST, Ayush Singh wrote:
> > > +  board:
> > > +    description: board attached to mikrobus connector
> > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > Shouldn't this be a subnode of the connector?
> > 
> > i.e.
> > 
> > connector {
> > 	compatible = "mikrobus-connector";
> > 
> > 	// phandles to the parent controllers

These are per bus, so put them in the child bus nodes:

> > 
> > 	spi {

                spi-bus = <&spiN>;
                spi-cs = ...

The base DT would have the spi node and these properties. The overlay 
would still apply to the connector node, but also have the 'spi' node 
along with the devices.

Note that whatever is done here, I expect to work on any connector with 
SPI, I2C, etc. So structuring the bindings for that would be nice. There 
is also this effort which needs the same bindings[1].


> > 		temp-sensor@0 {
> > 			compatible = "maxim,max31855k";
> > 			reg = <0>;
> > 		};
> > 	};
> > 
> > 	i2c {
> > 		..
> > 	};
> > };
> > 
> > I don't think you can introduce a new
> >    compatible = "maxim,max31855k", "mikrobus,spi";
> > if there is already a binding for "maxim,max31855k". But I might be
> > wrong. Why is this compatible needed at all?
> 
> So I did consider the design you just proposed, but I was not able to solve
> a few issues.
> 
> 1. How to deal with say 2 mikrobus connectors in a single system?

I don't understand why that's a problem? It's no different than the same 
overlay working on multiple vendor's boards which I imagine you want 
too. The connector node in the base DT has to remap everything from base 
DT into a mikrobus defined number/name space. For example, host GPIO N 
is mapped to mikrobus connector GPIO 0 and so on.

There is one issue in knowing what the target node is. Standardizing the 
target path or connector node label only works for 1 connector per 
system. You can have an empty target path in the overlay and something 
else can decide the target. This is what's being done for overlays with 
the dynamic PCI nodes. For example, maybe an eeprom tells the driver 
what overlay to apply.

Rob


[1] https://lore.kernel.org/all/20240510-hotplug-drm-bridge-v2-0-ec32f2c66d56@bootlin.com/

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Rob Herring]

On Thu, Jun 27, 2024 at 09:56:11PM +0530, Ayush Singh wrote:
> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
> developed by MikroElektronika for connecting add-on boards to
> microcontrollers or microprocessors.
> 
> mikroBUS is a connector and does not have a controller. Instead the
> software is responsible for identification of board and setting up uart,
> spi, i2c, pwm and other buses. Additionally, some new mikroBUS boards
> contain 1-wire EEPROM that contains a manifest to describe the addon
> board to provide plug and play capabilities.
> 
> A mikroBUS addon board is free to leave some of the pins unused which
> are marked as NC or Not Connected.
> 
> Some of the pins might need to be configured as GPIOs deviating from their
> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.
> 
> For some add-on boards the driver may not take care of some additional
> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
> (RST pin on the mikrobus port) needs to be pulled high.
> 
> Some SPI addon boards use other pins like RST, AN etc as chipselect (eg.
> SPI Extend Click). Thus, `spi-cs` and `spi-cs-names` property is added
> to allow mikroBUS addon board to specify chipselect by name.
> 
> Here's the list of pins in mikroBUS connector:
> AN - Analog
> RST - Reset
> CS - SPI Chip Select
> SCK - SPI Clock
> MISO - SPI Master Input Slave Output
> MOSI - SPI Master Output Slave Input
> +3.3V - VCC-3.3V power
> GND - Reference Ground
> PWM - PWM output
> INT - Hardware Interrupt
> RX - UART Receive
> TX - UART Transmit
> SCL - I2C Clock
> SDA - I2C Data
> +5V - VCC-5V power
> GND - Reference Ground
> 
> Link: https://www.mikroe.com/mikrobus
> Link:
> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
> mikroBUS specification
> Link: https://www.mikroe.com/sht1x-click SHT15 Click
> Link: https://www.mikroe.com/eth-click ENC28J60 Click
> Link: https://www.mikroe.com/spi-extend-click SPI Extend Click
> 
> Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org>
> Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  .../bindings/connector/mikrobus-connector.yaml     | 107 +++++++++++++++++++++
>  MAINTAINERS                                        |   6 ++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
> new file mode 100644
> index 000000000000..033479f8604f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/connector/mikrobus-connector.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: mikroBUS add-on board connector
> +
> +maintainers:
> +  - Ayush Singh <ayush@beagleboard.org>
> +
> +properties:
> +  compatible:
> +    const: mikrobus-connector
> +
> +  pinctrl-0: true
> +  pinctrl-1: true
> +  pinctrl-2: true
> +  pinctrl-3: true
> +  pinctrl-4: true
> +  pinctrl-5: true
> +  pinctrl-6: true
> +  pinctrl-7: true
> +  pinctrl-8: true
> +
> +  pinctrl-names:
> +    minItems: 1
> +    maxItems: 9
> +    items:
> +      enum: [default, pwm_default, pwm_gpio, uart_default, uart_gpio, i2c_default, i2c_gpio, spi_default,
> +             spi_gpio]

Generally, each pinctrl-N is mutually exclusive. It looks like here you 
want multiple states active at one time. Does this work?

> +
> +  spi-controller:
> +    description: spi-controller of mikroBUS SPI pins along with cs pins.
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  spi-cs:
> +    description: spi chip-select corresponding to the chip-selects on the mikrobus socket.

Wrap lines at 80 char.

The array index is the chip-select number on the connector and the 
value is the host SPI controller CS numbers? Or the other way around? 
This needs a better description.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array

Maximum number of entries? 

> +
> +  spi-cs-names:
> +    minItems: 1
> +    maxItems: 12
> +    items:
> +      enum: [default, pwm, int, rx, tx, scl, sda, an, rst, sck, cipo, copi]
> +
> +  i2c-controller:
> +    description: i2c controller attached to the mikrobus socket.
> +    $ref: /schemas/types.yaml#/definitions/phandle

i2c-bus is the somewhat standard property for this.

Really, I'd expect connectors to look something like this:

connector {
  i2c-0 {
    i2c-bus = <&i2c3>;
    #address-cells = <1>;
    #size-cells = <0>;
    device@12 {
      compatible = "some-i2c-device";
    };
  };
};

That form allows for multiple buses (of the same type or different) on 
the connector.

> +
> +  uart-controller:
> +    description: uart controller attached to the mikrobus socket
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  pwms:
> +    description: the pwm-controller corresponding to the mikroBUS PWM pin.
> +    maxItems: 1
> +
> +  mikrobus-gpios:
> +    minItems: 1
> +    maxItems: 12
> +
> +  mikrobus-gpio-names:

The GPIO binding does not work this way as the name is in the property. 
Either drop if you want to keep the array or you have to do something 
like this:

pwm-gpios
int-gpios
rx-gpios

Really, the intention was for connectors to use gpio-map property to 
renumber GPIOs relative to the connector.

> +    minItems: 1
> +    maxItems: 12
> +    items:
> +      enum: [pwm, int, rx, tx, scl, sda, an, rst, cs, sck, cipo, copi]
> +
> +  board:
> +    description: board attached to mikrobus connector
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

What is this for?

> +
> +required:
> +  - compatible
> +  - pinctrl-0
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    mikrobus {
> +      compatible = "mikrobus-connector";
> +      pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",
> +                      "i2c_gpio", "spi_default", "spi_gpio";
> +      pinctrl-0 = <&P2_03_gpio_input_pin &P1_04_gpio_pin &P1_02_gpio_pin>;
> +      pinctrl-1 = <&P2_01_pwm_pin>;
> +      pinctrl-2 = <&P2_01_gpio_pin>;
> +      pinctrl-3 = <&P2_05_uart_pin &P2_07_uart_pin>;
> +      pinctrl-4 = <&P2_05_gpio_pin &P2_07_gpio_pin>;
> +      pinctrl-5 = <&P2_09_i2c_pin &P2_11_i2c_pin>;
> +      pinctrl-6 = <&P2_09_gpio_pin &P2_11_gpio_pin>;
> +      pinctrl-7 = <&P1_12_spi_pin &P1_10_spi_pin &P1_08_spi_sclk_pin &P1_06_spi_cs_pin>;
> +      pinctrl-8 = <&P1_12_gpio_pin &P1_10_gpio_pin &P1_08_gpio_pin &P1_06_gpio_pin>;
> +      pwms = <&ehrpwm1 0 500000 0>;
> +      i2c-controller = <&i2c1>;
> +      uart-controller = <&uart1>;
> +      spi-controller = <&spi1>;
> +      spi-cs = <0 1>;
> +      spi-cs-names = "default", "rst";
> +      mikrobus-gpios = <&gpio1 18 GPIO_ACTIVE_HIGH>, <&gpio0 23 GPIO_ACTIVE_HIGH>,
> +                       <&gpio0 30 GPIO_ACTIVE_HIGH>, <&gpio0 31 GPIO_ACTIVE_HIGH>,
> +                       <&gpio0 15 GPIO_ACTIVE_HIGH>, <&gpio0 14 GPIO_ACTIVE_HIGH>,
> +                       <&gpio0 4 GPIO_ACTIVE_HIGH>, <&gpio0 3 GPIO_ACTIVE_HIGH>,
> +                       <&gpio0 2 GPIO_ACTIVE_HIGH>, <&gpio0 5 GPIO_ACTIVE_HIGH>,
> +                       <&gpio2 25 GPIO_ACTIVE_HIGH>, <&gpio2 3 GPIO_ACTIVE_HIGH>;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 807feae089c4..8e4115e93aeb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15108,6 +15108,12 @@ M:	Oliver Neukum <oliver@neukum.org>
>  S:	Maintained
>  F:	drivers/usb/image/microtek.*
>  
> +MIKROBUS
> +M:	Ayush Singh <ayush@beagleboard.org>
> +M:	Vaishnav M A <vaishnav@beagleboard.org>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
> +
>  MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
>  M:	Luka Kovacic <luka.kovacic@sartura.hr>
>  M:	Luka Perkov <luka.perkov@sartura.hr>
> 
> -- 
> 2.45.2
> 

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Ayush Singh]

On 6/28/24 21:58, Rob Herring wrote:

> On Thu, Jun 27, 2024 at 09:56:11PM +0530, Ayush Singh wrote:
>> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
>> developed by MikroElektronika for connecting add-on boards to
>> microcontrollers or microprocessors.
>>
>> mikroBUS is a connector and does not have a controller. Instead the
>> software is responsible for identification of board and setting up uart,
>> spi, i2c, pwm and other buses. Additionally, some new mikroBUS boards
>> contain 1-wire EEPROM that contains a manifest to describe the addon
>> board to provide plug and play capabilities.
>>
>> A mikroBUS addon board is free to leave some of the pins unused which
>> are marked as NC or Not Connected.
>>
>> Some of the pins might need to be configured as GPIOs deviating from their
>> reserved purposes Eg: SHT15 Click where the SCL and SDA Pins need to be
>> configured as GPIOs for the driver (drivers/hwmon/sht15.c) to work.
>>
>> For some add-on boards the driver may not take care of some additional
>> signals like reset/wake-up/other. Eg: ENC28J60 click where the reset line
>> (RST pin on the mikrobus port) needs to be pulled high.
>>
>> Some SPI addon boards use other pins like RST, AN etc as chipselect (eg.
>> SPI Extend Click). Thus, `spi-cs` and `spi-cs-names` property is added
>> to allow mikroBUS addon board to specify chipselect by name.
>>
>> Here's the list of pins in mikroBUS connector:
>> AN - Analog
>> RST - Reset
>> CS - SPI Chip Select
>> SCK - SPI Clock
>> MISO - SPI Master Input Slave Output
>> MOSI - SPI Master Output Slave Input
>> +3.3V - VCC-3.3V power
>> GND - Reference Ground
>> PWM - PWM output
>> INT - Hardware Interrupt
>> RX - UART Receive
>> TX - UART Transmit
>> SCL - I2C Clock
>> SDA - I2C Data
>> +5V - VCC-5V power
>> GND - Reference Ground
>>
>> Link: https://www.mikroe.com/mikrobus
>> Link:
>> https://download.mikroe.com/documents/standards/mikrobus/mikrobus-standard-specification-v200.pdf
>> mikroBUS specification
>> Link: https://www.mikroe.com/sht1x-click SHT15 Click
>> Link: https://www.mikroe.com/eth-click ENC28J60 Click
>> Link: https://www.mikroe.com/spi-extend-click SPI Extend Click
>>
>> Co-developed-by: Vaishnav M A <vaishnav@beagleboard.org>
>> Signed-off-by: Vaishnav M A <vaishnav@beagleboard.org>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   .../bindings/connector/mikrobus-connector.yaml     | 107 +++++++++++++++++++++
>>   MAINTAINERS                                        |   6 ++
>>   2 files changed, 113 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
>> new file mode 100644
>> index 000000000000..033479f8604f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
>> @@ -0,0 +1,107 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/connector/mikrobus-connector.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: mikroBUS add-on board connector
>> +
>> +maintainers:
>> +  - Ayush Singh <ayush@beagleboard.org>
>> +
>> +properties:
>> +  compatible:
>> +    const: mikrobus-connector
>> +
>> +  pinctrl-0: true
>> +  pinctrl-1: true
>> +  pinctrl-2: true
>> +  pinctrl-3: true
>> +  pinctrl-4: true
>> +  pinctrl-5: true
>> +  pinctrl-6: true
>> +  pinctrl-7: true
>> +  pinctrl-8: true
>> +
>> +  pinctrl-names:
>> +    minItems: 1
>> +    maxItems: 9
>> +    items:
>> +      enum: [default, pwm_default, pwm_gpio, uart_default, uart_gpio, i2c_default, i2c_gpio, spi_default,
>> +             spi_gpio]
> Generally, each pinctrl-N is mutually exclusive. It looks like here you
> want multiple states active at one time. Does this work?

I see. In mikrobus case, these pinctrl are not mutually exclusive. The 
ones that are mutually exclusive are as follows:

- pwm_default and pwm_gpio

- uart_default and uart_gpio

- i2c_default and i2c_gpio

- spi_default and spi_gpio


It still does lead to 16 combinations so not sure if it is the best 
approach.

>> +
>> +  spi-controller:
>> +    description: spi-controller of mikroBUS SPI pins along with cs pins.
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> +  spi-cs:
>> +    description: spi chip-select corresponding to the chip-selects on the mikrobus socket.
> Wrap lines at 80 char.
>
> The array index is the chip-select number on the connector and the
> value is the host SPI controller CS numbers? Or the other way around?
> This needs a better description.
>
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> Maximum number of entries?
>
>> +
>> +  spi-cs-names:
>> +    minItems: 1
>> +    maxItems: 12
>> +    items:
>> +      enum: [default, pwm, int, rx, tx, scl, sda, an, rst, sck, cipo, copi]
>> +
>> +  i2c-controller:
>> +    description: i2c controller attached to the mikrobus socket.
>> +    $ref: /schemas/types.yaml#/definitions/phandle
> i2c-bus is the somewhat standard property for this.
>
> Really, I'd expect connectors to look something like this:
>
> connector {
>    i2c-0 {
>      i2c-bus = <&i2c3>;
>      #address-cells = <1>;
>      #size-cells = <0>;
>      device@12 {
>        compatible = "some-i2c-device";
>      };
>    };
> };
>
> That form allows for multiple buses (of the same type or different) on
> the connector.
>
>> +
>> +  uart-controller:
>> +    description: uart controller attached to the mikrobus socket
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> +  pwms:
>> +    description: the pwm-controller corresponding to the mikroBUS PWM pin.
>> +    maxItems: 1
>> +
>> +  mikrobus-gpios:
>> +    minItems: 1
>> +    maxItems: 12
>> +
>> +  mikrobus-gpio-names:
> The GPIO binding does not work this way as the name is in the property.
> Either drop if you want to keep the array or you have to do something
> like this:
>
> pwm-gpios
> int-gpios
> rx-gpios
>
> Really, the intention was for connectors to use gpio-map property to
> renumber GPIOs relative to the connector.

Can you point me to what you mean by gpio-map property?

>> +    minItems: 1
>> +    maxItems: 12
>> +    items:
>> +      enum: [pwm, int, rx, tx, scl, sda, an, rst, cs, sck, cipo, copi]
>> +
>> +  board:
>> +    description: board attached to mikrobus connector
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> What is this for?
>
>> +
>> +required:
>> +  - compatible
>> +  - pinctrl-0
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    mikrobus {
>> +      compatible = "mikrobus-connector";
>> +      pinctrl-names = "default", "pwm_default", "pwm_gpio","uart_default", "uart_gpio", "i2c_default",
>> +                      "i2c_gpio", "spi_default", "spi_gpio";
>> +      pinctrl-0 = <&P2_03_gpio_input_pin &P1_04_gpio_pin &P1_02_gpio_pin>;
>> +      pinctrl-1 = <&P2_01_pwm_pin>;
>> +      pinctrl-2 = <&P2_01_gpio_pin>;
>> +      pinctrl-3 = <&P2_05_uart_pin &P2_07_uart_pin>;
>> +      pinctrl-4 = <&P2_05_gpio_pin &P2_07_gpio_pin>;
>> +      pinctrl-5 = <&P2_09_i2c_pin &P2_11_i2c_pin>;
>> +      pinctrl-6 = <&P2_09_gpio_pin &P2_11_gpio_pin>;
>> +      pinctrl-7 = <&P1_12_spi_pin &P1_10_spi_pin &P1_08_spi_sclk_pin &P1_06_spi_cs_pin>;
>> +      pinctrl-8 = <&P1_12_gpio_pin &P1_10_gpio_pin &P1_08_gpio_pin &P1_06_gpio_pin>;
>> +      pwms = <&ehrpwm1 0 500000 0>;
>> +      i2c-controller = <&i2c1>;
>> +      uart-controller = <&uart1>;
>> +      spi-controller = <&spi1>;
>> +      spi-cs = <0 1>;
>> +      spi-cs-names = "default", "rst";
>> +      mikrobus-gpios = <&gpio1 18 GPIO_ACTIVE_HIGH>, <&gpio0 23 GPIO_ACTIVE_HIGH>,
>> +                       <&gpio0 30 GPIO_ACTIVE_HIGH>, <&gpio0 31 GPIO_ACTIVE_HIGH>,
>> +                       <&gpio0 15 GPIO_ACTIVE_HIGH>, <&gpio0 14 GPIO_ACTIVE_HIGH>,
>> +                       <&gpio0 4 GPIO_ACTIVE_HIGH>, <&gpio0 3 GPIO_ACTIVE_HIGH>,
>> +                       <&gpio0 2 GPIO_ACTIVE_HIGH>, <&gpio0 5 GPIO_ACTIVE_HIGH>,
>> +                       <&gpio2 25 GPIO_ACTIVE_HIGH>, <&gpio2 3 GPIO_ACTIVE_HIGH>;
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 807feae089c4..8e4115e93aeb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15108,6 +15108,12 @@ M:	Oliver Neukum <oliver@neukum.org>
>>   S:	Maintained
>>   F:	drivers/usb/image/microtek.*
>>   
>> +MIKROBUS
>> +M:	Ayush Singh <ayush@beagleboard.org>
>> +M:	Vaishnav M A <vaishnav@beagleboard.org>
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
>> +
>>   MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
>>   M:	Luka Kovacic <luka.kovacic@sartura.hr>
>>   M:	Luka Perkov <luka.perkov@sartura.hr>
>>
>> -- 
>> 2.45.2
>>

I am switching to child-node based structure from the next patch since I 
was able to replicate applying board overlay on a generic connector with 
child node.


Ayush Singh

Re: [PATCH v5 1/7] dt-bindings: connector: Add mikrobus-connector

[Rob Herring]

On Tue, Jul 2, 2024 at 9:14 AM Ayush Singh <ayush@beagleboard.org> wrote:
>
> On 6/28/24 21:58, Rob Herring wrote:
>
> > On Thu, Jun 27, 2024 at 09:56:11PM +0530, Ayush Singh wrote:
> >> Add DT bindings for mikroBUS interface. MikroBUS is an open standard
> >> developed by MikroElektronika for connecting add-on boards to
> >> microcontrollers or microprocessors.

[...]

> >> +  mikrobus-gpios:
> >> +    minItems: 1
> >> +    maxItems: 12
> >> +
> >> +  mikrobus-gpio-names:
> > The GPIO binding does not work this way as the name is in the property.
> > Either drop if you want to keep the array or you have to do something
> > like this:
> >
> > pwm-gpios
> > int-gpios
> > rx-gpios
> >
> > Really, the intention was for connectors to use gpio-map property to
> > renumber GPIOs relative to the connector.
>
> Can you point me to what you mean by gpio-map property?

It is defined in the DT specification.

Rob

[PATCH v5 2/7] dt-bindings: mikrobus: Add mikrobus board base

[Ayush Singh]

Base dt bindings for mikrobus addon boards. Contains properties that are
part of all types of boards (SPI, I2C, etc).

Each pin in mikroBUS connector can either be used for it's original
purpose (UART, I2C, SPI, etc) or as a normal GPIO. Introducing
`pinctrl-apply` allows selecting the pin configuration by name.

Note: Some mikrobus-connectors might not support all valid pinctrl.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 .../devicetree/bindings/mikrobus/mikrobus-board.yaml | 20 ++++++++++++++++++++
 MAINTAINERS                                          |  1 +
 2 files changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml b/Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
new file mode 100644
index 000000000000..42e2219c596f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mikrobus/mikrobus-board.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: mikroBUS add-on board properties
+
+maintainers:
+  - Ayush Singh <ayush@beagleboard.org>
+
+properties:
+  pinctrl-apply:
+    minItems: 1
+    maxItems: 9
+    items:
+      enum: [default, pwm_default, pwm_gpio, uart_default, uart_gpio, i2c_default, i2c_gpio, spi_default,
+             spi_gpio]
+
+additionalProperties: false
diff --git a/MAINTAINERS b/MAINTAINERS
index 8e4115e93aeb..14eba18832d5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15113,6 +15113,7 @@ M:	Ayush Singh <ayush@beagleboard.org>
 M:	Vaishnav M A <vaishnav@beagleboard.org>
 S:	Maintained
 F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
+F:	Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
 
 MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
 M:	Luka Kovacic <luka.kovacic@sartura.hr>

-- 
2.45.2

Re: [PATCH v5 2/7] dt-bindings: mikrobus: Add mikrobus board base

[Rob Herring]

On Thu, Jun 27, 2024 at 09:56:12PM +0530, Ayush Singh wrote:
> Base dt bindings for mikrobus addon boards. Contains properties that are
> part of all types of boards (SPI, I2C, etc).
> 
> Each pin in mikroBUS connector can either be used for it's original
> purpose (UART, I2C, SPI, etc) or as a normal GPIO. Introducing
> `pinctrl-apply` allows selecting the pin configuration by name.

This seems pointless. If a board uses UART, then uart_default has to be 
supported. Why does the board need to list it?

> 
> Note: Some mikrobus-connectors might not support all valid pinctrl.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  .../devicetree/bindings/mikrobus/mikrobus-board.yaml | 20 ++++++++++++++++++++
>  MAINTAINERS                                          |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml b/Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
> new file mode 100644
> index 000000000000..42e2219c596f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mikrobus/mikrobus-board.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: mikroBUS add-on board properties
> +
> +maintainers:
> +  - Ayush Singh <ayush@beagleboard.org>
> +
> +properties:
> +  pinctrl-apply:

Missing a description.

> +    minItems: 1
> +    maxItems: 9
> +    items:
> +      enum: [default, pwm_default, pwm_gpio, uart_default, uart_gpio, i2c_default, i2c_gpio, spi_default,
> +             spi_gpio]
> +
> +additionalProperties: false
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e4115e93aeb..14eba18832d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15113,6 +15113,7 @@ M:	Ayush Singh <ayush@beagleboard.org>
>  M:	Vaishnav M A <vaishnav@beagleboard.org>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
> +F:	Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
>  
>  MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
>  M:	Luka Kovacic <luka.kovacic@sartura.hr>
> 
> -- 
> 2.45.2
> 

[PATCH v5 3/7] dt-bindings: mikrobus: Add mikrobus-spi binding

[Ayush Singh]

Add bindings for MikroBUS boards using SPI interface.

Almost all of the properties that are valid for SPI devices can be used
except reg. Since the goal is to allow use of the same MikroBUS board
across different connectors, config needs to be independent of the actual
SPI controller in mikroBUS port(s), it is not possible to define the
chipselect by number in advance. Thus, `spi-cs-apply` property is used to
specify the chipselect(s) by name.

Another important fact is that while there is a CS pin in the mikroBUS
connector, some boards (eg SPI Extend Click) use additional pins as
chipselect. Thus we need a way to specify the CS pin(s) in terms of
mikcrobus-connector which can then handle bindings the actual CS pin(s).

Link: https://www.mikroe.com/spi-extend-click SPI Extend Click

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 .../devicetree/bindings/mikrobus/mikrobus-spi.yaml | 37 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml b/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml
new file mode 100644
index 000000000000..35ca2cce3b03
--- /dev/null
+++ b/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mikrobus/mikrobus-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: mikroBUS add-on board using SPI
+
+maintainers:
+  - Ayush Singh <ayush@beagleboard.org>
+
+allOf:
+  - $ref: /schemas/mikrobus/mikrobus-board.yaml#
+
+properties:
+  compatible:
+    const: mikrobus-spi
+
+  spi-cs-apply:
+    minItems: 1
+    maxItems: 12
+    items:
+      enum: [default, pwm, int, rx, tx, scl, sda, an, rst, sck, cipo, copi]
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    thermo-click {
+      compatible = "maxim,max31855k", "mikrobus,spi";
+      spi-max-frequency = <1000000>;
+      pinctrl-apply = "default", "spi_default";
+      spi-cs-apply = "default";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 14eba18832d5..88f2b3adc824 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15114,6 +15114,7 @@ M:	Vaishnav M A <vaishnav@beagleboard.org>
 S:	Maintained
 F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
 F:	Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
+F:	Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml
 
 MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
 M:	Luka Kovacic <luka.kovacic@sartura.hr>

-- 
2.45.2

Re: [PATCH v5 3/7] dt-bindings: mikrobus: Add mikrobus-spi binding

[Rob Herring (Arm)]

On Thu, 27 Jun 2024 21:56:13 +0530, Ayush Singh wrote:
> Add bindings for MikroBUS boards using SPI interface.
> 
> Almost all of the properties that are valid for SPI devices can be used
> except reg. Since the goal is to allow use of the same MikroBUS board
> across different connectors, config needs to be independent of the actual
> SPI controller in mikroBUS port(s), it is not possible to define the
> chipselect by number in advance. Thus, `spi-cs-apply` property is used to
> specify the chipselect(s) by name.
> 
> Another important fact is that while there is a CS pin in the mikroBUS
> connector, some boards (eg SPI Extend Click) use additional pins as
> chipselect. Thus we need a way to specify the CS pin(s) in terms of
> mikcrobus-connector which can then handle bindings the actual CS pin(s).
> 
> Link: https://www.mikroe.com/spi-extend-click SPI Extend Click
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  .../devicetree/bindings/mikrobus/mikrobus-spi.yaml | 37 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 38 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.example.dtb: thermo-click: compatible: ['maxim,max31855k', 'mikrobus,spi'] is too long
	from schema $id: http://devicetree.org/schemas/iio/temperature/maxim,max31855k.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.example.dtb: thermo-click: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/iio/temperature/maxim,max31855k.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.example.dtb: thermo-click: Unevaluated properties are not allowed ('compatible', 'pinctrl-apply', 'spi-cs-apply' were unexpected)
	from schema $id: http://devicetree.org/schemas/iio/temperature/maxim,max31855k.yaml#
Documentation/devicetree/bindings/mikrobus/mikrobus-spi.example.dtb: /example-0/thermo-click: failed to match any schema with compatible: ['maxim,max31855k', 'mikrobus,spi']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240627-mikrobus-scratch-spi-v5-3-9e6c148bf5f0@beagleboard.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v5 3/7] dt-bindings: mikrobus: Add mikrobus-spi binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3339 bytes --]

On Thu, Jun 27, 2024 at 09:56:13PM +0530, Ayush Singh wrote:
> Add bindings for MikroBUS boards using SPI interface.
> 
> Almost all of the properties that are valid for SPI devices can be used
> except reg. Since the goal is to allow use of the same MikroBUS board
> across different connectors, config needs to be independent of the actual
> SPI controller in mikroBUS port(s), it is not possible to define the
> chipselect by number in advance. Thus, `spi-cs-apply` property is used to
> specify the chipselect(s) by name.
> 
> Another important fact is that while there is a CS pin in the mikroBUS
> connector, some boards (eg SPI Extend Click) use additional pins as
> chipselect. Thus we need a way to specify the CS pin(s) in terms of
> mikcrobus-connector which can then handle bindings the actual CS pin(s).
> 
> Link: https://www.mikroe.com/spi-extend-click SPI Extend Click
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  .../devicetree/bindings/mikrobus/mikrobus-spi.yaml | 37 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml b/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml
> new file mode 100644
> index 000000000000..35ca2cce3b03
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mikrobus/mikrobus-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: mikroBUS add-on board using SPI
> +
> +maintainers:
> +  - Ayush Singh <ayush@beagleboard.org>
> +
> +allOf:
> +  - $ref: /schemas/mikrobus/mikrobus-board.yaml#
> +
> +properties:
> +  compatible:
> +    const: mikrobus-spi
> +
> +  spi-cs-apply:
> +    minItems: 1
> +    maxItems: 12
> +    items:
> +      enum: [default, pwm, int, rx, tx, scl, sda, an, rst, sck, cipo, copi]

Property descriptions belong in the property, not in the commit message.


> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    thermo-click {
> +      compatible = "maxim,max31855k", "mikrobus,spi";

I am really not keen on what this implies, as I think Rob and I have
already mentioned, the connector should handle the "mapping" and the
regular SPI/I2C/whatever bindings for the SPI devices themselves
should be usable.

Also you clearly didn't test this binding - please test them.

Thanks,
Conor.

> +      spi-max-frequency = <1000000>;
> +      pinctrl-apply = "default", "spi_default";
> +      spi-cs-apply = "default";
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 14eba18832d5..88f2b3adc824 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15114,6 +15114,7 @@ M:	Vaishnav M A <vaishnav@beagleboard.org>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
>  F:	Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
> +F:	Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml
>  
>  MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
>  M:	Luka Kovacic <luka.kovacic@sartura.hr>
> 
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 3/7] dt-bindings: mikrobus: Add mikrobus-spi binding

[Geert Uytterhoeven]

On Fri, Jun 28, 2024 at 6:48 PM Conor Dooley <conor@kernel.org> wrote:
> On Thu, Jun 27, 2024 at 09:56:13PM +0530, Ayush Singh wrote:
> > Add bindings for MikroBUS boards using SPI interface.
> >
> > Almost all of the properties that are valid for SPI devices can be used
> > except reg. Since the goal is to allow use of the same MikroBUS board
> > across different connectors, config needs to be independent of the actual
> > SPI controller in mikroBUS port(s), it is not possible to define the
> > chipselect by number in advance. Thus, `spi-cs-apply` property is used to
> > specify the chipselect(s) by name.
> >
> > Another important fact is that while there is a CS pin in the mikroBUS
> > connector, some boards (eg SPI Extend Click) use additional pins as
> > chipselect. Thus we need a way to specify the CS pin(s) in terms of
> > mikcrobus-connector which can then handle bindings the actual CS pin(s).
> >
> > Link: https://www.mikroe.com/spi-extend-click SPI Extend Click
> >
> > Signed-off-by: Ayush Singh <ayush@beagleboard.org>

Thanks for your patch!

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml

> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    thermo-click {
> > +      compatible = "maxim,max31855k", "mikrobus,spi";
>
> I am really not keen on what this implies, as I think Rob and I have
> already mentioned, the connector should handle the "mapping" and the
> regular SPI/I2C/whatever bindings for the SPI devices themselves
> should be usable.

Indeed.

The (thermocouple component on the) click itself is not compatible with
"mikrobus,spi", but with "maxim,max31855k". "mikrobus,spi" here means
SPI is used as the transport layer.

I guess you need "mikrobus,spi" because the click is pointed to by the
"board" phandle in the connector, instead of being a subnode of the
connector, like it should be?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v5 4/7] spi: Make of_find_spi_controller_by_node() available

[Ayush Singh]

From: Ayush Singh <ayushdevel1325@gmail.com>

DONOTMERGE

Externalize and export the symbol of_find_spi_controller_by_node() from
the SPI core akin to how of_find_i2c_adapter_by_node() is already
available.

Also, move it under a CONFIG_OF.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 drivers/spi/spi.c       | 206 ++++++++++++++++++++++++------------------------
 include/linux/spi/spi.h |   4 +
 2 files changed, 108 insertions(+), 102 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 54cbe652a4df..565b2e2dd5b9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2340,6 +2340,93 @@ void spi_flush_queue(struct spi_controller *ctlr)
 
 /*-------------------------------------------------------------------------*/
 
+static void spi_controller_release(struct device *dev)
+{
+	struct spi_controller *ctlr;
+
+	ctlr = container_of(dev, struct spi_controller, dev);
+	kfree(ctlr);
+}
+
+static struct class spi_master_class = {
+	.name		= "spi_master",
+	.dev_release	= spi_controller_release,
+	.dev_groups	= spi_master_groups,
+};
+
+static ssize_t slave_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
+						   dev);
+	struct device *child;
+
+	child = device_find_any_child(&ctlr->dev);
+	return sysfs_emit(buf, "%s\n", child ? to_spi_device(child)->modalias : NULL);
+}
+
+static ssize_t slave_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
+						   dev);
+	struct spi_device *spi;
+	struct device *child;
+	char name[32];
+	int rc;
+
+	rc = sscanf(buf, "%31s", name);
+	if (rc != 1 || !name[0])
+		return -EINVAL;
+
+	child = device_find_any_child(&ctlr->dev);
+	if (child) {
+		/* Remove registered slave */
+		device_unregister(child);
+		put_device(child);
+	}
+
+	if (strcmp(name, "(null)")) {
+		/* Register new slave */
+		spi = spi_alloc_device(ctlr);
+		if (!spi)
+			return -ENOMEM;
+
+		strscpy(spi->modalias, name, sizeof(spi->modalias));
+
+		rc = spi_add_device(spi);
+		if (rc) {
+			spi_dev_put(spi);
+			return rc;
+		}
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(slave);
+
+static struct attribute *spi_slave_attrs[] = {
+	&dev_attr_slave.attr,
+	NULL,
+};
+
+static const struct attribute_group spi_slave_group = {
+	.attrs = spi_slave_attrs,
+};
+
+static const struct attribute_group *spi_slave_groups[] = {
+	&spi_controller_statistics_group,
+	&spi_slave_group,
+	NULL,
+};
+
+static struct class spi_slave_class = {
+	.name		= "spi_slave",
+	.dev_release	= spi_controller_release,
+	.dev_groups	= spi_slave_groups,
+};
+
 #if defined(CONFIG_OF)
 static void of_spi_parse_dt_cs_delay(struct device_node *nc,
 				     struct spi_delay *delay, const char *prop)
@@ -2549,6 +2636,23 @@ static void of_register_spi_devices(struct spi_controller *ctlr)
 		}
 	}
 }
+
+/* The spi controllers are not using spi_bus, so we find it with another way */
+struct spi_controller *of_find_spi_controller_by_node(struct device_node *node)
+{
+	struct device *dev;
+
+	dev = class_find_device_by_of_node(&spi_master_class, node);
+	if (!dev && IS_ENABLED(CONFIG_SPI_SLAVE))
+		dev = class_find_device_by_of_node(&spi_slave_class, node);
+	if (!dev)
+		return NULL;
+
+	/* Reference got in class_find_device */
+	return container_of(dev, struct spi_controller, dev);
+}
+EXPORT_SYMBOL_GPL(of_find_spi_controller_by_node);
+
 #else
 static void of_register_spi_devices(struct spi_controller *ctlr) { }
 #endif
@@ -2917,20 +3021,6 @@ static void acpi_register_spi_devices(struct spi_controller *ctlr)
 static inline void acpi_register_spi_devices(struct spi_controller *ctlr) {}
 #endif /* CONFIG_ACPI */
 
-static void spi_controller_release(struct device *dev)
-{
-	struct spi_controller *ctlr;
-
-	ctlr = container_of(dev, struct spi_controller, dev);
-	kfree(ctlr);
-}
-
-static struct class spi_master_class = {
-	.name		= "spi_master",
-	.dev_release	= spi_controller_release,
-	.dev_groups	= spi_master_groups,
-};
-
 #ifdef CONFIG_SPI_SLAVE
 /**
  * spi_slave_abort - abort the ongoing transfer request on an SPI slave
@@ -2958,79 +3048,6 @@ int spi_target_abort(struct spi_device *spi)
 	return -ENOTSUPP;
 }
 EXPORT_SYMBOL_GPL(spi_target_abort);
-
-static ssize_t slave_show(struct device *dev, struct device_attribute *attr,
-			  char *buf)
-{
-	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
-						   dev);
-	struct device *child;
-
-	child = device_find_any_child(&ctlr->dev);
-	return sysfs_emit(buf, "%s\n", child ? to_spi_device(child)->modalias : NULL);
-}
-
-static ssize_t slave_store(struct device *dev, struct device_attribute *attr,
-			   const char *buf, size_t count)
-{
-	struct spi_controller *ctlr = container_of(dev, struct spi_controller,
-						   dev);
-	struct spi_device *spi;
-	struct device *child;
-	char name[32];
-	int rc;
-
-	rc = sscanf(buf, "%31s", name);
-	if (rc != 1 || !name[0])
-		return -EINVAL;
-
-	child = device_find_any_child(&ctlr->dev);
-	if (child) {
-		/* Remove registered slave */
-		device_unregister(child);
-		put_device(child);
-	}
-
-	if (strcmp(name, "(null)")) {
-		/* Register new slave */
-		spi = spi_alloc_device(ctlr);
-		if (!spi)
-			return -ENOMEM;
-
-		strscpy(spi->modalias, name, sizeof(spi->modalias));
-
-		rc = spi_add_device(spi);
-		if (rc) {
-			spi_dev_put(spi);
-			return rc;
-		}
-	}
-
-	return count;
-}
-
-static DEVICE_ATTR_RW(slave);
-
-static struct attribute *spi_slave_attrs[] = {
-	&dev_attr_slave.attr,
-	NULL,
-};
-
-static const struct attribute_group spi_slave_group = {
-	.attrs = spi_slave_attrs,
-};
-
-static const struct attribute_group *spi_slave_groups[] = {
-	&spi_controller_statistics_group,
-	&spi_slave_group,
-	NULL,
-};
-
-static struct class spi_slave_class = {
-	.name		= "spi_slave",
-	.dev_release	= spi_controller_release,
-	.dev_groups	= spi_slave_groups,
-};
 #else
 extern struct class spi_slave_class;	/* dummy */
 #endif
@@ -4720,21 +4737,6 @@ static struct spi_device *of_find_spi_device_by_node(struct device_node *node)
 	return dev ? to_spi_device(dev) : NULL;
 }
 
-/* The spi controllers are not using spi_bus, so we find it with another way */
-static struct spi_controller *of_find_spi_controller_by_node(struct device_node *node)
-{
-	struct device *dev;
-
-	dev = class_find_device_by_of_node(&spi_master_class, node);
-	if (!dev && IS_ENABLED(CONFIG_SPI_SLAVE))
-		dev = class_find_device_by_of_node(&spi_slave_class, node);
-	if (!dev)
-		return NULL;
-
-	/* Reference got in class_find_device */
-	return container_of(dev, struct spi_controller, dev);
-}
-
 static int of_spi_notify(struct notifier_block *nb, unsigned long action,
 			 void *arg)
 {
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 85785bcd20c1..58e692226475 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -1684,4 +1684,8 @@ spi_transfer_is_last(struct spi_controller *ctlr, struct spi_transfer *xfer)
 	return list_is_last(&xfer->transfer_list, &ctlr->cur_msg->transfers);
 }
 
+#if IS_ENABLED(CONFIG_OF)
+struct spi_controller *of_find_spi_controller_by_node(struct device_node *node);
+#endif
+
 #endif /* __LINUX_SPI_H */

-- 
2.45.2

[PATCH v5 5/7] spi: Make of_register_spi_device() available

[Ayush Singh]

DONOTMERGE

Export of_register_spi_device() from SPI Core to allow registering SPI
devices from device tree when the device node is not a child node of spi
controller.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 drivers/spi/spi.c       | 3 ++-
 include/linux/spi/spi.h | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 565b2e2dd5b9..8cd4d61958a2 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2566,7 +2566,7 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	return 0;
 }
 
-static struct spi_device *
+struct spi_device *
 of_register_spi_device(struct spi_controller *ctlr, struct device_node *nc)
 {
 	struct spi_device *spi;
@@ -2612,6 +2612,7 @@ of_register_spi_device(struct spi_controller *ctlr, struct device_node *nc)
 	spi_dev_put(spi);
 	return ERR_PTR(rc);
 }
+EXPORT_SYMBOL_GPL(of_register_spi_device);
 
 /**
  * of_register_spi_devices() - Register child devices onto the SPI bus
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 58e692226475..861b1cb4cca6 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -1686,6 +1686,9 @@ spi_transfer_is_last(struct spi_controller *ctlr, struct spi_transfer *xfer)
 
 #if IS_ENABLED(CONFIG_OF)
 struct spi_controller *of_find_spi_controller_by_node(struct device_node *node);
+
+struct spi_device *
+of_register_spi_device(struct spi_controller *ctlr, struct device_node *nc);
 #endif
 
 #endif /* __LINUX_SPI_H */

-- 
2.45.2

[PATCH v5 6/7] mikrobus: Add mikroBUS driver

[Ayush Singh]

Adds support for SPI mikroBUS addon boards with configuration based on
device tree. The goal is to get a minimal version in mainline to sort
out the device tree structure that should be used.

A mikroBUS board can use any combination of the following based protocols:
I2C, SPI, UART, PWM, Analog, GPIO with possibility of all pins being used
as GPIO instead of their original purpose. This requires the driver to be
flexible and identify the type of board based on the compatible string.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 MAINTAINERS             |   1 +
 drivers/misc/Kconfig    |  16 +++
 drivers/misc/Makefile   |   1 +
 drivers/misc/mikrobus.c | 361 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 379 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 88f2b3adc824..01a0ac261e6c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15115,6 +15115,7 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/connector/mikrobus-connector.yaml
 F:	Documentation/devicetree/bindings/mikrobus/mikrobus-board.yaml
 F:	Documentation/devicetree/bindings/mikrobus/mikrobus-spi.yaml
+F:	drivers/misc/mikrobus.c
 
 MIKROTIK CRS3XX 98DX3236 BOARD SUPPORT
 M:	Luka Kovacic <luka.kovacic@sartura.hr>
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index faf983680040..320f408cc612 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -585,6 +585,22 @@ config NSM
 	  To compile this driver as a module, choose M here.
 	  The module will be called nsm.
 
+menuconfig MIKROBUS
+	tristate "Module for instantiating devices on mikroBUS ports"
+	depends on GPIOLIB
+	help
+	  This option enables the mikroBUS driver. mikroBUS is an add-on
+	  board socket standard that offers maximum expandability with
+	  the smallest number of pins. The mikroBUS driver instantiates
+	  devices on a mikroBUS port described mikroBUS manifest which is
+	  passed using a sysfs interface.
+
+
+	  Say Y here to enable support for this driver.
+
+	  To compile this code as a module, chose M here: the module
+	  will be called mikrobus.ko
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 153a3f4837e8..f10f1414270b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -69,3 +69,4 @@ obj-$(CONFIG_TMR_INJECT)	+= xilinx_tmr_inject.o
 obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
 obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
 obj-$(CONFIG_NSM)		+= nsm.o
+obj-$(CONFIG_MIKROBUS)		+= mikrobus.o
diff --git a/drivers/misc/mikrobus.c b/drivers/misc/mikrobus.c
new file mode 100644
index 000000000000..bf160a0e8903
--- /dev/null
+++ b/drivers/misc/mikrobus.c
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0:
+/*
+ * Copyright 2024 Ayush Singh <ayush@beagleboard.org>
+ */
+
+#define pr_fmt(fmt) "mikrobus:%s: " fmt, __func__
+
+#include <linux/device.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/spi/spi.h>
+
+struct mikrobus_spi_cs_item {
+	const char *cs_name;
+	u32 cs;
+};
+
+/**
+ * struct mikrobus_port - MikroBUS Driver
+ *
+ * @dev: underlying platform_device
+ * @board_ocs: board device tree changeset
+ * @pinctrl: mikroBUS pinctrl
+ * @mikrobus_spi_cs: list of supported chipselect address and name
+ * @mikrobus_spi_cs_count: length of mikrobus_spi_cs
+ * @spi_ctrl: spi controller of mikroBUS connector
+ * @spi_dev: spi mikroBUS board
+ */
+struct mikrobus_port {
+	struct platform_device *dev;
+	struct of_changeset board_ocs;
+	struct pinctrl *pctrl;
+
+	struct mikrobus_spi_cs_item *spi_cs;
+	int spi_cs_count;
+	struct spi_controller *spi_ctrl;
+	struct spi_device *spi_dev;
+};
+
+/*
+ * mikrobus_pinctrl_select: Select pinctrl state for mikrobus pin
+ *
+ * @port: mikrobus port
+ * @pinctrl_selected: pinctrl state to be selected
+ */
+static int mikrobus_pinctrl_select(struct device *dev,
+				   const char *pinctrl_selected)
+{
+	int ret;
+	struct pinctrl_state *state;
+	struct mikrobus_port *mb = dev_get_drvdata(dev);
+
+	state = pinctrl_lookup_state(mb->pctrl, pinctrl_selected);
+	if (IS_ERR(state))
+		return dev_err_probe(dev, PTR_ERR(state),
+				     "failed to find state %s",
+				     pinctrl_selected);
+
+	ret = pinctrl_select_state(mb->pctrl, state);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to select state %s",
+				     pinctrl_selected);
+
+	dev_dbg_ratelimited(dev, "setting pinctrl %s", pinctrl_selected);
+
+	return 0;
+}
+
+/*
+ * mikrobus_lookup_cs - lookup mikroBUS SPI chipselect by name
+ *
+ * @mb: mikroBUS port
+ * @cs_name: chipselect name
+ */
+static int mikrobus_lookup_cs(const struct mikrobus_port *mb,
+			      const char *cs_name)
+{
+	for (int i = 0; i < mb->spi_cs_count; ++i) {
+		if (strcmp(cs_name, mb->spi_cs[i].cs_name) == 0)
+			return mb->spi_cs[i].cs;
+	}
+
+	return -1;
+}
+
+static int mikrobus_spi_set_cs(struct device *dev, struct device_node *np)
+{
+	struct mikrobus_port *mb = dev_get_drvdata(dev);
+	const char *temp_str;
+	int reg_len;
+	int ret, i;
+	u32 *reg = NULL;
+
+	reg_len = of_property_count_strings(np, "spi-cs");
+	/* Use default cs if spi-cs property not present */
+	if (reg_len <= 0) {
+		reg_len = 1;
+		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
+					 GFP_KERNEL);
+		if (!reg)
+			return -ENOMEM;
+
+		ret = mikrobus_lookup_cs(mb, "default");
+		if (ret < 0)
+			goto free_reg;
+
+		reg[0] = ret;
+	} else {
+		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
+					 GFP_KERNEL);
+		if (!reg)
+			return -ENOMEM;
+
+		for (i = 0; i < reg_len; ++i) {
+			ret = of_property_read_string_index(np, "spi-cs", i,
+							    &temp_str);
+			if (ret < 0)
+				goto free_reg;
+
+			ret = mikrobus_lookup_cs(mb, temp_str);
+			if (ret < 0)
+				goto free_reg;
+
+			reg[i] = ret;
+		}
+	}
+
+	ret = of_changeset_add_prop_u32_array(&mb->board_ocs, np, "reg", reg,
+					      reg_len);
+	if (ret < 0)
+		goto free_reg;
+
+	ret = of_changeset_apply(&mb->board_ocs);
+	if (ret < 0)
+		goto free_reg;
+
+	devm_kfree(dev, reg);
+	return 0;
+
+free_reg:
+	devm_kfree(dev, reg);
+	return ret;
+}
+
+static int of_register_mikrobus_device(struct mikrobus_port *mb,
+				       struct device_node *np)
+{
+	const char *temp_str;
+	int i, pinctrl_count, ret;
+	struct spi_device *spi_dev;
+	struct device *dev = &mb->dev->dev;
+
+	pinctrl_count = of_property_count_strings(np, "pinctrl-apply");
+	if (pinctrl_count < 0)
+		return dev_err_probe(dev, pinctrl_count,
+				     "Missing required property pinctrl-apply");
+
+	for (i = 0; i < pinctrl_count; ++i) {
+		ret = of_property_read_string_index(np, "pinctrl-apply", i,
+						    &temp_str);
+		if (ret < 0)
+			return ret;
+
+		ret = mikrobus_pinctrl_select(dev, temp_str);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Failed to set pinctrl");
+	}
+
+	if (mb->spi_ctrl && !mb->spi_dev &&
+	    of_device_is_compatible(np, "mikrobus-spi")) {
+		ret = mikrobus_spi_set_cs(dev, np);
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Failed to set SPI chipselect");
+
+		spi_dev = of_register_spi_device(mb->spi_ctrl, np);
+		if (IS_ERR(spi_dev))
+			return dev_err_probe(dev, PTR_ERR(spi_dev),
+					     "Failed to register SPI device");
+		mb->spi_dev = spi_dev;
+	}
+
+	return 0;
+}
+
+static int of_register_mikrobus_board(struct mikrobus_port *mb)
+{
+	struct device *dev = &mb->dev->dev;
+	int board_len, i, ret;
+	struct device_node *np;
+
+	board_len = of_count_phandle_with_args(dev->of_node, "board", NULL);
+	for (i = 0; i < board_len; ++i) {
+		np = of_parse_phandle(dev->of_node, "board", i);
+		if (!np) {
+			ret = dev_err_probe(dev, -ENODEV, "Board not found");
+			goto free_np;
+		}
+
+		ret = of_register_mikrobus_device(mb, np);
+		if (ret < 0)
+			goto free_np;
+
+		of_node_put(np);
+	}
+
+	return 0;
+free_np:
+	of_node_put(np);
+	return ret;
+}
+
+static int mikrobus_port_probe(struct platform_device *pdev)
+{
+	int ret, i;
+	struct mikrobus_port *mb;
+	struct device_node *np;
+	struct device *dev = &pdev->dev;
+
+	mb = devm_kmalloc(dev, sizeof(*mb), GFP_KERNEL);
+	if (!mb)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, mb);
+
+	of_changeset_init(&mb->board_ocs);
+	mb->dev = pdev;
+	mb->pctrl = NULL;
+	mb->spi_ctrl = NULL;
+	mb->spi_dev = NULL;
+	mb->spi_cs = NULL;
+	mb->spi_cs_count = 0;
+
+	mb->pctrl = devm_pinctrl_get(dev);
+	if (IS_ERR(mb->pctrl))
+		return dev_err_probe(dev, PTR_ERR(mb->pctrl),
+				     "failed to get pinctrl [%ld]",
+				     PTR_ERR(mb->pctrl));
+
+	np = of_parse_phandle(dev->of_node, "spi-controller", 0);
+	if (np) {
+		mb->spi_ctrl = of_find_spi_controller_by_node(np);
+		if (mb->spi_ctrl) {
+			ret = of_property_count_u32_elems(dev->of_node,
+							  "spi-cs");
+			if (ret < 0) {
+				dev_err(dev, "Missing property spi-cs");
+				goto free_np;
+			}
+
+			mb->spi_cs_count = ret;
+
+			ret = of_property_count_strings(dev->of_node,
+							"spi-cs-names");
+			if (ret < 0) {
+				dev_err(dev, "Missing property spi-cs-names");
+				goto free_np;
+			}
+
+			if (mb->spi_cs_count != ret) {
+				ret = dev_err_probe(
+					dev, -EINVAL,
+					"spi-cs and spi-cs-names out of sync");
+				goto free_np;
+			}
+
+			mb->spi_cs = devm_kmalloc_array(dev, mb->spi_cs_count,
+							sizeof(*mb->spi_cs),
+							GFP_KERNEL);
+			if (!mb->spi_cs) {
+				ret = -ENOMEM;
+				goto free_np;
+			}
+
+			for (i = 0; i < mb->spi_cs_count; ++i) {
+				of_property_read_u32_index(dev->of_node,
+							   "spi-cs", i,
+							   &mb->spi_cs->cs);
+				of_property_read_string_index(
+					dev->of_node, "spi-cs-names", i,
+					&mb->spi_cs->cs_name);
+			}
+		}
+	}
+	of_node_put(np);
+
+	ret = of_register_mikrobus_board(mb);
+	if (ret < 0)
+		return dev_err_probe(dev, -EINVAL,
+				     "Failed to register mikrobus board");
+
+	return 0;
+
+free_np:
+	of_node_put(np);
+	return ret;
+}
+
+static void mikrobus_port_remove(struct platform_device *pdev)
+{
+	struct mikrobus_port *mb = dev_get_drvdata(&pdev->dev);
+
+	spi_unregister_device(mb->spi_dev);
+
+	of_changeset_revert(&mb->board_ocs);
+}
+
+static const struct of_device_id mikrobus_port_of_match[] = {
+	{ .compatible = "mikrobus-connector" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mikrobus_port_of_match);
+
+static struct platform_driver mikrobus_port_driver = {
+	.probe = mikrobus_port_probe,
+	.remove = mikrobus_port_remove,
+	.driver = {
+		.name = "mikrobus",
+		.of_match_table = mikrobus_port_of_match,
+	},
+};
+
+static const struct bus_type mikrobus_bus_type = {
+	.name = "mikrobus",
+};
+
+static int mikrobus_init(void)
+{
+	int ret;
+
+	ret = bus_register(&mikrobus_bus_type);
+	if (ret) {
+		pr_err("bus_register failed (%d)", ret);
+		return ret;
+	}
+
+	ret = platform_driver_register(&mikrobus_port_driver);
+	if (ret)
+		pr_err("driver register failed [%d]", ret);
+
+	return 0;
+}
+
+module_init(mikrobus_init);
+
+static void mikrobus_exit(void)
+{
+	platform_driver_unregister(&mikrobus_port_driver);
+	bus_unregister(&mikrobus_bus_type);
+}
+
+module_exit(mikrobus_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ayush Singh <ayush@beagleboard.org>");
+MODULE_DESCRIPTION("mikroBUS driver for linux");
+MODULE_VERSION("0.1");

-- 
2.45.2

Re: [PATCH v5 6/7] mikrobus: Add mikroBUS driver

[Greg Kroah-Hartman]

On Thu, Jun 27, 2024 at 09:56:16PM +0530, Ayush Singh wrote:
> Adds support for SPI mikroBUS addon boards with configuration based on
> device tree. The goal is to get a minimal version in mainline to sort
> out the device tree structure that should be used.
> 
> A mikroBUS board can use any combination of the following based protocols:
> I2C, SPI, UART, PWM, Analog, GPIO with possibility of all pins being used
> as GPIO instead of their original purpose. This requires the driver to be
> flexible and identify the type of board based on the compatible string.

So this has nothing to do with greybus?  Or am I thinking of something
else?

> +menuconfig MIKROBUS
> +	tristate "Module for instantiating devices on mikroBUS ports"
> +	depends on GPIOLIB
> +	help
> +	  This option enables the mikroBUS driver. mikroBUS is an add-on
> +	  board socket standard that offers maximum expandability with
> +	  the smallest number of pins. The mikroBUS driver instantiates
> +	  devices on a mikroBUS port described mikroBUS manifest which is
> +	  passed using a sysfs interface.
> +
> +

Remove extra blank line.

> +	  Say Y here to enable support for this driver.

This isn't needed.

> +
> +	  To compile this code as a module, chose M here: the module
> +	  will be called mikrobus.ko
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 153a3f4837e8..f10f1414270b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -69,3 +69,4 @@ obj-$(CONFIG_TMR_INJECT)	+= xilinx_tmr_inject.o
>  obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
>  obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
>  obj-$(CONFIG_NSM)		+= nsm.o
> +obj-$(CONFIG_MIKROBUS)		+= mikrobus.o
> diff --git a/drivers/misc/mikrobus.c b/drivers/misc/mikrobus.c
> new file mode 100644
> index 000000000000..bf160a0e8903
> --- /dev/null
> +++ b/drivers/misc/mikrobus.c
> @@ -0,0 +1,361 @@
> +// SPDX-License-Identifier: GPL-2.0:
> +/*
> + * Copyright 2024 Ayush Singh <ayush@beagleboard.org>
> + */
> +
> +#define pr_fmt(fmt) "mikrobus:%s: " fmt, __func__

KBUILD_MODNAME?  Also, why is this needed at all, as you are a
bus/subsystem you should never need a pr_*() call, but instead just use
dev_*() ones instead.

> +
> +#include <linux/device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/spi/spi.h>
> +
> +struct mikrobus_spi_cs_item {
> +	const char *cs_name;
> +	u32 cs;

Documentation?  What is "cs"?  More vowels please...

> +};
> +
> +/**
> + * struct mikrobus_port - MikroBUS Driver
> + *
> + * @dev: underlying platform_device

Why must this be a platform device?  What requires that?

> + * @board_ocs: board device tree changeset
> + * @pinctrl: mikroBUS pinctrl
> + * @mikrobus_spi_cs: list of supported chipselect address and name
> + * @mikrobus_spi_cs_count: length of mikrobus_spi_cs
> + * @spi_ctrl: spi controller of mikroBUS connector
> + * @spi_dev: spi mikroBUS board
> + */
> +struct mikrobus_port {
> +	struct platform_device *dev;
> +	struct of_changeset board_ocs;
> +	struct pinctrl *pctrl;
> +
> +	struct mikrobus_spi_cs_item *spi_cs;
> +	int spi_cs_count;
> +	struct spi_controller *spi_ctrl;
> +	struct spi_device *spi_dev;

What controls the lifespan of this object?  You have multiple devices
pointed to here with different lifecycles, what controls this one?

> +};
> +
> +/*
> + * mikrobus_pinctrl_select: Select pinctrl state for mikrobus pin

Either use kerneldoc or not, should be /** right?

> + *
> + * @port: mikrobus port
> + * @pinctrl_selected: pinctrl state to be selected
> + */
> +static int mikrobus_pinctrl_select(struct device *dev,
> +				   const char *pinctrl_selected)
> +{
> +	int ret;
> +	struct pinctrl_state *state;
> +	struct mikrobus_port *mb = dev_get_drvdata(dev);
> +
> +	state = pinctrl_lookup_state(mb->pctrl, pinctrl_selected);
> +	if (IS_ERR(state))
> +		return dev_err_probe(dev, PTR_ERR(state),
> +				     "failed to find state %s",
> +				     pinctrl_selected);
> +
> +	ret = pinctrl_select_state(mb->pctrl, state);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to select state %s",
> +				     pinctrl_selected);
> +
> +	dev_dbg_ratelimited(dev, "setting pinctrl %s", pinctrl_selected);

Why rate limited?  What is going to cause this to spam the log?

> +
> +	return 0;
> +}
> +
> +/*
> + * mikrobus_lookup_cs - lookup mikroBUS SPI chipselect by name
> + *
> + * @mb: mikroBUS port
> + * @cs_name: chipselect name

Use "chipselect" instead of "cs" everywhere please.

> + */
> +static int mikrobus_lookup_cs(const struct mikrobus_port *mb,
> +			      const char *cs_name)
> +{
> +	for (int i = 0; i < mb->spi_cs_count; ++i) {
> +		if (strcmp(cs_name, mb->spi_cs[i].cs_name) == 0)
> +			return mb->spi_cs[i].cs;
> +	}
> +
> +	return -1;

what does -1 mean?  Use real error numbers please.

> +}
> +
> +static int mikrobus_spi_set_cs(struct device *dev, struct device_node *np)
> +{
> +	struct mikrobus_port *mb = dev_get_drvdata(dev);
> +	const char *temp_str;
> +	int reg_len;
> +	int ret, i;
> +	u32 *reg = NULL;
> +
> +	reg_len = of_property_count_strings(np, "spi-cs");
> +	/* Use default cs if spi-cs property not present */
> +	if (reg_len <= 0) {
> +		reg_len = 1;
> +		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
> +					 GFP_KERNEL);
> +		if (!reg)
> +			return -ENOMEM;
> +
> +		ret = mikrobus_lookup_cs(mb, "default");
> +		if (ret < 0)
> +			goto free_reg;
> +
> +		reg[0] = ret;
> +	} else {
> +		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
> +					 GFP_KERNEL);
> +		if (!reg)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < reg_len; ++i) {
> +			ret = of_property_read_string_index(np, "spi-cs", i,
> +							    &temp_str);
> +			if (ret < 0)
> +				goto free_reg;
> +
> +			ret = mikrobus_lookup_cs(mb, temp_str);
> +			if (ret < 0)
> +				goto free_reg;
> +
> +			reg[i] = ret;
> +		}
> +	}
> +
> +	ret = of_changeset_add_prop_u32_array(&mb->board_ocs, np, "reg", reg,
> +					      reg_len);
> +	if (ret < 0)
> +		goto free_reg;
> +
> +	ret = of_changeset_apply(&mb->board_ocs);
> +	if (ret < 0)
> +		goto free_reg;
> +
> +	devm_kfree(dev, reg);
> +	return 0;
> +
> +free_reg:
> +	devm_kfree(dev, reg);
> +	return ret;
> +}
> +
> +static int of_register_mikrobus_device(struct mikrobus_port *mb,
> +				       struct device_node *np)
> +{
> +	const char *temp_str;
> +	int i, pinctrl_count, ret;
> +	struct spi_device *spi_dev;
> +	struct device *dev = &mb->dev->dev;

That's some pointer dereferencing without checking anything, what could
go wrong...

Why don't you have your own real device?  Why are you relying on a
platform device without actually showing your device anywhere in the
kernel's device topology?  Are you sure that is ok?


> +
> +	pinctrl_count = of_property_count_strings(np, "pinctrl-apply");
> +	if (pinctrl_count < 0)
> +		return dev_err_probe(dev, pinctrl_count,
> +				     "Missing required property pinctrl-apply");
> +
> +	for (i = 0; i < pinctrl_count; ++i) {
> +		ret = of_property_read_string_index(np, "pinctrl-apply", i,
> +						    &temp_str);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = mikrobus_pinctrl_select(dev, temp_str);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to set pinctrl");
> +	}
> +
> +	if (mb->spi_ctrl && !mb->spi_dev &&
> +	    of_device_is_compatible(np, "mikrobus-spi")) {
> +		ret = mikrobus_spi_set_cs(dev, np);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to set SPI chipselect");
> +
> +		spi_dev = of_register_spi_device(mb->spi_ctrl, np);
> +		if (IS_ERR(spi_dev))
> +			return dev_err_probe(dev, PTR_ERR(spi_dev),
> +					     "Failed to register SPI device");
> +		mb->spi_dev = spi_dev;
> +	}
> +
> +	return 0;
> +}
> +
> +static int of_register_mikrobus_board(struct mikrobus_port *mb)
> +{
> +	struct device *dev = &mb->dev->dev;
> +	int board_len, i, ret;
> +	struct device_node *np;
> +
> +	board_len = of_count_phandle_with_args(dev->of_node, "board", NULL);
> +	for (i = 0; i < board_len; ++i) {
> +		np = of_parse_phandle(dev->of_node, "board", i);
> +		if (!np) {
> +			ret = dev_err_probe(dev, -ENODEV, "Board not found");
> +			goto free_np;
> +		}
> +
> +		ret = of_register_mikrobus_device(mb, np);
> +		if (ret < 0)
> +			goto free_np;
> +
> +		of_node_put(np);
> +	}
> +
> +	return 0;
> +free_np:
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static int mikrobus_port_probe(struct platform_device *pdev)
> +{
> +	int ret, i;
> +	struct mikrobus_port *mb;
> +	struct device_node *np;
> +	struct device *dev = &pdev->dev;
> +
> +	mb = devm_kmalloc(dev, sizeof(*mb), GFP_KERNEL);
> +	if (!mb)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, mb);
> +
> +	of_changeset_init(&mb->board_ocs);
> +	mb->dev = pdev;
> +	mb->pctrl = NULL;
> +	mb->spi_ctrl = NULL;
> +	mb->spi_dev = NULL;
> +	mb->spi_cs = NULL;
> +	mb->spi_cs_count = 0;
> +
> +	mb->pctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(mb->pctrl))
> +		return dev_err_probe(dev, PTR_ERR(mb->pctrl),
> +				     "failed to get pinctrl [%ld]",
> +				     PTR_ERR(mb->pctrl));
> +
> +	np = of_parse_phandle(dev->of_node, "spi-controller", 0);
> +	if (np) {
> +		mb->spi_ctrl = of_find_spi_controller_by_node(np);
> +		if (mb->spi_ctrl) {
> +			ret = of_property_count_u32_elems(dev->of_node,
> +							  "spi-cs");
> +			if (ret < 0) {
> +				dev_err(dev, "Missing property spi-cs");
> +				goto free_np;
> +			}
> +
> +			mb->spi_cs_count = ret;
> +
> +			ret = of_property_count_strings(dev->of_node,
> +							"spi-cs-names");
> +			if (ret < 0) {
> +				dev_err(dev, "Missing property spi-cs-names");
> +				goto free_np;
> +			}
> +
> +			if (mb->spi_cs_count != ret) {
> +				ret = dev_err_probe(
> +					dev, -EINVAL,
> +					"spi-cs and spi-cs-names out of sync");
> +				goto free_np;
> +			}
> +
> +			mb->spi_cs = devm_kmalloc_array(dev, mb->spi_cs_count,
> +							sizeof(*mb->spi_cs),
> +							GFP_KERNEL);
> +			if (!mb->spi_cs) {
> +				ret = -ENOMEM;
> +				goto free_np;
> +			}
> +
> +			for (i = 0; i < mb->spi_cs_count; ++i) {
> +				of_property_read_u32_index(dev->of_node,
> +							   "spi-cs", i,
> +							   &mb->spi_cs->cs);
> +				of_property_read_string_index(
> +					dev->of_node, "spi-cs-names", i,
> +					&mb->spi_cs->cs_name);
> +			}
> +		}
> +	}
> +	of_node_put(np);
> +
> +	ret = of_register_mikrobus_board(mb);
> +	if (ret < 0)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Failed to register mikrobus board");
> +
> +	return 0;
> +
> +free_np:
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static void mikrobus_port_remove(struct platform_device *pdev)
> +{
> +	struct mikrobus_port *mb = dev_get_drvdata(&pdev->dev);
> +
> +	spi_unregister_device(mb->spi_dev);
> +
> +	of_changeset_revert(&mb->board_ocs);
> +}
> +
> +static const struct of_device_id mikrobus_port_of_match[] = {
> +	{ .compatible = "mikrobus-connector" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mikrobus_port_of_match);
> +
> +static struct platform_driver mikrobus_port_driver = {
> +	.probe = mikrobus_port_probe,
> +	.remove = mikrobus_port_remove,

Again, why is this a platform driver?  Why is a platform device used at
all here?

> +	.driver = {
> +		.name = "mikrobus",
> +		.of_match_table = mikrobus_port_of_match,
> +	},
> +};
> +
> +static const struct bus_type mikrobus_bus_type = {
> +	.name = "mikrobus",
> +};
> +
> +static int mikrobus_init(void)
> +{
> +	int ret;
> +
> +	ret = bus_register(&mikrobus_bus_type);
> +	if (ret) {
> +		pr_err("bus_register failed (%d)", ret);
> +		return ret;
> +	}
> +
> +	ret = platform_driver_register(&mikrobus_port_driver);
> +	if (ret)
> +		pr_err("driver register failed [%d]", ret);

It fails yet you leave your bus around?  Not good :(

thanks,

greg k-h

Re: [PATCH v5 6/7] mikrobus: Add mikroBUS driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

On Thu, Jul 04, 2024 at 03:06:26PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 27, 2024 at 09:56:16PM +0530, Ayush Singh wrote:

> > +struct mikrobus_spi_cs_item {
> > +	const char *cs_name;
> > +	u32 cs;

> Documentation?  What is "cs"?  More vowels please...

cs is absolutely the standard term for the SPI chip select signal, this
will be immediately comprehensible to anyone familiar with SPI and using
another term might be confusing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 6/7] mikrobus: Add mikroBUS driver

[Ayush Singh]

If Grove Sunlight patches become the primary way of handling connector 
addon-board design, then most of this driver will probably be redundant. 
Still, I will try answer the questions as much as I can.

On 7/4/24 18:36, Greg Kroah-Hartman wrote:
> On Thu, Jun 27, 2024 at 09:56:16PM +0530, Ayush Singh wrote:
>> Adds support for SPI mikroBUS addon boards with configuration based on
>> device tree. The goal is to get a minimal version in mainline to sort
>> out the device tree structure that should be used.
>>
>> A mikroBUS board can use any combination of the following based protocols:
>> I2C, SPI, UART, PWM, Analog, GPIO with possibility of all pins being used
>> as GPIO instead of their original purpose. This requires the driver to be
>> flexible and identify the type of board based on the compatible string.
> So this has nothing to do with greybus?  Or am I thinking of something
> else?

MikroBUS is it's own connector. But most of the prior work on MikroBUS 
was done in relation to using it over greybus. So you are not completely 
wrong. These patches approach to solve the local mikroBUS connector 
problem, but should be extendable enough to allow support over greybus 
in future.

>> +menuconfig MIKROBUS
>> +	tristate "Module for instantiating devices on mikroBUS ports"
>> +	depends on GPIOLIB
>> +	help
>> +	  This option enables the mikroBUS driver. mikroBUS is an add-on
>> +	  board socket standard that offers maximum expandability with
>> +	  the smallest number of pins. The mikroBUS driver instantiates
>> +	  devices on a mikroBUS port described mikroBUS manifest which is
>> +	  passed using a sysfs interface.
>> +
>> +
> Remove extra blank line.
>
>> +	  Say Y here to enable support for this driver.
> This isn't needed.
>
>> +
>> +	  To compile this code as a module, chose M here: the module
>> +	  will be called mikrobus.ko
>> +
>>   source "drivers/misc/c2port/Kconfig"
>>   source "drivers/misc/eeprom/Kconfig"
>>   source "drivers/misc/cb710/Kconfig"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 153a3f4837e8..f10f1414270b 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -69,3 +69,4 @@ obj-$(CONFIG_TMR_INJECT)	+= xilinx_tmr_inject.o
>>   obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
>>   obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
>>   obj-$(CONFIG_NSM)		+= nsm.o
>> +obj-$(CONFIG_MIKROBUS)		+= mikrobus.o
>> diff --git a/drivers/misc/mikrobus.c b/drivers/misc/mikrobus.c
>> new file mode 100644
>> index 000000000000..bf160a0e8903
>> --- /dev/null
>> +++ b/drivers/misc/mikrobus.c
>> @@ -0,0 +1,361 @@
>> +// SPDX-License-Identifier: GPL-2.0:
>> +/*
>> + * Copyright 2024 Ayush Singh <ayush@beagleboard.org>
>> + */
>> +
>> +#define pr_fmt(fmt) "mikrobus:%s: " fmt, __func__
> KBUILD_MODNAME?  Also, why is this needed at all, as you are a
> bus/subsystem you should never need a pr_*() call, but instead just use
> dev_*() ones instead.
>
>> +
>> +#include <linux/device.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/spi/spi.h>
>> +
>> +struct mikrobus_spi_cs_item {
>> +	const char *cs_name;
>> +	u32 cs;
> Documentation?  What is "cs"?  More vowels please...
>
>> +};
>> +
>> +/**
>> + * struct mikrobus_port - MikroBUS Driver
>> + *
>> + * @dev: underlying platform_device
> Why must this be a platform device?  What requires that?

That is a bit of a hack. My bad.

>> + * @board_ocs: board device tree changeset
>> + * @pinctrl: mikroBUS pinctrl
>> + * @mikrobus_spi_cs: list of supported chipselect address and name
>> + * @mikrobus_spi_cs_count: length of mikrobus_spi_cs
>> + * @spi_ctrl: spi controller of mikroBUS connector
>> + * @spi_dev: spi mikroBUS board
>> + */
>> +struct mikrobus_port {
>> +	struct platform_device *dev;
>> +	struct of_changeset board_ocs;
>> +	struct pinctrl *pctrl;
>> +
>> +	struct mikrobus_spi_cs_item *spi_cs;
>> +	int spi_cs_count;
>> +	struct spi_controller *spi_ctrl;
>> +	struct spi_device *spi_dev;
> What controls the lifespan of this object?  You have multiple devices
> pointed to here with different lifecycles, what controls this one?

Yes, so the lifecycle is supposed to be tied to the addon-board. Since 
they are not hot plugable, it is the same as the lifecycle of the port 
unless there is a sysfs entry for runtime detection of board. Still it 
should be moved to a `mikrobus_board` struct or somewhere else, if we 
are not going to use the Grove Patch setup.

>> +};
>> +
>> +/*
>> + * mikrobus_pinctrl_select: Select pinctrl state for mikrobus pin
> Either use kerneldoc or not, should be /** right?
>
>> + *
>> + * @port: mikrobus port
>> + * @pinctrl_selected: pinctrl state to be selected
>> + */
>> +static int mikrobus_pinctrl_select(struct device *dev,
>> +				   const char *pinctrl_selected)
>> +{
>> +	int ret;
>> +	struct pinctrl_state *state;
>> +	struct mikrobus_port *mb = dev_get_drvdata(dev);
>> +
>> +	state = pinctrl_lookup_state(mb->pctrl, pinctrl_selected);
>> +	if (IS_ERR(state))
>> +		return dev_err_probe(dev, PTR_ERR(state),
>> +				     "failed to find state %s",
>> +				     pinctrl_selected);
>> +
>> +	ret = pinctrl_select_state(mb->pctrl, state);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to select state %s",
>> +				     pinctrl_selected);
>> +
>> +	dev_dbg_ratelimited(dev, "setting pinctrl %s", pinctrl_selected);
> Why rate limited?  What is going to cause this to spam the log?
>
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * mikrobus_lookup_cs - lookup mikroBUS SPI chipselect by name
>> + *
>> + * @mb: mikroBUS port
>> + * @cs_name: chipselect name
> Use "chipselect" instead of "cs" everywhere please.
>
>> + */
>> +static int mikrobus_lookup_cs(const struct mikrobus_port *mb,
>> +			      const char *cs_name)
>> +{
>> +	for (int i = 0; i < mb->spi_cs_count; ++i) {
>> +		if (strcmp(cs_name, mb->spi_cs[i].cs_name) == 0)
>> +			return mb->spi_cs[i].cs;
>> +	}
>> +
>> +	return -1;
> what does -1 mean?  Use real error numbers please.
>
>> +}
>> +
>> +static int mikrobus_spi_set_cs(struct device *dev, struct device_node *np)
>> +{
>> +	struct mikrobus_port *mb = dev_get_drvdata(dev);
>> +	const char *temp_str;
>> +	int reg_len;
>> +	int ret, i;
>> +	u32 *reg = NULL;
>> +
>> +	reg_len = of_property_count_strings(np, "spi-cs");
>> +	/* Use default cs if spi-cs property not present */
>> +	if (reg_len <= 0) {
>> +		reg_len = 1;
>> +		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
>> +					 GFP_KERNEL);
>> +		if (!reg)
>> +			return -ENOMEM;
>> +
>> +		ret = mikrobus_lookup_cs(mb, "default");
>> +		if (ret < 0)
>> +			goto free_reg;
>> +
>> +		reg[0] = ret;
>> +	} else {
>> +		reg = devm_kmalloc_array(dev, reg_len, sizeof(*reg),
>> +					 GFP_KERNEL);
>> +		if (!reg)
>> +			return -ENOMEM;
>> +
>> +		for (i = 0; i < reg_len; ++i) {
>> +			ret = of_property_read_string_index(np, "spi-cs", i,
>> +							    &temp_str);
>> +			if (ret < 0)
>> +				goto free_reg;
>> +
>> +			ret = mikrobus_lookup_cs(mb, temp_str);
>> +			if (ret < 0)
>> +				goto free_reg;
>> +
>> +			reg[i] = ret;
>> +		}
>> +	}
>> +
>> +	ret = of_changeset_add_prop_u32_array(&mb->board_ocs, np, "reg", reg,
>> +					      reg_len);
>> +	if (ret < 0)
>> +		goto free_reg;
>> +
>> +	ret = of_changeset_apply(&mb->board_ocs);
>> +	if (ret < 0)
>> +		goto free_reg;
>> +
>> +	devm_kfree(dev, reg);
>> +	return 0;
>> +
>> +free_reg:
>> +	devm_kfree(dev, reg);
>> +	return ret;
>> +}
>> +
>> +static int of_register_mikrobus_device(struct mikrobus_port *mb,
>> +				       struct device_node *np)
>> +{
>> +	const char *temp_str;
>> +	int i, pinctrl_count, ret;
>> +	struct spi_device *spi_dev;
>> +	struct device *dev = &mb->dev->dev;
> That's some pointer dereferencing without checking anything, what could
> go wrong...
>
> Why don't you have your own real device?  Why are you relying on a
> platform device without actually showing your device anywhere in the
> kernel's device topology?  Are you sure that is ok?

Yes, it should be a real device. I do have patches locally where I am 
creating and registering a device on mikrobus bus, so will do that in 
the next patch series.

>> +
>> +	pinctrl_count = of_property_count_strings(np, "pinctrl-apply");
>> +	if (pinctrl_count < 0)
>> +		return dev_err_probe(dev, pinctrl_count,
>> +				     "Missing required property pinctrl-apply");
>> +
>> +	for (i = 0; i < pinctrl_count; ++i) {
>> +		ret = of_property_read_string_index(np, "pinctrl-apply", i,
>> +						    &temp_str);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = mikrobus_pinctrl_select(dev, temp_str);
>> +		if (ret < 0)
>> +			return dev_err_probe(dev, ret, "Failed to set pinctrl");
>> +	}
>> +
>> +	if (mb->spi_ctrl && !mb->spi_dev &&
>> +	    of_device_is_compatible(np, "mikrobus-spi")) {
>> +		ret = mikrobus_spi_set_cs(dev, np);
>> +		if (ret < 0)
>> +			return dev_err_probe(dev, ret,
>> +					     "Failed to set SPI chipselect");
>> +
>> +		spi_dev = of_register_spi_device(mb->spi_ctrl, np);
>> +		if (IS_ERR(spi_dev))
>> +			return dev_err_probe(dev, PTR_ERR(spi_dev),
>> +					     "Failed to register SPI device");
>> +		mb->spi_dev = spi_dev;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int of_register_mikrobus_board(struct mikrobus_port *mb)
>> +{
>> +	struct device *dev = &mb->dev->dev;
>> +	int board_len, i, ret;
>> +	struct device_node *np;
>> +
>> +	board_len = of_count_phandle_with_args(dev->of_node, "board", NULL);
>> +	for (i = 0; i < board_len; ++i) {
>> +		np = of_parse_phandle(dev->of_node, "board", i);
>> +		if (!np) {
>> +			ret = dev_err_probe(dev, -ENODEV, "Board not found");
>> +			goto free_np;
>> +		}
>> +
>> +		ret = of_register_mikrobus_device(mb, np);
>> +		if (ret < 0)
>> +			goto free_np;
>> +
>> +		of_node_put(np);
>> +	}
>> +
>> +	return 0;
>> +free_np:
>> +	of_node_put(np);
>> +	return ret;
>> +}
>> +
>> +static int mikrobus_port_probe(struct platform_device *pdev)
>> +{
>> +	int ret, i;
>> +	struct mikrobus_port *mb;
>> +	struct device_node *np;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	mb = devm_kmalloc(dev, sizeof(*mb), GFP_KERNEL);
>> +	if (!mb)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(dev, mb);
>> +
>> +	of_changeset_init(&mb->board_ocs);
>> +	mb->dev = pdev;
>> +	mb->pctrl = NULL;
>> +	mb->spi_ctrl = NULL;
>> +	mb->spi_dev = NULL;
>> +	mb->spi_cs = NULL;
>> +	mb->spi_cs_count = 0;
>> +
>> +	mb->pctrl = devm_pinctrl_get(dev);
>> +	if (IS_ERR(mb->pctrl))
>> +		return dev_err_probe(dev, PTR_ERR(mb->pctrl),
>> +				     "failed to get pinctrl [%ld]",
>> +				     PTR_ERR(mb->pctrl));
>> +
>> +	np = of_parse_phandle(dev->of_node, "spi-controller", 0);
>> +	if (np) {
>> +		mb->spi_ctrl = of_find_spi_controller_by_node(np);
>> +		if (mb->spi_ctrl) {
>> +			ret = of_property_count_u32_elems(dev->of_node,
>> +							  "spi-cs");
>> +			if (ret < 0) {
>> +				dev_err(dev, "Missing property spi-cs");
>> +				goto free_np;
>> +			}
>> +
>> +			mb->spi_cs_count = ret;
>> +
>> +			ret = of_property_count_strings(dev->of_node,
>> +							"spi-cs-names");
>> +			if (ret < 0) {
>> +				dev_err(dev, "Missing property spi-cs-names");
>> +				goto free_np;
>> +			}
>> +
>> +			if (mb->spi_cs_count != ret) {
>> +				ret = dev_err_probe(
>> +					dev, -EINVAL,
>> +					"spi-cs and spi-cs-names out of sync");
>> +				goto free_np;
>> +			}
>> +
>> +			mb->spi_cs = devm_kmalloc_array(dev, mb->spi_cs_count,
>> +							sizeof(*mb->spi_cs),
>> +							GFP_KERNEL);
>> +			if (!mb->spi_cs) {
>> +				ret = -ENOMEM;
>> +				goto free_np;
>> +			}
>> +
>> +			for (i = 0; i < mb->spi_cs_count; ++i) {
>> +				of_property_read_u32_index(dev->of_node,
>> +							   "spi-cs", i,
>> +							   &mb->spi_cs->cs);
>> +				of_property_read_string_index(
>> +					dev->of_node, "spi-cs-names", i,
>> +					&mb->spi_cs->cs_name);
>> +			}
>> +		}
>> +	}
>> +	of_node_put(np);
>> +
>> +	ret = of_register_mikrobus_board(mb);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Failed to register mikrobus board");
>> +
>> +	return 0;
>> +
>> +free_np:
>> +	of_node_put(np);
>> +	return ret;
>> +}
>> +
>> +static void mikrobus_port_remove(struct platform_device *pdev)
>> +{
>> +	struct mikrobus_port *mb = dev_get_drvdata(&pdev->dev);
>> +
>> +	spi_unregister_device(mb->spi_dev);
>> +
>> +	of_changeset_revert(&mb->board_ocs);
>> +}
>> +
>> +static const struct of_device_id mikrobus_port_of_match[] = {
>> +	{ .compatible = "mikrobus-connector" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, mikrobus_port_of_match);
>> +
>> +static struct platform_driver mikrobus_port_driver = {
>> +	.probe = mikrobus_port_probe,
>> +	.remove = mikrobus_port_remove,
> Again, why is this a platform driver?  Why is a platform device used at
> all here?

You mean it will be better to have a mikrobus bus driver similar to SPI 
and I2C? While creating the mikrobus bus device seems simple, I will 
need to look at how creating a driver from scratch works.

>> +	.driver = {
>> +		.name = "mikrobus",
>> +		.of_match_table = mikrobus_port_of_match,
>> +	},
>> +};
>> +
>> +static const struct bus_type mikrobus_bus_type = {
>> +	.name = "mikrobus",
>> +};
>> +
>> +static int mikrobus_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = bus_register(&mikrobus_bus_type);
>> +	if (ret) {
>> +		pr_err("bus_register failed (%d)", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = platform_driver_register(&mikrobus_port_driver);
>> +	if (ret)
>> +		pr_err("driver register failed [%d]", ret);
> It fails yet you leave your bus around?  Not good :(
>
> thanks,
>
> greg k-h


I will be observing the Grove Sunlight Patch series and adopt it if it 
gets merged in mainline, so almost all of this might go away. Let's wait 
and see I guess.


Ayush Singh


Grove Sunlight Patch: 
https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/

[PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Ayush Singh]

DONOTMERGE

Add mikroBUS connector and some mikroBUS boards support for Beagleplay.
The mikroBUS boards node should probably be moved to a more appropriate
location but I am not quite sure where it should go since it is not
dependent on specific arch.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94 +++++++++++++++++++++++---
 1 file changed, 86 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
index 70de288d728e..3f3cd70345c4 100644
--- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
+++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
@@ -38,6 +38,7 @@ aliases {
 		serial2 = &main_uart0;
 		usb0 = &usb0;
 		usb1 = &usb1;
+		mikrobus0 = &mikrobus0;
 	};
 
 	chosen {
@@ -227,6 +228,56 @@ simple-audio-card,codec {
 		};
 	};
 
+	mikrobus0: mikrobus-connector {
+		compatible = "mikrobus-connector";
+		pinctrl-names = "default", "pwm_default", "pwm_gpio",
+				"uart_default", "uart_gpio", "i2c_default",
+				"i2c_gpio", "spi_default", "spi_gpio";
+		pinctrl-0 = <&mikrobus_gpio_pins_default>;
+		pinctrl-1 = <&mikrobus_pwm_pins_default>;
+		pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
+		pinctrl-3 = <&mikrobus_uart_pins_default>;
+		pinctrl-4 = <&mikrobus_uart_pins_gpio>;
+		pinctrl-5 = <&mikrobus_i2c_pins_default>;
+		pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
+		pinctrl-7 = <&mikrobus_spi_pins_default>;
+		pinctrl-8 = <&mikrobus_spi_pins_gpio>;
+
+		mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
+				      "mosi", "miso", "sck", "cs", "rst", "an";
+		mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
+				 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
+
+		spi-controller = <&main_spi2>;
+		spi-cs = <0>;
+		spi-cs-names = "default";
+
+		board = <&lsm6dsl_click>;
+	};
+
+	mikrobus_boards {
+		thermo_click: thermo-click {
+			compatible = "maxim,max31855k", "mikrobus-spi";
+			spi-max-frequency = <1000000>;
+			pinctrl-apply = "spi_default";
+		};
+
+		lsm6dsl_click: lsm6dsl-click {
+			compatible = "st,lsm6ds3", "mikrobus-spi";
+			spi-max-frequency = <1000000>;
+			pinctrl-apply = "spi_default";
+		};
+	};
 };
 
 &main_pmx0 {
@@ -387,6 +438,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) EXT_REFCLK1.CLKOUT0 */
 		>;
 	};
 
+	mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
+		>;
+	};
+
+	mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) MCASP0_ACLKX.GPIO1_11 */
+		>;
+	};
+
 	mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
 		pinctrl-single,pins = <
 			AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) UART0_CTSn.I2C3_SCL */
@@ -394,6 +457,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* (B15) UART0_RTSn.I2C3_SDA */
 		>;
 	};
 
+	mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) UART0_CTSn.GPIO1_22 */
+			AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) UART0_RTSn.GPIO1_23 */
+		>;
+	};
+
 	mikrobus_uart_pins_default: mikrobus-uart-default-pins {
 		pinctrl-single,pins = <
 			AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) MCAN0_TX.UART5_RXD */
@@ -401,6 +471,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) MCAN0_RX.UART5_TXD */
 		>;
 	};
 
+	mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) MCAN0_TX.GPIO1_24 */
+			AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) MCAN0_RX.GPIO1_25 */
+		>;
+	};
+
 	mikrobus_spi_pins_default: mikrobus-spi-default-pins {
 		pinctrl-single,pins = <
 			AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) MCASP0_ACLKR.SPI2_CLK */
@@ -410,6 +487,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) MCASP0_AXR2.SPI2_D1 */
 		>;
 	};
 
+	mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) MCASP0_AXR3.GPIO1_7 */
+			AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) MCASP0_AXR2.GPIO1_8 */
+			AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) MCASP0_AFSR.GPIO1_13 */
+			AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) MCASP0_ACLKR.GPIO1_14 */
+		>;
+	};
+
 	mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
 		bootph-all;
 		pinctrl-single,pins = <
@@ -630,8 +716,6 @@ &main_gpio0 {
 
 &main_gpio1 {
 	bootph-all;
-	pinctrl-names = "default";
-	pinctrl-0 = <&mikrobus_gpio_pins_default>;
 	gpio-line-names = "", "", "", "", "",			/* 0-4 */
 		"SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7",	/* 5-7 */
 		"MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9",		/* 8-9 */
@@ -804,15 +888,11 @@ it66121_out: endpoint {
 };
 
 &main_i2c3 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&mikrobus_i2c_pins_default>;
 	clock-frequency = <400000>;
 	status = "okay";
 };
 
 &main_spi2 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&mikrobus_spi_pins_default>;
 	status = "okay";
 };
 
@@ -876,8 +956,6 @@ &main_uart1 {
 };
 
 &main_uart5 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&mikrobus_uart_pins_default>;
 	status = "okay";
 };
 

-- 
2.45.2

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Nishanth Menon]

On 21:56-20240627, Ayush Singh wrote:
> DONOTMERGE
^^ might be better off in the diffstat and explain why DONOT MERGE :)
[...]

> +	mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) MCASP0_AXR3.GPIO1_7 */
> +			AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) MCASP0_AXR2.GPIO1_8 */
> +			AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) MCASP0_AFSR.GPIO1_13 */
> +			AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) MCASP0_ACLKR.GPIO1_14 */
> +		>;
> +	};

we could potentially get rid of these if we get the gpio-ranges correct
on pinctrl? I have not gotten around to am62x yet - but see this:

https://lore.kernel.org/linux-arm-kernel/20240627162539.691223-1-nm@ti.com/T/#t

[...]
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Ayush Singh]

On 6/27/24 22:12, Nishanth Menon wrote:

> On 21:56-20240627, Ayush Singh wrote:
>> DONOTMERGE
> ^^ might be better off in the diffstat and explain why DONOT MERGE :)
> [...]

There are 2 reasons for DONOTMERGE

1. Mikrobus boards config should not live here. It is supposed to be 
independent of the system and arch.

2. I am going to be cleaning up and sending this once at least the 
dt-bindings are approved.


I have included it in the patch to provide some idea about how it will 
supposedly look like (and anyone who might want to test this stuff). I 
will try to remember to add it in diffstat in the future.

>> +	mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
>> +		pinctrl-single,pins = <
>> +			AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) MCASP0_AXR3.GPIO1_7 */
>> +			AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) MCASP0_AXR2.GPIO1_8 */
>> +			AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) MCASP0_AFSR.GPIO1_13 */
>> +			AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) MCASP0_ACLKR.GPIO1_14 */
>> +		>;
>> +	};
> we could potentially get rid of these if we get the gpio-ranges correct
> on pinctrl? I have not gotten around to am62x yet - but see this:
>
> https://lore.kernel.org/linux-arm-kernel/20240627162539.691223-1-nm@ti.com/T/#t
>
> [...]


Thanks. Will look into this before sending an actual patch for 
beagleplay dts as well.


Ayush Singh

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Andrew Davis]

On 6/27/24 11:26 AM, Ayush Singh wrote:
> DONOTMERGE
> 
> Add mikroBUS connector and some mikroBUS boards support for Beagleplay.
> The mikroBUS boards node should probably be moved to a more appropriate
> location but I am not quite sure where it should go since it is not
> dependent on specific arch.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94 +++++++++++++++++++++++---
>   1 file changed, 86 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> index 70de288d728e..3f3cd70345c4 100644
> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> @@ -38,6 +38,7 @@ aliases {
>   		serial2 = &main_uart0;
>   		usb0 = &usb0;
>   		usb1 = &usb1;
> +		mikrobus0 = &mikrobus0;
>   	};
>   
>   	chosen {
> @@ -227,6 +228,56 @@ simple-audio-card,codec {
>   		};
>   	};
>   
> +	mikrobus0: mikrobus-connector {
> +		compatible = "mikrobus-connector";
> +		pinctrl-names = "default", "pwm_default", "pwm_gpio",
> +				"uart_default", "uart_gpio", "i2c_default",
> +				"i2c_gpio", "spi_default", "spi_gpio";
> +		pinctrl-0 = <&mikrobus_gpio_pins_default>;
> +		pinctrl-1 = <&mikrobus_pwm_pins_default>;
> +		pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
> +		pinctrl-3 = <&mikrobus_uart_pins_default>;
> +		pinctrl-4 = <&mikrobus_uart_pins_gpio>;
> +		pinctrl-5 = <&mikrobus_i2c_pins_default>;
> +		pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
> +		pinctrl-7 = <&mikrobus_spi_pins_default>;
> +		pinctrl-8 = <&mikrobus_spi_pins_gpio>;
> +
> +		mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
> +				      "mosi", "miso", "sck", "cs", "rst", "an";
> +		mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
> +
> +		spi-controller = <&main_spi2>;
> +		spi-cs = <0>;
> +		spi-cs-names = "default";
> +
> +		board = <&lsm6dsl_click>;
> +	};
> +
> +	mikrobus_boards {
> +		thermo_click: thermo-click {
> +			compatible = "maxim,max31855k", "mikrobus-spi";

I might be missing something, but your solution cannot possibly be
to list every click board that could be connected (all 1500+ of them)
to every mikroBUS connector on every device's DT file..

Each click board should have a single DTSO overlay file to describe the
click board, one per click board total. And then that overlay should
apply cleanly to any device that has a mikroBUS interface.

Which means you have not completely solved the fundamental problem of
abstracting the mikroBUS connector in DT. Each of these click device child
nodes has to be under the parent connector node. Which means a phandle
to the parent node, which is not generically named. For instance
if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
the click board's overlay would look like this:

/dts-v1/;
/plugin/;

&mikrobus0 {
	status = "okay";

	mikrobus_board {
		thermo-click {
			compatible = "maxim,max31855k", "mikrobus-spi";
			spi-max-frequency = <1000000>;
			pinctrl-apply = "spi_default";
		};
	};
};

I think this solution is almost there, but once you solve the above
issue, we could just apply the right overlay for our attached click
board ahead of time and not need the mikroBUS bus driver at all.

Andrew

> +			spi-max-frequency = <1000000>;
> +			pinctrl-apply = "spi_default";
> +		};
> +
> +		lsm6dsl_click: lsm6dsl-click {
> +			compatible = "st,lsm6ds3", "mikrobus-spi";
> +			spi-max-frequency = <1000000>;
> +			pinctrl-apply = "spi_default";
> +		};
> +	};
>   };
>   
>   &main_pmx0 {
> @@ -387,6 +438,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) EXT_REFCLK1.CLKOUT0 */
>   		>;
>   	};
>   
> +	mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
> +		>;
> +	};
> +
> +	mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) MCASP0_ACLKX.GPIO1_11 */
> +		>;
> +	};
> +
>   	mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
>   		pinctrl-single,pins = <
>   			AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) UART0_CTSn.I2C3_SCL */
> @@ -394,6 +457,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* (B15) UART0_RTSn.I2C3_SDA */
>   		>;
>   	};
>   
> +	mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) UART0_CTSn.GPIO1_22 */
> +			AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) UART0_RTSn.GPIO1_23 */
> +		>;
> +	};
> +
>   	mikrobus_uart_pins_default: mikrobus-uart-default-pins {
>   		pinctrl-single,pins = <
>   			AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) MCAN0_TX.UART5_RXD */
> @@ -401,6 +471,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) MCAN0_RX.UART5_TXD */
>   		>;
>   	};
>   
> +	mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) MCAN0_TX.GPIO1_24 */
> +			AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) MCAN0_RX.GPIO1_25 */
> +		>;
> +	};
> +
>   	mikrobus_spi_pins_default: mikrobus-spi-default-pins {
>   		pinctrl-single,pins = <
>   			AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) MCASP0_ACLKR.SPI2_CLK */
> @@ -410,6 +487,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) MCASP0_AXR2.SPI2_D1 */
>   		>;
>   	};
>   
> +	mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) MCASP0_AXR3.GPIO1_7 */
> +			AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) MCASP0_AXR2.GPIO1_8 */
> +			AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) MCASP0_AFSR.GPIO1_13 */
> +			AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) MCASP0_ACLKR.GPIO1_14 */
> +		>;
> +	};
> +
>   	mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
>   		bootph-all;
>   		pinctrl-single,pins = <
> @@ -630,8 +716,6 @@ &main_gpio0 {
>   
>   &main_gpio1 {
>   	bootph-all;
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_gpio_pins_default>;
>   	gpio-line-names = "", "", "", "", "",			/* 0-4 */
>   		"SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7",	/* 5-7 */
>   		"MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9",		/* 8-9 */
> @@ -804,15 +888,11 @@ it66121_out: endpoint {
>   };
>   
>   &main_i2c3 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_i2c_pins_default>;
>   	clock-frequency = <400000>;
>   	status = "okay";
>   };
>   
>   &main_spi2 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_spi_pins_default>;
>   	status = "okay";
>   };
>   
> @@ -876,8 +956,6 @@ &main_uart1 {
>   };
>   
>   &main_uart5 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_uart_pins_default>;
>   	status = "okay";
>   };
>   
> 

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Ayush Singh]

On 6/27/24 22:37, Andrew Davis wrote:
> On 6/27/24 11:26 AM, Ayush Singh wrote:
>> DONOTMERGE
>>
>> Add mikroBUS connector and some mikroBUS boards support for Beagleplay.
>> The mikroBUS boards node should probably be moved to a more appropriate
>> location but I am not quite sure where it should go since it is not
>> dependent on specific arch.
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94 
>> +++++++++++++++++++++++---
>>   1 file changed, 86 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts 
>> b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> index 70de288d728e..3f3cd70345c4 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> @@ -38,6 +38,7 @@ aliases {
>>           serial2 = &main_uart0;
>>           usb0 = &usb0;
>>           usb1 = &usb1;
>> +        mikrobus0 = &mikrobus0;
>>       };
>>         chosen {
>> @@ -227,6 +228,56 @@ simple-audio-card,codec {
>>           };
>>       };
>>   +    mikrobus0: mikrobus-connector {
>> +        compatible = "mikrobus-connector";
>> +        pinctrl-names = "default", "pwm_default", "pwm_gpio",
>> +                "uart_default", "uart_gpio", "i2c_default",
>> +                "i2c_gpio", "spi_default", "spi_gpio";
>> +        pinctrl-0 = <&mikrobus_gpio_pins_default>;
>> +        pinctrl-1 = <&mikrobus_pwm_pins_default>;
>> +        pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
>> +        pinctrl-3 = <&mikrobus_uart_pins_default>;
>> +        pinctrl-4 = <&mikrobus_uart_pins_gpio>;
>> +        pinctrl-5 = <&mikrobus_i2c_pins_default>;
>> +        pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
>> +        pinctrl-7 = <&mikrobus_spi_pins_default>;
>> +        pinctrl-8 = <&mikrobus_spi_pins_gpio>;
>> +
>> +        mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
>> +                      "mosi", "miso", "sck", "cs", "rst", "an";
>> +        mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
>> +
>> +        spi-controller = <&main_spi2>;
>> +        spi-cs = <0>;
>> +        spi-cs-names = "default";
>> +
>> +        board = <&lsm6dsl_click>;
>> +    };
>> +
>> +    mikrobus_boards {
>> +        thermo_click: thermo-click {
>> +            compatible = "maxim,max31855k", "mikrobus-spi";
>
> I might be missing something, but your solution cannot possibly be
> to list every click board that could be connected (all 1500+ of them)
> to every mikroBUS connector on every device's DT file..


I think you missed something. `mikrobus-boards` is not a child node of 
`mikrobus0`. See the `board` property in `mikrobus0`. That is what 
selects the board attached to the connector.

The `mikcrobus-boards` node itself should be moved to some independent 
location and included from a system that wants to support mikrobus 
boards. The connector will only have a phandle to the board (or boards 
in case a single mikroBUS board has 1 i2c and 1 spi sensor or some 
combination).


>
> Each click board should have a single DTSO overlay file to describe the
> click board, one per click board total. And then that overlay should
> apply cleanly to any device that has a mikroBUS interface.


Yes, that is the goal.


>
> Which means you have not completely solved the fundamental problem of
> abstracting the mikroBUS connector in DT. Each of these click device 
> child
> nodes has to be under the parent connector node. Which means a phandle
> to the parent node, which is not generically named. For instance
> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
> the click board's overlay would look like this:
>
> /dts-v1/;
> /plugin/;
>
> &mikrobus0 {
>     status = "okay";
>
>     mikrobus_board {
>         thermo-click {
>             compatible = "maxim,max31855k", "mikrobus-spi";
>             spi-max-frequency = <1000000>;
>             pinctrl-apply = "spi_default";
>         };
>     };
> };


No, it will look as follows:

```

&mikrobus0 {
     status = "okay";

     board = <&thermo-click>;

};


&mikrobus_board {
         thermo-click {
             compatible = "maxim,max31855k", "mikrobus-spi";
             spi-max-frequency = <1000000>;
             pinctrl-apply = "spi_default";
         };
   };

```


>
> I think this solution is almost there, but once you solve the above
> issue, we could just apply the right overlay for our attached click
> board ahead of time and not need the mikroBUS bus driver at all.


Well, the driver is still needed because some things cannot be done 
generically in dt. Eg:

1. SPI chipselect. Each connector will have different chipselect number 
mapped to CS pin. In fact a mikrobus board might use other pins like RST 
as chipselect as well.

2. Using pins other than their intended purpose like GPIO.


>
> Andrew
>
>> +            spi-max-frequency = <1000000>;
>> +            pinctrl-apply = "spi_default";
>> +        };
>> +
>> +        lsm6dsl_click: lsm6dsl-click {
>> +            compatible = "st,lsm6ds3", "mikrobus-spi";
>> +            spi-max-frequency = <1000000>;
>> +            pinctrl-apply = "spi_default";
>> +        };
>> +    };
>>   };
>>     &main_pmx0 {
>> @@ -387,6 +438,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) 
>> EXT_REFCLK1.CLKOUT0 */
>>           >;
>>       };
>>   +    mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) 
>> MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
>> +        >;
>> +    };
>> +
>> +    mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) 
>> MCASP0_ACLKX.GPIO1_11 */
>> +        >;
>> +    };
>> +
>>       mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
>>           pinctrl-single,pins = <
>>               AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) 
>> UART0_CTSn.I2C3_SCL */
>> @@ -394,6 +457,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* 
>> (B15) UART0_RTSn.I2C3_SDA */
>>           >;
>>       };
>>   +    mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) 
>> UART0_CTSn.GPIO1_22 */
>> +            AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) 
>> UART0_RTSn.GPIO1_23 */
>> +        >;
>> +    };
>> +
>>       mikrobus_uart_pins_default: mikrobus-uart-default-pins {
>>           pinctrl-single,pins = <
>>               AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) 
>> MCAN0_TX.UART5_RXD */
>> @@ -401,6 +471,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) 
>> MCAN0_RX.UART5_TXD */
>>           >;
>>       };
>>   +    mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) 
>> MCAN0_TX.GPIO1_24 */
>> +            AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) 
>> MCAN0_RX.GPIO1_25 */
>> +        >;
>> +    };
>> +
>>       mikrobus_spi_pins_default: mikrobus-spi-default-pins {
>>           pinctrl-single,pins = <
>>               AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) 
>> MCASP0_ACLKR.SPI2_CLK */
>> @@ -410,6 +487,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) 
>> MCASP0_AXR2.SPI2_D1 */
>>           >;
>>       };
>>   +    mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) 
>> MCASP0_AXR3.GPIO1_7 */
>> +            AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) 
>> MCASP0_AXR2.GPIO1_8 */
>> +            AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) 
>> MCASP0_AFSR.GPIO1_13 */
>> +            AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) 
>> MCASP0_ACLKR.GPIO1_14 */
>> +        >;
>> +    };
>> +
>>       mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
>>           bootph-all;
>>           pinctrl-single,pins = <
>> @@ -630,8 +716,6 @@ &main_gpio0 {
>>     &main_gpio1 {
>>       bootph-all;
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_gpio_pins_default>;
>>       gpio-line-names = "", "", "", "", "",            /* 0-4 */
>>           "SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7",    /* 5-7 */
>>           "MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9",        /* 8-9 */
>> @@ -804,15 +888,11 @@ it66121_out: endpoint {
>>   };
>>     &main_i2c3 {
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_i2c_pins_default>;
>>       clock-frequency = <400000>;
>>       status = "okay";
>>   };
>>     &main_spi2 {
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_spi_pins_default>;
>>       status = "okay";
>>   };
>>   @@ -876,8 +956,6 @@ &main_uart1 {
>>   };
>>     &main_uart5 {
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_uart_pins_default>;
>>       status = "okay";
>>   };
>>

Ayush Singh

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Andrew Davis]

On 6/27/24 12:16 PM, Ayush Singh wrote:
> 
> On 6/27/24 22:37, Andrew Davis wrote:
>> On 6/27/24 11:26 AM, Ayush Singh wrote:
>>> DONOTMERGE
>>>
>>> Add mikroBUS connector and some mikroBUS boards support for Beagleplay.
>>> The mikroBUS boards node should probably be moved to a more appropriate
>>> location but I am not quite sure where it should go since it is not
>>> dependent on specific arch.
>>>
>>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>>> ---
>>>   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94 +++++++++++++++++++++++---
>>>   1 file changed, 86 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>>> index 70de288d728e..3f3cd70345c4 100644
>>> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>>> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>>> @@ -38,6 +38,7 @@ aliases {
>>>           serial2 = &main_uart0;
>>>           usb0 = &usb0;
>>>           usb1 = &usb1;
>>> +        mikrobus0 = &mikrobus0;
>>>       };
>>>         chosen {
>>> @@ -227,6 +228,56 @@ simple-audio-card,codec {
>>>           };
>>>       };
>>>   +    mikrobus0: mikrobus-connector {
>>> +        compatible = "mikrobus-connector";
>>> +        pinctrl-names = "default", "pwm_default", "pwm_gpio",
>>> +                "uart_default", "uart_gpio", "i2c_default",
>>> +                "i2c_gpio", "spi_default", "spi_gpio";
>>> +        pinctrl-0 = <&mikrobus_gpio_pins_default>;
>>> +        pinctrl-1 = <&mikrobus_pwm_pins_default>;
>>> +        pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
>>> +        pinctrl-3 = <&mikrobus_uart_pins_default>;
>>> +        pinctrl-4 = <&mikrobus_uart_pins_gpio>;
>>> +        pinctrl-5 = <&mikrobus_i2c_pins_default>;
>>> +        pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
>>> +        pinctrl-7 = <&mikrobus_spi_pins_default>;
>>> +        pinctrl-8 = <&mikrobus_spi_pins_gpio>;
>>> +
>>> +        mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
>>> +                      "mosi", "miso", "sck", "cs", "rst", "an";
>>> +        mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
>>> +                 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
>>> +
>>> +        spi-controller = <&main_spi2>;
>>> +        spi-cs = <0>;
>>> +        spi-cs-names = "default";
>>> +
>>> +        board = <&lsm6dsl_click>;
>>> +    };
>>> +
>>> +    mikrobus_boards {
>>> +        thermo_click: thermo-click {
>>> +            compatible = "maxim,max31855k", "mikrobus-spi";
>>
>> I might be missing something, but your solution cannot possibly be
>> to list every click board that could be connected (all 1500+ of them)
>> to every mikroBUS connector on every device's DT file..
> 
> 
> I think you missed something. `mikrobus-boards` is not a child node of `mikrobus0`. See the `board` property in `mikrobus0`. That is what selects the board attached to the connector.
> 

That seems even worse.. That means the board file needs to know about the
attached board, which is not how DT works. It describes hardware in a
hierarchical/acyclic graph. For instance, take an I2C device, its node
is a child of the I2C bus, and has phandle pointers to the IRQ it uses
(or whatever else provider it needs). What you have here is like the
I2C bus node phandle pointing to the connected child devices.

> The `mikcrobus-boards` node itself should be moved to some independent location and included from a system that wants to support mikrobus boards. The connector will only have a phandle to the board (or boards in case a single mikroBUS board has 1 i2c and 1 spi sensor or some combination).
> 

How about providing the full/final example then (this series should be marked
as RFC as it is now has missing parts). Move the click board node into a DTSO
file and put that in a common location (click boards are common to all boards
right, so lets say in drivers/of/click for now just for the RFC).

> 
>>
>> Each click board should have a single DTSO overlay file to describe the
>> click board, one per click board total. And then that overlay should
>> apply cleanly to any device that has a mikroBUS interface.
> 
> 
> Yes, that is the goal.
> 
> 
>>
>> Which means you have not completely solved the fundamental problem of
>> abstracting the mikroBUS connector in DT. Each of these click device child
>> nodes has to be under the parent connector node. Which means a phandle
>> to the parent node, which is not generically named. For instance
>> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
>> the click board's overlay would look like this:
>>
>> /dts-v1/;
>> /plugin/;
>>
>> &mikrobus0 {
>>     status = "okay";
>>
>>     mikrobus_board {
>>         thermo-click {
>>             compatible = "maxim,max31855k", "mikrobus-spi";
>>             spi-max-frequency = <1000000>;
>>             pinctrl-apply = "spi_default";
>>         };
>>     };
>> };
> 
> 
> No, it will look as follows:
> 
> ```
> 
> &mikrobus0 {

           ^^^
So same issue, what if I want to attach this click board
to the second mikrobus connector on my board (i.e. mikrobus1),
I'd have to modify the overlay.. Or have an overlay for every
possible connector instance number.

>      status = "okay";
> 
>      board = <&thermo-click>;
> 
> };
> 
> 
> &mikrobus_board {
>          thermo-click {
>              compatible = "maxim,max31855k", "mikrobus-spi";
>              spi-max-frequency = <1000000>;
>              pinctrl-apply = "spi_default";
>          };
>    };
> 
> ```
> 
> 
>>
>> I think this solution is almost there, but once you solve the above
>> issue, we could just apply the right overlay for our attached click
>> board ahead of time and not need the mikroBUS bus driver at all.
> 
> 
> Well, the driver is still needed because some things cannot be done generically in dt. Eg:
> 
> 1. SPI chipselect. Each connector will have different chipselect number mapped to CS pin. In fact a mikrobus board might use other pins like RST as chipselect as well.
> 
> 2. Using pins other than their intended purpose like GPIO.
> 

Then these are two things you'll need to solve. We can add
these functions to the base DT/OF support if they are generic
problems (which they are, other connectors/daughterboards have
this same issue, RPi header, Seeed Grove connector, Sparkfun QWIIC
headers, etc..).

Andrew

> 
>>
>> Andrew
>>
>>> +            spi-max-frequency = <1000000>;
>>> +            pinctrl-apply = "spi_default";
>>> +        };
>>> +
>>> +        lsm6dsl_click: lsm6dsl-click {
>>> +            compatible = "st,lsm6ds3", "mikrobus-spi";
>>> +            spi-max-frequency = <1000000>;
>>> +            pinctrl-apply = "spi_default";
>>> +        };
>>> +    };
>>>   };
>>>     &main_pmx0 {
>>> @@ -387,6 +438,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) EXT_REFCLK1.CLKOUT0 */
>>>           >;
>>>       };
>>>   +    mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
>>> +        pinctrl-single,pins = <
>>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
>>> +        >;
>>> +    };
>>> +
>>> +    mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
>>> +        pinctrl-single,pins = <
>>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) MCASP0_ACLKX.GPIO1_11 */
>>> +        >;
>>> +    };
>>> +
>>>       mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
>>>           pinctrl-single,pins = <
>>>               AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) UART0_CTSn.I2C3_SCL */
>>> @@ -394,6 +457,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* (B15) UART0_RTSn.I2C3_SDA */
>>>           >;
>>>       };
>>>   +    mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
>>> +        pinctrl-single,pins = <
>>> +            AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) UART0_CTSn.GPIO1_22 */
>>> +            AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) UART0_RTSn.GPIO1_23 */
>>> +        >;
>>> +    };
>>> +
>>>       mikrobus_uart_pins_default: mikrobus-uart-default-pins {
>>>           pinctrl-single,pins = <
>>>               AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) MCAN0_TX.UART5_RXD */
>>> @@ -401,6 +471,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) MCAN0_RX.UART5_TXD */
>>>           >;
>>>       };
>>>   +    mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
>>> +        pinctrl-single,pins = <
>>> +            AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) MCAN0_TX.GPIO1_24 */
>>> +            AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) MCAN0_RX.GPIO1_25 */
>>> +        >;
>>> +    };
>>> +
>>>       mikrobus_spi_pins_default: mikrobus-spi-default-pins {
>>>           pinctrl-single,pins = <
>>>               AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) MCASP0_ACLKR.SPI2_CLK */
>>> @@ -410,6 +487,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) MCASP0_AXR2.SPI2_D1 */
>>>           >;
>>>       };
>>>   +    mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
>>> +        pinctrl-single,pins = <
>>> +            AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) MCASP0_AXR3.GPIO1_7 */
>>> +            AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) MCASP0_AXR2.GPIO1_8 */
>>> +            AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) MCASP0_AFSR.GPIO1_13 */
>>> +            AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) MCASP0_ACLKR.GPIO1_14 */
>>> +        >;
>>> +    };
>>> +
>>>       mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
>>>           bootph-all;
>>>           pinctrl-single,pins = <
>>> @@ -630,8 +716,6 @@ &main_gpio0 {
>>>     &main_gpio1 {
>>>       bootph-all;
>>> -    pinctrl-names = "default";
>>> -    pinctrl-0 = <&mikrobus_gpio_pins_default>;
>>>       gpio-line-names = "", "", "", "", "",            /* 0-4 */
>>>           "SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7",    /* 5-7 */
>>>           "MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9",        /* 8-9 */
>>> @@ -804,15 +888,11 @@ it66121_out: endpoint {
>>>   };
>>>     &main_i2c3 {
>>> -    pinctrl-names = "default";
>>> -    pinctrl-0 = <&mikrobus_i2c_pins_default>;
>>>       clock-frequency = <400000>;
>>>       status = "okay";
>>>   };
>>>     &main_spi2 {
>>> -    pinctrl-names = "default";
>>> -    pinctrl-0 = <&mikrobus_spi_pins_default>;
>>>       status = "okay";
>>>   };
>>>   @@ -876,8 +956,6 @@ &main_uart1 {
>>>   };
>>>     &main_uart5 {
>>> -    pinctrl-names = "default";
>>> -    pinctrl-0 = <&mikrobus_uart_pins_default>;
>>>       status = "okay";
>>>   };
>>>
> 
> Ayush Singh
> 

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Ayush Singh]

On 6/27/24 23:20, Andrew Davis wrote:
> On 6/27/24 12:16 PM, Ayush Singh wrote:
>>
>> On 6/27/24 22:37, Andrew Davis wrote:
>>> On 6/27/24 11:26 AM, Ayush Singh wrote:
>>>> DONOTMERGE
>>>>
>>>> Add mikroBUS connector and some mikroBUS boards support for 
>>>> Beagleplay.
>>>> The mikroBUS boards node should probably be moved to a more 
>>>> appropriate
>>>> location but I am not quite sure where it should go since it is not
>>>> dependent on specific arch.
>>>>
>>>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>>>> ---
>>>>   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94 
>>>> +++++++++++++++++++++++---
>>>>   1 file changed, 86 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts 
>>>> b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>>>> index 70de288d728e..3f3cd70345c4 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>>>> @@ -38,6 +38,7 @@ aliases {
>>>>           serial2 = &main_uart0;
>>>>           usb0 = &usb0;
>>>>           usb1 = &usb1;
>>>> +        mikrobus0 = &mikrobus0;
>>>>       };
>>>>         chosen {
>>>> @@ -227,6 +228,56 @@ simple-audio-card,codec {
>>>>           };
>>>>       };
>>>>   +    mikrobus0: mikrobus-connector {
>>>> +        compatible = "mikrobus-connector";
>>>> +        pinctrl-names = "default", "pwm_default", "pwm_gpio",
>>>> +                "uart_default", "uart_gpio", "i2c_default",
>>>> +                "i2c_gpio", "spi_default", "spi_gpio";
>>>> +        pinctrl-0 = <&mikrobus_gpio_pins_default>;
>>>> +        pinctrl-1 = <&mikrobus_pwm_pins_default>;
>>>> +        pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
>>>> +        pinctrl-3 = <&mikrobus_uart_pins_default>;
>>>> +        pinctrl-4 = <&mikrobus_uart_pins_gpio>;
>>>> +        pinctrl-5 = <&mikrobus_i2c_pins_default>;
>>>> +        pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
>>>> +        pinctrl-7 = <&mikrobus_spi_pins_default>;
>>>> +        pinctrl-8 = <&mikrobus_spi_pins_gpio>;
>>>> +
>>>> +        mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
>>>> +                      "mosi", "miso", "sck", "cs", "rst", "an";
>>>> +        mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
>>>> +                 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
>>>> +
>>>> +        spi-controller = <&main_spi2>;
>>>> +        spi-cs = <0>;
>>>> +        spi-cs-names = "default";
>>>> +
>>>> +        board = <&lsm6dsl_click>;
>>>> +    };
>>>> +
>>>> +    mikrobus_boards {
>>>> +        thermo_click: thermo-click {
>>>> +            compatible = "maxim,max31855k", "mikrobus-spi";
>>>
>>> I might be missing something, but your solution cannot possibly be
>>> to list every click board that could be connected (all 1500+ of them)
>>> to every mikroBUS connector on every device's DT file..
>>
>>
>> I think you missed something. `mikrobus-boards` is not a child node 
>> of `mikrobus0`. See the `board` property in `mikrobus0`. That is what 
>> selects the board attached to the connector.
>>
>
> That seems even worse.. That means the board file needs to know about the
> attached board, which is not how DT works. It describes hardware in a
> hierarchical/acyclic graph. For instance, take an I2C device, its node
> is a child of the I2C bus, and has phandle pointers to the IRQ it uses
> (or whatever else provider it needs). What you have here is like the
> I2C bus node phandle pointing to the connected child devices.
>
>> The `mikcrobus-boards` node itself should be moved to some 
>> independent location and included from a system that wants to support 
>> mikrobus boards. The connector will only have a phandle to the board 
>> (or boards in case a single mikroBUS board has 1 i2c and 1 spi sensor 
>> or some combination).
>>
>
> How about providing the full/final example then (this series should be 
> marked
> as RFC as it is now has missing parts). Move the click board node into 
> a DTSO
> file and put that in a common location (click boards are common to all 
> boards
> right, so lets say in drivers/of/click for now just for the RFC).
>
>>
>>>
>>> Each click board should have a single DTSO overlay file to describe the
>>> click board, one per click board total. And then that overlay should
>>> apply cleanly to any device that has a mikroBUS interface.
>>
>>
>> Yes, that is the goal.
>>
>>
>>>
>>> Which means you have not completely solved the fundamental problem of
>>> abstracting the mikroBUS connector in DT. Each of these click device 
>>> child
>>> nodes has to be under the parent connector node. Which means a phandle
>>> to the parent node, which is not generically named. For instance
>>> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
>>> the click board's overlay would look like this:
>>>
>>> /dts-v1/;
>>> /plugin/;
>>>
>>> &mikrobus0 {
>>>     status = "okay";
>>>
>>>     mikrobus_board {
>>>         thermo-click {
>>>             compatible = "maxim,max31855k", "mikrobus-spi";
>>>             spi-max-frequency = <1000000>;
>>>             pinctrl-apply = "spi_default";
>>>         };
>>>     };
>>> };
>>
>>
>> No, it will look as follows:
>>
>> ```
>>
>> &mikrobus0 {
>
>           ^^^
> So same issue, what if I want to attach this click board
> to the second mikrobus connector on my board (i.e. mikrobus1),
> I'd have to modify the overlay.. Or have an overlay for every
> possible connector instance number.


The plan is to have a sysfs `new_device` and `delete_device` entry like 
I2C has where the board name is passed. The driver can then create a dt 
changeset apply to live tree. In the current dt, the changeset is to add 
a `board` property with the phandle of a board (if found).

Can you suggest how something similar will be possible if the board node 
is a child of the connector node? Maybe it is possible to take a generic 
dt overlay and change the name of parent node on it or something?


>
>>      status = "okay";
>>
>>      board = <&thermo-click>;
>>
>> };
>>
>>
>> &mikrobus_board {
>>          thermo-click {
>>              compatible = "maxim,max31855k", "mikrobus-spi";
>>              spi-max-frequency = <1000000>;
>>              pinctrl-apply = "spi_default";
>>          };
>>    };
>>
>> ```
>>
>>
>>>
>>> I think this solution is almost there, but once you solve the above
>>> issue, we could just apply the right overlay for our attached click
>>> board ahead of time and not need the mikroBUS bus driver at all.
>>
>>
>> Well, the driver is still needed because some things cannot be done 
>> generically in dt. Eg:
>>
>> 1. SPI chipselect. Each connector will have different chipselect 
>> number mapped to CS pin. In fact a mikrobus board might use other 
>> pins like RST as chipselect as well.
>>
>> 2. Using pins other than their intended purpose like GPIO.
>>
>
> Then these are two things you'll need to solve. We can add
> these functions to the base DT/OF support if they are generic
> problems (which they are, other connectors/daughterboards have
> this same issue, RPi header, Seeed Grove connector, Sparkfun QWIIC
> headers, etc..).
>
> Andrew
>
>>
>>>
>>> Andrew
>>>
>>>> +            spi-max-frequency = <1000000>;
>>>> +            pinctrl-apply = "spi_default";
>>>> +        };
>>>> +
>>>> +        lsm6dsl_click: lsm6dsl-click {
>>>> +            compatible = "st,lsm6ds3", "mikrobus-spi";
>>>> +            spi-max-frequency = <1000000>;
>>>> +            pinctrl-apply = "spi_default";
>>>> +        };
>>>> +    };
>>>>   };
>>>>     &main_pmx0 {
>>>> @@ -387,6 +438,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) 
>>>> EXT_REFCLK1.CLKOUT0 */
>>>>           >;
>>>>       };
>>>>   +    mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
>>>> +        pinctrl-single,pins = <
>>>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) 
>>>> MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
>>>> +        >;
>>>> +    };
>>>> +
>>>> +    mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
>>>> +        pinctrl-single,pins = <
>>>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) 
>>>> MCASP0_ACLKX.GPIO1_11 */
>>>> +        >;
>>>> +    };
>>>> +
>>>>       mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
>>>>           pinctrl-single,pins = <
>>>>               AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) 
>>>> UART0_CTSn.I2C3_SCL */
>>>> @@ -394,6 +457,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* 
>>>> (B15) UART0_RTSn.I2C3_SDA */
>>>>           >;
>>>>       };
>>>>   +    mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
>>>> +        pinctrl-single,pins = <
>>>> +            AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) 
>>>> UART0_CTSn.GPIO1_22 */
>>>> +            AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) 
>>>> UART0_RTSn.GPIO1_23 */
>>>> +        >;
>>>> +    };
>>>> +
>>>>       mikrobus_uart_pins_default: mikrobus-uart-default-pins {
>>>>           pinctrl-single,pins = <
>>>>               AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) 
>>>> MCAN0_TX.UART5_RXD */
>>>> @@ -401,6 +471,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) 
>>>> MCAN0_RX.UART5_TXD */
>>>>           >;
>>>>       };
>>>>   +    mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
>>>> +        pinctrl-single,pins = <
>>>> +            AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) 
>>>> MCAN0_TX.GPIO1_24 */
>>>> +            AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) 
>>>> MCAN0_RX.GPIO1_25 */
>>>> +        >;
>>>> +    };
>>>> +
>>>>       mikrobus_spi_pins_default: mikrobus-spi-default-pins {
>>>>           pinctrl-single,pins = <
>>>>               AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) 
>>>> MCASP0_ACLKR.SPI2_CLK */
>>>> @@ -410,6 +487,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) 
>>>> MCASP0_AXR2.SPI2_D1 */
>>>>           >;
>>>>       };
>>>>   +    mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
>>>> +        pinctrl-single,pins = <
>>>> +            AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) 
>>>> MCASP0_AXR3.GPIO1_7 */
>>>> +            AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) 
>>>> MCASP0_AXR2.GPIO1_8 */
>>>> +            AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) 
>>>> MCASP0_AFSR.GPIO1_13 */
>>>> +            AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) 
>>>> MCASP0_ACLKR.GPIO1_14 */
>>>> +        >;
>>>> +    };
>>>> +
>>>>       mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
>>>>           bootph-all;
>>>>           pinctrl-single,pins = <
>>>> @@ -630,8 +716,6 @@ &main_gpio0 {
>>>>     &main_gpio1 {
>>>>       bootph-all;
>>>> -    pinctrl-names = "default";
>>>> -    pinctrl-0 = <&mikrobus_gpio_pins_default>;
>>>>       gpio-line-names = "", "", "", "", "",            /* 0-4 */
>>>>           "SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7",    /* 5-7 */
>>>>           "MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9",        /* 8-9 */
>>>> @@ -804,15 +888,11 @@ it66121_out: endpoint {
>>>>   };
>>>>     &main_i2c3 {
>>>> -    pinctrl-names = "default";
>>>> -    pinctrl-0 = <&mikrobus_i2c_pins_default>;
>>>>       clock-frequency = <400000>;
>>>>       status = "okay";
>>>>   };
>>>>     &main_spi2 {
>>>> -    pinctrl-names = "default";
>>>> -    pinctrl-0 = <&mikrobus_spi_pins_default>;
>>>>       status = "okay";
>>>>   };
>>>>   @@ -876,8 +956,6 @@ &main_uart1 {
>>>>   };
>>>>     &main_uart5 {
>>>> -    pinctrl-names = "default";
>>>> -    pinctrl-0 = <&mikrobus_uart_pins_default>;
>>>>       status = "okay";
>>>>   };
>>>>
>>
>> Ayush Singh
>>

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Andrew Lunn]

> Can you suggest how something similar will be possible if the board node is
> a child of the connector node? Maybe it is possible to take a generic dt
> overlay and change the name of parent node on it or something?

Maybe that answers my question on an earlier patch.

So the parent node of the overlay is embedded inside the overlay?

Making a guess without looking at the code... The code loading an
overlay must first parse the overlay and find that node name. It then
must somehow traverses the graph, and find that node. It then must
apply the overlay at that point.

Could you not take this code apart, so you can pass it the name of the
node you want to apply the overlay to, rather than read it from the
overlay itself? The connector should known its own node in the
graph. So it can find the overlay, load it, and then ask for it to be
applied over itself.

	Andrew

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Rob Herring]

On Thu, Jun 27, 2024 at 11:46:31PM +0530, Ayush Singh wrote:
> 
> On 6/27/24 23:20, Andrew Davis wrote:
> > On 6/27/24 12:16 PM, Ayush Singh wrote:
> > > 
> > > On 6/27/24 22:37, Andrew Davis wrote:
> > > > On 6/27/24 11:26 AM, Ayush Singh wrote:
> > > > > DONOTMERGE
> > > > > 
> > > > > Add mikroBUS connector and some mikroBUS boards support for
> > > > > Beagleplay.
> > > > > The mikroBUS boards node should probably be moved to a more
> > > > > appropriate
> > > > > location but I am not quite sure where it should go since it is not
> > > > > dependent on specific arch.
> > > > > 
> > > > > Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> > > > > ---
> > > > >   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94
> > > > > +++++++++++++++++++++++---
> > > > >   1 file changed, 86 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> > > > > b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> > > > > index 70de288d728e..3f3cd70345c4 100644
> > > > > --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> > > > > +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> > > > > @@ -38,6 +38,7 @@ aliases {
> > > > >           serial2 = &main_uart0;
> > > > >           usb0 = &usb0;
> > > > >           usb1 = &usb1;
> > > > > +        mikrobus0 = &mikrobus0;
> > > > >       };
> > > > >         chosen {
> > > > > @@ -227,6 +228,56 @@ simple-audio-card,codec {
> > > > >           };
> > > > >       };
> > > > >   +    mikrobus0: mikrobus-connector {
> > > > > +        compatible = "mikrobus-connector";
> > > > > +        pinctrl-names = "default", "pwm_default", "pwm_gpio",
> > > > > +                "uart_default", "uart_gpio", "i2c_default",
> > > > > +                "i2c_gpio", "spi_default", "spi_gpio";
> > > > > +        pinctrl-0 = <&mikrobus_gpio_pins_default>;
> > > > > +        pinctrl-1 = <&mikrobus_pwm_pins_default>;
> > > > > +        pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
> > > > > +        pinctrl-3 = <&mikrobus_uart_pins_default>;
> > > > > +        pinctrl-4 = <&mikrobus_uart_pins_gpio>;
> > > > > +        pinctrl-5 = <&mikrobus_i2c_pins_default>;
> > > > > +        pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
> > > > > +        pinctrl-7 = <&mikrobus_spi_pins_default>;
> > > > > +        pinctrl-8 = <&mikrobus_spi_pins_gpio>;
> > > > > +
> > > > > +        mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
> > > > > +                      "mosi", "miso", "sck", "cs", "rst", "an";
> > > > > +        mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
> > > > > +                 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
> > > > > +
> > > > > +        spi-controller = <&main_spi2>;
> > > > > +        spi-cs = <0>;
> > > > > +        spi-cs-names = "default";
> > > > > +
> > > > > +        board = <&lsm6dsl_click>;
> > > > > +    };
> > > > > +
> > > > > +    mikrobus_boards {
> > > > > +        thermo_click: thermo-click {
> > > > > +            compatible = "maxim,max31855k", "mikrobus-spi";
> > > > 
> > > > I might be missing something, but your solution cannot possibly be
> > > > to list every click board that could be connected (all 1500+ of them)
> > > > to every mikroBUS connector on every device's DT file..
> > > 
> > > 
> > > I think you missed something. `mikrobus-boards` is not a child node
> > > of `mikrobus0`. See the `board` property in `mikrobus0`. That is
> > > what selects the board attached to the connector.
> > > 
> > 
> > That seems even worse.. That means the board file needs to know about the
> > attached board, which is not how DT works. It describes hardware in a
> > hierarchical/acyclic graph. For instance, take an I2C device, its node
> > is a child of the I2C bus, and has phandle pointers to the IRQ it uses
> > (or whatever else provider it needs). What you have here is like the
> > I2C bus node phandle pointing to the connected child devices.
> > 
> > > The `mikcrobus-boards` node itself should be moved to some
> > > independent location and included from a system that wants to
> > > support mikrobus boards. The connector will only have a phandle to
> > > the board (or boards in case a single mikroBUS board has 1 i2c and 1
> > > spi sensor or some combination).
> > > 
> > 
> > How about providing the full/final example then (this series should be
> > marked
> > as RFC as it is now has missing parts). Move the click board node into a
> > DTSO
> > file and put that in a common location (click boards are common to all
> > boards
> > right, so lets say in drivers/of/click for now just for the RFC).
> > 
> > > 
> > > > 
> > > > Each click board should have a single DTSO overlay file to describe the
> > > > click board, one per click board total. And then that overlay should
> > > > apply cleanly to any device that has a mikroBUS interface.
> > > 
> > > 
> > > Yes, that is the goal.
> > > 
> > > 
> > > > 
> > > > Which means you have not completely solved the fundamental problem of
> > > > abstracting the mikroBUS connector in DT. Each of these click
> > > > device child
> > > > nodes has to be under the parent connector node. Which means a phandle
> > > > to the parent node, which is not generically named. For instance
> > > > if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
> > > > the click board's overlay would look like this:
> > > > 
> > > > /dts-v1/;
> > > > /plugin/;
> > > > 
> > > > &mikrobus0 {
> > > >     status = "okay";
> > > > 
> > > >     mikrobus_board {
> > > >         thermo-click {
> > > >             compatible = "maxim,max31855k", "mikrobus-spi";
> > > >             spi-max-frequency = <1000000>;
> > > >             pinctrl-apply = "spi_default";
> > > >         };
> > > >     };
> > > > };
> > > 
> > > 
> > > No, it will look as follows:
> > > 
> > > ```
> > > 
> > > &mikrobus0 {
> > 
> >           ^^^
> > So same issue, what if I want to attach this click board
> > to the second mikrobus connector on my board (i.e. mikrobus1),
> > I'd have to modify the overlay.. Or have an overlay for every
> > possible connector instance number.
> 
> 
> The plan is to have a sysfs `new_device` and `delete_device` entry like I2C
> has where the board name is passed. The driver can then create a dt
> changeset apply to live tree. In the current dt, the changeset is to add a
> `board` property with the phandle of a board (if found).
> 
> Can you suggest how something similar will be possible if the board node is
> a child of the connector node? Maybe it is possible to take a generic dt
> overlay and change the name of parent node on it or something?

You need to describe the problem(s) to solve first, and then a solution.
You're just giving us a solution to review.

Let's start with you have an add-on board and an overlay for that board. 
No one wants N overlays for N base board for a single physical board. 
One board, one overlay. Any solution must provide that.

Who knows when a board is connected and what board it is? Each one 
could be the OS or the user. Worst case is nothing is detected and it is 
just the user knows "I've connected board X to connector A" and has to 
tell the OS that. Or the OS can detect a board and figure out what board 
via EEPROM with no user intervention. In between is OS detects a board, 
but needs the user to say which one. The detecting everything case is 
easy. The connector driver knows which connector it is and which 
overlay. You just need to define how you select the overlay file based 
on EEPROM data. The others will need some sort of interface for 
the user to provide the information.

Rob

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Michael Walle]

On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote:
> > +	mikrobus_boards {
> > +		thermo_click: thermo-click {
> > +			compatible = "maxim,max31855k", "mikrobus-spi";
>
> I might be missing something, but your solution cannot possibly be
> to list every click board that could be connected (all 1500+ of them)
> to every mikroBUS connector on every device's DT file..
>
> Each click board should have a single DTSO overlay file to describe the
> click board, one per click board total. And then that overlay should
> apply cleanly to any device that has a mikroBUS interface.
>
> Which means you have not completely solved the fundamental problem of
> abstracting the mikroBUS connector in DT. Each of these click device child
> nodes has to be under the parent connector node. Which means a phandle
> to the parent node, which is not generically named. For instance
> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
> the click board's overlay would look like this:
>
> /dts-v1/;
> /plugin/;
>
> &mikrobus0 {
> 	status = "okay";
>
> 	mikrobus_board {
> 		thermo-click {
> 			compatible = "maxim,max31855k", "mikrobus-spi";
> 			spi-max-frequency = <1000000>;
> 			pinctrl-apply = "spi_default";
> 		};
> 	};
> };

If there should only be one DT overlay per click board, how would
you apply that to to different connectors?

> I think this solution is almost there, but once you solve the above
> issue, we could just apply the right overlay for our attached click
> board ahead of time and not need the mikroBUS bus driver at all.

The bus driver would still be needed to do the enumeration of the
children, no? And it could make the chip select translations etc. So
you can use the normal bindings of these devices.

-michael

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Ayush Singh]

On 6/27/24 22:51, Michael Walle wrote:
> On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote:
>>> +	mikrobus_boards {
>>> +		thermo_click: thermo-click {
>>> +			compatible = "maxim,max31855k", "mikrobus-spi";
>> I might be missing something, but your solution cannot possibly be
>> to list every click board that could be connected (all 1500+ of them)
>> to every mikroBUS connector on every device's DT file..
>>
>> Each click board should have a single DTSO overlay file to describe the
>> click board, one per click board total. And then that overlay should
>> apply cleanly to any device that has a mikroBUS interface.
>>
>> Which means you have not completely solved the fundamental problem of
>> abstracting the mikroBUS connector in DT. Each of these click device child
>> nodes has to be under the parent connector node. Which means a phandle
>> to the parent node, which is not generically named. For instance
>> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
>> the click board's overlay would look like this:
>>
>> /dts-v1/;
>> /plugin/;
>>
>> &mikrobus0 {
>> 	status = "okay";
>>
>> 	mikrobus_board {
>> 		thermo-click {
>> 			compatible = "maxim,max31855k", "mikrobus-spi";
>> 			spi-max-frequency = <1000000>;
>> 			pinctrl-apply = "spi_default";
>> 		};
>> 	};
>> };
> If there should only be one DT overlay per click board, how would
> you apply that to to different connectors?


See my other two replies for more context:

https://lore.kernel.org/linux-arm-kernel/cef08d49-a462-4167-8b9d-bf09e8aac92f@beagleboard.org/

https://lore.kernel.org/linux-arm-kernel/e0f9754e-4d84-4ab4-82a4-34cb12800927@beagleboard.org/


My ideal design is that most mikroBUS board configs will be defined in a 
`dtsi` file which can be included by any system with mikroBUS support. 
This file might look as follows:

```

/dts-v1/;
/plugin/;

/ {
	mikrobus_boards {
		thermo_click: thermo-click {
			compatible = "maxim,max31855k", "mikrobus-spi";
			spi-max-frequency = <1000000>;
			pinctrl-apply = "spi_default";
		};

		lsm6dsl_click: lsm6dsl-click {
			compatible = "st,lsm6ds3", "mikrobus-spi";
			spi-max-frequency = <1000000>;
			pinctrl-apply = "spi_default";
		};
	};
};

```


And the board overlay will look as follows:

```

&conector {

     board = <&lsm6dsl_click>;

};

```


I do have an experimental configfs entry that passes the mikroBUS board 
id(s) and creates and applies the dt changeset to the connector dynamically.


>
>> I think this solution is almost there, but once you solve the above
>> issue, we could just apply the right overlay for our attached click
>> board ahead of time and not need the mikroBUS bus driver at all.
> The bus driver would still be needed to do the enumeration of the
> children, no? And it could make the chip select translations etc. So
> you can use the normal bindings of these devices.
>
> -michael


If static dt was the only method to detect the board, then it would be 
fine. But boards with 1-wire-eeprom can allow for for dynamic detection 
if the overlay can be system agnostic.


To make the dt system agnostic, it should be possible for the board to 
specify the following information:

1. If a pin is supposed to perform its normal function (UART TX should 
actually do UART TX), or if it should work as say GPIO (Eg, using RST 
pin as SPI chipselect).

2. Which pin(s) are the SPI chipselect.

3. If a particular pin needs to be pulled high or low for the board to 
function, etc.


So while most of the normal properties of SPI devices can be reused, 
there is a need to introduce new properties for additional information. 
In the previous patches, MikroBUS manifest was being used for this 
purpose, but for a full dt driver, the device tree needs to be extended 
to specify these extra properties that are not relevant to the 
non-mikrobus variant of the device.


Ayush Singh

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Geert Uytterhoeven]

 	Hi Michael,

On Thu, 27 Jun 2024, Michael Walle wrote:
> On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote:
>>> +	mikrobus_boards {
>>> +		thermo_click: thermo-click {
>>> +			compatible = "maxim,max31855k", "mikrobus-spi";
>>
>> I might be missing something, but your solution cannot possibly be
>> to list every click board that could be connected (all 1500+ of them)
>> to every mikroBUS connector on every device's DT file..
>>
>> Each click board should have a single DTSO overlay file to describe the
>> click board, one per click board total. And then that overlay should
>> apply cleanly to any device that has a mikroBUS interface.
>>
>> Which means you have not completely solved the fundamental problem of
>> abstracting the mikroBUS connector in DT. Each of these click device child
>> nodes has to be under the parent connector node. Which means a phandle
>> to the parent node, which is not generically named. For instance
>> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
>> the click board's overlay would look like this:
>>
>> /dts-v1/;
>> /plugin/;
>>
>> &mikrobus0 {

Let's use just "&mikrobus" instead...

>> 	status = "okay";
>>
>> 	mikrobus_board {
>> 		thermo-click {
>> 			compatible = "maxim,max31855k", "mikrobus-spi";
>> 			spi-max-frequency = <1000000>;
>> 			pinctrl-apply = "spi_default";
>> 		};
>> 	};
>> };
>
> If there should only be one DT overlay per click board, how would
> you apply that to to different connectors?

You teach fdtoverlay[*] to translate anchors, e.g.

     fdtoverlay -i base.dtb -o final.dtb \
 	       -t mikrobus=mikrobus0 click1.dtbo \
 	       -t mikrobus=mikrobus1 click2.dtbo

I believe the Raspberry Pi people already have something like that.

The mikrobus node handles all other translations (e.g. mapping from
Mikrobus pins to GPIO numbers), so you do not have to handle these
explicitly when adding an overlay.

[*] And anything else that handles overlays (U-Boot, out-of-tree
     OF_CONFIGFS, ...)

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Geert Uytterhoeven]

On Fri, Jul 5, 2024 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, 27 Jun 2024, Michael Walle wrote:
> > On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote:
> >>> +   mikrobus_boards {
> >>> +           thermo_click: thermo-click {
> >>> +                   compatible = "maxim,max31855k", "mikrobus-spi";
> >>
> >> I might be missing something, but your solution cannot possibly be
> >> to list every click board that could be connected (all 1500+ of them)
> >> to every mikroBUS connector on every device's DT file..
> >>
> >> Each click board should have a single DTSO overlay file to describe the
> >> click board, one per click board total. And then that overlay should
> >> apply cleanly to any device that has a mikroBUS interface.
> >>
> >> Which means you have not completely solved the fundamental problem of
> >> abstracting the mikroBUS connector in DT. Each of these click device child
> >> nodes has to be under the parent connector node. Which means a phandle
> >> to the parent node, which is not generically named. For instance
> >> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
> >> the click board's overlay would look like this:
> >>
> >> /dts-v1/;
> >> /plugin/;
> >>
> >> &mikrobus0 {
>
> Let's use just "&mikrobus" instead...
>
> >>      status = "okay";
> >>
> >>      mikrobus_board {
> >>              thermo-click {
> >>                      compatible = "maxim,max31855k", "mikrobus-spi";

Max31855k is an SPI device, so its device node should be under an "spi"
subnode (with proper #{address,size}-cells) of the mikrobus connector,
and use a suitable unit-address and "reg" property, pointing to the
right SPI chip select.

> >>                      spi-max-frequency = <1000000>;

This belongs to the "spi" subnode, not the Max31855k device node.

> >>                      pinctrl-apply = "spi_default";

This belongs to the mikrobus connector node.

> >>              };
> >>      };
> >> };

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Andrew Davis]

On 7/5/24 3:01 AM, Geert Uytterhoeven wrote:
>      Hi Michael,
> 
> On Thu, 27 Jun 2024, Michael Walle wrote:
>> On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote:
>>>> +    mikrobus_boards {
>>>> +        thermo_click: thermo-click {
>>>> +            compatible = "maxim,max31855k", "mikrobus-spi";
>>>
>>> I might be missing something, but your solution cannot possibly be
>>> to list every click board that could be connected (all 1500+ of them)
>>> to every mikroBUS connector on every device's DT file..
>>>
>>> Each click board should have a single DTSO overlay file to describe the
>>> click board, one per click board total. And then that overlay should
>>> apply cleanly to any device that has a mikroBUS interface.
>>>
>>> Which means you have not completely solved the fundamental problem of
>>> abstracting the mikroBUS connector in DT. Each of these click device child
>>> nodes has to be under the parent connector node. Which means a phandle
>>> to the parent node, which is not generically named. For instance
>>> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
>>> the click board's overlay would look like this:
>>>
>>> /dts-v1/;
>>> /plugin/;
>>>
>>> &mikrobus0 {
> 
> Let's use just "&mikrobus" instead...
> 
>>>     status = "okay";
>>>
>>>     mikrobus_board {
>>>         thermo-click {
>>>             compatible = "maxim,max31855k", "mikrobus-spi";
>>>             spi-max-frequency = <1000000>;
>>>             pinctrl-apply = "spi_default";
>>>         };
>>>     };
>>> };
>>
>> If there should only be one DT overlay per click board, how would
>> you apply that to to different connectors?
> 
> You teach fdtoverlay[*] to translate anchors, e.g.
> 
>      fdtoverlay -i base.dtb -o final.dtb \
>             -t mikrobus=mikrobus0 click1.dtbo \
>             -t mikrobus=mikrobus1 click2.dtbo
> 

This basic idea is where I started also, the result is we end
up needing a huge number of "anchor" points. And they would
also be board specific. So we would want to store all these
anchor points in a file, and what better file than another
DT file.

Putting all the translations in a DT file to allow the DT overlay
to become generic is the core idea of this series[0] (looks like
you already found it, linking for other following along).

And as you note, the symbol table trick allows us to do this
without teaching fdtoverlay anything new, so it should work
as-is today for any project already supporting overlays.

> I believe the Raspberry Pi people already have something like that.
> 
> The mikrobus node handles all other translations (e.g. mapping from
> Mikrobus pins to GPIO numbers), so you do not have to handle these
> explicitly when adding an overlay.

This part seems to still be an open item. For pinmux we can name
the pinmux nodes such that their phandles are resolved on overlay
application. For Pin number/name to GPIO number we have "gpio-names",
and the names can also be generic. But for Interrupts and a couple
others, we are still missing a good way to provide a generic mapping
from pin name to number.

Andrew

[0] https://lore.kernel.org/linux-arm-kernel/20240702164403.29067-1-afd@ti.com/

> 
> [*] And anything else that handles overlays (U-Boot, out-of-tree
>      OF_CONFIGFS, ...)
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Geert Uytterhoeven]

Hi Andrew,

On Fri, Jul 5, 2024 at 6:34 PM Andrew Davis <afd@ti.com> wrote:
> On 7/5/24 3:01 AM, Geert Uytterhoeven wrote:
> > On Thu, 27 Jun 2024, Michael Walle wrote:
> >> On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote:
> >>>> +    mikrobus_boards {
> >>>> +        thermo_click: thermo-click {
> >>>> +            compatible = "maxim,max31855k", "mikrobus-spi";
> >>>
> >>> I might be missing something, but your solution cannot possibly be
> >>> to list every click board that could be connected (all 1500+ of them)
> >>> to every mikroBUS connector on every device's DT file..
> >>>
> >>> Each click board should have a single DTSO overlay file to describe the
> >>> click board, one per click board total. And then that overlay should
> >>> apply cleanly to any device that has a mikroBUS interface.
> >>>
> >>> Which means you have not completely solved the fundamental problem of
> >>> abstracting the mikroBUS connector in DT. Each of these click device child
> >>> nodes has to be under the parent connector node. Which means a phandle
> >>> to the parent node, which is not generically named. For instance
> >>> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
> >>> the click board's overlay would look like this:
> >>>
> >>> /dts-v1/;
> >>> /plugin/;
> >>>
> >>> &mikrobus0 {
> >
> > Let's use just "&mikrobus" instead...
> >
> >>>     status = "okay";
> >>>
> >>>     mikrobus_board {
> >>>         thermo-click {
> >>>             compatible = "maxim,max31855k", "mikrobus-spi";
> >>>             spi-max-frequency = <1000000>;
> >>>             pinctrl-apply = "spi_default";
> >>>         };
> >>>     };
> >>> };
> >>
> >> If there should only be one DT overlay per click board, how would
> >> you apply that to to different connectors?
> >
> > You teach fdtoverlay[*] to translate anchors, e.g.
> >
> >      fdtoverlay -i base.dtb -o final.dtb \
> >             -t mikrobus=mikrobus0 click1.dtbo \
> >             -t mikrobus=mikrobus1 click2.dtbo
> >
>
> This basic idea is where I started also, the result is we end
> up needing a huge number of "anchor" points. And they would
> also be board specific. So we would want to store all these
> anchor points in a file, and what better file than another
> DT file.

Why wouldn't a single anchor point suffice?
For Mikrobus, the anchor point should have well-known subnodes
like "spi", "i2c", ... which would take care of the translation.

> Putting all the translations in a DT file to allow the DT overlay
> to become generic is the core idea of this series[0] (looks like
> you already found it, linking for other following along).
>
> And as you note, the symbol table trick allows us to do this
> without teaching fdtoverlay anything new, so it should work
> as-is today for any project already supporting overlays.

Yes, and it sounds cool!

> > I believe the Raspberry Pi people already have something like that.
> >
> > The mikrobus node handles all other translations (e.g. mapping from
> > Mikrobus pins to GPIO numbers), so you do not have to handle these
> > explicitly when adding an overlay.
>
> This part seems to still be an open item. For pinmux we can name
> the pinmux nodes such that their phandles are resolved on overlay
> application. For Pin number/name to GPIO number we have "gpio-names",
> and the names can also be generic. But for Interrupts and a couple
> others, we are still missing a good way to provide a generic mapping
> from pin name to number.

Isn't that the purpose of nexus nodes in the DT spec?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On Thu, Jun 27, 2024 at 09:56:17PM +0530, Ayush Singh wrote:

> +	mikrobus_boards {
> +		thermo_click: thermo-click {
> +			compatible = "maxim,max31855k", "mikrobus-spi";
> +			spi-max-frequency = <1000000>;
> +			pinctrl-apply = "spi_default";
> +		};
> +
> +		lsm6dsl_click: lsm6dsl-click {
> +			compatible = "st,lsm6ds3", "mikrobus-spi";
> +			spi-max-frequency = <1000000>;
> +			pinctrl-apply = "spi_default";

So every single device that could possibly go on a mikrobus board will
need a new binding (or significant binding modifications)?

tbh, I was expecting something where the mikrobus connector looks like
what "spi-mux" does. Ditto "i2c-mux" for the i2c component.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ayush Singh]

On 6/27/24 21:56, Ayush Singh wrote:

> MikroBUS is an open standard  developed by MikroElektronika for connecting
> add-on boards to microcontrollers or microprocessors. It essentially
> allows you to easily expand the functionality of your main boards using
> these add-on boards.
>
> This patchset adds mikroBUS as a Linux bus type and provides a driver to
> parse and register the mikroBUS board using device tree infrastructure.
>
> The patchset is based on work originally done by Vaishnav.
>
> Link: https://www.mikroe.com/mikrobus
> Link: https://docs.beagleboard.org/latest/boards/beagleplay/
> Link: https://lore.kernel.org/all/20240317193714.403132-1-ayushdevel1325@gmail.com/ Patch v4
>
> Changes v5
> - Complete rewrite to use device tree instead of mikroBUS manifest.
> - Only support for SPI.
> - Adds `mikrobus,spi` compatible property.
>
> Changes v4:
> - Better commit messages
> - Remove clickID, serdev, pwm, regulator, clocks etc. Just the basic
>    mikroBUS driver.
> - Fix a lot of memory leaks, unused variables, etc.
> - Create accompanying PR in Greybus Spec repository
> - Switch to 80 columns formatting
> - Some other fixes pointed out in v3
>
> Changes in v3:
> - Use phandle instead of busname for spi
> - Use spi board info for registering new device
> - Convert dt bindings to yaml
> - Add support for clickID
> - Code cleanup and style changes
> - Additions required to spi, serdev, w1 and regulator subsystems
>
> Changes in v2:
> - support for adding mikroBUS ports from DT overlays,
> - remove debug sysFS interface for adding mikrobus ports,
> - consider extended pin usage/deviations from mikrobus standard
>    specifications
> - use greybus CPort protocol enum instead of new protocol enums
> - Fix cases of wrong indentation, ignoring return values, freeing allocated
>    resources in case of errors and other style suggestions in v1 review.
>
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
> Ayush Singh (7):
>        dt-bindings: connector: Add mikrobus-connector
>        dt-bindings: mikrobus: Add mikrobus board base
>        dt-bindings: mikrobus: Add mikrobus-spi binding
>        spi: Make of_find_spi_controller_by_node() available
>        spi: Make of_register_spi_device() available
>        mikrobus: Add mikroBUS driver
>        dts: ti: k3-am625-beagleplay: Add mikroBUS
>
>   .../bindings/connector/mikrobus-connector.yaml     | 107 ++++++
>   .../bindings/mikrobus/mikrobus-board.yaml          |  20 ++
>   .../devicetree/bindings/mikrobus/mikrobus-spi.yaml |  37 +++
>   MAINTAINERS                                        |   9 +
>   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts     |  94 +++++-
>   drivers/misc/Kconfig                               |  16 +
>   drivers/misc/Makefile                              |   1 +
>   drivers/misc/mikrobus.c                            | 361 +++++++++++++++++++++
>   drivers/spi/spi.c                                  | 209 ++++++------
>   include/linux/spi/spi.h                            |   7 +
>   10 files changed, 750 insertions(+), 111 deletions(-)
> ---
> base-commit: f76698bd9a8ca01d3581236082d786e9a6b72bb7
> change-id: 20240627-mikrobus-scratch-spi-ad8c98dcec98
>
> Best regards,


I would just like to summarize the discussions on different patches here 
to give information regarding why the board is not subnode of 
mikrobus-connector along with what questions need to be answered for a 
subnode based architecture.

I will be using (```) to differentiate between code section and non-code 
section. It is just for seperation not for any formatting since I am 
using plaintext.


Let me first summarise the goals that should be possible with any 
architecture chosen.

1. Keeping the device tree properties upstream in a system independent way.

2. Editing system dt at kernel build time to add the pre-defined board 
or applying dt overlay using uboot or dynamic overlays.

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

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 {

             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 {

         ...

     };

};

```


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. Systems with multiple mikrobus 
connectors will need multiple overlays adding the boards as child node 
of each connector (with status = "disabled"). Considering how many 
mikrobus boards are available, this can also lead to problem (especially 
in embeded Linux) with the dt binary size since each connector is 
replicating the same overlay.

```

&connector1 {

     spi = {

         board 2 {

             ...

         };

         board 3 {

             ...

         };

     };

};


&connector2 {

     spi = {

         board 2 {

             ...

         };

         board 3 {

             ...

         };

     };

};

```

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.


2. Editing system dt at kernel build time to add the pre-defined board 
or applying dt overlay using uboot or dynamic overlays.

a. mikrobus-connector has phandle to mikrobus-board

```

&connector1 {

     board = <&board1>;

};

```


b. mikrobus board is a child node of mikrobus-connector:

```

&connector1 {

     spi = {

         board 2 {

             ...

         };

     };

};

```

Both the cases will need to generate these overlays at build time. 
However, in case (a), the overlay will be much smaller than case (b). 
This is important for embeded Linux.


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

a. mikrobus-connector has phandle to mikrobus-board

It is quite simple with the current changeset APIs. I have an example 
implementation here: 
https://github.com/Ayush1325/linux/blob/c4e3d5138b7ad5c24bdbc1dd02d89720d3a5de82/drivers/misc/mikrobus.c#L59 
.

Essentially, it is possible to pass the mikroBUS board name or id to 
create changeset as long as the board has been defined in dt. The boards 
definition can be added using overlay in uboot of dynamic overlays using 
configfs patch.


b. mikrobus board is a child node of mikrobus-connector:

Since even the board definition overlay is now dependent on the 
connector, any person writing the board overlay needs to know the name's 
of the connector nodes and generate overlays for all connectors. We can 
toggle a `status` property to `okay` based on the board id passed 
through sysfs.


4. Allow using 1-wire-eeprom in a fashion that allows automatic board 
discovery.

a. mikrobus-connector has phandle to mikrobus-board

1-wire-eeprom only needs to contain the board definition overlay which 
is not dependent on the connector. The connector can generate the 
changeset of add `board` property to itself. The board should work 
irrespective of if the dt overlay is actually present in the kernel 
config since we can read the overlay from 1-wire-eeprom and apply it 
using `of_overlay_fdt_apply()`.


b. mikrobus board is a child node of mikrobus-connector:

Cannot really use the normal dt overlay. Maybe we can use the mikroBUS 
manifest to dynamically create the overlay, but well, I do not wish to 
support both the manifest and devicetree at the same time.

Maybe we can introduce something like partial device tree which only 
contains properties to be applied to a target device node? Since 
`of_overlay_fdt_apply` does contain target node property, maybe it is 
already possible to have an overlay that is generic over a type of node 
instead of the exact node?


I will also go through the overlay kernel internals to see if there are 
any better ways to use child-nodes. Feel free to chime in if you have 
any ideas.


Yours Sincerely,

Ayush Singh

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

[Andrew Lunn]

> 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

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

[Ayush Singh]

On 6/28/24 19:22, Andrew Lunn wrote:
>> 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.

I think I had a fundamental misunderstanding of what dt overlays can do. 
My understanding was that to say add thermo click in the above style 
with child nodes, the overlay needs to look as follows

&connector1 {

     spi {

         thermo_click: {

             compatible = "maxim,max31855k", "mikrobus-spi";
             spi-max-frequency = <1000000>;
             pinctrl-apply = "spi_default";

         };

     };

};


However, after going through the drm PR pointed by Rob and your 
description, it seems it is possible to create an overlay as follows:

&spi {

     thermo_click: {

         compatible = "maxim,max31855k";
         spi-max-frequency = <1000000>;
         pinctrl-apply = "spi_default";

     };

};


and apply it to the particular connector node (say connector1), at least 
from the driver. If that is the case, then yes, all of my reasons for 
not wanting to use child nodes become irrelevant.


DRM PR: 
https://lore.kernel.org/all/20240510-hotplug-drm-bridge-v2-0-ec32f2c66d56@bootlin.com/


Ayush Singh

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

[Rob Herring (Arm)]

On Thu, 27 Jun 2024 21:56:10 +0530, Ayush Singh wrote:
> MikroBUS is an open standard  developed by MikroElektronika for connecting
> add-on boards to microcontrollers or microprocessors. It essentially
> allows you to easily expand the functionality of your main boards using
> these add-on boards.
> 
> This patchset adds mikroBUS as a Linux bus type and provides a driver to
> parse and register the mikroBUS board using device tree infrastructure.
> 
> The patchset is based on work originally done by Vaishnav.
> 
> Link: https://www.mikroe.com/mikrobus
> Link: https://docs.beagleboard.org/latest/boards/beagleplay/
> Link: https://lore.kernel.org/all/20240317193714.403132-1-ayushdevel1325@gmail.com/ Patch v4
> 
> Changes v5
> - Complete rewrite to use device tree instead of mikroBUS manifest.
> - Only support for SPI.
> - Adds `mikrobus,spi` compatible property.
> 
> Changes v4:
> - Better commit messages
> - Remove clickID, serdev, pwm, regulator, clocks etc. Just the basic
>   mikroBUS driver.
> - Fix a lot of memory leaks, unused variables, etc.
> - Create accompanying PR in Greybus Spec repository
> - Switch to 80 columns formatting
> - Some other fixes pointed out in v3
> 
> Changes in v3:
> - Use phandle instead of busname for spi
> - Use spi board info for registering new device
> - Convert dt bindings to yaml
> - Add support for clickID
> - Code cleanup and style changes
> - Additions required to spi, serdev, w1 and regulator subsystems
> 
> Changes in v2:
> - support for adding mikroBUS ports from DT overlays,
> - remove debug sysFS interface for adding mikrobus ports,
> - consider extended pin usage/deviations from mikrobus standard
>   specifications
> - use greybus CPort protocol enum instead of new protocol enums
> - Fix cases of wrong indentation, ignoring return values, freeing allocated
>   resources in case of errors and other style suggestions in v1 review.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
> Ayush Singh (7):
>       dt-bindings: connector: Add mikrobus-connector
>       dt-bindings: mikrobus: Add mikrobus board base
>       dt-bindings: mikrobus: Add mikrobus-spi binding
>       spi: Make of_find_spi_controller_by_node() available
>       spi: Make of_register_spi_device() available
>       mikrobus: Add mikroBUS driver
>       dts: ti: k3-am625-beagleplay: Add mikroBUS
> 
>  .../bindings/connector/mikrobus-connector.yaml     | 107 ++++++
>  .../bindings/mikrobus/mikrobus-board.yaml          |  20 ++
>  .../devicetree/bindings/mikrobus/mikrobus-spi.yaml |  37 +++
>  MAINTAINERS                                        |   9 +
>  arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts     |  94 +++++-
>  drivers/misc/Kconfig                               |  16 +
>  drivers/misc/Makefile                              |   1 +
>  drivers/misc/mikrobus.c                            | 361 +++++++++++++++++++++
>  drivers/spi/spi.c                                  | 209 ++++++------
>  include/linux/spi/spi.h                            |   7 +
>  10 files changed, 750 insertions(+), 111 deletions(-)
> ---
> base-commit: f76698bd9a8ca01d3581236082d786e9a6b72bb7
> change-id: 20240627-mikrobus-scratch-spi-ad8c98dcec98
> 
> Best regards,
> --
> Ayush Singh <ayush@beagleboard.org>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y ti/k3-am625-beagleplay.dtb' for 20240627-mikrobus-scratch-spi-v5-0-9e6c148bf5f0@beagleboard.org:

arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: mikrobus-connector: mikrobus-gpio-names:6: 'mosi' is not one of ['pwm', 'int', 'rx', 'tx', 'scl', 'sda', 'an', 'rst', 'cs', 'sck', 'cipo', 'copi']
	from schema $id: http://devicetree.org/schemas/connector/mikrobus-connector.yaml#
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: mikrobus-connector: mikrobus-gpio-names:7: 'miso' is not one of ['pwm', 'int', 'rx', 'tx', 'scl', 'sda', 'an', 'rst', 'cs', 'sck', 'cipo', 'copi']
	from schema $id: http://devicetree.org/schemas/connector/mikrobus-connector.yaml#
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: thermo-click: compatible: ['maxim,max31855k', 'mikrobus-spi'] is too long
	from schema $id: http://devicetree.org/schemas/iio/temperature/maxim,max31855k.yaml#
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: thermo-click: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/iio/temperature/maxim,max31855k.yaml#
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: thermo-click: Unevaluated properties are not allowed ('compatible', 'pinctrl-apply' were unexpected)
	from schema $id: http://devicetree.org/schemas/iio/temperature/maxim,max31855k.yaml#
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: lsm6dsl-click: compatible: 'oneOf' conditional failed, one must be fixed:
	['st,lsm6ds3', 'mikrobus-spi'] is too long
	'st,lsm6ds3' is not one of ['st,asm330lhhx', 'st,asm330lhhxg1']
	'st,lsm6dstx' was expected
	'st,lsm6dsv16x' was expected
	'st,ism330is' was expected
	'st,asm330lhb' was expected
	'st,lsm6dsr' was expected
	'st,lsm6dst' was expected
	'st,lsm6dsv' was expected
	'st,lsm6dso16is' was expected
	'st,asm330lhh' was expected
	from schema $id: http://devicetree.org/schemas/iio/imu/st,lsm6dsx.yaml#
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: lsm6dsl-click: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/iio/imu/st,lsm6dsx.yaml#
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dtb: lsm6dsl-click: Unevaluated properties are not allowed ('compatible', 'pinctrl-apply' were unexpected)
	from schema $id: http://devicetree.org/schemas/iio/imu/st,lsm6dsx.yaml#