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
