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

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

[firas ashkar]

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

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

[Arnd Bergmann]

On Thu, Oct 20, 2022, at 16:42, firas ashkar wrote:
>> 
>> > +
>> > +#define DRIVER_NAME "ts7800v1_sdmmc"
>> 
>> Maybe drop this macro and just use the string directly.
> This define macro used in multiple locations

Yes, exactly that is the problem. Other drivers use the
same macro name, so it's very hard to grep for the actual
string.

    Arnd