linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
@ 2022-08-14  8:04 Khalid Masum
  2022-08-14  9:45 ` Greg KH
  2022-08-14 18:46 ` Andy Shevchenko
  0 siblings, 2 replies; 9+ messages in thread
From: Khalid Masum @ 2022-08-14  8:04 UTC (permalink / raw)
  To: alsa-devel, linux-kernel, linux-acpi, linux-kernel-mentees
  Cc: Heikki Krogerus, Rafael J . Wysocki, Pierre-Louis Bossart,
	Daniel Scally, Vinod Koul, Sakari Ailus, Sanyog Kale,
	Andy Shevchenko, Bard Liao, Len Brown

According to the TODO, In sw_bus_master_add, bus->multi_link is to be
populated with properties from FW node props. Make this happen by 
creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use 
the flag to store the multi_link value from intel_link_startup. Use 
this flag to initialize bus->multi_link.

Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
---
I do not think adding a new flag for fwnode_handle is a good idea.
So, what would be the best way to initialize bus->multilink with 
fwnode props?

  -- Khalid Masum

 drivers/soundwire/bus.c   | 4 ++--
 drivers/soundwire/intel.c | 1 +
 include/linux/fwnode.h    | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index a2bfb0434a67..80df1672c60b 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -74,9 +74,9 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
 
 	/*
 	 * Initialize multi_link flag
-	 * TODO: populate this flag by reading property from FW node
 	 */
