[U-Boot] [PATCH v4 02/10] spi: Add Atmel QuadSPI driver

[Tudor.Ambarus at microchip.com]

Hi, Jagan,

On 06/12/2019 11:02 AM, Jagan Teki wrote:
> External E-Mail
> 
> 
> On Wed, May 15, 2019 at 12:33 PM <Tudor.Ambarus at microchip.com> wrote:
>>
>> From: Tudor Ambarus <tudor.ambarus at microchip.com>
>>
>> Backport the driver from linux v5.1-rc5 and adapt it for u-boot.
>> Tested on sama5d2_xplained Rev B with mx25l25635e spi-nor flash.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>> ---
>> v4: update Kconfig description
>> v3: no change
>> v2: no change
>>
>>  drivers/spi/Kconfig         |   8 +
>>  drivers/spi/Makefile        |   1 +
>>  drivers/spi/atmel-quadspi.c | 535 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 544 insertions(+)
>>  create mode 100644 drivers/spi/atmel-quadspi.c
>>

cut

>> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
>> new file mode 100644
>> index 000000000000..c54fbe6bc6cf
>> --- /dev/null
>> +++ b/drivers/spi/atmel-quadspi.c
>> @@ -0,0 +1,535 @@

cut

>> +struct atmel_qspi {
>> +       void __iomem *regs;
>> +       void __iomem *mem;
>> +       struct platform_device *pdev;
> 
> Linux copy, please drop this.
> 

will do

>> +static int atmel_qspi_probe(struct udevice *dev)
>> +{
>> +       struct atmel_qspi *aq = dev_get_priv(dev);
>> +       struct resource res;
>> +       int ret;
>> +
>> +       aq->caps = (struct atmel_qspi_caps *)dev_get_driver_data(dev);
>> +       if (!aq->caps) {
>> +               dev_err(dev, "Could not retrieve QSPI caps\n");
>> +               return -EINVAL;
>> +       };
>> +
>> +       /* Map the registers */
>> +       ret = dev_read_resource_byname(dev, "qspi_base", &res);
>> +       if (ret) {
>> +               dev_err(dev, "missing registers\n");
>> +               return ret;
>> +       }
> 
> space

ok

> 
>> +       aq->regs = devm_ioremap(dev, res.start, resource_size(&res));
>> +       if (IS_ERR(aq->regs))
>> +               return PTR_ERR(aq->regs);
>> +
>> +       /* Map the AHB memory */
>> +       ret = dev_read_resource_byname(dev, "qspi_mmap", &res);
>> +       if (ret) {
>> +               dev_err(dev, "missing AHB memory\n");
>> +               return ret;
>> +       }
> 
> space

ok

> 
>> +       aq->mem = devm_ioremap(dev, res.start, resource_size(&res));
>> +       if (IS_ERR(aq->mem))
>> +               return PTR_ERR(aq->mem);
>> +
>> +       ret = atmel_qspi_enable_clk(dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       atmel_qspi_init(aq);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct spi_controller_mem_ops atmel_qspi_mem_ops = {
>> +       .supports_op = atmel_qspi_supports_op,
>> +       .exec_op = atmel_qspi_exec_op,
>> +};
>> +
>> +static const struct dm_spi_ops atmel_qspi_ops = {
>> +       .set_speed = atmel_qspi_set_speed,
>> +       .set_mode = atmel_qspi_set_mode,
>> +       .mem_ops = &atmel_qspi_mem_ops,
>> +};
>> +
>> +static const struct atmel_qspi_caps atmel_sama5d2_qspi_caps = {};
>> +
>> +static const struct atmel_qspi_caps atmel_sam9x60_qspi_caps = {
>> +       .has_qspick = true,
>> +       .has_ricr = true,
>> +};
>> +
>> +static const struct udevice_id atmel_qspi_ids[] = {
>> +       {
>> +               .compatible = "atmel,sama5d2-qspi",
>> +               .data = (ulong)&atmel_sama5d2_qspi_caps,
> 
> Better assign NULL

I deliberately added a zeroed caps instance to the sama5d2 entry to not allow
aq->caps to be NULL. This way I avoid redundant checks like:

+	if (aq->caps && aq->caps->has_qspick) {
and
+	if (aq->caps && aq->caps->has_ricr) {


Since this patch is just a backport from the linux driver and does not modify
the code logic, can we keep it as it is, and modify it later on if you have a
strong opinion on this?

Cheers,
ta