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

[firas ashkar <firas.ashkar@savoirfairelinux.com>]

thank you for the comments, pls check below for replies and acks 
-- 
Firas Ashkar
Developpeur Système Embarqué

savoirfairelinux.com | Montréal, Québec

Tél.: +1 514 276 5468 ext. 118


On Tue, 2022-10-18 at 21:06 +0200, Arnd Bergmann wrote:
> 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.
This define macro used in multiple locations
> 
> > +
> > +       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.
ack
> 
> > + 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.
ack
> 
>      Arnd