subject:"Re\: \[U\-Boot\] \[PATCH v3 3\/3\] spi\: mxs

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Akash Gajjar

Hello Marek,

On Fri, May 11, 2018 at 4:09 PM, Marek Vasut  wrote:

> On 05/11/2018 12:08 PM, Gajjar Akash wrote:
> > Hi Marek,
> >
> > Thanks for the review comments.
> >
> > > -struct mxs_spi_slave {
> > > - struct spi_slaveslave;
> > > - uint32_tmax_khz;
> > > - uint32_tmode;
> > > - struct mxs_ssp_regs *regs;
> > > +struct mxs_spi_priv {
> > > + struct mxs_ssp_regs *regs;
> > > + u32 max_khz;
> > > + u32 mode;
> > > + u32 bus;
> > > + u32 cs;
> >
> > Type cleanup should be a separate patch
> >
> >
> > Okay, I will prepare seperate patch for type cleanup.
> >
> >
> > >  };
> > >   if (mxs_wait_mask_set(_regs->hw_ssp_ctrl0_reg,
> > >   SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> > > - printf("MXS SPI: Timeout waiting for
> start\n");
> > > + debug("MXS SPI: Timeout waiting for
> start\n");
> >
> > printf , we don't want to hide errors
> >
> >
> > okay, will revert it back to printf.
> >
> >
> > >   return -ETIMEDOUT;
> > >   }
> >
> > > +
> > > +#ifndef __SPI_MXS_H
> > > +#define __SPI_MXS_H
> > > +
> > > +struct mxs_spi_platdata {
> > > + struct mxs_ssp_regs *regs;
> > > + u32 bus;
> > > + u32 max_hz;
> > > + u32 cs;
> >
> > Why is this header here at all ?
> >
> >
> > I didnt get this comment. do I need to place it somewhere else?
>
> See the beginning of this email, it seems the same structure exists twice.
>

My intention was to have two individual structure for private and platform
data.
But now I could use one structre and access its members using two structure
variables(one for private and one for platadata).

Is That looks okay?


>
> --
> Best regards,
> Marek Vasut
>


Thanks,

*Akash Gajjar*
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Marek Vasut

On 05/11/2018 01:09 PM, Akash Gajjar wrote:
> Hello Marek,
> 
> On Fri, May 11, 2018 at 4:09 PM, Marek Vasut  > wrote:
> 
> On 05/11/2018 12:08 PM, Gajjar Akash wrote:
> > Hi Marek,
> >
> > Thanks for the review comments.
> >
> >     > -struct mxs_spi_slave {
> >     > -     struct spi_slave        slave;
> >     > -     uint32_t                max_khz;
> >     > -     uint32_t                mode;
> >     > -     struct mxs_ssp_regs     *regs;
> >     > +struct mxs_spi_priv {
> >     > +     struct mxs_ssp_regs *regs;
> >     > +     u32     max_khz;
> >     > +     u32     mode;
> >     > +     u32     bus;
> >     > +     u32     cs;
> >
> >     Type cleanup should be a separate patch
> >
> >  
> > Okay, I will prepare seperate patch for type cleanup.
> >
> >
> >     >  };
> >     >               if (mxs_wait_mask_set(_regs->hw_ssp_ctrl0_reg,
> >     >                       SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> >     > -                     printf("MXS SPI: Timeout waiting for
> start\n");
> >     > +                     debug("MXS SPI: Timeout waiting for
> start\n");
> >
> >     printf , we don't want to hide errors
> >
> >  
> > okay, will revert it back to printf.
> >
> >
> >     >                       return -ETIMEDOUT;
> >     >               }
> >
> >     > +
> >     > +#ifndef __SPI_MXS_H
> >     > +#define __SPI_MXS_H
> >     > +
> >     > +struct mxs_spi_platdata {
> >     > +     struct mxs_ssp_regs *regs;
> >     > +     u32 bus;
> >     > +     u32 max_hz;
> >     > +     u32 cs;
> >
> >     Why is this header here at all ?
> >
> >  
> > I didnt get this comment. do I need to place it somewhere else?
> 
> See the beginning of this email, it seems the same structure exists
> twice.
> 
> 
> My intention was to have two individual structure for private and
> platform data.
> But now I could use one structre and access its members using two
> structure variables(one for private and one for platadata).
> 
> Is That looks okay?

I do not quite understand what you mean, but there is a duplication of
information here. That's a problem and should be fixed.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Marek Vasut

On 05/11/2018 12:08 PM, Gajjar Akash wrote:
> Hi Marek,
> 
> Thanks for the review comments.
> 
> > -struct mxs_spi_slave {
> > -     struct spi_slave        slave;
> > -     uint32_t                max_khz;
> > -     uint32_t                mode;
> > -     struct mxs_ssp_regs     *regs;
> > +struct mxs_spi_priv {
> > +     struct mxs_ssp_regs *regs;
> > +     u32     max_khz;
> > +     u32     mode;
> > +     u32     bus;
> > +     u32     cs;
> 
> Type cleanup should be a separate patch
> 
>  
> Okay, I will prepare seperate patch for type cleanup.
> 
> 
> >  };
> >               if (mxs_wait_mask_set(_regs->hw_ssp_ctrl0_reg,
> >                       SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> > -                     printf("MXS SPI: Timeout waiting for start\n");
> > +                     debug("MXS SPI: Timeout waiting for start\n");
> 
> printf , we don't want to hide errors
> 
>  
> okay, will revert it back to printf.
> 
> 
> >                       return -ETIMEDOUT;
> >               }
> 
> > +
> > +#ifndef __SPI_MXS_H
> > +#define __SPI_MXS_H
> > +
> > +struct mxs_spi_platdata {
> > +     struct mxs_ssp_regs *regs;
> > +     u32 bus;
> > +     u32 max_hz;
> > +     u32 cs;
> 
> Why is this header here at all ?
> 
>  
> I didnt get this comment. do I need to place it somewhere else?

See the beginning of this email, it seems the same structure exists twice.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Gajjar Akash

Hi Marek,

Thanks for the review comments.

> -struct mxs_spi_slave {
> > - struct spi_slaveslave;
> > - uint32_tmax_khz;
> > - uint32_tmode;
> > - struct mxs_ssp_regs *regs;
> > +struct mxs_spi_priv {
> > + struct mxs_ssp_regs *regs;
> > + u32 max_khz;
> > + u32 mode;
> > + u32 bus;
> > + u32 cs;
>
> Type cleanup should be a separate patch
>

Okay, I will prepare seperate patch for type cleanup.


> >  };
> >   if (mxs_wait_mask_set(_regs->hw_ssp_ctrl0_reg,
> >   SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> > - printf("MXS SPI: Timeout waiting for start\n");
> > + debug("MXS SPI: Timeout waiting for start\n");
>
> printf , we don't want to hide errors
>

okay, will revert it back to printf.


> >   return -ETIMEDOUT;
> >   }

> +
> > +#ifndef __SPI_MXS_H
> > +#define __SPI_MXS_H
> > +
> > +struct mxs_spi_platdata {
> > + struct mxs_ssp_regs *regs;
> > + u32 bus;
> > + u32 max_hz;
> > + u32 cs;
>
> Why is this header here at all ?
>

I didnt get this comment. do I need to place it somewhere else?

> +};
> > +
> > +#endif /* __SPI_MXS_H */
> >
>
>
> --
> Best regards,
> Marek Vasut
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

Thanks,
Akash
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-10 Thread Marek Vasut

On 05/10/2018 04:17 PM, Akash Gajjar wrote:
> This patch adds support for DM driver model to the mxs spi driver. Some TODOs
> are left over for later. These would be enhancements to the original
> functionality, and can come later. The legacy functionality is removed in
> this version.
> 
> Signed-off-by: Akash Gajjar 
> ---
> changes in v2:
>  Register cs_info method 
>  Remove unused function __mxs_spi_setup
>  Merged __spi_xfer function to spi_xfer
>  printf replaced by debug
> 
> changes in v3:
>  changes made on top of previous patch is merged in v3
> ---
>  drivers/spi/Kconfig|  12 +-
>  drivers/spi/mxs_spi.c  | 258 
> +++--
>  include/dm/platform_data/spi_mxs.h |  18 +++
>  3 files changed, 183 insertions(+), 105 deletions(-)
>  create mode 100644 include/dm/platform_data/spi_mxs.h
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ec92b84..5d3e152 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -106,6 +106,12 @@ config MVEBU_A3700_SPI
> used to access the SPI NOR flash on platforms embedding this
> Marvell IP core.
>  
> +config MXS_SPI
> + bool "MXS SPI Driver"
> + help
> +   Enable the MXS SPI controller driver. This driver can be used
> +   on the i.MX23 and i.MX28 SoCs.
> +
>  config PIC32_SPI
>   bool "Microchip PIC32 SPI driver"
>   depends on MACH_PIC32
> @@ -299,12 +305,6 @@ config MXC_SPI
> Enable the MXC SPI controller driver. This driver can be used
> on various i.MX SoCs such as i.MX31/35/51/6/7.
>  
> -config MXS_SPI
> - bool "MXS SPI Driver"
> - help
> -   Enable the MXS SPI controller driver. This driver can be used
> -   on the i.MX23 and i.MX28 SoCs.
> -
>  config OMAP3_SPI
>   bool "McSPI driver for OMAP"
>   help
> diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
> index 790db78..3054438 100644
> --- a/drivers/spi/mxs_spi.c
> +++ b/drivers/spi/mxs_spi.c
> @@ -1,6 +1,9 @@
>  /*
>   * Freescale i.MX28 SPI driver
>   *
> + * Support for device model:
> + * Copyright (C) 2018 Akash Gajjar 
> + *
>   * Copyright (C) 2011 Marek Vasut 
>   * on behalf of DENX Software Engineering GmbH
>   *
> @@ -20,6 +23,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #define  MXS_SPI_MAX_TIMEOUT 100
>  #define  MXS_SPI_PORT_OFFSET 0x2000
> @@ -28,93 +33,14 @@
>  
>  #define MXSSSP_SMALL_TRANSFER512
>  
> -struct mxs_spi_slave {
> - struct spi_slaveslave;
> - uint32_tmax_khz;
> - uint32_tmode;
> - struct mxs_ssp_regs *regs;
> +struct mxs_spi_priv {
> + struct mxs_ssp_regs *regs;
> + u32 max_khz;
> + u32 mode;
> + u32 bus;
> + u32 cs;

Type cleanup should be a separate patch

>  };
>  
> -static inline struct mxs_spi_slave *to_mxs_slave(struct spi_slave *slave)
> -{
> - return container_of(slave, struct mxs_spi_slave, slave);
> -}
> -
> -void spi_init(void)
> -{
> -}
> -
> -int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> -{
> - /* MXS SPI: 4 ports and 3 chip selects maximum */
> - if (!mxs_ssp_bus_id_valid(bus) || cs > 2)
> - return 0;
> - else
> - return 1;
> -}
> -
> -struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> -   unsigned int max_hz, unsigned int mode)
> -{
> - struct mxs_spi_slave *mxs_slave;
> -
> - if (!spi_cs_is_valid(bus, cs)) {
> - printf("mxs_spi: invalid bus %d / chip select %d\n", bus, cs);
> - return NULL;
> - }
> -
> - mxs_slave = spi_alloc_slave(struct mxs_spi_slave, bus, cs);
> - if (!mxs_slave)
> - return NULL;
> -
> - if (mxs_dma_init_channel(MXS_DMA_CHANNEL_AHB_APBH_SSP0 + bus))
> - goto err_init;
> -
> - mxs_slave->max_khz = max_hz / 1000;
> - mxs_slave->mode = mode;
> - mxs_slave->regs = mxs_ssp_regs_by_bus(bus);
> -
> - return _slave->slave;
> -
> -err_init:
> - free(mxs_slave);
> - return NULL;
> -}
> -
> -void spi_free_slave(struct spi_slave *slave)
> -{
> - struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
> - free(mxs_slave);
> -}
> -
> -int spi_claim_bus(struct spi_slave *slave)
> -{
> - struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
> - struct mxs_ssp_regs *ssp_regs = mxs_slave->regs;
> - uint32_t reg = 0;
> -
> - mxs_reset_block(_regs->hw_ssp_ctrl0_reg);
> -
> - writel((slave->cs << MXS_SSP_CHIPSELECT_SHIFT) |
> -SSP_CTRL0_BUS_WIDTH_ONE_BIT,
> -_regs->hw_ssp_ctrl0);
> -
> - reg = SSP_CTRL1_SSP_MODE_SPI | SSP_CTRL1_WORD_LENGTH_EIGHT_BITS;
> - reg |= (mxs_slave->mode & SPI_CPOL) ? SSP_CTRL1_POLARITY : 0;
> - reg |= (mxs_slave->mode & SPI_CPHA) ? SSP_CTRL1_PHASE : 0;
> - writel(reg, _regs->hw_ssp_ctrl1);

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

5 matches

Site Navigation

Mail list logo

Footer information