Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Przemyslaw Gaj <pgaj@cadence.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: linux-i3c@lists.infradead.org, vitor.soares@synopsys.com,
	rafalc@cadence.com, agolec@cadence.com, bbrezillon@kernel.org
Subject: Re: [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.
Date: Mon, 29 Apr 2019 11:36:42 +0100
Message-ID: <20190429103639.GA19777@global.cadence.com> (raw)
In-Reply-To: <20190330164409.45e6edfb@collabora.com>

Hi Boris,

I'm sorry for my late response. I hope you remember this thread :-)
I'm implementing this and have some questions.

The 03/30/2019 16:44, Boris Brezillon wrote:
> >  
> > @@ -1274,9 +1353,32 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
> >  
> >  	cdns_i3c_master_enable(master);
> >  
> > +	if (m->secondary) {
> > +		i3c_bus_maintenance_lock(&master->base.bus);
> > +		cdns_i3c_master_update_devs(&master->base);
> > +		i3c_bus_maintenance_unlock(&master->base.bus);
> > +	}
> 
> Okay, I changed my mind on this solution (it's not the first time that
> happens, and unfortunately won't be the last time either :-)). I think
> I don't like the idea of exposing the i3c_bus_maintenance_unlock/lock()
> functions in the end.

Ok :-)

> 
> I'd like to reconsider what you initially proposed: having an
> ->update_devs() hook that is called by the core, except I'd call it
> ->populate_bus().

Ok, we can back to previous approach.

> 
> BTW, there's still something that's unclear to me. You seem to populate
> the bus here and also when acquiring bus ownership. Is this correct?

Yes, this is correct. I'm doing this here to register all the devices received
by DEFSLVS on master initialization time. I'm also populating new devices when
acquiring the bus because some device could join the bus dynamically and we
want to register this new devices on our side also.

> I'd expect it to be 'partially' populated at bus-init+defslvs time,
> which would trigger a mastership request if I3C devices are present (as
> we need to send a GETPID to know more about I3C devs).

So, you want to allocate and attach devices and then, when possible get devices
info and register them? I mean when mastership request is possible. If not,
just leave devices allocated and register them when ENEC(MR) received, correct?

Previously, I allocated and registered all the devices after successful
mastership request. Which way is better in your opinion?

>
> Also, what happens if i3c_master_add_i3c_dev_locked() fails? You
> don't seem to handle that case at all.

For now, I just skipped it silently.

> 
> > +
> > +static void cdns_i3c_master_mastership_takeover(struct cdns_i3c_master *master)
> > +{
> > +	if (master->base.init_done) {
> 
> Can this really happen that init_done is not set when you reach this
> point.

Yes, it was possible. Mastership was taken but master wasn't registered yet.
With new approach I think this won't happen.

> 
> > +		i3c_bus_maintenance_lock(&master->base.bus);
> > +		cdns_i3c_master_update_devs(&master->base);
> > +		i3c_bus_maintenance_unlock(&master->base.bus);
> > +
> > +		i3c_master_register_new_devs(&master->base);
> 
> The core will somehow be informed that this master now owns the bus, so
> it can call i3c_master_register_new_devs() for us, right?

I think it can. I'm sure it worked like that before. When HC driver changed
cur_master, new devices were populated.

> 
> But as said above, I'm not even sure this is correct to do it from
> here. I'd expect this to happen at DEFSLVS or BUS init time.
> 

Ok. New(Previous) approach allows that.

-- 
-- 
Przemyslaw Gaj

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-10 13:58 [PATCH v4 0/6] Add the I3C mastership request Przemyslaw Gaj
2019-03-10 13:58 ` [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure Przemyslaw Gaj
2019-03-30 14:36   ` Boris Brezillon
2019-04-01 18:17     ` vitor
2019-04-01 18:31       ` Boris Brezillon
2019-04-01 18:48         ` vitor
2019-04-01 19:10           ` Przemyslaw Gaj
2019-04-01 19:24           ` Boris Brezillon
2019-04-02 11:10             ` vitor
2019-04-02 11:22               ` Przemyslaw Gaj
2019-04-02 11:32                 ` vitor
2019-04-02 11:53                   ` Boris Brezillon
2019-04-02 11:48     ` Boris Brezillon
2019-03-10 13:58 ` [PATCH v4 2/6] i3c: export bus maintenance lock and unlock functions Przemyslaw Gaj
2019-03-10 13:58 ` [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
2019-03-30 15:06   ` Boris Brezillon
2019-04-01 18:41   ` vitor
2019-04-01 19:18     ` Boris Brezillon
2019-04-09 14:31     ` Vitor Soares
2019-04-09 15:20       ` Przemyslaw Gaj
2019-04-09 15:46         ` Vitor Soares
2019-04-10  6:53           ` Przemyslaw Gaj
2019-04-10 10:05             ` Vitor Soares
2019-04-10 10:36               ` Boris Brezillon
2019-04-12 14:28                 ` Vitor Soares
2019-04-12 14:57                   ` Boris Brezillon
2019-04-15 12:02                     ` Vitor Soares
2019-04-15 13:08                       ` Boris Brezillon
2019-03-10 13:58 ` [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
2019-03-30 15:44   ` Boris Brezillon
2019-04-29 10:36     ` Przemyslaw Gaj [this message]
2019-05-18  7:34       ` Boris Brezillon
2019-03-10 13:58 ` [PATCH v4 5/6] i3c: master: Add module author Przemyslaw Gaj
2019-03-10 13:58 ` [PATCH v4 6/6] MAINTAINERS: add myself as co-maintainer of i3c subsystem Przemyslaw Gaj

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190429103639.GA19777@global.cadence.com \
    --to=pgaj@cadence.com \
    --cc=agolec@cadence.com \
    --cc=bbrezillon@kernel.org \
    --cc=boris.brezillon@collabora.com \
    --cc=linux-i3c@lists.infradead.org \
    --cc=rafalc@cadence.com \
    --cc=vitor.soares@synopsys.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org linux-i3c@archiver.kernel.org
	public-inbox-index linux-i3c


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


AGPL code for this site: git clone https://public-inbox.org/ public-inbox