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
