Re: [PATCH v7 3/3] mmc: Add driver for LiteX's LiteSDCard interface

[Andy Shevchenko]

On Fri, Jan 7, 2022 at 7:08 PM Gabriel L. Somlo <gsomlo@xxxxxxxxx> wrote:
> On Fri, Jan 07, 2022 at 12:06:16PM -0500, Gabriel Somlo wrote:

...

> > Cc: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> > Cc: Karol Gugala <kgugala@xxxxxxxxxxxx>
> > Cc: Joel Stanley <joel@xxxxxxxxx>
> > Cc: Stafford Horne <shorne@xxxxxxxxx>
> > Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Cc: David Abdurachmanov <david.abdurachmanov@xxxxxxxxxx>
> > Cc: Florent Kermarrec <florent@xxxxxxxxxxxxxxxx>

It would be nice if you can use `git send-email --cc ...` instead of
putting a long list into a commit message.

...

> It looked to me like you thought I should `#include <linux/bits.h>` here
> (even though I'm not getting any compiler warnings regarding it). If so,
> why? If not, apologies for the misunderstanding :)

The rule of thumb is to explicitly use the headers you are the direct
user of with the remark that some of them are guaranteed to be
included by others and some of them should be used (in most cases)
instead of their low-level parts (the example is types.h vs
compiler_attributes.h, so former is more standard than the letter and
it's almost 100% guarantee you want to have something from types.h
anyway in your code).

So, BIT() is defined in bits.h and in the below list none of the
header _guarantees_ its indirect inclusion.

> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/litex.h>
> > +#include <linux/module.h>
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/sd.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>

...

> Any more ordering or devm vs. non-devm mixing violations here? If so,
> can you please link me to an example or some docs where I ould figure
> out what it is I'm still doing wrong?

Device managed resources are attached to the instance of the device
object and removed in the order they have been attached to, but with
the caveat that they have no clue about non-managed calls in between.
Now you may figure out what happens. Ex.:

probe()
  A
  devm_B
  C
  devm_D

remove()
  un_C
  un_A

WRONG!

> > +static int litex_mmc_remove(struct platform_device *pdev)
> > +{
> > +     struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev);
> > +     struct mmc_host *mmc = host->mmc;
> > +
> > +     mmc_remove_host(mmc);
> > +     if (host->irq > 0)
> > +             free_irq(host->irq, mmc);
> > +     mmc_free_host(mmc);
> > +
> > +     return 0;
> > +}
>
> Ditto here...

Ditto.

...

> > +             .of_match_table = of_match_ptr(litex_match),
>
> You said "Wrong usage of of_match_ptr()" here, and all I have to go by
> is a bunch of other `drivers/mmc/host/*.c` files that use it in a
> similar way, so can you please clarify and/or provide an example of how
> to do it properly?

First of all, you have a dependency to OF, try to remove it and
compile with OF=n and you will immediately see the issue. You may also
go for  `git log --no-merges --grep of_match_ptr` and analyze the
result.

-- 
With Best Regards,
Andy Shevchenko