-	bus->multi_link = false;
+	bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
+		== FWNODE_FLAG_MULTI_LINKED;
 	if (bus->ops->read_prop) {
 		ret = bus->ops->read_prop(bus);
 		if (ret < 0) {
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 505c5ef061e3..034d1c523ddf 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1347,6 +1347,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
 		 */
 		bus->multi_link = true;
 		bus->hw_sync_min_links = 1;
+		dev->fwnode->flags |= FWNODE_FLAG_MULTI_LINKED;
 	}
 
 	/* Initialize shim, controller */
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 9a81c4410b9f..446a52744953 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -32,6 +32,7 @@ struct device;
 #define FWNODE_FLAG_NOT_DEVICE			BIT(1)
 #define FWNODE_FLAG_INITIALIZED			BIT(2)
 #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	BIT(3)
+#define FWNODE_FLAG_MULTI_LINKED		BIT(4)
 
 struct fwnode_handle {
 	struct fwnode_handle *secondary;
-- 
2.37.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-14  8:04 [RFC PATCH] Soundwire: Initialize multi_link with fwnode props Khalid Masum
@ 2022-08-14  9:45 ` Greg KH
  2022-08-15  4:08   ` Khalid Masum
  2022-08-14 18:46 ` Andy Shevchenko
  1 sibling, 1 reply; 9+ messages in thread
From: Greg KH @ 2022-08-14  9:45 UTC (permalink / raw)
  To: Khalid Masum
  Cc: alsa-devel, Heikki Krogerus, Rafael J . Wysocki, Bard Liao,
	linux-kernel, Pierre-Louis Bossart, Daniel Scally, linux-acpi,
	Vinod Koul, Sakari Ailus, Sanyog Kale, Andy Shevchenko,
	linux-kernel-mentees, Len Brown

On Sun, Aug 14, 2022 at 02:04:15PM +0600, Khalid Masum wrote:
> According to the TODO, In sw_bus_master_add, bus->multi_link is to be
> populated with properties from FW node props. Make this happen by 
> creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use 
> the flag to store the multi_link value from intel_link_startup. Use 
> this flag to initialize bus->multi_link.
> 
> Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
> ---
> I do not think adding a new flag for fwnode_handle is a good idea.
> So, what would be the best way to initialize bus->multilink with 
> fwnode props?
> 
>   -- Khalid Masum
> 
>  drivers/soundwire/bus.c   | 4 ++--
>  drivers/soundwire/intel.c | 1 +
>  include/linux/fwnode.h    | 1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index a2bfb0434a67..80df1672c60b 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -74,9 +74,9 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
>  
>  	/*
>  	 * Initialize multi_link flag
> -	 * TODO: populate this flag by reading property from FW node
>  	 */
> -	bus->multi_link = false;
> +	bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
> +		== FWNODE_FLAG_MULTI_LINKED;
>  	if (bus->ops->read_prop) {
>  		ret = bus->ops->read_prop(bus);
>  		if (ret < 0) {
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 505c5ef061e3..034d1c523ddf 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1347,6 +1347,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
>  		 */
>  		bus->multi_link = true;
>  		bus->hw_sync_min_links = 1;
> +		dev->fwnode->flags |= FWNODE_FLAG_MULTI_LINKED;
>  	}
>  
>  	/* Initialize shim, controller */
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 9a81c4410b9f..446a52744953 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -32,6 +32,7 @@ struct device;
>  #define FWNODE_FLAG_NOT_DEVICE			BIT(1)
>  #define FWNODE_FLAG_INITIALIZED			BIT(2)
>  #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	BIT(3)
> +#define FWNODE_FLAG_MULTI_LINKED		BIT(4)

What does this commit actually change?

Did you test this on real hardware?

thanks,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-14  8:04 [RFC PATCH] Soundwire: Initialize multi_link with fwnode props Khalid Masum
  2022-08-14  9:45 ` Greg KH
@ 2022-08-14 18:46 ` Andy Shevchenko
  2022-08-15  4:28   ` Khalid Masum
  1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2022-08-14 18:46 UTC (permalink / raw)
  To: Khalid Masum
  Cc: ALSA Development Mailing List, Heikki Krogerus,
	Rafael J . Wysocki, Bard Liao, Linux Kernel Mailing List,
	Pierre-Louis Bossart, Daniel Scally, ACPI Devel Maling List,
	Vinod Koul, Sakari Ailus, Sanyog Kale, Andy Shevchenko,
	linux-kernel-mentees, Len Brown

On Sun, Aug 14, 2022 at 11:31 AM Khalid Masum <khalid.masum.92@gmail.com> wrote:
>
> According to the TODO, In sw_bus_master_add, bus->multi_link is to be
> populated with properties from FW node props. Make this happen by
> creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use
> the flag to store the multi_link value from intel_link_startup. Use
> this flag to initialize bus->multi_link.

...

>         /*
>          * Initialize multi_link flag
> -        * TODO: populate this flag by reading property from FW node
>          */
> -       bus->multi_link = false;
> +       bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
> +               == FWNODE_FLAG_MULTI_LINKED;

NAK (as far as I understood the context of the comment and the change itself).

These flags are for devlink, we do not mix FW properties with those
internal flags anyhow. The comment suggests that this should be a
property. Also commit message doesn't explain the relation to devlink.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-14  9:45 ` Greg KH
@ 2022-08-15  4:08   ` Khalid Masum
  2022-08-15  6:37     ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Khalid Masum @ 2022-08-15  4:08 UTC (permalink / raw)
  To: Greg KH
  Cc: alsa-devel, Heikki Krogerus, Rafael J . Wysocki, Bard Liao,
	linux-kernel, Pierre-Louis Bossart, Daniel Scally, linux-acpi,
	Vinod Koul, Sakari Ailus, Sanyog Kale, Andy Shevchenko,
	linux-kernel-mentees, Len Brown

On 8/14/22 15:45, Greg KH wrote:
> On Sun, Aug 14, 2022 at 02:04:15PM +0600, Khalid Masum wrote:
>> According to the TODO, In sw_bus_master_add, bus->multi_link is to be
>> populated with properties from FW node props. Make this happen by
>> creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use
>> the flag to store the multi_link value from intel_link_startup. Use
>> this flag to initialize bus->multi_link.
>>
>> Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
>> ---
>> I do not think adding a new flag for fwnode_handle is a good idea.
>> So, what would be the best way to initialize bus->multilink with
>> fwnode props?
>>
>>    -- Khalid Masum
>>
>>   drivers/soundwire/bus.c   | 4 ++--
>>   drivers/soundwire/intel.c | 1 +
>>   include/linux/fwnode.h    | 1 +
>>   3 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index a2bfb0434a67..80df1672c60b 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -74,9 +74,9 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
>>   
>>   	/*
>>   	 * Initialize multi_link flag
>> -	 * TODO: populate this flag by reading property from FW node
>>   	 */
>> -	bus->multi_link = false;
>> +	bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
>> +		== FWNODE_FLAG_MULTI_LINKED;
>>   	if (bus->ops->read_prop) {
>>   		ret = bus->ops->read_prop(bus);
>>   		if (ret < 0) {
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index 505c5ef061e3..034d1c523ddf 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -1347,6 +1347,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
>>   		 */
>>   		bus->multi_link = true;
>>   		bus->hw_sync_min_links = 1;
>> +		dev->fwnode->flags |= FWNODE_FLAG_MULTI_LINKED;
>>   	}
>>   
>>   	/* Initialize shim, controller */
>> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
>> index 9a81c4410b9f..446a52744953 100644
>> --- a/include/linux/fwnode.h
>> +++ b/include/linux/fwnode.h
>> @@ -32,6 +32,7 @@ struct device;
>>   #define FWNODE_FLAG_NOT_DEVICE			BIT(1)
>>   #define FWNODE_FLAG_INITIALIZED			BIT(2)
>>   #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	BIT(3)
>> +#define FWNODE_FLAG_MULTI_LINKED		BIT(4)
> 
> What does this commit actually change?

The new flag will lets us save if the device has multilink in 
fwnode_handle whenever needed.
Then for soundwire/intel, save the multi_link flag into fwnode during 
startup.
Later at master_add, as written in todo, initialize the multilink flag 
with fwnode's flag property.

> 
> Did you test this on real hardware?

I did not test this on real hardware.
> 
> thanks,
> 
> greg k-h

thanks,
  -- Khalid Masum

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-14 18:46 ` Andy Shevchenko
@ 2022-08-15  4:28   ` Khalid Masum
  2022-08-15  8:16     ` Pierre-Louis Bossart
  0 siblings, 1 reply; 9+ messages in thread
From: Khalid Masum @ 2022-08-15  4:28 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: ALSA Development Mailing List, Heikki Krogerus,
	Rafael J . Wysocki, Bard Liao, Linux Kernel Mailing List,
	Pierre-Louis Bossart, Daniel Scally, ACPI Devel Maling List,
	Vinod Koul, Sakari Ailus, Sanyog Kale, Andy Shevchenko,
	linux-kernel-mentees, Len Brown

On 8/15/22 00:46, Andy Shevchenko wrote:
> On Sun, Aug 14, 2022 at 11:31 AM Khalid Masum <khalid.masum.92@gmail.com> wrote:
>>
>> According to the TODO, In sw_bus_master_add, bus->multi_link is to be
>> populated with properties from FW node props. Make this happen by
>> creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use
>> the flag to store the multi_link value from intel_link_startup. Use
>> this flag to initialize bus->multi_link.
> 
> ...
> 
>>          /*
>>           * Initialize multi_link flag
>> -        * TODO: populate this flag by reading property from FW node
>>           */
>> -       bus->multi_link = false;
>> +       bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
>> +               == FWNODE_FLAG_MULTI_LINKED;
> 
> NAK (as far as I understood the context of the comment and the change itself).
> 
> These flags are for devlink, we do not mix FW properties with those
> internal flags anyhow. The comment suggests that this should be az
> property. Also commit message doesn't explain the relation to devlink.
> 
That is a good information to know. Thanks.

I shall try to find out if I can somehow get multi_link's value from 
fwnode in any other way and look into devlink in the process.

If you have any suggestions regarding this TODO, please let me know.

Thanks,
   -- Khalid Masum
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-15  4:08   ` Khalid Masum
@ 2022-08-15  6:37     ` Greg KH
  2022-08-15 12:30       ` Khalid Masum
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2022-08-15  6:37 UTC (permalink / raw)
  To: Khalid Masum
  Cc: alsa-devel, Heikki Krogerus, Rafael J . Wysocki, Bard Liao,
	linux-kernel, Pierre-Louis Bossart, Daniel Scally, linux-acpi,
	Vinod Koul, Sakari Ailus, Sanyog Kale, Andy Shevchenko,
	linux-kernel-mentees, Len Brown

On Mon, Aug 15, 2022 at 10:08:07AM +0600, Khalid Masum wrote:
> On 8/14/22 15:45, Greg KH wrote:
> > On Sun, Aug 14, 2022 at 02:04:15PM +0600, Khalid Masum wrote:
> > > According to the TODO, In sw_bus_master_add, bus->multi_link is to be
> > > populated with properties from FW node props. Make this happen by
> > > creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use
> > > the flag to store the multi_link value from intel_link_startup. Use
> > > this flag to initialize bus->multi_link.
> > > 
> > > Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
> > > ---
> > > I do not think adding a new flag for fwnode_handle is a good idea.
> > > So, what would be the best way to initialize bus->multilink with
> > > fwnode props?
> > > 
> > >    -- Khalid Masum
> > > 
> > >   drivers/soundwire/bus.c   | 4 ++--
> > >   drivers/soundwire/intel.c | 1 +
> > >   include/linux/fwnode.h    | 1 +
> > >   3 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> > > index a2bfb0434a67..80df1672c60b 100644
> > > --- a/drivers/soundwire/bus.c
> > > +++ b/drivers/soundwire/bus.c
> > > @@ -74,9 +74,9 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
> > >   	/*
> > >   	 * Initialize multi_link flag
> > > -	 * TODO: populate this flag by reading property from FW node
> > >   	 */
> > > -	bus->multi_link = false;
> > > +	bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
> > > +		== FWNODE_FLAG_MULTI_LINKED;

I missed that this was an if statement here, please write this to be
more obvious and readable.

> > >   	if (bus->ops->read_prop) {
> > >   		ret = bus->ops->read_prop(bus);
> > >   		if (ret < 0) {
> > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > > index 505c5ef061e3..034d1c523ddf 100644
> > > --- a/drivers/soundwire/intel.c
> > > +++ b/drivers/soundwire/intel.c
> > > @@ -1347,6 +1347,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
> > >   		 */
> > >   		bus->multi_link = true;
> > >   		bus->hw_sync_min_links = 1;
> > > +		dev->fwnode->flags |= FWNODE_FLAG_MULTI_LINKED;
> > >   	}
> > >   	/* Initialize shim, controller */
> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > > index 9a81c4410b9f..446a52744953 100644
> > > --- a/include/linux/fwnode.h
> > > +++ b/include/linux/fwnode.h
> > > @@ -32,6 +32,7 @@ struct device;
> > >   #define FWNODE_FLAG_NOT_DEVICE			BIT(1)
> > >   #define FWNODE_FLAG_INITIALIZED			BIT(2)
> > >   #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	BIT(3)
> > > +#define FWNODE_FLAG_MULTI_LINKED		BIT(4)
> > 
> > What does this commit actually change?
> 
> The new flag will lets us save if the device has multilink in fwnode_handle
> whenever needed.
> Then for soundwire/intel, save the multi_link flag into fwnode during
> startup.
> Later at master_add, as written in todo, initialize the multilink flag with
> fwnode's flag property.

And what does that allow to happen?  What changes with all of this?

thanks,

greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-15  4:28   ` Khalid Masum
@ 2022-08-15  8:16     ` Pierre-Louis Bossart
  2022-08-15 12:10       ` Khalid Masum
  0 siblings, 1 reply; 9+ messages in thread
From: Pierre-Louis Bossart @ 2022-08-15  8:16 UTC (permalink / raw)
  To: Khalid Masum, Andy Shevchenko
  Cc: ALSA Development Mailing List, Heikki Krogerus,
	Rafael J . Wysocki, Bard Liao, Linux Kernel Mailing List,
	Daniel Scally, ACPI Devel Maling List, Vinod Koul, Sakari Ailus,
	Sanyog Kale, Andy Shevchenko, linux-kernel-mentees, Len Brown



On 8/15/22 06:28, Khalid Masum wrote:
> On 8/15/22 00:46, Andy Shevchenko wrote:
>> On Sun, Aug 14, 2022 at 11:31 AM Khalid Masum
>> <khalid.masum.92@gmail.com> wrote:
>>>
>>> According to the TODO, In sw_bus_master_add, bus->multi_link is to be
>>> populated with properties from FW node props. Make this happen by
>>> creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use
>>> the flag to store the multi_link value from intel_link_startup. Use
>>> this flag to initialize bus->multi_link.
>>
>> ...
>>
>>>          /*
>>>           * Initialize multi_link flag
>>> -        * TODO: populate this flag by reading property from FW node
>>>           */
>>> -       bus->multi_link = false;
>>> +       bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
>>> +               == FWNODE_FLAG_MULTI_LINKED;
>>
>> NAK (as far as I understood the context of the comment and the change
>> itself).
>>
>> These flags are for devlink, we do not mix FW properties with those
>> internal flags anyhow. The comment suggests that this should be az
>> property. Also commit message doesn't explain the relation to devlink.
>>
> That is a good information to know. Thanks.
> 
> I shall try to find out if I can somehow get multi_link's value from
> fwnode in any other way and look into devlink in the process.
> 
> If you have any suggestions regarding this TODO, please let me know.

The suggestion is to remove the TODO and leave the code as is: the
capabilities enabled for multi-link are required as part of the
programming sequences, even when a stream uses a single link we still
use the syncArm/syncGo sequences.

Please don't change this.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-15  8:16     ` Pierre-Louis Bossart
@ 2022-08-15 12:10       ` Khalid Masum
  0 siblings, 0 replies; 9+ messages in thread
From: Khalid Masum @ 2022-08-15 12:10 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Andy Shevchenko
  Cc: ALSA Development Mailing List, Heikki Krogerus,
	Rafael J . Wysocki, Bard Liao, Linux Kernel Mailing List,
	Daniel Scally, ACPI Devel Maling List, Vinod Koul, Sakari Ailus,
	Sanyog Kale, Andy Shevchenko, linux-kernel-mentees, Len Brown

On 8/15/22 14:16, Pierre-Louis Bossart wrote:
> 
> 
> On 8/15/22 06:28, Khalid Masum wrote:
>> On 8/15/22 00:46, Andy Shevchenko wrote:
>>> On Sun, Aug 14, 2022 at 11:31 AM Khalid Masum
>>> <khalid.masum.92@gmail.com> wrote:
>>>>
>>>> According to the TODO, In sw_bus_master_add, bus->multi_link is to be
>>>> populated with properties from FW node props. Make this happen by
>>>> creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use
>>>> the flag to store the multi_link value from intel_link_startup. Use
>>>> this flag to initialize bus->multi_link.
>>>
>>> ...
>>>
>>>>           /*
>>>>            * Initialize multi_link flag
>>>> -        * TODO: populate this flag by reading property from FW node
>>>>            */
>>>> -       bus->multi_link = false;
>>>> +       bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
>>>> +               == FWNODE_FLAG_MULTI_LINKED;
>>>
>>> NAK (as far as I understood the context of the comment and the change
>>> itself).
>>>
>>> These flags are for devlink, we do not mix FW properties with those
>>> internal flags anyhow. The comment suggests that this should be az
>>> property. Also commit message doesn't explain the relation to devlink.
>>>
>> That is a good information to know. Thanks.
>>
>> I shall try to find out if I can somehow get multi_link's value from
>> fwnode in any other way and look into devlink in the process.
>>
>> If you have any suggestions regarding this TODO, please let me know.
> 
> The suggestion is to remove the TODO and leave the code as is: the
> capabilities enabled for multi-link are required as part of the
> programming sequences, even when a stream uses a single link we still
> use the syncArm/syncGo sequences.
> 
> Please don't change this.

Thanks for the suggestion! I shall send a patch with the TODO removed, 
leaving the code as is.

   -- Khalid Masum

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] Soundwire: Initialize multi_link with fwnode props
  2022-08-15  6:37     ` Greg KH
@ 2022-08-15 12:30       ` Khalid Masum
  0 siblings, 0 replies; 9+ messages in thread
From: Khalid Masum @ 2022-08-15 12:30 UTC (permalink / raw)
  To: Greg KH
  Cc: alsa-devel, Heikki Krogerus, Rafael J . Wysocki, Bard Liao,
	linux-kernel, Pierre-Louis Bossart, Daniel Scally, linux-acpi,
	Vinod Koul, Sakari Ailus, Sanyog Kale, Andy Shevchenko,
	linux-kernel-mentees, Len Brown

On 8/15/22 12:37, Greg KH wrote:
> On Mon, Aug 15, 2022 at 10:08:07AM +0600, Khalid Masum wrote:
>> On 8/14/22 15:45, Greg KH wrote:
>>> On Sun, Aug 14, 2022 at 02:04:15PM +0600, Khalid Masum wrote:
>>>> According to the TODO, In sw_bus_master_add, bus->multi_link is to be
>>>> populated with properties from FW node props. Make this happen by
>>>> creating a new fwnode_handle flag FWNODE_FLAG_MULTI_LINKED and use
>>>> the flag to store the multi_link value from intel_link_startup. Use
>>>> this flag to initialize bus->multi_link.
>>>>
>>>> Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
>>>> ---
>>>> I do not think adding a new flag for fwnode_handle is a good idea.
>>>> So, what would be the best way to initialize bus->multilink with
>>>> fwnode props?
>>>>
>>>>     -- Khalid Masum
>>>>
>>>>    drivers/soundwire/bus.c   | 4 ++--
>>>>    drivers/soundwire/intel.c | 1 +
>>>>    include/linux/fwnode.h    | 1 +
>>>>    3 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>>>> index a2bfb0434a67..80df1672c60b 100644
>>>> --- a/drivers/soundwire/bus.c
>>>> +++ b/drivers/soundwire/bus.c
>>>> @@ -74,9 +74,9 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
>>>>    	/*
>>>>    	 * Initialize multi_link flag
>>>> -	 * TODO: populate this flag by reading property from FW node
>>>>    	 */
>>>> -	bus->multi_link = false;
>>>> +	bus->multi_link = (fwnode->flags & FWNODE_FLAG_MULTI_LINKED)
>>>> +		== FWNODE_FLAG_MULTI_LINKED;
> 
> I missed that this was an if statement here, please write this to be
> more obvious and readable.
> 
>>>>    	if (bus->ops->read_prop) {
>>>>    		ret = bus->ops->read_prop(bus);
>>>>    		if (ret < 0) {
>>>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>>>> index 505c5ef061e3..034d1c523ddf 100644
>>>> --- a/drivers/soundwire/intel.c
>>>> +++ b/drivers/soundwire/intel.c
>>>> @@ -1347,6 +1347,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
>>>>    		 */
>>>>    		bus->multi_link = true;
>>>>    		bus->hw_sync_min_links = 1;
>>>> +		dev->fwnode->flags |= FWNODE_FLAG_MULTI_LINKED;
>>>>    	}
>>>>    	/* Initialize shim, controller */
>>>> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
>>>> index 9a81c4410b9f..446a52744953 100644
>>>> --- a/include/linux/fwnode.h
>>>> +++ b/include/linux/fwnode.h
>>>> @@ -32,6 +32,7 @@ struct device;
>>>>    #define FWNODE_FLAG_NOT_DEVICE			BIT(1)
>>>>    #define FWNODE_FLAG_INITIALIZED			BIT(2)
>>>>    #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	BIT(3)
>>>> +#define FWNODE_FLAG_MULTI_LINKED		BIT(4)
>>>
>>> What does this commit actually change?
>>
>> The new flag will lets us save if the device has multilink in fwnode_handle
>> whenever needed.
>> Then for soundwire/intel, save the multi_link flag into fwnode during
>> startup.
>> Later at master_add, as written in todo, initialize the multilink flag with
>> fwnode's flag property.
> 
> And what does that allow to happen?  What changes with all of this?

As suggested by Pierre-Louis Bossart these changes are not necessary and 
the TODO is to be removed. Earlier my intention was to create a new flag 
that lets us know whether multi_link is to be read from 
sdw_bus_master_add just as instructed in TODO.

I shall send another patch, that removes the TODO, as suggested by 
Pierre-Louis Bossart.
> 
> thanks,
> 
> greg k-h

thanks,
   -- Khalid Masum

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-08-15 12:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-14  8:04 [RFC PATCH] Soundwire: Initialize multi_link with fwnode props Khalid Masum
2022-08-14  9:45 ` Greg KH
2022-08-15  4:08   ` Khalid Masum
2022-08-15  6:37     ` Greg KH
2022-08-15 12:30       ` Khalid Masum
2022-08-14 18:46 ` Andy Shevchenko
2022-08-15  4:28   ` Khalid Masum
2022-08-15  8:16     ` Pierre-Louis Bossart
2022-08-15 12:10       ` Khalid Masum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).