diff mbox series

[v2,1/6] i3c: master: add enable(disable) hot join in sys entry

Message ID 20231018205929.3435110-2-Frank.Li@nxp.com
State New
Headers
Series i3c: master: some improvment for i3c master |

Commit Message

Frank Li Oct. 18, 2023, 8:59 p.m. UTC 
  Add hotjoin entry in sys file system allow user enable/disable hotjoin
feature.

Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
Add api i3c_master_enable(disable)_hotjoin();

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c       | 84 ++++++++++++++++++++++++++++++++++++++
 include/linux/i3c/master.h |  5 +++
 2 files changed, 89 insertions(+)

Comments

Zbigniew, Lukwinski Oct. 20, 2023, 8:55 a.m. UTC | #1 
On 10/18/2023 10:59 PM, Frank Li wrote:
> Add hotjoin entry in sys file system allow user enable/disable hotjoin
> feature.
>
> Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> Add api i3c_master_enable(disable)_hotjoin();

What is the use case for having HJ enable knob in sysfs available for 
user space other than for debug stuff? In other words, does user space 
really need to enable/disable HJ in runtime for other reason but debug? 
If it is only for debug maybe it  could be move to debugFS?

I think that maybe HJ enable knob shall be available in DT so you could 
control the default state for this feature also at compilation phase?

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>   drivers/i3c/master.c       | 84 ++++++++++++++++++++++++++++++++++++++
>   include/linux/i3c/master.h |  5 +++
>   2 files changed, 89 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 08aeb69a78003..ed5e27cd20811 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -526,6 +526,89 @@ static ssize_t i2c_scl_frequency_show(struct device *dev,
>   }
>   static DEVICE_ATTR_RO(i2c_scl_frequency);
>   
> +static int i3c_set_hotjoin(struct i3c_master_controller *master, bool enable)
> +{
> +	int ret;
> +
> +	if (!master ||
> +	    !master->ops ||
> +	    !master->ops->enable_hotjoin ||
> +	    !master->ops->disable_hotjoin
> +	   )
> +		return -EINVAL;
> +
> +	i3c_bus_normaluse_lock(&master->bus);
> +
> +	if (enable)
> +		ret = master->ops->enable_hotjoin(master);
> +	else
> +		ret = master->ops->disable_hotjoin(master);
> +
> +	master->hotjoin = enable;
> +
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	return ret;
> +}
> +
> +static ssize_t hotjoin_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	int ret;
> +	long res;
> +
> +	if (!i3cbus->cur_master)
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &res))
> +		return -EINVAL;
> +
> +	ret = i3c_set_hotjoin(i3cbus->cur_master->common.master, !!res);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +/*
> + * i3c_master_enable_hotjoin - Enable hotjoin
> + * @master: I3C master object
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +int i3c_master_enable_hotjoin(struct i3c_master_controller *master)
> +{
> +	return i3c_set_hotjoin(master, true);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_enable_hotjoin);
> +
> +/*
> + * i3c_master_disable_hotjoin - Disable hotjoin
> + * @master: I3C master object
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +int i3c_master_disable_hotjoin(struct i3c_master_controller *master)
> +{
> +	return i3c_set_hotjoin(master, false);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_disable_hotjoin);
> +
> +static ssize_t hotjoin_show(struct device *dev, struct device_attribute *da, char *buf)
> +{
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +	ssize_t ret;
> +
> +	i3c_bus_normaluse_lock(i3cbus);
> +	ret = sysfs_emit(buf, "%d\n", i3cbus->cur_master->common.master->hotjoin);
> +	i3c_bus_normaluse_unlock(i3cbus);
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR_RW(hotjoin);
> +
>   static struct attribute *i3c_masterdev_attrs[] = {
>   	&dev_attr_mode.attr,
>   	&dev_attr_current_master.attr,
> @@ -536,6 +619,7 @@ static struct attribute *i3c_masterdev_attrs[] = {
>   	&dev_attr_pid.attr,
>   	&dev_attr_dynamic_address.attr,
>   	&dev_attr_hdrcap.attr,
> +	&dev_attr_hotjoin.attr,
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(i3c_masterdev);
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 0b52da4f23467..65b8965968af2 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -452,6 +452,8 @@ struct i3c_master_controller_ops {
>   	int (*disable_ibi)(struct i3c_dev_desc *dev);
>   	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
>   				 struct i3c_ibi_slot *slot);
> +	int (*enable_hotjoin)(struct i3c_master_controller *master);
> +	int (*disable_hotjoin)(struct i3c_master_controller *master);
>   };
>   
>   /**
> @@ -487,6 +489,7 @@ struct i3c_master_controller {
>   	const struct i3c_master_controller_ops *ops;
>   	unsigned int secondary : 1;
>   	unsigned int init_done : 1;
> +	unsigned int hotjoin: 1;
>   	struct {
>   		struct list_head i3c;
>   		struct list_head i2c;
> @@ -543,6 +546,8 @@ int i3c_master_register(struct i3c_master_controller *master,
>   			const struct i3c_master_controller_ops *ops,
>   			bool secondary);
>   void i3c_master_unregister(struct i3c_master_controller *master);
> +int i3c_master_enable_hotjoin(struct i3c_master_controller *master);
> +int i3c_master_disable_hotjoin(struct i3c_master_controller *master);
>   
>   /**
>    * i3c_dev_get_master_data() - get master private data attached to an I3C
Miquel Raynal Oct. 20, 2023, 1:45 p.m. UTC | #2 
Hi Lukwinski,

zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
+0200:

> On 10/18/2023 10:59 PM, Frank Li wrote:
> > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > feature.
> >
> > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > Add api i3c_master_enable(disable)_hotjoin();  
> 
> What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?

I don't think hotjoin should be considered as a debug feature. The
problem here is the power consumption which is higher if you enable
this feature (you need to keep everything clocked and ready to handle
an IBI) whereas if your design is "fixed" (more like an I2C bus) you
may save power by disabling this feature.

A module parameter does not fit here because it's a per-bus
configuration.

Thanks,
Miquèl
Frank Li Oct. 20, 2023, 2:20 p.m. UTC | #3 
On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> Hi Lukwinski,
> 
> zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
> +0200:
> 
> > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > feature.
> > >
> > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > Add api i3c_master_enable(disable)_hotjoin();  
> > 
> > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> 
> I don't think hotjoin should be considered as a debug feature. The
> problem here is the power consumption which is higher if you enable
> this feature (you need to keep everything clocked and ready to handle
> an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> may save power by disabling this feature.
> 
> A module parameter does not fit here because it's a per-bus
> configuration.

I agree. sys entry is more flexiable. and let controller choose better
power saving policy for difference user case.

Frank

> 
> Thanks,
> Miquèl
Miquel Raynal Oct. 20, 2023, 2:33 p.m. UTC | #4 
Hi Frank,

Frank.li@nxp.com wrote on Fri, 20 Oct 2023 10:20:57 -0400:

> On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > Hi Lukwinski,
> > 
> > zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
> > +0200:
> >   
> > > On 10/18/2023 10:59 PM, Frank Li wrote:  
> > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > feature.
> > > >
> > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > Add api i3c_master_enable(disable)_hotjoin();    
> > > 
> > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?  
> > 
> > I don't think hotjoin should be considered as a debug feature. The
> > problem here is the power consumption which is higher if you enable
> > this feature (you need to keep everything clocked and ready to handle
> > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > may save power by disabling this feature.
> > 
> > A module parameter does not fit here because it's a per-bus
> > configuration.  
> 
> I agree. sys entry is more flexiable. and let controller choose better
> power saving policy for difference user case.

Maybe it's not the first time this case is faced, would you mind
including power management maintainers in this discussion? Perhaps they
might have pointers or even have the solution already.

Thanks,
Miquèl
Frank Li Oct. 20, 2023, 4:24 p.m. UTC | #5 
On Fri, Oct 20, 2023 at 04:33:48PM +0200, Miquel Raynal wrote:
> Hi Frank,
> 
> Frank.li@nxp.com wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> 
> > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > Hi Lukwinski,
> > > 
> > > zbigniew.lukwinski@linux.intel.com wrote on Fri, 20 Oct 2023 10:55:27
> > > +0200:
> > >   
> > > > On 10/18/2023 10:59 PM, Frank Li wrote:  
> > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > feature.
> > > > >
> > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > Add api i3c_master_enable(disable)_hotjoin();    
> > > > 
> > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?  
> > > 
> > > I don't think hotjoin should be considered as a debug feature. The
> > > problem here is the power consumption which is higher if you enable
> > > this feature (you need to keep everything clocked and ready to handle
> > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > may save power by disabling this feature.
> > > 
> > > A module parameter does not fit here because it's a per-bus
> > > configuration.  
> > 
> > I agree. sys entry is more flexiable. and let controller choose better
> > power saving policy for difference user case.
> 
> Maybe it's not the first time this case is faced, would you mind
> including power management maintainers in this discussion? Perhaps they
> might have pointers or even have the solution already.

@Ulf and pm experts.

I3C have a features, which call hotjoin. Some controller need enable clock
and some power domain if support hotjoin.

there are two kinds user case.

case1:  All devices attached into I3C bus, not hotjoin happen. So
controller can use runtime_pm frame to make clock and power domain on only
when transferring data.

case2: some devices can dynamitc join when system running. Some clocks or
power domain need be kept to make hotjoin event detect logic work.

In one system, two cases may exist at same time. I3C bus1 for case1, I3C
bus2 for case 2. 

I add sys entry in I3C bus driver to let user can turn on/off this feature
for specific controller. 

My question: any exited solution can handle these in current power
mnanagement system. 

Frank

> 
> Thanks,
> Miquèl
Alexandre Belloni Oct. 20, 2023, 8:25 p.m. UTC | #6 
On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
> On 10/20/2023 4:33 PM, Miquel Raynal wrote:
> > Hi Frank,
> > 
> > Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> > 
> > > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > > Hi Lukwinski,
> > > > 
> > > > zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
> > > > +0200:
> > > > > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > > feature.
> > > > > > 
> > > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > > Add api i3c_master_enable(disable)_hotjoin();
> > > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> > > > I don't think hotjoin should be considered as a debug feature. The
> > > > problem here is the power consumption which is higher if you enable
> > > > this feature (you need to keep everything clocked and ready to handle
> > > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > > may save power by disabling this feature.
> > > > 
> > > > A module parameter does not fit here because it's a per-bus
> > > > configuration.
> > > I agree. sys entry is more flexiable. and let controller choose better
> > > power saving policy for difference user case.
> > Maybe it's not the first time this case is faced, would you mind
> > including power management maintainers in this discussion? Perhaps they
> > might have pointers or even have the solution already.
> 
> I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
> to me like debug option.
> 
> So the flow you are considering here is like this:?
> 
>     1. system boot with HJ enabled, so HJ works during initial bus
>        discovery
>     2. some entity in user space decides to disable HJ because power
>        consumption?
>     3. some entity in use space decide some time later to re-enable HJ
>        because some reason?
> 
> I am just wondering whether there is real use case when you starts with HJ
> enabled and than disable it
> 
> in runtime or start with HJ disabled and enable it in runtime. If you are
> taking care about power saving
> 
>  let's keep HJ disabled all the time. Default state for HJ could be
> controlled by DT entry.
> 

This would be HW configuration and not HW description.


> 
> > Thanks,
> > Miquèl
Frank Li Oct. 20, 2023, 8:36 p.m. UTC | #7 
On Fri, Oct 20, 2023 at 10:25:19PM +0200, Alexandre Belloni wrote:
> On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
> > On 10/20/2023 4:33 PM, Miquel Raynal wrote:
> > > Hi Frank,
> > > 
> > > Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> > > 
> > > > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > > > Hi Lukwinski,
> > > > > 
> > > > > zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
> > > > > +0200:
> > > > > > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > > > feature.
> > > > > > > 
> > > > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > > > Add api i3c_master_enable(disable)_hotjoin();
> > > > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> > > > > I don't think hotjoin should be considered as a debug feature. The
> > > > > problem here is the power consumption which is higher if you enable
> > > > > this feature (you need to keep everything clocked and ready to handle
> > > > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > > > may save power by disabling this feature.
> > > > > 
> > > > > A module parameter does not fit here because it's a per-bus
> > > > > configuration.
> > > > I agree. sys entry is more flexiable. and let controller choose better
> > > > power saving policy for difference user case.
> > > Maybe it's not the first time this case is faced, would you mind
> > > including power management maintainers in this discussion? Perhaps they
> > > might have pointers or even have the solution already.
> > 
> > I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
> > to me like debug option.
> > 
> > So the flow you are considering here is like this:?
> > 
> >     1. system boot with HJ enabled, so HJ works during initial bus
> >        discovery
> >     2. some entity in user space decides to disable HJ because power
> >        consumption?
> >     3. some entity in use space decide some time later to re-enable HJ
> >        because some reason?
> > 
> > I am just wondering whether there is real use case when you starts with HJ
> > enabled and than disable it

I think it is validate user case. Assume a I3C GPS device, user only use
it when open map. Before map application open, enable i3c hotjoin and
power on GPS module. After map application close, disable i3c hotjoin and
power off GPS module.

Frank

> > 
> > in runtime or start with HJ disabled and enable it in runtime. If you are
> > taking care about power saving
> > 
> >  let's keep HJ disabled all the time. Default state for HJ could be
> > controlled by DT entry.
> > 
> 
> This would be HW configuration and not HW description.

Yes, DT maintainer may not accept this entry because it is not HW
description.

> 
> 
> > 
> > > Thanks,
> > > Miquèl
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Zbigniew, Lukwinski Oct. 20, 2023, 8:42 p.m. UTC | #8 
On 10/20/2023 10:36 PM, Frank Li wrote:
> On Fri, Oct 20, 2023 at 10:25:19PM +0200, Alexandre Belloni wrote:
>> On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
>>> On 10/20/2023 4:33 PM, Miquel Raynal wrote:
>>>> Hi Frank,
>>>>
>>>> Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
>>>>
>>>>> On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
>>>>>> Hi Lukwinski,
>>>>>>
>>>>>> zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
>>>>>> +0200:
>>>>>>> On 10/18/2023 10:59 PM, Frank Li wrote:
>>>>>>>> Add hotjoin entry in sys file system allow user enable/disable hotjoin
>>>>>>>> feature.
>>>>>>>>
>>>>>>>> Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
>>>>>>>> Add api i3c_master_enable(disable)_hotjoin();
>>>>>>> What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
>>>>>> I don't think hotjoin should be considered as a debug feature. The
>>>>>> problem here is the power consumption which is higher if you enable
>>>>>> this feature (you need to keep everything clocked and ready to handle
>>>>>> an IBI) whereas if your design is "fixed" (more like an I2C bus) you
>>>>>> may save power by disabling this feature.
>>>>>>
>>>>>> A module parameter does not fit here because it's a per-bus
>>>>>> configuration.
>>>>> I agree. sys entry is more flexiable. and let controller choose better
>>>>> power saving policy for difference user case.
>>>> Maybe it's not the first time this case is faced, would you mind
>>>> including power management maintainers in this discussion? Perhaps they
>>>> might have pointers or even have the solution already.
>>>
>>> I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
>>> to me like debug option.
>>>
>>> So the flow you are considering here is like this:?
>>>
>>>      1. system boot with HJ enabled, so HJ works during initial bus
>>>         discovery
>>>      2. some entity in user space decides to disable HJ because power
>>>         consumption?
>>>      3. some entity in use space decide some time later to re-enable HJ
>>>         because some reason?
>>>
>>> I am just wondering whether there is real use case when you starts with HJ
>>> enabled and than disable it
> 
> I think it is validate user case. Assume a I3C GPS device, user only use
> it when open map. Before map application open, enable i3c hotjoin and
> power on GPS module. After map application close, disable i3c hotjoin and
> power off GPS module.
> 
Got it. I think we are on the same page. Thanks for explanations!
> Frank
> 
>>>
>>> in runtime or start with HJ disabled and enable it in runtime. If you are
>>> taking care about power saving
>>>
>>>   let's keep HJ disabled all the time. Default state for HJ could be
>>> controlled by DT entry.
>>>
>>
>> This would be HW configuration and not HW description.
> 
> Yes, DT maintainer may not accept this entry because it is not HW
> description.
> 
Sure. Makes sense.
>>
>>
>>>
>>>> Thanks,
>>>> Miquèl
>>
>> -- 
>> Alexandre Belloni, co-owner and COO, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
Frank Li Nov. 5, 2023, 3:36 p.m. UTC | #9 
On Fri, Oct 20, 2023 at 10:42:20PM +0200, Zbigniew, Lukwinski wrote:
> On 10/20/2023 10:36 PM, Frank Li wrote:
> > On Fri, Oct 20, 2023 at 10:25:19PM +0200, Alexandre Belloni wrote:
> > > On 20/10/2023 17:12:53+0200, Zbigniew, Lukwinski wrote:
> > > > On 10/20/2023 4:33 PM, Miquel Raynal wrote:
> > > > > Hi Frank,
> > > > > 
> > > > > Frank.li@nxp.com  wrote on Fri, 20 Oct 2023 10:20:57 -0400:
> > > > > 
> > > > > > On Fri, Oct 20, 2023 at 03:45:28PM +0200, Miquel Raynal wrote:
> > > > > > > Hi Lukwinski,
> > > > > > > 
> > > > > > > zbigniew.lukwinski@linux.intel.com  wrote on Fri, 20 Oct 2023 10:55:27
> > > > > > > +0200:
> > > > > > > > On 10/18/2023 10:59 PM, Frank Li wrote:
> > > > > > > > > Add hotjoin entry in sys file system allow user enable/disable hotjoin
> > > > > > > > > feature.
> > > > > > > > > 
> > > > > > > > > Add (*enable(disable)_hotjoin)() to i3c_master_controller_ops.
> > > > > > > > > Add api i3c_master_enable(disable)_hotjoin();
> > > > > > > > What is the use case for having HJ enable knob in sysfs available for user space other than for debug stuff? In other words, does user space really need to enable/disable HJ in runtime for other reason but debug? If it is only for debug maybe it  could be move to debugFS?
> > > > > > > I don't think hotjoin should be considered as a debug feature. The
> > > > > > > problem here is the power consumption which is higher if you enable
> > > > > > > this feature (you need to keep everything clocked and ready to handle
> > > > > > > an IBI) whereas if your design is "fixed" (more like an I2C bus) you
> > > > > > > may save power by disabling this feature.
> > > > > > > 
> > > > > > > A module parameter does not fit here because it's a per-bus
> > > > > > > configuration.
> > > > > > I agree. sys entry is more flexiable. and let controller choose better
> > > > > > power saving policy for difference user case.
> > > > > Maybe it's not the first time this case is faced, would you mind
> > > > > including power management maintainers in this discussion? Perhaps they
> > > > > might have pointers or even have the solution already.
> > > > 
> > > > I did not mind HJ as debug feature. But enabling / disabling the HJ sounds
> > > > to me like debug option.
> > > > 
> > > > So the flow you are considering here is like this:?
> > > > 
> > > >      1. system boot with HJ enabled, so HJ works during initial bus
> > > >         discovery
> > > >      2. some entity in user space decides to disable HJ because power
> > > >         consumption?
> > > >      3. some entity in use space decide some time later to re-enable HJ
> > > >         because some reason?
> > > > 
> > > > I am just wondering whether there is real use case when you starts with HJ
> > > > enabled and than disable it
> > 
> > I think it is validate user case. Assume a I3C GPS device, user only use
> > it when open map. Before map application open, enable i3c hotjoin and
> > power on GPS module. After map application close, disable i3c hotjoin and
> > power off GPS module.
> > 
> Got it. I think we are on the same page. Thanks for explanations!
> > Frank

Does everyone agree on this method? If still need further discusion, I
can repost patch 3-6, which is independent on these.

Frank Li 

> > 
> > > > 
> > > > in runtime or start with HJ disabled and enable it in runtime. If you are
> > > > taking care about power saving
> > > > 
> > > >   let's keep HJ disabled all the time. Default state for HJ could be
> > > > controlled by DT entry.
> > > > 
> > > 
> > > This would be HW configuration and not HW description.
> > 
> > Yes, DT maintainer may not accept this entry because it is not HW
> > description.
> > 
> Sure. Makes sense.
> > > 
> > > 
> > > > 
> > > > > Thanks,
> > > > > Miquèl
> > > 
> > > -- 
> > > Alexandre Belloni, co-owner and COO, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
>
diff mbox series

Patch

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 08aeb69a78003..ed5e27cd20811 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -526,6 +526,89 @@  static ssize_t i2c_scl_frequency_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(i2c_scl_frequency);
 
+static int i3c_set_hotjoin(struct i3c_master_controller *master, bool enable)
+{
+	int ret;
+
+	if (!master ||
+	    !master->ops ||
+	    !master->ops->enable_hotjoin ||
+	    !master->ops->disable_hotjoin
+	   )
+		return -EINVAL;
+
+	i3c_bus_normaluse_lock(&master->bus);
+
+	if (enable)
+		ret = master->ops->enable_hotjoin(master);
+	else
+		ret = master->ops->disable_hotjoin(master);
+
+	master->hotjoin = enable;
+
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	return ret;
+}
+
+static ssize_t hotjoin_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	int ret;
+	long res;
+
+	if (!i3cbus->cur_master)
+		return -EINVAL;
+
+	if (kstrtol(buf, 10, &res))
+		return -EINVAL;
+
+	ret = i3c_set_hotjoin(i3cbus->cur_master->common.master, !!res);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+/*
+ * i3c_master_enable_hotjoin - Enable hotjoin
+ * @master: I3C master object
+ *
+ * Return: a 0 in case of success, an negative error code otherwise.
+ */
+int i3c_master_enable_hotjoin(struct i3c_master_controller *master)
+{
+	return i3c_set_hotjoin(master, true);
+}
+EXPORT_SYMBOL_GPL(i3c_master_enable_hotjoin);
+
+/*
+ * i3c_master_disable_hotjoin - Disable hotjoin
+ * @master: I3C master object
+ *
+ * Return: a 0 in case of success, an negative error code otherwise.
+ */
+int i3c_master_disable_hotjoin(struct i3c_master_controller *master)
+{
+	return i3c_set_hotjoin(master, false);
+}
+EXPORT_SYMBOL_GPL(i3c_master_disable_hotjoin);
+
+static ssize_t hotjoin_show(struct device *dev, struct device_attribute *da, char *buf)
+{
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+	ssize_t ret;
+
+	i3c_bus_normaluse_lock(i3cbus);
+	ret = sysfs_emit(buf, "%d\n", i3cbus->cur_master->common.master->hotjoin);
+	i3c_bus_normaluse_unlock(i3cbus);
+
+	return ret;
+}
+
+static DEVICE_ATTR_RW(hotjoin);
+
 static struct attribute *i3c_masterdev_attrs[] = {
 	&dev_attr_mode.attr,
 	&dev_attr_current_master.attr,
@@ -536,6 +619,7 @@  static struct attribute *i3c_masterdev_attrs[] = {
 	&dev_attr_pid.attr,
 	&dev_attr_dynamic_address.attr,
 	&dev_attr_hdrcap.attr,
+	&dev_attr_hotjoin.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(i3c_masterdev);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 0b52da4f23467..65b8965968af2 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -452,6 +452,8 @@  struct i3c_master_controller_ops {
 	int (*disable_ibi)(struct i3c_dev_desc *dev);
 	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
 				 struct i3c_ibi_slot *slot);
+	int (*enable_hotjoin)(struct i3c_master_controller *master);
+	int (*disable_hotjoin)(struct i3c_master_controller *master);
 };
 
 /**
@@ -487,6 +489,7 @@  struct i3c_master_controller {
 	const struct i3c_master_controller_ops *ops;
 	unsigned int secondary : 1;
 	unsigned int init_done : 1;
+	unsigned int hotjoin: 1;
 	struct {
 		struct list_head i3c;
 		struct list_head i2c;
@@ -543,6 +546,8 @@  int i3c_master_register(struct i3c_master_controller *master,
 			const struct i3c_master_controller_ops *ops,
 			bool secondary);
 void i3c_master_unregister(struct i3c_master_controller *master);
+int i3c_master_enable_hotjoin(struct i3c_master_controller *master);
+int i3c_master_disable_hotjoin(struct i3c_master_controller *master);
 
 /**
  * i3c_dev_get_master_data() - get master private data attached to an I3C