Message-ID: <656ca446-9e56-4879-bb42-cd29063e0a82@gmail.com>
Date: Sat, 16 Mar 2024 03:11:29 +0530
From: Ayush Singh <ayushdevel1325@...il.com>
To: Vaishnav M A <vaishnav@...gleboard.org>
Cc: linux-kernel@...r.kernel.org, jkridner@...gleboard.org,
 robertcnelson@...gleboard.org, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Nishanth Menon <nm@...com>,
 Vignesh Raghavendra <vigneshr@...com>, Tero Kristo <kristo@...nel.org>,
 Derek Kiernan <derek.kiernan@....com>, Dragan Cvetic
 <dragan.cvetic@....com>, Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jiri Slaby <jirislaby@...nel.org>, Johan Hovold <johan@...nel.org>,
 Alex Elder <elder@...nel.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org,
 linux-serial@...r.kernel.org, greybus-dev@...ts.linaro.org
Subject: Re: [PATCH v3 0/8] misc: Add mikroBUS driver

On 3/16/24 02:50, Vaishnav M A wrote:

> Hi Ayush,
>
> On Sat, Mar 16, 2024 at 12:19 AM Ayush Singh <ayushdevel1325@...il.com> 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@beagleboard.org/ 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.

>> 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.

> 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.

>> 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@...com>
>
> 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.


Ayush Singh

Powered by blists - more mailing lists