The I3C devices described in DT might not be attached to the master which
doesn't allow to assign a specific dynamic address.
This patch check if a device has i3c_dev_boardinfo and add it to
i3c_dev_desc structure. In this conditions, the framework will try to
assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
Signed-off-by: Vitor Soares <[email protected]>
---
drivers/i3c/master.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 4d29e1f..85fbda6 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
return NULL;
}
+static struct i3c_dev_boardinfo *
+i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
+{
+ struct i3c_master_controller *master = i3c_dev_get_master(dev);
+ struct i3c_dev_boardinfo *boardinfo;
+
+ if (dev->boardinfo)
+ return NULL;
+
+ list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
+ if (dev->info.pid == boardinfo->pid)
+ return boardinfo;
+ }
+
+ return NULL;
+}
+
/**
* i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
* @master: master used to send frames on the bus
@@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
{
struct i3c_device_info info = { .dyn_addr = addr };
struct i3c_dev_desc *newdev, *olddev;
+ struct i3c_dev_boardinfo *boardinfo;
u8 old_dyn_addr = addr, expected_dyn_addr;
struct i3c_ibi_setup ibireq = { };
bool enable_ibi = false;
@@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
if (ret)
goto err_detach_dev;
+ boardinfo = i3c_master_search_i3c_boardinfo(newdev);
+ if (boardinfo)
+ newdev->boardinfo = boardinfo;
+
/*
* Depending on our previous state, the expected dynamic address might
* differ:
--
2.7.4
On Thu, 29 Aug 2019 12:19:33 +0200
Vitor Soares <[email protected]> wrote:
> The I3C devices described in DT might not be attached to the master which
> doesn't allow to assign a specific dynamic address.
I remember testing this when developing the framework, so, unless
another patch regressed it, it should already work. I suspect patch 1
is actually the regressing this use case.
>
> This patch check if a device has i3c_dev_boardinfo and add it to
> i3c_dev_desc structure. In this conditions, the framework will try to
> assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
>
> Signed-off-by: Vitor Soares <[email protected]>
> ---
> drivers/i3c/master.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4d29e1f..85fbda6 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> return NULL;
> }
>
> +static struct i3c_dev_boardinfo *
> +i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
> +{
> + struct i3c_master_controller *master = i3c_dev_get_master(dev);
> + struct i3c_dev_boardinfo *boardinfo;
> +
> + if (dev->boardinfo)
> + return NULL;
> +
> + list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> + if (dev->info.pid == boardinfo->pid)
> + return boardinfo;
> + }
> +
> + return NULL;
> +}
> +
> /**
> * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> * @master: master used to send frames on the bus
> @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> {
> struct i3c_device_info info = { .dyn_addr = addr };
> struct i3c_dev_desc *newdev, *olddev;
> + struct i3c_dev_boardinfo *boardinfo;
> u8 old_dyn_addr = addr, expected_dyn_addr;
> struct i3c_ibi_setup ibireq = { };
> bool enable_ibi = false;
> @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> if (ret)
> goto err_detach_dev;
>
> + boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> + if (boardinfo)
> + newdev->boardinfo = boardinfo;
> +
> /*
> * Depending on our previous state, the expected dynamic address might
> * differ:
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57
> On Thu, 29 Aug 2019 12:19:33 +0200
> Vitor Soares <[email protected]> wrote:
>
> > The I3C devices described in DT might not be attached to the master which
> > doesn't allow to assign a specific dynamic address.
>
> I remember testing this when developing the framework, so, unless
> another patch regressed it, it should already work. I suspect patch 1
> is actually the regressing this use case.
For today it doesn't address the case where the device is described with
static address = 0, which isn't attached to the controller.
With this change both cases are covered.
>
> >
> > This patch check if a device has i3c_dev_boardinfo and add it to
> > i3c_dev_desc structure. In this conditions, the framework will try to
> > assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
> >
> > Signed-off-by: Vitor Soares <[email protected]>
> > ---
> > drivers/i3c/master.c | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 4d29e1f..85fbda6 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> > return NULL;
> > }
> >
> > +static struct i3c_dev_boardinfo *
> > +i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
> > +{
> > + struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > + struct i3c_dev_boardinfo *boardinfo;
> > +
> > + if (dev->boardinfo)
> > + return NULL;
> > +
> > + list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> > + if (dev->info.pid == boardinfo->pid)
> > + return boardinfo;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > /**
> > * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> > * @master: master used to send frames on the bus
> > @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> > {
> > struct i3c_device_info info = { .dyn_addr = addr };
> > struct i3c_dev_desc *newdev, *olddev;
> > + struct i3c_dev_boardinfo *boardinfo;
> > u8 old_dyn_addr = addr, expected_dyn_addr;
> > struct i3c_ibi_setup ibireq = { };
> > bool enable_ibi = false;
> > @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> > if (ret)
> > goto err_detach_dev;
> >
> > + boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> > + if (boardinfo)
> > + newdev->boardinfo = boardinfo;
> > +
> > /*
> > * Depending on our previous state, the expected dynamic address might
> > * differ:
Best regards,
Vitor Soares
On Thu, 29 Aug 2019 14:00:44 +0000
Vitor Soares <[email protected]> wrote:
> Hi Boris,
>
> From: Boris Brezillon <[email protected]>
> Date: Thu, Aug 29, 2019 at 11:44:57
>
> > On Thu, 29 Aug 2019 12:19:33 +0200
> > Vitor Soares <[email protected]> wrote:
> >
> > > The I3C devices described in DT might not be attached to the master which
> > > doesn't allow to assign a specific dynamic address.
> >
> > I remember testing this when developing the framework, so, unless
> > another patch regressed it, it should already work. I suspect patch 1
> > is actually the regressing this use case.
>
> For today it doesn't address the case where the device is described with
> static address = 0, which isn't attached to the controller.
Hm, I'm pretty sure I had designed the code to support that case (see
[1]). It might be buggy, but nothing we can't fix I guess.
On Thu, 29 Aug 2019 16:39:18 +0200
Boris Brezillon <[email protected]> wrote:
> On Thu, 29 Aug 2019 14:00:44 +0000
> Vitor Soares <[email protected]> wrote:
>
> > Hi Boris,
> >
> > From: Boris Brezillon <[email protected]>
> > Date: Thu, Aug 29, 2019 at 11:44:57
> >
> > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > Vitor Soares <[email protected]> wrote:
> > >
> > > > The I3C devices described in DT might not be attached to the master which
> > > > doesn't allow to assign a specific dynamic address.
> > >
> > > I remember testing this when developing the framework, so, unless
> > > another patch regressed it, it should already work. I suspect patch 1
> > > is actually the regressing this use case.
> >
> > For today it doesn't address the case where the device is described with
> > static address = 0, which isn't attached to the controller.
>
> Hm, I'm pretty sure I had designed the code to support that case (see
> [1]). It might be buggy, but nothing we can't fix I guess.
>
[1]https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/i3c/master.c#L1898
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 15:39:41
> On Thu, 29 Aug 2019 16:39:18 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > On Thu, 29 Aug 2019 14:00:44 +0000
> > Vitor Soares <[email protected]> wrote:
> >
> > > Hi Boris,
> > >
> > > From: Boris Brezillon <[email protected]>
> > > Date: Thu, Aug 29, 2019 at 11:44:57
> > >
> > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > Vitor Soares <[email protected]> wrote:
> > > >
> > > > > The I3C devices described in DT might not be attached to the master which
> > > > > doesn't allow to assign a specific dynamic address.
> > > >
> > > > I remember testing this when developing the framework, so, unless
> > > > another patch regressed it, it should already work. I suspect patch 1
> > > > is actually the regressing this use case.
> > >
> > > For today it doesn't address the case where the device is described with
> > > static address = 0, which isn't attached to the controller.
> >
> > Hm, I'm pretty sure I had designed the code to support that case (see
> > [1]). It might be buggy, but nothing we can't fix I guess.
> >
>
> [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=
That is only valid if you have olddev which will only exist if static
address != 0.
On Thu, 29 Aug 2019 15:07:08 +0000
Vitor Soares <[email protected]> wrote:
> From: Boris Brezillon <[email protected]>
> Date: Thu, Aug 29, 2019 at 15:39:41
>
> > On Thu, 29 Aug 2019 16:39:18 +0200
> > Boris Brezillon <[email protected]> wrote:
> >
> > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > Vitor Soares <[email protected]> wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > From: Boris Brezillon <[email protected]>
> > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > >
> > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > Vitor Soares <[email protected]> wrote:
> > > > >
> > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > doesn't allow to assign a specific dynamic address.
> > > > >
> > > > > I remember testing this when developing the framework, so, unless
> > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > is actually the regressing this use case.
> > > >
> > > > For today it doesn't address the case where the device is described with
> > > > static address = 0, which isn't attached to the controller.
> > >
> > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > [1]). It might be buggy, but nothing we can't fix I guess.
> > >
> >
> > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=
>
> That is only valid if you have olddev which will only exist if static
> address != 0.
Hm, if you revert patch 1 (and assuming the device is properly defined
in the DT), you should have olddev != NULL when reaching that point. If
that's not the case there's a bug somewhere that should be fixed.
-----Original Message-----
From: Boris Brezillon
<[email protected]>
Sent: Thursday, August 29, 2019 4:25
PM
To: Vitor Soares <[email protected]>
Cc:
[email protected]; [email protected];
[email protected]; [email protected]; [email protected];
[email protected]; [email protected]
Subject: Re: [PATCH 2/4]
i3c: master: Check if devices have i3c_dev_boardinfo on
i3c_master_add_i3c_dev_locked()
On Thu, 29 Aug 2019 15:07:08 +0000
Vitor Soares <[email protected]> wrote:
> From: Boris Brezillon
<[email protected]>
> Date: Thu, Aug 29, 2019 at 15:39:41
>
> > On Thu, 29 Aug 2019 16:39:18 +0200
> > Boris Brezillon <[email protected]> wrote:
> >
> > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > Vitor Soares <[email protected]> wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > From: Boris Brezillon <[email protected]>
> > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > >
> > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > Vitor Soares <[email protected]> wrote:
> > > > >
> > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > doesn't allow to assign a specific dynamic address.
> > > > >
> > > > > I remember testing this when developing the framework, so, unless
> > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > is actually the regressing this use case.
> > > >
> > > > For today it doesn't address the case where the device is described with
> > > > static address = 0, which isn't attached to the controller.
> > >
> > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > [1]). It might be buggy, but nothing we can't fix I guess.
> > >
> >
> > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=
>
> That is only valid if you have olddev which will only exist if static
> address != 0.
Hm, if you revert patch 1 (and assuming the device is properly defined
in the DT), you should have olddev != NULL when reaching that point. If
that's not the case there's a bug somewhere that should be fixed.
No, because the device is not attached.
On Thu, 29 Aug 2019 15:57:32 +0000
Vitor Soares <[email protected]> wrote:
> -----Original Message-----
> From: Boris Brezillon
> <[email protected]>
> Sent: Thursday, August 29, 2019 4:25
> PM
> To: Vitor Soares <[email protected]>
> Cc:
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/4]
> i3c: master: Check if devices have i3c_dev_boardinfo on
> i3c_master_add_i3c_dev_locked()
>
> On Thu, 29 Aug 2019 15:07:08 +0000
> Vitor Soares <[email protected]> wrote:
>
> > From: Boris Brezillon
> <[email protected]>
> > Date: Thu, Aug 29, 2019 at 15:39:41
> >
>
> > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > Boris Brezillon <[email protected]> wrote:
> > >
> > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > Vitor Soares <[email protected]> wrote:
> > > >
> > > > > Hi Boris,
> > > > >
> > > > > From: Boris Brezillon <[email protected]>
> > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > >
> > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > Vitor Soares <[email protected]> wrote:
> > > > > >
> > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > doesn't allow to assign a specific dynamic address.
> > > > > >
> > > > > > I remember testing this when developing the framework, so, unless
> > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > is actually the regressing this use case.
> > > > >
> > > > > For today it doesn't address the case where the device is described with
> > > > > static address = 0, which isn't attached to the controller.
> > > >
> > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > >
> > >
> > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=
> >
> > That is only valid if you have olddev which will only exist if static
> > address != 0.
>
> Hm, if you revert patch 1 (and assuming the device is properly defined
> in the DT), you should have olddev != NULL when reaching that point. If
> that's not the case there's a bug somewhere that should be fixed.
>
> No, because the device is not attached.
Oh, my bad, I see what you mean now. This is definitely a bug and should
have the Fixes tags. I mean, even if we don't care about dynamic
address assignment, I3C drivers might care about the ->of_node that's
attached to the device.
On Thu, 29 Aug 2019 12:19:33 +0200
Vitor Soares <[email protected]> wrote:
> The I3C devices described in DT might not be attached to the master which
> doesn't allow to assign a specific dynamic address.
Dynamic address assignment is not the only problem here (see my
comment about missing ->of_node info). I would simply say that
newdev->boardinfo assignment was missing in
i3c_master_add_i3c_dev_locked() which is already a bug by itself.
I would also change the subject to
"i3c: master: Make sure ->boardinfo is initialized in add_i3c_dev_locked()"
>
> This patch check if a device has i3c_dev_boardinfo and add it to
> i3c_dev_desc structure. In this conditions, the framework will try to
> assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
I would get rid of this explanation and add Fixes/Cc-stable tags.
>
> Signed-off-by: Vitor Soares <[email protected]>
> ---
> drivers/i3c/master.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4d29e1f..85fbda6 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> return NULL;
> }
>
> +static struct i3c_dev_boardinfo *
> +i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
> +{
> + struct i3c_master_controller *master = i3c_dev_get_master(dev);
> + struct i3c_dev_boardinfo *boardinfo;
> +
> + if (dev->boardinfo)
> + return NULL;
> +
> + list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> + if (dev->info.pid == boardinfo->pid)
> + return boardinfo;
> + }
> +
> + return NULL;
> +}
Can we instead have:
static void i3c_master_init_i3c_dev_boardinfo(struct i3c_dev_desc *dev)
{
struct i3c_master_controller *master = i3c_dev_get_master(dev);
list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
if (dev->info.pid == boardinfo->pid)
dev->boardinfo = boardinfo;
}
}
> +
> /**
> * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> * @master: master used to send frames on the bus
> @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> {
> struct i3c_device_info info = { .dyn_addr = addr };
> struct i3c_dev_desc *newdev, *olddev;
> + struct i3c_dev_boardinfo *boardinfo;
> u8 old_dyn_addr = addr, expected_dyn_addr;
> struct i3c_ibi_setup ibireq = { };
> bool enable_ibi = false;
> @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> if (ret)
> goto err_detach_dev;
>
> + boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> + if (boardinfo)
> + newdev->boardinfo = boardinfo;
> +
And here:
i3c_master_init_i3c_dev_boardinfo(newdev);
> /*
> * Depending on our previous state, the expected dynamic address might
> * differ:
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 17:15:20
> On Thu, 29 Aug 2019 15:57:32 +0000
> Vitor Soares <[email protected]> wrote:
>
> > -----Original Message-----
> > From: Boris Brezillon
> > <[email protected]>
> > Sent: Thursday, August 29, 2019 4:25
> > PM
> > To: Vitor Soares <[email protected]>
> > Cc:
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH 2/4]
> > i3c: master: Check if devices have i3c_dev_boardinfo on
> > i3c_master_add_i3c_dev_locked()
> >
> > On Thu, 29 Aug 2019 15:07:08 +0000
> > Vitor Soares <[email protected]> wrote:
> >
> > > From: Boris Brezillon
> > <[email protected]>
> > > Date: Thu, Aug 29, 2019 at 15:39:41
> > >
> >
> > > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > > Boris Brezillon <[email protected]> wrote:
> > > >
> > > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > > Vitor Soares <[email protected]> wrote:
> > > > >
> > > > > > Hi Boris,
> > > > > >
> > > > > > From: Boris Brezillon <[email protected]>
> > > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > > >
> > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > > Vitor Soares <[email protected]> wrote:
> > > > > > >
> > > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > > doesn't allow to assign a specific dynamic address.
> > > > > > >
> > > > > > > I remember testing this when developing the framework, so, unless
> > > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > > is actually the regressing this use case.
> > > > > >
> > > > > > For today it doesn't address the case where the device is described with
> > > > > > static address = 0, which isn't attached to the controller.
> > > > >
> > > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > > >
> > > >
> > > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=
> > >
> > > That is only valid if you have olddev which will only exist if static
> > > address != 0.
> >
> > Hm, if you revert patch 1 (and assuming the device is properly defined
> > in the DT), you should have olddev != NULL when reaching that point. If
> > that's not the case there's a bug somewhere that should be fixed.
> >
> > No, because the device is not attached.
>
> Oh, my bad, I see what you mean now. This is definitely a bug and should
> have the Fixes tags. I mean, even if we don't care about dynamic
> address assignment, I3C drivers might care about the ->of_node that's
> attached to the device.
I didn't consider a bug because in dt-bindings says to not use 'assigned
address' if SA = 0.
Do you think there is a better way to solve it?
Best regards,
Vitor Soares
On Thu, 29 Aug 2019 16:33:43 +0000
Vitor Soares <[email protected]> wrote:
> From: Boris Brezillon <[email protected]>
> Date: Thu, Aug 29, 2019 at 17:15:20
>
> > On Thu, 29 Aug 2019 15:57:32 +0000
> > Vitor Soares <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Boris Brezillon
> > > <[email protected]>
> > > Sent: Thursday, August 29, 2019 4:25
> > > PM
> > > To: Vitor Soares <[email protected]>
> > > Cc:
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH 2/4]
> > > i3c: master: Check if devices have i3c_dev_boardinfo on
> > > i3c_master_add_i3c_dev_locked()
> > >
> > > On Thu, 29 Aug 2019 15:07:08 +0000
> > > Vitor Soares <[email protected]> wrote:
> > >
> > > > From: Boris Brezillon
> > > <[email protected]>
> > > > Date: Thu, Aug 29, 2019 at 15:39:41
> > > >
> > >
> > > > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > > > Boris Brezillon <[email protected]> wrote:
> > > > >
> > > > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > > > Vitor Soares <[email protected]> wrote:
> > > > > >
> > > > > > > Hi Boris,
> > > > > > >
> > > > > > > From: Boris Brezillon <[email protected]>
> > > > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > > > >
> > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > > > Vitor Soares <[email protected]> wrote:
> > > > > > > >
> > > > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > > > doesn't allow to assign a specific dynamic address.
> > > > > > > >
> > > > > > > > I remember testing this when developing the framework, so, unless
> > > > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > > > is actually the regressing this use case.
> > > > > > >
> > > > > > > For today it doesn't address the case where the device is described with
> > > > > > > static address = 0, which isn't attached to the controller.
> > > > > >
> > > > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > > > >
> > > > >
> > > > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=
> > > >
> > > > That is only valid if you have olddev which will only exist if static
> > > > address != 0.
> > >
> > > Hm, if you revert patch 1 (and assuming the device is properly defined
> > > in the DT), you should have olddev != NULL when reaching that point. If
> > > that's not the case there's a bug somewhere that should be fixed.
> > >
> > > No, because the device is not attached.
> >
> > Oh, my bad, I see what you mean now. This is definitely a bug and should
> > have the Fixes tags. I mean, even if we don't care about dynamic
> > address assignment, I3C drivers might care about the ->of_node that's
> > attached to the device.
>
> I didn't consider a bug because in dt-bindings says to not use 'assigned
> address' if SA = 0.
The fact that we don't try to assign a predefined dynamic address
when !SA is indeed not a bug, but failing to propagate the ->of_node
info to the i3c_device is one.
> Do you think there is a better way to solve it?
Your patch is correct, I just proposed a few adjustments.