On 12/01/2024 02:42, Tao Zhang wrote:
On 1/8/2024 6:42 PM, Suzuki K Poulose wrote:
On 05/01/2024 07:49, Tao Zhang wrote:
On 12/30/2023 5:39 PM, Suzuki K Poulose wrote:
On 25/12/2023 01:55, Tao Zhang wrote:
On 12/20/2023 7:07 PM, Suzuki K Poulose wrote:
On 20/12/2023 09:51, Tao Zhang wrote: > > On 12/19/2023 9:51 PM, Suzuki K Poulose wrote: >> On 19/12/2023 02:43, Tao Zhang wrote: >>> >>> On 12/18/2023 6:46 PM, Suzuki K Poulose wrote: >>>> On 21/11/2023 02:24, Tao Zhang wrote:
...
>>>>> char *buf) >>>>> { >>>>> struct tpdm_drvdata *drvdata = >>>>> dev_get_drvdata(dev->parent); >>>>> + ssize_t size = 0; >>>>> - return sysfs_emit(buf, "%u\n", >>>>> - (unsigned int)drvdata->dsb->patt_ts); >>>>> + if (tpdm_has_dsb_dataset(drvdata)) >>>>> + size = sysfs_emit(buf, "%u\n", >>>>> + (unsigned int)drvdata->dsb->patt_ts); >>>>> + >>>>> + if (tpdm_has_cmb_dataset(drvdata)) >>>>> + size = sysfs_emit(buf, "%u\n", >>>>> + (unsigned int)drvdata->cmb->patt_ts); >>>> >>>> Why does this need to show two values ? This must only show >>>> ONE value. >>>> How you deduce that might be based on the availability of the >>>> feature >>>> set. Or store the TS value in the drvdata and use that instead >>>> for >>>> controlling CMB/DSB. >>> >>> Since both of CMB/DSB need to have "enable_ts" SysFs file, can >>> I separate them >> >> The question really is, do we need fine grained control. i.e., >> >> enable TS for DSB but not for CMB or vice versa. >> >> I am not an expert on the usage scenario of the same. So, if >> you/Qcomm >> thinks the users need separate, fine grained control for timestamp >> for the DSB and CMB, then yes, follow your recommendation below. >> i.e., tpdm.../dsb_patt/enable_ts >> >>> as "enable_dsb_ts" and "enable_cmb_ts"? The path will be like >>> below. >>> >>> tpdm0/dsb_patt/enable_dsb_ts >> >> You don't need enable_dsb_ts. It could be "enable_ts" >> >>> >>> tpdm1/cmb_patt/enable_cmb_ts >>> >>> Is this design appropriate? >> >> >> Otherwise, stick to single enable_ts : which enables the ts for >> both >> CMB/DSB. And it only ever show one value : 0 (TS is disabled for >> both >> CMB/DSB) 1 : TS enabled for both. > > We have a very special case, such as the TPDM supporting both CMB > and > > DSB datasets. Although this case is very rare, it still exists. > > Can I use the data bit to instruct whether timestamp is enabled > for CMB/DSB or not? For example, > > size = sysfs_emit(buf, "%u\n", > (unsigned int)(drvdata->dsb->patt_ts << 1 | > drvdata->cmb->patt_ts)); > > Thus, this value can instruct the following situations. > > 0 - TS is disabled for both CMB/DSB > > 1 - TS is enabled for CMB > > 2 - TS is enabled for DSB > > 3 - TS is enabled for both > > Is this approach acceptable? >
No, please stick to separate controls for TS. Do not complicate the user interface.
i.e., tpdm0/dsb_patt/enable_ts tpdm0/cmb_patt/enable_ts
We need to be able to control/show dsb and cmb timestamp enablement independently.
Can we achieve this requirement if we use a sysfs file with the same name?
They are independent and in their respective directory (group) for CMB and DSB. What am I missing ? e.g., if you want to enable TS for DSB, you do :
$ echo 1 > dsb_patt/enable_ts
And that only works for DSB not for CMB.
We have a special case that the TPDM supports both DSB and CMB dataset. In this special case, when we
issue this command to enable timestamp, it will call enable_ts_store API, right?
if (tpdm_has_dsb_dataset(drvdata)) drvdata->dsb->patt_ts = !!val;
if (tpdm_has_cmb_dataset(drvdata)) drvdata->cmb->patt_ts = !!val;
I don't understand. If they both are under different subgroups, why should they be conflicting ? Are you not able to distinguish them, when you creat those attributes ? i.e., create two different "attributes" ?
Yes, although some TPDMs can support both CMB dataset and DSB dataset, we need to configure them separately
in some scenarios. Based on your suggestion, I want to use the following approach to implement it.
See below.
See below.
Since this special TPDM supports both DSB and CMB dataset, both DSB patt_ts and CMB patt_ts will be set
in this case even if I only configure the file in the DSB directory, right?
This is the problem we have now.
>>>> >>>> >>>>> + >>>>> + return size; >>>>> } >>>>> /* >>>>> @@ -715,8 +755,13 @@ static ssize_t enable_ts_store(struct >>>>> device *dev, >>>>> return -EINVAL; >>>>> spin_lock(&drvdata->spinlock); >>>>> - drvdata->dsb->patt_ts = !!val; >>>>> + if (tpdm_has_dsb_dataset(drvdata)) >>>>> + drvdata->dsb->patt_ts = !!val; >>>>> + >>>>> + if (tpdm_has_cmb_dataset(drvdata)) >>>>> + drvdata->cmb->patt_ts = !!val; >>>>> spin_unlock(&drvdata->spinlock); >>>>> + >>>>> return size; >>>>> } >>>>> static DEVICE_ATTR_RW(enable_ts);
Do not overload the same for both DSB and CMB. Create one for each in DSB and CMB ? They could share the same show/store routines, but could be done with additional variable to indicate which attribute they are controlling. Like the other attributes, using dev_ext_attribute or such.
New approach below, please help review to see if it is acceptable?
#define tpdm_patt_enable_ts_rw(name, mem) \ (&((struct tpdm_dataset_attribute[]) { \ { \ __ATTR(name, 0644, enable_ts_show, \ enable_ts_store), \ mem, \ } \ })[0].attr.attr)
#define DSB_PATT_ENABLE_TS \ tpdm_patt_enable_ts_rw(enable_ts, \ DSB_PATT)
#define CMB_PATT_ENABLE_TS \ tpdm_patt_enable_ts_rw(enable_ts, \ CMB_PATT)
static ssize_t enable_ts_show(struct device *dev, struct device_attribute *attr, char *buf) { struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); struct tpdm_dataset_attribute *tpdm_attr = container_of(attr, struct tpdm_dataset_attribute, attr); ssize_t size = 0;
if (tpdm_attr->mem == DSB_PATT) { size = sysfs_emit(buf, "%u\n", (unsigned int)drvdata->dsb->patt_ts); } else if (tpdm_attr->mem == CMB_PATT) { size = sysfs_emit(buf, "%u\n", (unsigned int)drvdata->cmb->patt_ts); } else return -EINVAL;
return size; }
static ssize_t enable_ts_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size) { struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); struct tpdm_dataset_attribute *tpdm_attr = container_of(attr, struct tpdm_dataset_attribute, attr); unsigned long val;
if ((kstrtoul(buf, 0, &val)) || (val & ~1UL)) return -EINVAL;
spin_lock(&drvdata->spinlock); if (tpdm_attr->mem == DSB_PATT) { drvdata->dsb->patt_ts = !!val; } else if (tpdm_attr->mem == CMB_PATT) { drvdata->cmb->patt_ts = !!val; } else return -EINVAL; spin_unlock(&drvdata->spinlock);
return size; }
Yes, that is what I had in mind.
Thanks Suzuki
Best,
Tao
Suzuki