Re: [PATCH V3] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
From: Rob Herring
Date: Wed Aug 29 2018 - 20:31:21 EST
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 more logical.