Re: [PATCH V3] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Rob Herring]

On Wed, Aug 29, 2018 at 6:19 AM <dkota@xxxxxxxxxxxxxx> wrote:
>
> On 2018-08-29 05:55, Rob Herring wrote:
> > On Fri, Aug 24, 2018 at 04:12:15PM +0530, Dilip Kota wrote:
> >> From: Girish Mahadevan <girishm@xxxxxxxxxxxxxx>
> >>
> >> This driver supports GENI based SPI Controller in the Qualcomm SOCs.
> >> The
> >> Qualcomm Generic Interface (GENI) is a programmable module supporting
> >> a
> >> wide range of serial interfaces including SPI. This driver supports
> >> SPI
> >> operations using FIFO mode of transfer.
> >>
> >> Signed-off-by: Girish Mahadevan <girishm@xxxxxxxxxxxxxx>
> >> Signed-off-by: Dilip Kota <dkota@xxxxxxxxxxxxxx>
> >> ---
> >> Addressing all the reviewer commets given in Patchset1.
> >> Summerizing all the comments below:
> >>
> >>      MAKEFILE: Arrange SPI-GENI driver in alphabetical order
> >>      Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE
> >>      Enable SPI core auto runtime pm, and remove runtime pm calls.
> >>      Remove spi_geni_unprepare_message(),
> >> spi_geni_unprepare_transfer_hardware()
> >>      Remove likely/unlikely keywords.
> >>      Remove get_spi_master() and use dev_get_drvdata()
> >>      Move request_irq to probe()
> >>      Mark bus number assignment to -1 as SPI core framework will assign
> >> dynamically
> >>      Use devm_spi_register_master()
> >>      Include platform_device.h instead of of_platform.h
> >>      Removing macros which are used only once:
> >>              #define SPI_NUM_CHIPSELECT     4
> >>              #define SPI_XFER_TIMEOUT_MS    250
> >>      Place Register field definitions next to respective Register
> >> definitions.
> >>      Replace int and u32 declerations to unsigned int.
> >>      Remove Hex numbers in debug prints.
> >>      Declare mode as u16 in spi_setup_word_len()
> >>      Remove the labels: setup_fifo_params_exit:
> >> exit_prepare_transfer_hardware:
> >>      Declaring struct spi_master as spi everywhere in the file.
> >>      Calling spi_finalize_current_transfer() for end of transfer.
> >>      Hard code the SPI controller max frequency instead of reading from
> >> DTSI node.
> >>      Spinlock not required, removed it.
> >>      Removed unrequired error prints.
> >>      Fix KASAN error in geni_spi_isr().
> >>      Remove spi-geni-qcom.h
> >>      Remove inter words delay and CS to Clock toggle delay logic in the
> >> driver, as of now no clients are using it.
> >>      Will submit this logic in the next patchset.
> >>      Use major, minor and step macros to read from hardware version
> >> register.
> >>
> >>  .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -
> >
> > Please split to a separate patch and explain why you are removing
> > spi-max-frequency?
>
> Hi Rob Herring,
>
> In this patch, added changes for Driver not to read the SPI controller
> Maximum frequency from the device tree. Accordingly I removed it in the
> device tree documentation file. As both the files need to updated so did
> in the same patch.

Just because the Linux driver doesn't use it isn't a reason. What if
there is another OS driver for it? The binding is the h/w description,
not what the driver uses currently.

> Could you please let me know the reason for making a separate patch.

- Step 1 of Documentation/devicetree/bindings/submitting-patches.txt says so
- I only review the binding generally, so my ack or reviewed by only
applies to it.
- It makes the commit history of the DT only tree[1] more logical.

 Rob

[1] https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/