mbox series

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

Message ID 20240627-mikrobus-scratch-spi-v5-0-9e6c148bf5f0@beagleboard.org
Headers show
Series misc: Add mikroBUS driver | expand

Message

Ayush Singh June 27, 2024, 4:26 p.m. UTC
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,

Comments

Michael Walle June 27, 2024, 5:12 p.m. UTC | #1
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
Ayush Singh June 27, 2024, 5:29 p.m. UTC | #2
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
Ayush Singh June 27, 2024, 5:43 p.m. UTC | #3
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
Andrew Davis June 27, 2024, 5:50 p.m. UTC | #4
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
>
Andrew Lunn June 27, 2024, 6:44 p.m. UTC | #5
> 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
Rob Herring June 27, 2024, 7:21 p.m. UTC | #6
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.
Ayush Singh June 28, 2024, 6:31 a.m. UTC | #7
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
Conor Dooley June 28, 2024, 3:14 p.m. UTC | #8
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.
Rob Herring June 28, 2024, 3:41 p.m. UTC | #9
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#
Rob Herring June 28, 2024, 4:28 p.m. UTC | #10
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
>
Rob Herring June 28, 2024, 5 p.m. UTC | #11
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/
Ayush Singh June 28, 2024, 6:05 p.m. UTC | #12
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
Ayush Singh July 2, 2024, 3:14 p.m. UTC | #13
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
Rob Herring July 2, 2024, 3:17 p.m. UTC | #14
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
Geert Uytterhoeven July 5, 2024, 7:44 a.m. UTC | #15
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 July 5, 2024, 8:01 a.m. UTC | #16
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
Geert Uytterhoeven July 5, 2024, 8:19 a.m. UTC | #17
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
Andrew Davis July 5, 2024, 4:34 p.m. UTC | #18
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
Geert Uytterhoeven July 5, 2024, 5:36 p.m. UTC | #19
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
Ayush Singh Aug. 31, 2024, 6:11 p.m. UTC | #20
>> 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
Ayush Singh Sept. 5, 2024, 8:24 p.m. UTC | #21
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