Re: [PATCH] mmc: add TS7800 FPGA based MMC controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 18, 2022, at 15:38, Firas Ashkar wrote:
> add standard mmc/host controller driver for TS-7800v1, instead of the
> original block based 'tssdcore' driver provided by EmbeddedTS linux-2.6.x
> code base.

I'm looking at this driver since Marc pointed me to your
work on the platform and I noticed this post as well.
Not doing a full review, but I'm still pointing out a few
issue that caught my eye.

> +
> +#define DRIVER_NAME "ts7800v1_sdmmc"

Maybe drop this macro and just use the string directly.

> +
> +	if (IS_ERR_OR_NULL(pslot->rw_dma_buf)) {
> +		dev_warn(mmc_dev(ts_sdmmc_host->mmc_host),
> +			 "%s|%d - Error, No allocated DMA read buffer %ld\n",
> +			 __func__, __LINE__, PTR_ERR(pslot->rw_dma_buf));
> +		*data_error = ret = -ENOMEM;
> +		goto done;
> +	}
> +
> +	dat0_sent_crc16_buf = kzalloc(sizeof(u16), GFP_KERNEL);
> +	if (IS_ERR_OR_NULL(dat0_sent_crc16_buf)) {

You should never need IS_ERR_OR_NULL, as all interfaces
in the kernel are supposed to either return an error code
or return NULL on error. Please fix the error handling
for this throughout the driver.

> + spin_lock_bh(&ts_sdmmc_host->bh_lock);

I'm a bit confused by your locking. Why do you use
spin_lock_bh() instead of a normal spin_lock() or
a spin_lock_irq()? I don't see any use of softirqs
(typically tasklets and timers) in this driver, so
disabling softirqs in the critical section should
not change anything.

     Arnd



[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux