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

[Ayush Singh <ayush@beagleboard.org>]

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