Re: [PATCH v3 0/8] misc: Add mikroBUS driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ayush,

On 16/03/24 03:11, Ayush Singh wrote:
On 3/16/24 02:50, Vaishnav M A wrote:

Hi Ayush,

On Sat, Mar 16, 2024 at 12:19 AM Ayush Singh <ayushdevel1325@xxxxxxxxx> 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 flash mikroBUS manifest and register the mikroBUS board.

As Russel had provided the feedback, this patchset does not add support
for mikrobus, but a subset of mikrobus add-on boards which have a
1-wire click ID EEPROM with an identifier blob that is similar to the greybus
manifest. This series lacks the necessary context and details to the
specifications that is implemented.

https://www.mikroe.com/clickid - you should atleast point to this specs.

The v1 and v2 of this patchset was submitted by Vaishnav M A back in
2020. This patchset also includes changes made over the years as part of
BeagleBoards kernel.

Link: https://www.mikroe.com/mikrobus
Link: https://docs.beagleboard.org/latest/boards/beagleplay/
Link: https://lore.kernel.org/lkml/20200818124815.11029-1-vaishnav@xxxxxxxxxxxxxxx/ Patch v2

Thank you for making the effort to upstream this, arriving at the
latest revision of the public available click ID hardware took almost 2-3 years and I could not personally keep up with upstreaming, my sincere apologies to the maintainers that provided feedback on the earlier revisions. I hope an updated version of this series lands upstream with your work as the efforts
made at BeagleBoard.org Foundation makes development simpler by adding
plug and play support to these add-on boards. Also this series mentions only the case where mikroBUS port is present physically on the board, but there
can be mikroBUS ports appearing over greybus and that is the reason why
greybus manifest was chose as descriptor format - the series needs to
describe these details so that a reviewer has the necessary information
to review your patches. A link to beagleconnect is also helpful to reviewers.

https://docs.beagleboard.org/latest/projects/beagleconnect/index.html


Yes, I left out the mikroBUS over greybus patches for now since this patch series is already too big.


Agreed, I would recommend splitting this series into logically separate changes, for example the W1 EEPROM driver could be separate, some extra features on the mikroBUS driver could be separate patches or be part of a separate series later.

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.

Ayush Singh (7):
It looks like the version you have sent is very similar to the
version I had previously updated for Beagleboard git with
only rebases and cleanup, but I don't see major functional
changes. I request you give credit to the original author
atleast as a Co-author with Co-developed by tag, As there
there was a significant amount of work done by myself to
come up with this specs and get everything working on close
to 150 mikrobus add-on boards on physical mikroBUS ports
and over greybus:
https://github.com/vaishnavachath/manifesto/tree/mikrobusv3/manifests

Yes, I will add Co-author and Co-developed tags. I think I should use your ti email? I would have preferred to keep you as the author in the git commit but I could not get the patches applied cleanly back when I tried it.


Thank you, please keep yourself as the primary author as you are putting in the effort to get the driver upstream and you will need to work on multiple revisions to address maintainer feedback and I feel you should get credit for that, please put my BeagleBoard.org e-mail with Co-developed-by tag as most of the work was done before I moved to the Linux team at TI.

The driver today is poorly written and was one of the first
Linux kernel development work I did while I was still in college
so I might have made a lot of blunders and just rebasing and
reposting will not get this to an acceptable state, here is what
I would recommend:

* Drop all the unnecessary changes in the mikroBUS driver to
support fixed-regulators, fixed-clocks, serdev device .etc and
implement minimal changes to support the mikroBUS clickid
devices.

* Provide necessary justification to why the particular descriptor
format (greybus manifest) is chosen, with pull request to manifesto
and greybus-specification.
https://github.com/projectara/greybus-spec
and similar to : https://github.com/projectara/manifesto/pull/2

* Move the mikrobus W1 driver to w1/ subsytem, it is a standard
W1 EEPROM driver (also a standard part with updated family code)
* Keep a RFC series of changes where mikroBUS ports can appear over
greybus to justify why the identifier format needs to be extended greybus
manifest.

   dt-bindings: misc: Add mikrobus-connector
   w1: Add w1_find_master_device
Dependent patches that goes to different subsytems should
be sent first separately to avoid confusion and then your series
should mention the necessary dependencies. (same for
spi).

   spi: Make of_find_spi_controller_by_node() available
   regulator: fixed-helper: export regulator_register_always_on
   greybus: Add mikroBUS manifest types
   mikrobus: Add mikrobus driver
   dts: ti: k3-am625-beagleplay: Add mikroBUS
Send this patch as DONOTMERGE till your bindings are
accepted.

Thanks, should I just add it in the message body? I cannot see anything in docs about that.


The reasoning behind this is that these patches go in to separate maintainer trees and without the bindings merged first the device tree changes cannot be validated, thus it is a usual practice to get the bindings and driver merged first and the device tree changes to go in the next cycle. Another alternative is you can point to your fork with all the changes together.

Vaishnav M A (1):
   serdev: add of_ helper to get serdev controller

Drop this from initial series,
I will provide further feedback from my TI e-mail,
Vaishnav Achath <vaishnav.a@xxxxxx>

Thank again for taking this up,

Thanks and Regards,
Vaishnav

  .../bindings/misc/mikrobus-connector.yaml     | 110 ++
  MAINTAINERS                                   |   7 +
  .../arm64/boot/dts/ti/k3-am625-beagleplay.dts |  76 +-
  drivers/misc/Kconfig                          |   1 +
  drivers/misc/Makefile                         |   1 +
  drivers/misc/mikrobus/Kconfig                 |  19 +
  drivers/misc/mikrobus/Makefile                |   6 +
  drivers/misc/mikrobus/mikrobus_core.c         | 942 ++++++++++++++++++
  drivers/misc/mikrobus/mikrobus_core.h         | 201 ++++
  drivers/misc/mikrobus/mikrobus_id.c           | 229 +++++
  drivers/misc/mikrobus/mikrobus_manifest.c     | 502 ++++++++++
  drivers/misc/mikrobus/mikrobus_manifest.h     |  20 +
  drivers/regulator/fixed-helper.c              |   1 +
  drivers/spi/spi.c                             | 206 ++--
  drivers/tty/serdev/core.c                     |  19 +
  drivers/w1/w1.c                               |   6 +-
  drivers/w1/w1_int.c                           |  27 +
  include/linux/greybus/greybus_manifest.h      |  49 +
  include/linux/serdev.h                        |   4 +
  include/linux/spi/spi.h                       |   4 +
  include/linux/w1.h                            |   1 +
  21 files changed, 2318 insertions(+), 113 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/misc/mikrobus-connector.yaml
  create mode 100644 drivers/misc/mikrobus/Kconfig
  create mode 100644 drivers/misc/mikrobus/Makefile
  create mode 100644 drivers/misc/mikrobus/mikrobus_core.c
  create mode 100644 drivers/misc/mikrobus/mikrobus_core.h
  create mode 100644 drivers/misc/mikrobus/mikrobus_id.c
  create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.c
  create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.h


base-commit: 61996c073c9b070922ad3a36c981ca6ddbea19a5
--
2.44.0


I guess I will start with only i2c and spi support and go from there.


Agreed, even with those you can get close to 100 add-on boards working.
But do keep the extension to the greybus manifest .etc for all buses/devices so that approvals for extending the greybus manifest is common.

Thanks and Regards,
Vaishnav

Thanks and Regards,
Vaishnav

Ayush Singh






[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux