[v2,02/10] mmc: Add init() API

Add an init() API for platform specific init() operations.

Signed-off-by: Faiz Abbas <faiz_abbas at ti.com>
Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
---
 drivers/mmc/mmc.c | 13 ++++++-------
 include/mmc.h     |  7 +++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas <faiz_abbas at ti.com> wrote:
>
> Add an init() API for platform specific init() operations.

Could you describe why this cannot be done in the probe callback? It's not
easily visible as the function you changed (mmc_get_op_cond) doesn't even have
a comment to describe what it does...

In general, I think commit messages could be more detailed than one line. If
only to make it easier in the future to recap why things have been done.

>
> Signed-off-by: Faiz Abbas <faiz_abbas at ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
> ---
>  drivers/mmc/mmc.c | 13 ++++++-------
>  include/mmc.h     |  7 +++++++
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index d43983d4a6..50df8c8626 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -2787,14 +2787,13 @@ int mmc_get_op_cond(struct mmc *mmc)
>         }
>         if (err)
>                 return err;
> -
>  #if CONFIG_IS_ENABLED(DM_MMC)
> -       /* The device has already been probed ready for use */
> -#else
> -       /* made sure it's not NULL earlier */
> -       err = mmc->cfg->ops->init(mmc);
> -       if (err)
> -               return err;

You're removing the init code for non-DM MMC here and did not describe it in
the commit message. Is this change intended at all?

Regards,
Simon

> +       struct dm_mmc_ops *ops = mmc_get_ops(mmc->dev);
> +       if (ops->init) {
> +               err = ops->init(mmc->dev);
> +               if (err)
> +                       return err;
> +       }
>  #endif
>         mmc->ddr_mode = 0;
>
> diff --git a/include/mmc.h b/include/mmc.h
> index 2f21dbf1b7..6aef125f25 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -406,6 +406,13 @@ struct mmc;
>
>  #if CONFIG_IS_ENABLED(DM_MMC)
>  struct dm_mmc_ops {
> +       /**
> +        * init() - platform specific initialization for the host controller
> +        *
> +        * @dev:        Device to init
> +        * @return 0 if Ok, -ve if error
> +        */
> +       int (*init)(struct udevice *dev);
>         /**
>          * send_cmd() - Send a command to the MMC device
>          *
> --
> 2.19.2
>

Forgot to ask: why isn't the subsystem maintainer on CC?
If you would use patman to send patches or at least the get_maintainer script,
he would have been...

Regards,
Simon

On Wed, Jan 29, 2020 at 9:03 AM Simon Goldschmidt
<simon.k.r.goldschmidt at gmail.com> wrote:
>
> On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas <faiz_abbas at ti.com> wrote:
> >
> > Add an init() API for platform specific init() operations.
>
> Could you describe why this cannot be done in the probe callback? It's not
> easily visible as the function you changed (mmc_get_op_cond) doesn't even have
> a comment to describe what it does...
>
> In general, I think commit messages could be more detailed than one line. If
> only to make it easier in the future to recap why things have been done.
>
> >
> > Signed-off-by: Faiz Abbas <faiz_abbas at ti.com>
> > Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
> > ---
> >  drivers/mmc/mmc.c | 13 ++++++-------
> >  include/mmc.h     |  7 +++++++
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> > index d43983d4a6..50df8c8626 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -2787,14 +2787,13 @@ int mmc_get_op_cond(struct mmc *mmc)
> >         }
> >         if (err)
> >                 return err;
> > -
> >  #if CONFIG_IS_ENABLED(DM_MMC)
> > -       /* The device has already been probed ready for use */
> > -#else
> > -       /* made sure it's not NULL earlier */
> > -       err = mmc->cfg->ops->init(mmc);
> > -       if (err)
> > -               return err;
>
> You're removing the init code for non-DM MMC here and did not describe it in
> the commit message. Is this change intended at all?
>
> Regards,
> Simon
>
> > +       struct dm_mmc_ops *ops = mmc_get_ops(mmc->dev);
> > +       if (ops->init) {
> > +               err = ops->init(mmc->dev);
> > +               if (err)
> > +                       return err;
> > +       }
> >  #endif
> >         mmc->ddr_mode = 0;
> >
> > diff --git a/include/mmc.h b/include/mmc.h
> > index 2f21dbf1b7..6aef125f25 100644
> > --- a/include/mmc.h
> > +++ b/include/mmc.h
> > @@ -406,6 +406,13 @@ struct mmc;
> >
> >  #if CONFIG_IS_ENABLED(DM_MMC)
> >  struct dm_mmc_ops {
> > +       /**
> > +        * init() - platform specific initialization for the host controller
> > +        *
> > +        * @dev:        Device to init
> > +        * @return 0 if Ok, -ve if error
> > +        */
> > +       int (*init)(struct udevice *dev);
> >         /**
> >          * send_cmd() - Send a command to the MMC device
> >          *
> > --
> > 2.19.2
> >

Hi Simon,

On 29/01/20 1:33 pm, Simon Goldschmidt wrote:
> On Fri, Jan 24, 2020 at 12:52 PM Faiz Abbas <faiz_abbas at ti.com> wrote:
>>
>> Add an init() API for platform specific init() operations.
> 
> Could you describe why this cannot be done in the probe callback? It's not
> easily visible as the function you changed (mmc_get_op_cond) doesn't even have
> a comment to describe what it does...

The reason is detailed in 06/10 patch description. probe() is always
called for all MMC instances. I only want to switch on power (by calling
sdhci_init()) and suffer the 1 second wait time when there is actually a
card in the slot and user wants to access it.
> 
> In general, I think commit messages could be more detailed than one line. If
> only to make it easier in the future to recap why things have been done.
> 

You're right. I will add a more detailed patch description in v2.

Thanks,
Faiz

Hi,

+Lokesh, Tom

On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
> Forgot to ask: why isn't the subsystem maintainer on CC?
> If you would use patman to send patches or at least the get_maintainer script,
> he would have been...
> 

I did use get_maintainer for my send-email CC list but everyone other
than Michal seems to have been dropped. Here is an excerpt from the
email header I received:

From: Faiz Abbas <faiz_abbas at ti.com>
To: <u-boot at lists.denx.de>
CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <lokeshvutla at ti.com>,
	<peng.fan at nxp.com>, <faiz_abbas at ti.com>
Subject: [PATCH v2 02/10] mmc: Add init() API
Date: Fri, 24 Jan 2020 17:22:44 +0530


But in the patchworks and in your reply, only Michal is remaining:
https://patchwork.ozlabs.org/patch/1228781/

Michal,

What do you see in your message header? Does it have other people copied?

Thanks,
Faiz

On 30. 01. 20 16:03, Faiz Abbas wrote:
> Hi,
> 
> +Lokesh, Tom
> 
> On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
>> Forgot to ask: why isn't the subsystem maintainer on CC?
>> If you would use patman to send patches or at least the get_maintainer script,
>> he would have been...
>>
> 
> I did use get_maintainer for my send-email CC list but everyone other
> than Michal seems to have been dropped. Here is an excerpt from the
> email header I received:
> 
> From: Faiz Abbas <faiz_abbas at ti.com>
> To: <u-boot at lists.denx.de>
> CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <lokeshvutla at ti.com>,
> 	<peng.fan at nxp.com>, <faiz_abbas at ti.com>
> Subject: [PATCH v2 02/10] mmc: Add init() API
> Date: Fri, 24 Jan 2020 17:22:44 +0530
> 
> 
> But in the patchworks and in your reply, only Michal is remaining:
> https://patchwork.ozlabs.org/patch/1228781/
> 
> Michal,
> 
> What do you see in your message header? Does it have other people copied?

[u-boot]$ ./scripts/get_maintainer.pl -f drivers/mmc/mmc.c
Peng Fan <peng.fan at nxp.com> (maintainer:MMC)
u-boot at lists.denx.de (open list)

I see Peng there.

Thanks,
Michal

Faiz Abbas <faiz_abbas at ti.com> schrieb am Do., 30. Jan. 2020, 16:01:

> Hi,
>
> +Lokesh, Tom
>
> On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
> > Forgot to ask: why isn't the subsystem maintainer on CC?
> > If you would use patman to send patches or at least the get_maintainer
> script,
> > he would have been...
> >
>
> I did use get_maintainer for my send-email CC list but everyone other
> than Michal seems to have been dropped. Here is an excerpt from the
> email header I received:
>
> From: Faiz Abbas <faiz_abbas at ti.com>
> To: <u-boot at lists.denx.de>
> CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <lokeshvutla at ti.com>,
>         <peng.fan at nxp.com>, <faiz_abbas at ti.com>
> Subject: [PATCH v2 02/10] mmc: Add init() API
> Date: Fri, 24 Jan 2020 17:22:44 +0530
>
>
> But in the patchworks and in your reply, only Michal is remaining:
> https://patchwork.ozlabs.org/patch/1228781/


I hit reply all in the gmail web interface. My header only shows Michal.

Regards,
Simon


>
> Michal,
>
> What do you see in your message header? Does it have other people copied?


> Thanks,
> Faiz
>

Hi Michal,

On 30/01/20 8:37 pm, Michal Simek wrote:
> On 30. 01. 20 16:03, Faiz Abbas wrote:
>> Hi,
>>
>> +Lokesh, Tom
>>
>> On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
>>> Forgot to ask: why isn't the subsystem maintainer on CC?
>>> If you would use patman to send patches or at least the get_maintainer script,
>>> he would have been...
>>>
>>
>> I did use get_maintainer for my send-email CC list but everyone other
>> than Michal seems to have been dropped. Here is an excerpt from the
>> email header I received:
>>
>> From: Faiz Abbas <faiz_abbas at ti.com>
>> To: <u-boot at lists.denx.de>
>> CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <lokeshvutla at ti.com>,
>> 	<peng.fan at nxp.com>, <faiz_abbas at ti.com>
>> Subject: [PATCH v2 02/10] mmc: Add init() API
>> Date: Fri, 24 Jan 2020 17:22:44 +0530
>>
>>
>> But in the patchworks and in your reply, only Michal is remaining:
>> https://patchwork.ozlabs.org/patch/1228781/
>>
>> Michal,
>>
>> What do you see in your message header? Does it have other people copied?
> 
> [u-boot]$ ./scripts/get_maintainer.pl -f drivers/mmc/mmc.c
> Peng Fan <peng.fan at nxp.com> (maintainer:MMC)
> u-boot at lists.denx.de (open list)
> 
> I see Peng there.
> 

You misunderstood. I am not asking if you see Peng in the get_maintainer
output. Do you see him CC'd in the original patch email?

Thanks,
Faiz

On 30. 01. 20 16:14, Faiz Abbas wrote:
> Hi Michal,
> 
> On 30/01/20 8:37 pm, Michal Simek wrote:
>> On 30. 01. 20 16:03, Faiz Abbas wrote:
>>> Hi,
>>>
>>> +Lokesh, Tom
>>>
>>> On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
>>>> Forgot to ask: why isn't the subsystem maintainer on CC?
>>>> If you would use patman to send patches or at least the get_maintainer script,
>>>> he would have been...
>>>>
>>>
>>> I did use get_maintainer for my send-email CC list but everyone other
>>> than Michal seems to have been dropped. Here is an excerpt from the
>>> email header I received:
>>>
>>> From: Faiz Abbas <faiz_abbas at ti.com>
>>> To: <u-boot at lists.denx.de>
>>> CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <lokeshvutla at ti.com>,
>>> 	<peng.fan at nxp.com>, <faiz_abbas at ti.com>
>>> Subject: [PATCH v2 02/10] mmc: Add init() API
>>> Date: Fri, 24 Jan 2020 17:22:44 +0530
>>>
>>>
>>> But in the patchworks and in your reply, only Michal is remaining:
>>> https://patchwork.ozlabs.org/patch/1228781/
>>>
>>> Michal,
>>>
>>> What do you see in your message header? Does it have other people copied?
>>
>> [u-boot]$ ./scripts/get_maintainer.pl -f drivers/mmc/mmc.c
>> Peng Fan <peng.fan at nxp.com> (maintainer:MMC)
>> u-boot at lists.denx.de (open list)
>>
>> I see Peng there.
>>
> 
> You misunderstood. I am not asking if you see Peng in the get_maintainer
> output. Do you see him CC'd in the original patch email?

Nope. I can't see him there.

M

On Thu, Jan 30, 2020 at 04:30:54PM +0100, Michal Simek wrote:
> On 30. 01. 20 16:14, Faiz Abbas wrote:
> > Hi Michal,
> > 
> > On 30/01/20 8:37 pm, Michal Simek wrote:
> >> On 30. 01. 20 16:03, Faiz Abbas wrote:
> >>> Hi,
> >>>
> >>> +Lokesh, Tom
> >>>
> >>> On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
> >>>> Forgot to ask: why isn't the subsystem maintainer on CC?
> >>>> If you would use patman to send patches or at least the get_maintainer script,
> >>>> he would have been...
> >>>>
> >>>
> >>> I did use get_maintainer for my send-email CC list but everyone other
> >>> than Michal seems to have been dropped. Here is an excerpt from the
> >>> email header I received:
> >>>
> >>> From: Faiz Abbas <faiz_abbas at ti.com>
> >>> To: <u-boot at lists.denx.de>
> >>> CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <lokeshvutla at ti.com>,
> >>> 	<peng.fan at nxp.com>, <faiz_abbas at ti.com>
> >>> Subject: [PATCH v2 02/10] mmc: Add init() API
> >>> Date: Fri, 24 Jan 2020 17:22:44 +0530
> >>>
> >>>
> >>> But in the patchworks and in your reply, only Michal is remaining:
> >>> https://patchwork.ozlabs.org/patch/1228781/
> >>>
> >>> Michal,
> >>>
> >>> What do you see in your message header? Does it have other people copied?
> >>
> >> [u-boot]$ ./scripts/get_maintainer.pl -f drivers/mmc/mmc.c
> >> Peng Fan <peng.fan at nxp.com> (maintainer:MMC)
> >> u-boot at lists.denx.de (open list)
> >>
> >> I see Peng there.
> >>
> > 
> > You misunderstood. I am not asking if you see Peng in the get_maintainer
> > output. Do you see him CC'd in the original patch email?
> 
> Nope. I can't see him there.

Wolfgang, is there some mailman setting that needs tweaking or looking
at here?  Thanks!

Tom Rini <trini at konsulko.com> schrieb am Do., 30. Jan. 2020, 16:32:

> On Thu, Jan 30, 2020 at 04:30:54PM +0100, Michal Simek wrote:
> > On 30. 01. 20 16:14, Faiz Abbas wrote:
> > > Hi Michal,
> > >
> > > On 30/01/20 8:37 pm, Michal Simek wrote:
> > >> On 30. 01. 20 16:03, Faiz Abbas wrote:
> > >>> Hi,
> > >>>
> > >>> +Lokesh, Tom
> > >>>
> > >>> On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
> > >>>> Forgot to ask: why isn't the subsystem maintainer on CC?
> > >>>> If you would use patman to send patches or at least the
> get_maintainer script,
> > >>>> he would have been...
> > >>>>
> > >>>
> > >>> I did use get_maintainer for my send-email CC list but everyone other
> > >>> than Michal seems to have been dropped. Here is an excerpt from the
> > >>> email header I received:
> > >>>
> > >>> From: Faiz Abbas <faiz_abbas at ti.com>
> > >>> To: <u-boot at lists.denx.de>
> > >>> CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <
> lokeshvutla at ti.com>,
> > >>>   <peng.fan at nxp.com>, <faiz_abbas at ti.com>
> > >>> Subject: [PATCH v2 02/10] mmc: Add init() API
> > >>> Date: Fri, 24 Jan 2020 17:22:44 +0530
> > >>>
> > >>>
> > >>> But in the patchworks and in your reply, only Michal is remaining:
> > >>> https://patchwork.ozlabs.org/patch/1228781/
> > >>>
> > >>> Michal,
> > >>>
> > >>> What do you see in your message header? Does it have other people
> copied?
> > >>
> > >> [u-boot]$ ./scripts/get_maintainer.pl -f drivers/mmc/mmc.c
> > >> Peng Fan <peng.fan at nxp.com> (maintainer:MMC)
> > >> u-boot at lists.denx.de (open list)
> > >>
> > >> I see Peng there.
> > >>
> > >
> > > You misunderstood. I am not asking if you see Peng in the
> get_maintainer
> > > output. Do you see him CC'd in the original patch email?
> >
> > Nope. I can't see him there.
>
> Wolfgang, is there some mailman setting that needs tweaking or looking
> at here?  Thanks!
>

Can this be a mailman issue? If Michal was CCed, is mailman involved in the
way to him? I would have thought that mail got delivered directly.

Regards,
Simon

On Thu, Jan 30, 2020 at 04:35:40PM +0100, Simon Goldschmidt wrote:
> Tom Rini <trini at konsulko.com> schrieb am Do., 30. Jan. 2020, 16:32:
> 
> > On Thu, Jan 30, 2020 at 04:30:54PM +0100, Michal Simek wrote:
> > > On 30. 01. 20 16:14, Faiz Abbas wrote:
> > > > Hi Michal,
> > > >
> > > > On 30/01/20 8:37 pm, Michal Simek wrote:
> > > >> On 30. 01. 20 16:03, Faiz Abbas wrote:
> > > >>> Hi,
> > > >>>
> > > >>> +Lokesh, Tom
> > > >>>
> > > >>> On 29/01/20 1:37 pm, Simon Goldschmidt wrote:
> > > >>>> Forgot to ask: why isn't the subsystem maintainer on CC?
> > > >>>> If you would use patman to send patches or at least the
> > get_maintainer script,
> > > >>>> he would have been...
> > > >>>>
> > > >>>
> > > >>> I did use get_maintainer for my send-email CC list but everyone other
> > > >>> than Michal seems to have been dropped. Here is an excerpt from the
> > > >>> email header I received:
> > > >>>
> > > >>> From: Faiz Abbas <faiz_abbas at ti.com>
> > > >>> To: <u-boot at lists.denx.de>
> > > >>> CC: <dannenberg at ti.com>, <michal.simek at xilinx.com>, <
> > lokeshvutla at ti.com>,
> > > >>>   <peng.fan at nxp.com>, <faiz_abbas at ti.com>
> > > >>> Subject: [PATCH v2 02/10] mmc: Add init() API
> > > >>> Date: Fri, 24 Jan 2020 17:22:44 +0530
> > > >>>
> > > >>>
> > > >>> But in the patchworks and in your reply, only Michal is remaining:
> > > >>> https://patchwork.ozlabs.org/patch/1228781/
> > > >>>
> > > >>> Michal,
> > > >>>
> > > >>> What do you see in your message header? Does it have other people
> > copied?
> > > >>
> > > >> [u-boot]$ ./scripts/get_maintainer.pl -f drivers/mmc/mmc.c
> > > >> Peng Fan <peng.fan at nxp.com> (maintainer:MMC)
> > > >> u-boot at lists.denx.de (open list)
> > > >>
> > > >> I see Peng there.
> > > >>
> > > >
> > > > You misunderstood. I am not asking if you see Peng in the
> > get_maintainer
> > > > output. Do you see him CC'd in the original patch email?
> > >
> > > Nope. I can't see him there.
> >
> > Wolfgang, is there some mailman setting that needs tweaking or looking
> > at here?  Thanks!
> >
> 
> Can this be a mailman issue? If Michal was CCed, is mailman involved in the
> way to him? I would have thought that mail got delivered directly.

I was thinking about the setting on if you get your own messages / ones
you're on CC to or not, and if that was the copy say in Michal's inbox
or U-Boot folder.

Dear Tom,

In message <20200130153216.GX13379 at bill-the-cat> you wrote:
> 
> > > You misunderstood. I am not asking if you see Peng in the get_maintainer
> > > output. Do you see him CC'd in the original patch email?
> >
> > Nope. I can't see him there.
>
> Wolfgang, is there some mailman setting that needs tweaking or looking
> at here?  Thanks!

I'm not sure.  I cannot see any trae of the Cc: s at list.denx.de,
as this is handled by the poster's MTA.  I can only see the posting
as it arrived at the list server, and this has just

	From: Faiz Abbas <faiz_abbas at ti.com>
	To: <u-boot at lists.denx.de>
	Cc: michal.simek at xilinx.com

It would be interesting to check the MTA los on the sender side if
the Cc:s were actually sent...

Best regards,

Wolfgang Denk

Dear Simon,

In message <CAAh8qswkmJhpgSZx4KcGiXrEugS47M+_VB7Nkcs27kpDLpVcAQ at mail.gmail.com> you wrote:
>
> Can this be a mailman issue? If Michal was CCed, is mailman involved in the
> way to him? I would have thought that mail got delivered directly.

Correct, it's the sender's MTA who handles the action transmission.
The question here is if there was actually more than the single
address in the Cc: header when the message to the list got sent.
For example, it would be interesting to know if any of the people
who were supposed to be on the Cc: list (but now don't show up)
got a mesage directly, or only through the mailing list.



Best regards,

Wolfgang Denk

Dear Tom,

In message <20200130153827.GY13379 at bill-the-cat> you wrote:
> 
> > way to him? I would have thought that mail got delivered directly.
>
> I was thinking about the setting on if you get your own messages / ones
> you're on CC to or not, and if that was the copy say in Michal's inbox
> or U-Boot folder.

Yes, but thisis a secondaryquestion here.  The more interesting one
(especially for patchwork etc.) is where the Cc: header lost the
additional addresses - was it at the sender's MUA, the sender's MTA
or on lists.denx.de ; I don't really trust mailman if it's getting
dark outside, but I have no good way to check this.

Best regards,

Wolfgang Denk