| Message ID | 20240315184908.500352-1-ayushdevel1325@gmail.com |
|---|---|
| Headers | show |
| Series | misc: Add mikroBUS driver | expand |
On Fri, Mar 15, 2024 at 09:09:11PM +0100, Krzysztof Kozlowski wrote: > > +properties: > > + compatible: > > + const: mikrobus-connector > > Hm, why do you create binding for the connector, not for some sort of > controller? Please provide some rationale for this in commit msg. I think you have a distorted view. I refer you to the Mikroe mikroBUS specification - it's _just_ a connector which provides a fairly standardised purpose for each pin and the electrical specifications. For example of the pins: power, UART, SPIs, I2C, PWM, and analogue pins. > > + pinctrl-names: > > + items: > > + - const: default > > + - const: pwm_default > > + - const: pwm_gpio > > + - const: uart_default > > + - const: uart_gpio > > + - const: i2c_default > > + - const: i2c_gpio > > + - const: spi_default > > + - const: spi_gpio > > I fail to see why such choice is related to the connector itself. This isn't a choice at all. Here's the list of pins: Analog - AN Reset - RST SPI Chip Select - CS SPI Clock - SCK SPI Master Input Slave Output - MISO SPI Master Output Slave Input - MOSI VCC-3.3V power - +3.3V Reference Ground - GND 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 Any data pin can be a GPIO if e.g. a relay board is plugged in, even if some of the other pins are used for e.g. UART purposes. For example, a GPS board that provides the GPS data over the UART pins, and the PPS signal through a different pin.
On 15/03/2024 21:20, Russell King (Oracle) wrote: > On Fri, Mar 15, 2024 at 09:09:11PM +0100, Krzysztof Kozlowski wrote: >>> +properties: >>> + compatible: >>> + const: mikrobus-connector >> >> Hm, why do you create binding for the connector, not for some sort of >> controller? Please provide some rationale for this in commit msg. > > I think you have a distorted view. I refer you to the Mikroe mikroBUS > specification - it's _just_ a connector which provides a fairly > standardised purpose for each pin and the electrical specifications. > For example of the pins: power, UART, SPIs, I2C, PWM, and analogue > pins. I refer to the commit msg or description in the binding and there is nothing explained like this. Yeah, true, I could google every possible bus specification, but I also expect some sort of help here by the patch submitter. The binding looks like binding for a connector, not for some sort of controller, then are you saying the control part it is purely in software? That's how DTS looks like, but then my question is are there some sort of controller which would also perform this? > >>> + pinctrl-names: >>> + items: >>> + - const: default >>> + - const: pwm_default >>> + - const: pwm_gpio >>> + - const: uart_default >>> + - const: uart_gpio >>> + - const: i2c_default >>> + - const: i2c_gpio >>> + - const: spi_default >>> + - const: spi_gpio >> >> I fail to see why such choice is related to the connector itself. > > This isn't a choice at all. Here's the list of pins: > > Analog - AN > Reset - RST > SPI Chip Select - CS > SPI Clock - SCK > SPI Master Input Slave Output - MISO > SPI Master Output Slave Input - MOSI > VCC-3.3V power - +3.3V > Reference Ground - GND > 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 > > Any data pin can be a GPIO if e.g. a relay board is plugged in, even > if some of the other pins are used for e.g. UART purposes. For example, > a GPS board that provides the GPS data over the UART pins, and the > PPS signal through a different pin. And could you not have some certain features supported? Could have some pins just pull down / not connected? Best regards, Krzysztof
On Fri, Mar 15, 2024 at 09:40:13PM +0100, Krzysztof Kozlowski wrote: > On 15/03/2024 21:20, Russell King (Oracle) wrote: > > On Fri, Mar 15, 2024 at 09:09:11PM +0100, Krzysztof Kozlowski wrote: > >>> +properties: > >>> + compatible: > >>> + const: mikrobus-connector > >> > >> Hm, why do you create binding for the connector, not for some sort of > >> controller? Please provide some rationale for this in commit msg. > > > > I think you have a distorted view. I refer you to the Mikroe mikroBUS > > specification - it's _just_ a connector which provides a fairly > > standardised purpose for each pin and the electrical specifications. > > For example of the pins: power, UART, SPIs, I2C, PWM, and analogue > > pins. > > I refer to the commit msg or description in the binding and there is > nothing explained like this. Yeah, true, I could google every possible > bus specification, but I also expect some sort of help here by the patch > submitter. > > The binding looks like binding for a connector, not for some sort of > controller, then are you saying the control part it is purely in > software? That's how DTS looks like, but then my question is are there > some sort of controller which would also perform this? There is, as far as I'm aware, no "controller" for a mikroBUS. As I tried to explain, it's a bunch of pins with defined standard functions. The idea seems to be they're connected to a SoC with a pinmux that can reconfigure the function of the pin. At least that's how the hardware implementations I've seen do it. > > This isn't a choice at all. Here's the list of pins: > > > > Analog - AN > > Reset - RST > > SPI Chip Select - CS > > SPI Clock - SCK > > SPI Master Input Slave Output - MISO > > SPI Master Output Slave Input - MOSI > > VCC-3.3V power - +3.3V > > Reference Ground - GND > > 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 > > > > Any data pin can be a GPIO if e.g. a relay board is plugged in, even > > if some of the other pins are used for e.g. UART purposes. For example, > > a GPS board that provides the GPS data over the UART pins, and the > > PPS signal through a different pin. > > And could you not have some certain features supported? Could have some > pins just pull down / not connected? A board plugged in doesn't have to use all the functions. I gave two examples. Apart from the power pins, The GPS board as I stated uses the two UART pins, and some GPS boards route the PPS signal to another pin on the connector, but that's entirely optional. Another pin might be used as a GPIO as an enable. In the case of a relay board I've had, the SPI CS pin is used for one relay, and the PWM pin for the other relay. I also have a BME280 humidity/pressure sensor board, which just uses the two I2C pins. What is supported by a board is down to the board. Which pins it connects to is down to the board. Which board is plugged in is up to the user. That is essentially the long and short of mikroBUS - I hope I've given a good overview of it. I am slightly confused by this series, because it seems the Linux "mikroBUS" layer requires a one-wire EEPROM on the boards, but the official mikroBUS specification doesn't state this. The author really needs to clarify what they are implementing here. Are they truly implementing mikroBUS as defined by Mikroe, or are they implementing someone's custom extension to it that adds an identification EEPROM - in which case I would argue that this code as it stands is not suitable for mainline as long as it purports to be implementing support for Mikroe's mikroBUS. Hence, I think we should back off on reviewing this until we have that clarification.
Hi Ayush, On Sat, Mar 16, 2024 at 12:19 AM Ayush Singh <ayushdevel1325@gmail.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 > 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 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. > > 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@ti.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 >
On 3/16/24 02:50, Vaishnav M A wrote: > Hi Ayush, > > On Sat, Mar 16, 2024 at 12:19 AM Ayush Singh <ayushdevel1325@gmail.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@ti.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
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@gmail.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. > 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@ti.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. > 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 > >
> 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. Probably not required now, but git commit --amend --author A U Thor <author@example.com> allows you to change the author of a patch in git. Andrew