Register DPLL sub-devices to expose the functionality provided
by ZL3073x chip family. Each sub-device represents one of
the available DPLL channels.
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v6->v7:
* use platform data to pass the channel to use
v4->v6:
* no change
v3->v4:
* use static mfd cells
---
drivers/mfd/zl3073x-core.c | 30 ++++++++++++++++++++++++++++++
include/linux/mfd/zl3073x.h | 9 +++++++++
2 files changed, 39 insertions(+)
diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index 0bea696a46b8..ebbad87354fd 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -7,6 +7,7 @@
#include <linux/device.h>
#include <linux/export.h>
#include <linux/math64.h>
+#include <linux/mfd/core.h>
#include <linux/mfd/zl3073x.h>
#include <linux/module.h>
#include <linux/netlink.h>
@@ -755,6 +756,26 @@ static void zl3073x_devlink_unregister(void *ptr)
devlink_unregister(ptr);
}
+static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
+ { .channel = 0, },
+ { .channel = 1, },
+ { .channel = 2, },
+ { .channel = 3, },
+ { .channel = 4, },
+};
+
+#define ZL3073X_CELL(_name, _channel) \
+ MFD_CELL_BASIC(_name, NULL, &zl3073x_pdata[_channel], \
+ sizeof(struct zl3073x_pdata), 0)
+
+static const struct mfd_cell zl3073x_devs[] = {
+ ZL3073X_CELL("zl3073x-dpll", 0),
+ ZL3073X_CELL("zl3073x-dpll", 1),
+ ZL3073X_CELL("zl3073x-dpll", 2),
+ ZL3073X_CELL("zl3073x-dpll", 3),
+ ZL3073X_CELL("zl3073x-dpll", 4),
+};
+
/**
* zl3073x_dev_probe - initialize zl3073x device
* @zldev: pointer to zl3073x device
@@ -826,6 +847,15 @@ int zl3073x_dev_probe(struct zl3073x_dev *zldev,
if (rc)
return rc;
+ /* Add DPLL sub-device cell for each DPLL channel */
+ rc = devm_mfd_add_devices(zldev->dev, PLATFORM_DEVID_AUTO, zl3073x_devs,
+ chip_info->num_channels, NULL, 0, NULL);
+ if (rc) {
+ dev_err_probe(zldev->dev, rc,
+ "Failed to add DPLL sub-device\n");
+ return rc;
+ }
+
/* Register the device as devlink device */
devlink = priv_to_devlink(zldev);
devlink_register(devlink);
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
index 4dc68013b69f..cf4663cab72a 100644
--- a/include/linux/mfd/zl3073x.h
+++ b/include/linux/mfd/zl3073x.h
@@ -12,6 +12,7 @@ struct regmap;
/*
* Hardware limits for ZL3073x chip family
*/
+#define ZL3073X_MAX_CHANNELS 5
#define ZL3073X_NUM_INPUTS 10
#define ZL3073X_NUM_OUTPUTS 10
#define ZL3073X_NUM_SYNTHS 5
@@ -48,6 +49,14 @@ struct zl3073x_synth {
u8 dpll;
};
+/**
+ * struct zl3073x_pdata - zl3073x sub-device platform data
+ * @channel: channel to use
+ */
+struct zl3073x_pdata {
+ u8 channel;
+};
+
/**
* struct zl3073x_dev - zl3073x device
* @dev: pointer to device
--
2.49.0On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>
> Register DPLL sub-devices to expose the functionality provided
> by ZL3073x chip family. Each sub-device represents one of
> the available DPLL channels.
...
> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> + { .channel = 0, },
> + { .channel = 1, },
> + { .channel = 2, },
> + { .channel = 3, },
> + { .channel = 4, },
> +};
> +static const struct mfd_cell zl3073x_devs[] = {
> + ZL3073X_CELL("zl3073x-dpll", 0),
> + ZL3073X_CELL("zl3073x-dpll", 1),
> + ZL3073X_CELL("zl3073x-dpll", 2),
> + ZL3073X_CELL("zl3073x-dpll", 3),
> + ZL3073X_CELL("zl3073x-dpll", 4),
> +};
> +#define ZL3073X_MAX_CHANNELS 5
Btw, wouldn't be better to keep the above lists synchronised like
1. Make ZL3073X_CELL() to use indexed variant
[idx] = ...
2. Define the channel numbers
and use them in both data structures.
...
OTOH, I'm not sure why we even need this. If this is going to be
sequential, can't we make a core to decide which cell will be given
which id?
--
With Best Regards,
Andy Shevchenko
On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>
>> Register DPLL sub-devices to expose the functionality provided
>> by ZL3073x chip family. Each sub-device represents one of
>> the available DPLL channels.
>
> ...
>
>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>> + { .channel = 0, },
>> + { .channel = 1, },
>> + { .channel = 2, },
>> + { .channel = 3, },
>> + { .channel = 4, },
>> +};
>
>> +static const struct mfd_cell zl3073x_devs[] = {
>> + ZL3073X_CELL("zl3073x-dpll", 0),
>> + ZL3073X_CELL("zl3073x-dpll", 1),
>> + ZL3073X_CELL("zl3073x-dpll", 2),
>> + ZL3073X_CELL("zl3073x-dpll", 3),
>> + ZL3073X_CELL("zl3073x-dpll", 4),
>> +};
>
>> +#define ZL3073X_MAX_CHANNELS 5
>
> Btw, wouldn't be better to keep the above lists synchronised like
>
> 1. Make ZL3073X_CELL() to use indexed variant
>
> [idx] = ...
>
> 2. Define the channel numbers
>
> and use them in both data structures.
>
It could be possible to drop zl3073x_pdata array and modify ZL3073X_CELL
this way:
#define ZL3073X_CHANNEL(_channel) \
&(const struct zl3073x_pdata) { .channel = _channel }
#define ZL3073X_CELL(_name, _channel) \
MFD_CELL_BASIC(_name, NULL, ZL3073X_CHANNEL(_channel), \
sizeof(struct zl3073x_pdata), 0)
WDYT?
Ivan
On Wed, May 07, 2025 at 04:19:29PM +0200, Ivan Vecera wrote:
> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> > On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
...
> > > +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > + { .channel = 0, },
> > > + { .channel = 1, },
> > > + { .channel = 2, },
> > > + { .channel = 3, },
> > > + { .channel = 4, },
> > > +};
> >
> > > +static const struct mfd_cell zl3073x_devs[] = {
> > > + ZL3073X_CELL("zl3073x-dpll", 0),
> > > + ZL3073X_CELL("zl3073x-dpll", 1),
> > > + ZL3073X_CELL("zl3073x-dpll", 2),
> > > + ZL3073X_CELL("zl3073x-dpll", 3),
> > > + ZL3073X_CELL("zl3073x-dpll", 4),
> > > +};
> >
> > > +#define ZL3073X_MAX_CHANNELS 5
> >
> > Btw, wouldn't be better to keep the above lists synchronised like
> >
> > 1. Make ZL3073X_CELL() to use indexed variant
> >
> > [idx] = ...
> >
> > 2. Define the channel numbers
> >
> > and use them in both data structures.
> >
> It could be possible to drop zl3073x_pdata array and modify ZL3073X_CELL
> this way:
>
> #define ZL3073X_CHANNEL(_channel) \
> &(const struct zl3073x_pdata) { .channel = _channel }
>
> #define ZL3073X_CELL(_name, _channel) \
> MFD_CELL_BASIC(_name, NULL, ZL3073X_CHANNEL(_channel), \
> sizeof(struct zl3073x_pdata), 0)
>
> WDYT?
Fine with me as it looks not ugly and addresses my point.
--
With Best Regards,
Andy Shevchenko
On 07. 05. 25 5:00 odp., Andy Shevchenko wrote:
> On Wed, May 07, 2025 at 04:19:29PM +0200, Ivan Vecera wrote:
>> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
>>> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>
> ...
>
>>>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>> + { .channel = 0, },
>>>> + { .channel = 1, },
>>>> + { .channel = 2, },
>>>> + { .channel = 3, },
>>>> + { .channel = 4, },
>>>> +};
>>>
>>>> +static const struct mfd_cell zl3073x_devs[] = {
>>>> + ZL3073X_CELL("zl3073x-dpll", 0),
>>>> + ZL3073X_CELL("zl3073x-dpll", 1),
>>>> + ZL3073X_CELL("zl3073x-dpll", 2),
>>>> + ZL3073X_CELL("zl3073x-dpll", 3),
>>>> + ZL3073X_CELL("zl3073x-dpll", 4),
>>>> +};
>>>
>>>> +#define ZL3073X_MAX_CHANNELS 5
>>>
>>> Btw, wouldn't be better to keep the above lists synchronised like
>>>
>>> 1. Make ZL3073X_CELL() to use indexed variant
>>>
>>> [idx] = ...
>>>
>>> 2. Define the channel numbers
>>>
>>> and use them in both data structures.
>>>
>> It could be possible to drop zl3073x_pdata array and modify ZL3073X_CELL
>> this way:
>>
>> #define ZL3073X_CHANNEL(_channel) \
>> &(const struct zl3073x_pdata) { .channel = _channel }
>>
>> #define ZL3073X_CELL(_name, _channel) \
>> MFD_CELL_BASIC(_name, NULL, ZL3073X_CHANNEL(_channel), \
>> sizeof(struct zl3073x_pdata), 0)
>>
>> WDYT?
>
> Fine with me as it looks not ugly and addresses my point.
>
Will submit v8 shortly..
Thanks for review and advice.
Ivan
On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>
>> Register DPLL sub-devices to expose the functionality provided
>> by ZL3073x chip family. Each sub-device represents one of
>> the available DPLL channels.
>
> ...
>
>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>> + { .channel = 0, },
>> + { .channel = 1, },
>> + { .channel = 2, },
>> + { .channel = 3, },
>> + { .channel = 4, },
>> +};
>
>> +static const struct mfd_cell zl3073x_devs[] = {
>> + ZL3073X_CELL("zl3073x-dpll", 0),
>> + ZL3073X_CELL("zl3073x-dpll", 1),
>> + ZL3073X_CELL("zl3073x-dpll", 2),
>> + ZL3073X_CELL("zl3073x-dpll", 3),
>> + ZL3073X_CELL("zl3073x-dpll", 4),
>> +};
>
>> +#define ZL3073X_MAX_CHANNELS 5
>
> Btw, wouldn't be better to keep the above lists synchronised like
>
> 1. Make ZL3073X_CELL() to use indexed variant
>
> [idx] = ...
>
> 2. Define the channel numbers
>
> and use them in both data structures.
>
> ...
WDYM?
> OTOH, I'm not sure why we even need this. If this is going to be
> sequential, can't we make a core to decide which cell will be given
> which id?
Just a note that after introduction of PHC sub-driver the array will
look like:
static const struct mfd_cell zl3073x_devs[] = {
ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
ZL3073X_CELL("zl3073x-dpll", 1), // ...
ZL3073X_CELL("zl3073x-phc", 1),
ZL3073X_CELL("zl3073x-dpll", 2),
ZL3073X_CELL("zl3073x-phc", 2),
ZL3073X_CELL("zl3073x-dpll", 3),
ZL3073X_CELL("zl3073x-phc", 3),
ZL3073X_CELL("zl3073x-dpll", 4),
ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
};
Ivan
On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> > On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
...
> > > +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > + { .channel = 0, },
> > > + { .channel = 1, },
> > > + { .channel = 2, },
> > > + { .channel = 3, },
> > > + { .channel = 4, },
> > > +};
> >
> > > +static const struct mfd_cell zl3073x_devs[] = {
> > > + ZL3073X_CELL("zl3073x-dpll", 0),
> > > + ZL3073X_CELL("zl3073x-dpll", 1),
> > > + ZL3073X_CELL("zl3073x-dpll", 2),
> > > + ZL3073X_CELL("zl3073x-dpll", 3),
> > > + ZL3073X_CELL("zl3073x-dpll", 4),
> > > +};
> >
> > > +#define ZL3073X_MAX_CHANNELS 5
> >
> > Btw, wouldn't be better to keep the above lists synchronised like
> >
> > 1. Make ZL3073X_CELL() to use indexed variant
> >
> > [idx] = ...
> >
> > 2. Define the channel numbers
> >
> > and use them in both data structures.
> >
> > ...
>
> WDYM?
>
> > OTOH, I'm not sure why we even need this. If this is going to be
> > sequential, can't we make a core to decide which cell will be given
> > which id?
>
> Just a note that after introduction of PHC sub-driver the array will look
> like:
> static const struct mfd_cell zl3073x_devs[] = {
> ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
> ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
> ZL3073X_CELL("zl3073x-dpll", 1), // ...
> ZL3073X_CELL("zl3073x-phc", 1),
> ZL3073X_CELL("zl3073x-dpll", 2),
> ZL3073X_CELL("zl3073x-phc", 2),
> ZL3073X_CELL("zl3073x-dpll", 3),
> ZL3073X_CELL("zl3073x-phc", 3),
> ZL3073X_CELL("zl3073x-dpll", 4),
> ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
> };
Ah, this is very important piece. Then I mean only this kind of change
enum {
// this or whatever meaningful names
..._CH_0 0
..._CH_1 1
...
};
static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
{ .channel = ..._CH_0, },
...
};
static const struct mfd_cell zl3073x_devs[] = {
ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
ZL3073X_CELL("zl3073x-phc", ..._CH_0),
...
};
--
With Best Regards,
Andy Shevchenko
On Wed, 07 May 2025, Andy Shevchenko wrote:
> On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
> > On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> > > On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>
> ...
>
> > > > +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > + { .channel = 0, },
> > > > + { .channel = 1, },
> > > > + { .channel = 2, },
> > > > + { .channel = 3, },
> > > > + { .channel = 4, },
> > > > +};
> > >
> > > > +static const struct mfd_cell zl3073x_devs[] = {
> > > > + ZL3073X_CELL("zl3073x-dpll", 0),
> > > > + ZL3073X_CELL("zl3073x-dpll", 1),
> > > > + ZL3073X_CELL("zl3073x-dpll", 2),
> > > > + ZL3073X_CELL("zl3073x-dpll", 3),
> > > > + ZL3073X_CELL("zl3073x-dpll", 4),
> > > > +};
> > >
> > > > +#define ZL3073X_MAX_CHANNELS 5
> > >
> > > Btw, wouldn't be better to keep the above lists synchronised like
> > >
> > > 1. Make ZL3073X_CELL() to use indexed variant
> > >
> > > [idx] = ...
> > >
> > > 2. Define the channel numbers
> > >
> > > and use them in both data structures.
> > >
> > > ...
> >
> > WDYM?
> >
> > > OTOH, I'm not sure why we even need this. If this is going to be
> > > sequential, can't we make a core to decide which cell will be given
> > > which id?
> >
> > Just a note that after introduction of PHC sub-driver the array will look
> > like:
> > static const struct mfd_cell zl3073x_devs[] = {
> > ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
> > ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
> > ZL3073X_CELL("zl3073x-dpll", 1), // ...
> > ZL3073X_CELL("zl3073x-phc", 1),
> > ZL3073X_CELL("zl3073x-dpll", 2),
> > ZL3073X_CELL("zl3073x-phc", 2),
> > ZL3073X_CELL("zl3073x-dpll", 3),
> > ZL3073X_CELL("zl3073x-phc", 3),
> > ZL3073X_CELL("zl3073x-dpll", 4),
> > ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
> > };
>
> Ah, this is very important piece. Then I mean only this kind of change
>
> enum {
> // this or whatever meaningful names
> ..._CH_0 0
> ..._CH_1 1
> ...
> };
>
> static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> { .channel = ..._CH_0, },
> ...
> };
>
> static const struct mfd_cell zl3073x_devs[] = {
> ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
> ZL3073X_CELL("zl3073x-phc", ..._CH_0),
> ...
> };
This is getting hectic. All for a sequential enumeration. Seeing as
there are no other differentiations, why not use IDA in the child
instead?
--
Lee Jones [李琼斯]
On 07. 05. 25 5:26 odp., Lee Jones wrote:
> On Wed, 07 May 2025, Andy Shevchenko wrote:
>
>> On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
>>> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
>>>> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>
>> ...
>>
>>>>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>> + { .channel = 0, },
>>>>> + { .channel = 1, },
>>>>> + { .channel = 2, },
>>>>> + { .channel = 3, },
>>>>> + { .channel = 4, },
>>>>> +};
>>>>
>>>>> +static const struct mfd_cell zl3073x_devs[] = {
>>>>> + ZL3073X_CELL("zl3073x-dpll", 0),
>>>>> + ZL3073X_CELL("zl3073x-dpll", 1),
>>>>> + ZL3073X_CELL("zl3073x-dpll", 2),
>>>>> + ZL3073X_CELL("zl3073x-dpll", 3),
>>>>> + ZL3073X_CELL("zl3073x-dpll", 4),
>>>>> +};
>>>>
>>>>> +#define ZL3073X_MAX_CHANNELS 5
>>>>
>>>> Btw, wouldn't be better to keep the above lists synchronised like
>>>>
>>>> 1. Make ZL3073X_CELL() to use indexed variant
>>>>
>>>> [idx] = ...
>>>>
>>>> 2. Define the channel numbers
>>>>
>>>> and use them in both data structures.
>>>>
>>>> ...
>>>
>>> WDYM?
>>>
>>>> OTOH, I'm not sure why we even need this. If this is going to be
>>>> sequential, can't we make a core to decide which cell will be given
>>>> which id?
>>>
>>> Just a note that after introduction of PHC sub-driver the array will look
>>> like:
>>> static const struct mfd_cell zl3073x_devs[] = {
>>> ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
>>> ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
>>> ZL3073X_CELL("zl3073x-dpll", 1), // ...
>>> ZL3073X_CELL("zl3073x-phc", 1),
>>> ZL3073X_CELL("zl3073x-dpll", 2),
>>> ZL3073X_CELL("zl3073x-phc", 2),
>>> ZL3073X_CELL("zl3073x-dpll", 3),
>>> ZL3073X_CELL("zl3073x-phc", 3),
>>> ZL3073X_CELL("zl3073x-dpll", 4),
>>> ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
>>> };
>>
>> Ah, this is very important piece. Then I mean only this kind of change
>>
>> enum {
>> // this or whatever meaningful names
>> ..._CH_0 0
>> ..._CH_1 1
>> ...
>> };
>>
>> static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>> { .channel = ..._CH_0, },
>> ...
>> };
>>
>> static const struct mfd_cell zl3073x_devs[] = {
>> ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
>> ZL3073X_CELL("zl3073x-phc", ..._CH_0),
>> ...
>> };
>
> This is getting hectic. All for a sequential enumeration. Seeing as
> there are no other differentiations, why not use IDA in the child
> instead?
For that, there have to be two IDAs, one for DPLLs and one for PHCs...
The approach in my second reply in this thread is simpler and taken
in v8.
<cite>
+#define ZL3073X_PDATA(_channel) \
+ (&(const struct zl3073x_pdata) { \
+ .channel = _channel, \
+ })
+
+#define ZL3073X_CELL(_name, _channel) \
+ MFD_CELL_BASIC(_name, NULL, ZL3073X_PDATA(_channel), \
+ sizeof(struct zl3073x_pdata), 0)
+
+static const struct mfd_cell zl3073x_devs[] = {
+ ZL3073X_CELL("zl3073x-dpll", 0),
+ ZL3073X_CELL("zl3073x-dpll", 1),
+ ZL3073X_CELL("zl3073x-dpll", 2),
+ ZL3073X_CELL("zl3073x-dpll", 3),
+ ZL3073X_CELL("zl3073x-dpll", 4),
+};
</cite>
Lee, WDYT?
Thanks,
Ivan
On Mon, 12 May 2025, Ivan Vecera wrote:
> On 07. 05. 25 5:26 odp., Lee Jones wrote:
> > On Wed, 07 May 2025, Andy Shevchenko wrote:
> >
> > > On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
> > > > On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> > > > > On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
> > >
> > > ...
> > >
> > > > > > +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > > > + { .channel = 0, },
> > > > > > + { .channel = 1, },
> > > > > > + { .channel = 2, },
> > > > > > + { .channel = 3, },
> > > > > > + { .channel = 4, },
> > > > > > +};
> > > > >
> > > > > > +static const struct mfd_cell zl3073x_devs[] = {
> > > > > > + ZL3073X_CELL("zl3073x-dpll", 0),
> > > > > > + ZL3073X_CELL("zl3073x-dpll", 1),
> > > > > > + ZL3073X_CELL("zl3073x-dpll", 2),
> > > > > > + ZL3073X_CELL("zl3073x-dpll", 3),
> > > > > > + ZL3073X_CELL("zl3073x-dpll", 4),
> > > > > > +};
> > > > >
> > > > > > +#define ZL3073X_MAX_CHANNELS 5
> > > > >
> > > > > Btw, wouldn't be better to keep the above lists synchronised like
> > > > >
> > > > > 1. Make ZL3073X_CELL() to use indexed variant
> > > > >
> > > > > [idx] = ...
> > > > >
> > > > > 2. Define the channel numbers
> > > > >
> > > > > and use them in both data structures.
> > > > >
> > > > > ...
> > > >
> > > > WDYM?
> > > >
> > > > > OTOH, I'm not sure why we even need this. If this is going to be
> > > > > sequential, can't we make a core to decide which cell will be given
> > > > > which id?
> > > >
> > > > Just a note that after introduction of PHC sub-driver the array will look
> > > > like:
> > > > static const struct mfd_cell zl3073x_devs[] = {
> > > > ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
> > > > ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
> > > > ZL3073X_CELL("zl3073x-dpll", 1), // ...
> > > > ZL3073X_CELL("zl3073x-phc", 1),
> > > > ZL3073X_CELL("zl3073x-dpll", 2),
> > > > ZL3073X_CELL("zl3073x-phc", 2),
> > > > ZL3073X_CELL("zl3073x-dpll", 3),
> > > > ZL3073X_CELL("zl3073x-phc", 3),
> > > > ZL3073X_CELL("zl3073x-dpll", 4),
> > > > ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
> > > > };
> > >
> > > Ah, this is very important piece. Then I mean only this kind of change
> > >
> > > enum {
> > > // this or whatever meaningful names
> > > ..._CH_0 0
> > > ..._CH_1 1
> > > ...
> > > };
> > >
> > > static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > { .channel = ..._CH_0, },
> > > ...
> > > };
> > >
> > > static const struct mfd_cell zl3073x_devs[] = {
> > > ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
> > > ZL3073X_CELL("zl3073x-phc", ..._CH_0),
> > > ...
> > > };
> >
> > This is getting hectic. All for a sequential enumeration. Seeing as
> > there are no other differentiations, why not use IDA in the child
> > instead?
>
> For that, there have to be two IDAs, one for DPLLs and one for PHCs...
Sorry, can you explain a bit more. Why is this a problem?
The IDA API is very simple.
Much better than building your own bespoke MACROs.
> The approach in my second reply in this thread is simpler and taken
> in v8.
>
> <cite>
> +#define ZL3073X_PDATA(_channel) \
> + (&(const struct zl3073x_pdata) { \
> + .channel = _channel, \
> + })
> +
> +#define ZL3073X_CELL(_name, _channel) \
> + MFD_CELL_BASIC(_name, NULL, ZL3073X_PDATA(_channel), \
> + sizeof(struct zl3073x_pdata), 0)
> +
> +static const struct mfd_cell zl3073x_devs[] = {
> + ZL3073X_CELL("zl3073x-dpll", 0),
> + ZL3073X_CELL("zl3073x-dpll", 1),
> + ZL3073X_CELL("zl3073x-dpll", 2),
> + ZL3073X_CELL("zl3073x-dpll", 3),
> + ZL3073X_CELL("zl3073x-dpll", 4),
> +};
> </cite>
>
> Lee, WDYT?
>
> Thanks,
> Ivan
>
--
Lee Jones [李琼斯]
On 13. 05. 25 11:41 dop., Lee Jones wrote:
> On Mon, 12 May 2025, Ivan Vecera wrote:
>
>> On 07. 05. 25 5:26 odp., Lee Jones wrote:
>>> On Wed, 07 May 2025, Andy Shevchenko wrote:
>>>
>>>> On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
>>>>> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
>>>>>> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>>>
>>>> ...
>>>>
>>>>>>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>>> + { .channel = 0, },
>>>>>>> + { .channel = 1, },
>>>>>>> + { .channel = 2, },
>>>>>>> + { .channel = 3, },
>>>>>>> + { .channel = 4, },
>>>>>>> +};
>>>>>>
>>>>>>> +static const struct mfd_cell zl3073x_devs[] = {
>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 0),
>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 1),
>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>> +};
>>>>>>
>>>>>>> +#define ZL3073X_MAX_CHANNELS 5
>>>>>>
>>>>>> Btw, wouldn't be better to keep the above lists synchronised like
>>>>>>
>>>>>> 1. Make ZL3073X_CELL() to use indexed variant
>>>>>>
>>>>>> [idx] = ...
>>>>>>
>>>>>> 2. Define the channel numbers
>>>>>>
>>>>>> and use them in both data structures.
>>>>>>
>>>>>> ...
>>>>>
>>>>> WDYM?
>>>>>
>>>>>> OTOH, I'm not sure why we even need this. If this is going to be
>>>>>> sequential, can't we make a core to decide which cell will be given
>>>>>> which id?
>>>>>
>>>>> Just a note that after introduction of PHC sub-driver the array will look
>>>>> like:
>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>> ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
>>>>> ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
>>>>> ZL3073X_CELL("zl3073x-dpll", 1), // ...
>>>>> ZL3073X_CELL("zl3073x-phc", 1),
>>>>> ZL3073X_CELL("zl3073x-dpll", 2),
>>>>> ZL3073X_CELL("zl3073x-phc", 2),
>>>>> ZL3073X_CELL("zl3073x-dpll", 3),
>>>>> ZL3073X_CELL("zl3073x-phc", 3),
>>>>> ZL3073X_CELL("zl3073x-dpll", 4),
>>>>> ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
>>>>> };
>>>>
>>>> Ah, this is very important piece. Then I mean only this kind of change
>>>>
>>>> enum {
>>>> // this or whatever meaningful names
>>>> ..._CH_0 0
>>>> ..._CH_1 1
>>>> ...
>>>> };
>>>>
>>>> static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>> { .channel = ..._CH_0, },
>>>> ...
>>>> };
>>>>
>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>> ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
>>>> ZL3073X_CELL("zl3073x-phc", ..._CH_0),
>>>> ...
>>>> };
>>>
>>> This is getting hectic. All for a sequential enumeration. Seeing as
>>> there are no other differentiations, why not use IDA in the child
>>> instead?
>>
>> For that, there have to be two IDAs, one for DPLLs and one for PHCs...
>
> Sorry, can you explain a bit more. Why is this a problem?
>
> The IDA API is very simple.
>
> Much better than building your own bespoke MACROs.
I will try to explain this in more detail... This MFD driver handles
chip family ZL3073x where the x == number of DPLL channels and can
be from <1, 5>.
The driver creates 'x' DPLL sub-devices during probe and has to pass
channel number that should this sub-device use. Here can be used IDA
in DPLL sub-driver:
e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
This way the DPLL sub-device get its own unique channel ID to use.
The situation is getting more complicated with PHC sub-devices because
the chip can provide UP TO 'x' PHC sub-devices depending on HW
configuration. To handle this the MFD driver has to check this HW config
for particular channel if it is capable to provide PHC functionality.
E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
PHC capable. Then the MFD driver should create 3 PHC sub-devices and
pass 0, 2 resp. 4 for them.
In that case IDA cannot be simply used as the allocation is not
sequential.
So yes, for DPLL sub-devices IDA could be used but for the PHCs another
approach (platform data) has to be used.
There could be a hacky way to use IDA for PHCs: MFD would create PHC
sub-devices for all channels and PHC sub-driver would check the channel
config during probe and if the channel is not capable then returns
-ENODEV. But I don't think this is good idea to create MFD cells this
way.
Thanks for advices.
Ivan
>> The approach in my second reply in this thread is simpler and taken
>> in v8.
>>
>> <cite>
>> +#define ZL3073X_PDATA(_channel) \
>> + (&(const struct zl3073x_pdata) { \
>> + .channel = _channel, \
>> + })
>> +
>> +#define ZL3073X_CELL(_name, _channel) \
>> + MFD_CELL_BASIC(_name, NULL, ZL3073X_PDATA(_channel), \
>> + sizeof(struct zl3073x_pdata), 0)
>> +
>> +static const struct mfd_cell zl3073x_devs[] = {
>> + ZL3073X_CELL("zl3073x-dpll", 0),
>> + ZL3073X_CELL("zl3073x-dpll", 1),
>> + ZL3073X_CELL("zl3073x-dpll", 2),
>> + ZL3073X_CELL("zl3073x-dpll", 3),
>> + ZL3073X_CELL("zl3073x-dpll", 4),
>> +};
>> </cite>
>>
>> Lee, WDYT?
>>
>> Thanks,
>> Ivan
>>
>
On Tue, 13 May 2025, Ivan Vecera wrote:
> On 13. 05. 25 11:41 dop., Lee Jones wrote:
> > On Mon, 12 May 2025, Ivan Vecera wrote:
> >
> > > On 07. 05. 25 5:26 odp., Lee Jones wrote:
> > > > On Wed, 07 May 2025, Andy Shevchenko wrote:
> > > >
> > > > > On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
> > > > > > On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> > > > > > > On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > > +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > > > > > + { .channel = 0, },
> > > > > > > > + { .channel = 1, },
> > > > > > > > + { .channel = 2, },
> > > > > > > > + { .channel = 3, },
> > > > > > > > + { .channel = 4, },
> > > > > > > > +};
> > > > > > >
> > > > > > > > +static const struct mfd_cell zl3073x_devs[] = {
> > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 0),
> > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 1),
> > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 2),
> > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 3),
> > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 4),
> > > > > > > > +};
> > > > > > >
> > > > > > > > +#define ZL3073X_MAX_CHANNELS 5
> > > > > > >
> > > > > > > Btw, wouldn't be better to keep the above lists synchronised like
> > > > > > >
> > > > > > > 1. Make ZL3073X_CELL() to use indexed variant
> > > > > > >
> > > > > > > [idx] = ...
> > > > > > >
> > > > > > > 2. Define the channel numbers
> > > > > > >
> > > > > > > and use them in both data structures.
> > > > > > >
> > > > > > > ...
> > > > > >
> > > > > > WDYM?
> > > > > >
> > > > > > > OTOH, I'm not sure why we even need this. If this is going to be
> > > > > > > sequential, can't we make a core to decide which cell will be given
> > > > > > > which id?
> > > > > >
> > > > > > Just a note that after introduction of PHC sub-driver the array will look
> > > > > > like:
> > > > > > static const struct mfd_cell zl3073x_devs[] = {
> > > > > > ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
> > > > > > ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
> > > > > > ZL3073X_CELL("zl3073x-dpll", 1), // ...
> > > > > > ZL3073X_CELL("zl3073x-phc", 1),
> > > > > > ZL3073X_CELL("zl3073x-dpll", 2),
> > > > > > ZL3073X_CELL("zl3073x-phc", 2),
> > > > > > ZL3073X_CELL("zl3073x-dpll", 3),
> > > > > > ZL3073X_CELL("zl3073x-phc", 3),
> > > > > > ZL3073X_CELL("zl3073x-dpll", 4),
> > > > > > ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
> > > > > > };
> > > > >
> > > > > Ah, this is very important piece. Then I mean only this kind of change
> > > > >
> > > > > enum {
> > > > > // this or whatever meaningful names
> > > > > ..._CH_0 0
> > > > > ..._CH_1 1
> > > > > ...
> > > > > };
> > > > >
> > > > > static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > > { .channel = ..._CH_0, },
> > > > > ...
> > > > > };
> > > > >
> > > > > static const struct mfd_cell zl3073x_devs[] = {
> > > > > ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
> > > > > ZL3073X_CELL("zl3073x-phc", ..._CH_0),
> > > > > ...
> > > > > };
> > > >
> > > > This is getting hectic. All for a sequential enumeration. Seeing as
> > > > there are no other differentiations, why not use IDA in the child
> > > > instead?
> > >
> > > For that, there have to be two IDAs, one for DPLLs and one for PHCs...
> >
> > Sorry, can you explain a bit more. Why is this a problem?
> >
> > The IDA API is very simple.
> >
> > Much better than building your own bespoke MACROs.
>
> I will try to explain this in more detail... This MFD driver handles
> chip family ZL3073x where the x == number of DPLL channels and can
> be from <1, 5>.
>
> The driver creates 'x' DPLL sub-devices during probe and has to pass
> channel number that should this sub-device use. Here can be used IDA
> in DPLL sub-driver:
> e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
>
> This way the DPLL sub-device get its own unique channel ID to use.
>
> The situation is getting more complicated with PHC sub-devices because
> the chip can provide UP TO 'x' PHC sub-devices depending on HW
> configuration. To handle this the MFD driver has to check this HW config
> for particular channel if it is capable to provide PHC functionality.
>
> E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
> create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
> PHC capable. Then the MFD driver should create 3 PHC sub-devices and
> pass 0, 2 resp. 4 for them.
Where is the code that determines which channels are PHC capable?
--
Lee Jones [李琼斯]
On 22. 05. 25 9:39 dop., Lee Jones wrote:
> On Tue, 13 May 2025, Ivan Vecera wrote:
>
>> On 13. 05. 25 11:41 dop., Lee Jones wrote:
>>> On Mon, 12 May 2025, Ivan Vecera wrote:
>>>
>>>> On 07. 05. 25 5:26 odp., Lee Jones wrote:
>>>>> On Wed, 07 May 2025, Andy Shevchenko wrote:
>>>>>
>>>>>> On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
>>>>>>> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
>>>>>>>> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>>>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>>>>> + { .channel = 0, },
>>>>>>>>> + { .channel = 1, },
>>>>>>>>> + { .channel = 2, },
>>>>>>>>> + { .channel = 3, },
>>>>>>>>> + { .channel = 4, },
>>>>>>>>> +};
>>>>>>>>
>>>>>>>>> +static const struct mfd_cell zl3073x_devs[] = {
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 0),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 1),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>>>> +};
>>>>>>>>
>>>>>>>>> +#define ZL3073X_MAX_CHANNELS 5
>>>>>>>>
>>>>>>>> Btw, wouldn't be better to keep the above lists synchronised like
>>>>>>>>
>>>>>>>> 1. Make ZL3073X_CELL() to use indexed variant
>>>>>>>>
>>>>>>>> [idx] = ...
>>>>>>>>
>>>>>>>> 2. Define the channel numbers
>>>>>>>>
>>>>>>>> and use them in both data structures.
>>>>>>>>
>>>>>>>> ...
>>>>>>>
>>>>>>> WDYM?
>>>>>>>
>>>>>>>> OTOH, I'm not sure why we even need this. If this is going to be
>>>>>>>> sequential, can't we make a core to decide which cell will be given
>>>>>>>> which id?
>>>>>>>
>>>>>>> Just a note that after introduction of PHC sub-driver the array will look
>>>>>>> like:
>>>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
>>>>>>> ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 1), // ...
>>>>>>> ZL3073X_CELL("zl3073x-phc", 1),
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>> ZL3073X_CELL("zl3073x-phc", 2),
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>> ZL3073X_CELL("zl3073x-phc", 3),
>>>>>>> ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>> ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
>>>>>>> };
>>>>>>
>>>>>> Ah, this is very important piece. Then I mean only this kind of change
>>>>>>
>>>>>> enum {
>>>>>> // this or whatever meaningful names
>>>>>> ..._CH_0 0
>>>>>> ..._CH_1 1
>>>>>> ...
>>>>>> };
>>>>>>
>>>>>> static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>> { .channel = ..._CH_0, },
>>>>>> ...
>>>>>> };
>>>>>>
>>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>>> ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
>>>>>> ZL3073X_CELL("zl3073x-phc", ..._CH_0),
>>>>>> ...
>>>>>> };
>>>>>
>>>>> This is getting hectic. All for a sequential enumeration. Seeing as
>>>>> there are no other differentiations, why not use IDA in the child
>>>>> instead?
>>>>
>>>> For that, there have to be two IDAs, one for DPLLs and one for PHCs...
>>>
>>> Sorry, can you explain a bit more. Why is this a problem?
>>>
>>> The IDA API is very simple.
>>>
>>> Much better than building your own bespoke MACROs.
>>
>> I will try to explain this in more detail... This MFD driver handles
>> chip family ZL3073x where the x == number of DPLL channels and can
>> be from <1, 5>.
>>
>> The driver creates 'x' DPLL sub-devices during probe and has to pass
>> channel number that should this sub-device use. Here can be used IDA
>> in DPLL sub-driver:
>> e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
>>
>> This way the DPLL sub-device get its own unique channel ID to use.
>>
>> The situation is getting more complicated with PHC sub-devices because
>> the chip can provide UP TO 'x' PHC sub-devices depending on HW
>> configuration. To handle this the MFD driver has to check this HW config
>> for particular channel if it is capable to provide PHC functionality.
>>
>> E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
>> create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
>> PHC capable. Then the MFD driver should create 3 PHC sub-devices and
>> pass 0, 2 resp. 4 for them.
>
> Where is the code that determines which channels are PHC capable?
It is not included in this series and will be added once the PTP driver
will be added. But the code looks like:
for (i = 0; i < ZL3073X_MAX_CHANNELS; i++) {
if (channel_is_in_nco_mode(..., i)) {
struct mfd_cell phc_dev = ZL3073X_CELL("zl3073x-phc", i);
rc = devm_mfd_add_devices(zldev->dev,
PLATFORM_DEVID_AUTO, &phc_dev,
1, NULL, 0, NULL);
...
}
}
Thanks,
Ivan
On Thu, 22 May 2025, Ivan Vecera wrote:
>
>
> On 22. 05. 25 9:39 dop., Lee Jones wrote:
> > On Tue, 13 May 2025, Ivan Vecera wrote:
> >
> > > On 13. 05. 25 11:41 dop., Lee Jones wrote:
> > > > On Mon, 12 May 2025, Ivan Vecera wrote:
> > > >
> > > > > On 07. 05. 25 5:26 odp., Lee Jones wrote:
> > > > > > On Wed, 07 May 2025, Andy Shevchenko wrote:
> > > > > >
> > > > > > > On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
> > > > > > > > On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> > > > > > > > > On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > > > +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > > > > > > > + { .channel = 0, },
> > > > > > > > > > + { .channel = 1, },
> > > > > > > > > > + { .channel = 2, },
> > > > > > > > > > + { .channel = 3, },
> > > > > > > > > > + { .channel = 4, },
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > > +static const struct mfd_cell zl3073x_devs[] = {
> > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 0),
> > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 1),
> > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 2),
> > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 3),
> > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 4),
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > > +#define ZL3073X_MAX_CHANNELS 5
> > > > > > > > >
> > > > > > > > > Btw, wouldn't be better to keep the above lists synchronised like
> > > > > > > > >
> > > > > > > > > 1. Make ZL3073X_CELL() to use indexed variant
> > > > > > > > >
> > > > > > > > > [idx] = ...
> > > > > > > > >
> > > > > > > > > 2. Define the channel numbers
> > > > > > > > >
> > > > > > > > > and use them in both data structures.
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > >
> > > > > > > > WDYM?
> > > > > > > >
> > > > > > > > > OTOH, I'm not sure why we even need this. If this is going to be
> > > > > > > > > sequential, can't we make a core to decide which cell will be given
> > > > > > > > > which id?
> > > > > > > >
> > > > > > > > Just a note that after introduction of PHC sub-driver the array will look
> > > > > > > > like:
> > > > > > > > static const struct mfd_cell zl3073x_devs[] = {
> > > > > > > > ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
> > > > > > > > ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
> > > > > > > > ZL3073X_CELL("zl3073x-dpll", 1), // ...
> > > > > > > > ZL3073X_CELL("zl3073x-phc", 1),
> > > > > > > > ZL3073X_CELL("zl3073x-dpll", 2),
> > > > > > > > ZL3073X_CELL("zl3073x-phc", 2),
> > > > > > > > ZL3073X_CELL("zl3073x-dpll", 3),
> > > > > > > > ZL3073X_CELL("zl3073x-phc", 3),
> > > > > > > > ZL3073X_CELL("zl3073x-dpll", 4),
> > > > > > > > ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
> > > > > > > > };
> > > > > > >
> > > > > > > Ah, this is very important piece. Then I mean only this kind of change
> > > > > > >
> > > > > > > enum {
> > > > > > > // this or whatever meaningful names
> > > > > > > ..._CH_0 0
> > > > > > > ..._CH_1 1
> > > > > > > ...
> > > > > > > };
> > > > > > >
> > > > > > > static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > > > > { .channel = ..._CH_0, },
> > > > > > > ...
> > > > > > > };
> > > > > > >
> > > > > > > static const struct mfd_cell zl3073x_devs[] = {
> > > > > > > ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
> > > > > > > ZL3073X_CELL("zl3073x-phc", ..._CH_0),
> > > > > > > ...
> > > > > > > };
> > > > > >
> > > > > > This is getting hectic. All for a sequential enumeration. Seeing as
> > > > > > there are no other differentiations, why not use IDA in the child
> > > > > > instead?
> > > > >
> > > > > For that, there have to be two IDAs, one for DPLLs and one for PHCs...
> > > >
> > > > Sorry, can you explain a bit more. Why is this a problem?
> > > >
> > > > The IDA API is very simple.
> > > >
> > > > Much better than building your own bespoke MACROs.
> > >
> > > I will try to explain this in more detail... This MFD driver handles
> > > chip family ZL3073x where the x == number of DPLL channels and can
> > > be from <1, 5>.
> > >
> > > The driver creates 'x' DPLL sub-devices during probe and has to pass
> > > channel number that should this sub-device use. Here can be used IDA
> > > in DPLL sub-driver:
> > > e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
> > >
> > > This way the DPLL sub-device get its own unique channel ID to use.
> > >
> > > The situation is getting more complicated with PHC sub-devices because
> > > the chip can provide UP TO 'x' PHC sub-devices depending on HW
> > > configuration. To handle this the MFD driver has to check this HW config
> > > for particular channel if it is capable to provide PHC functionality.
> > >
> > > E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
> > > create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
> > > PHC capable. Then the MFD driver should create 3 PHC sub-devices and
> > > pass 0, 2 resp. 4 for them.
> >
> > Where is the code that determines which channels are PHC capable?
>
> It is not included in this series and will be added once the PTP driver
> will be added. But the code looks like:
>
> for (i = 0; i < ZL3073X_MAX_CHANNELS; i++) {
> if (channel_is_in_nco_mode(..., i)) {
> struct mfd_cell phc_dev = ZL3073X_CELL("zl3073x-phc", i);
> rc = devm_mfd_add_devices(zldev->dev,
> PLATFORM_DEVID_AUTO, &phc_dev,
> 1, NULL, 0, NULL);
> ...
> }
> }
It's the channel_is_in_nco_mode() code I wanted to see.
What if you register all PHC devices, then bomb out if
!channel_is_in_nco_mode()? Presumably this can / should also live in
the PHC driver as well?
--
Lee Jones [李琼斯]
On 22. 05. 25 12:45 odp., Lee Jones wrote:
> On Thu, 22 May 2025, Ivan Vecera wrote:
>
>>
>>
>> On 22. 05. 25 9:39 dop., Lee Jones wrote:
>>> On Tue, 13 May 2025, Ivan Vecera wrote:
>>>
>>>> On 13. 05. 25 11:41 dop., Lee Jones wrote:
>>>>> On Mon, 12 May 2025, Ivan Vecera wrote:
>>>>>
>>>>>> On 07. 05. 25 5:26 odp., Lee Jones wrote:
>>>>>>> On Wed, 07 May 2025, Andy Shevchenko wrote:
>>>>>>>
>>>>>>>> On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
>>>>>>>>> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
>>>>>>>>>> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>>>>>>>
>>>>>>>> ...
>>>>>>>>
>>>>>>>>>>> +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>>>>>>> + { .channel = 0, },
>>>>>>>>>>> + { .channel = 1, },
>>>>>>>>>>> + { .channel = 2, },
>>>>>>>>>>> + { .channel = 3, },
>>>>>>>>>>> + { .channel = 4, },
>>>>>>>>>>> +};
>>>>>>>>>>
>>>>>>>>>>> +static const struct mfd_cell zl3073x_devs[] = {
>>>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 0),
>>>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 1),
>>>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>>>>>> +};
>>>>>>>>>>
>>>>>>>>>>> +#define ZL3073X_MAX_CHANNELS 5
>>>>>>>>>>
>>>>>>>>>> Btw, wouldn't be better to keep the above lists synchronised like
>>>>>>>>>>
>>>>>>>>>> 1. Make ZL3073X_CELL() to use indexed variant
>>>>>>>>>>
>>>>>>>>>> [idx] = ...
>>>>>>>>>>
>>>>>>>>>> 2. Define the channel numbers
>>>>>>>>>>
>>>>>>>>>> and use them in both data structures.
>>>>>>>>>>
>>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> WDYM?
>>>>>>>>>
>>>>>>>>>> OTOH, I'm not sure why we even need this. If this is going to be
>>>>>>>>>> sequential, can't we make a core to decide which cell will be given
>>>>>>>>>> which id?
>>>>>>>>>
>>>>>>>>> Just a note that after introduction of PHC sub-driver the array will look
>>>>>>>>> like:
>>>>>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>>>>>> ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
>>>>>>>>> ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
>>>>>>>>> ZL3073X_CELL("zl3073x-dpll", 1), // ...
>>>>>>>>> ZL3073X_CELL("zl3073x-phc", 1),
>>>>>>>>> ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>>>> ZL3073X_CELL("zl3073x-phc", 2),
>>>>>>>>> ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>>>> ZL3073X_CELL("zl3073x-phc", 3),
>>>>>>>>> ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>>>> ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
>>>>>>>>> };
>>>>>>>>
>>>>>>>> Ah, this is very important piece. Then I mean only this kind of change
>>>>>>>>
>>>>>>>> enum {
>>>>>>>> // this or whatever meaningful names
>>>>>>>> ..._CH_0 0
>>>>>>>> ..._CH_1 1
>>>>>>>> ...
>>>>>>>> };
>>>>>>>>
>>>>>>>> static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>>>> { .channel = ..._CH_0, },
>>>>>>>> ...
>>>>>>>> };
>>>>>>>>
>>>>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>>>>> ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
>>>>>>>> ZL3073X_CELL("zl3073x-phc", ..._CH_0),
>>>>>>>> ...
>>>>>>>> };
>>>>>>>
>>>>>>> This is getting hectic. All for a sequential enumeration. Seeing as
>>>>>>> there are no other differentiations, why not use IDA in the child
>>>>>>> instead?
>>>>>>
>>>>>> For that, there have to be two IDAs, one for DPLLs and one for PHCs...
>>>>>
>>>>> Sorry, can you explain a bit more. Why is this a problem?
>>>>>
>>>>> The IDA API is very simple.
>>>>>
>>>>> Much better than building your own bespoke MACROs.
>>>>
>>>> I will try to explain this in more detail... This MFD driver handles
>>>> chip family ZL3073x where the x == number of DPLL channels and can
>>>> be from <1, 5>.
>>>>
>>>> The driver creates 'x' DPLL sub-devices during probe and has to pass
>>>> channel number that should this sub-device use. Here can be used IDA
>>>> in DPLL sub-driver:
>>>> e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
>>>>
>>>> This way the DPLL sub-device get its own unique channel ID to use.
>>>>
>>>> The situation is getting more complicated with PHC sub-devices because
>>>> the chip can provide UP TO 'x' PHC sub-devices depending on HW
>>>> configuration. To handle this the MFD driver has to check this HW config
>>>> for particular channel if it is capable to provide PHC functionality.
>>>>
>>>> E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
>>>> create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
>>>> PHC capable. Then the MFD driver should create 3 PHC sub-devices and
>>>> pass 0, 2 resp. 4 for them.
>>>
>>> Where is the code that determines which channels are PHC capable?
>>
>> It is not included in this series and will be added once the PTP driver
>> will be added. But the code looks like:
>>
>> for (i = 0; i < ZL3073X_MAX_CHANNELS; i++) {
>> if (channel_is_in_nco_mode(..., i)) {
>> struct mfd_cell phc_dev = ZL3073X_CELL("zl3073x-phc", i);
>> rc = devm_mfd_add_devices(zldev->dev,
>> PLATFORM_DEVID_AUTO, &phc_dev,
>> 1, NULL, 0, NULL);
>> ...
>> }
>> }
>
> It's the channel_is_in_nco_mode() code I wanted to see.
The function is like this:
static bool zl3073x_chan_in_nco_mode(struct zl3073x_dev *zldev, u8 ch)
{
u8 mode, mode_refsel;
int rc;
rc = zl3073x_read_u8(zlptp->mfd,
ZL_REG_DPLL_MODE_REFSEL(ch), &mode_refsel);
if (rc)
return false;
mode = FIELD_GET(ZL_DPLL_MODE_REFSEL_MODE, mode_refsel);
return (mode == ZL_DPLL_MODE_REFSEL_MODE_NCO);
}
> What if you register all PHC devices, then bomb out if
> !channel_is_in_nco_mode()? Presumably this can / should also live in
> the PHC driver as well?
Yes, we can register PHC sub-dev for all channels disregard to channel
mode. The PHC driver checks for the mode and return -ENODEV when it is
different from NCO. But in this case the user will see PHC platform
devices under /sys/bus/platform/device and some of them won't have
driver bound (they will look like some kind of phantom devices).
I'm not sure if this is OK and not confusing.
Thanks for an opinion.
Ivan
On Thu, 22 May 2025, Ivan Vecera wrote:
> On 22. 05. 25 12:45 odp., Lee Jones wrote:
> > On Thu, 22 May 2025, Ivan Vecera wrote:
> >
> > >
> > >
> > > On 22. 05. 25 9:39 dop., Lee Jones wrote:
> > > > On Tue, 13 May 2025, Ivan Vecera wrote:
> > > >
> > > > > On 13. 05. 25 11:41 dop., Lee Jones wrote:
> > > > > > On Mon, 12 May 2025, Ivan Vecera wrote:
> > > > > >
> > > > > > > On 07. 05. 25 5:26 odp., Lee Jones wrote:
> > > > > > > > On Wed, 07 May 2025, Andy Shevchenko wrote:
> > > > > > > >
> > > > > > > > > On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
> > > > > > > > > > On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
> > > > > > > > > > > On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > > > > +static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > > > > > > > > > + { .channel = 0, },
> > > > > > > > > > > > + { .channel = 1, },
> > > > > > > > > > > > + { .channel = 2, },
> > > > > > > > > > > > + { .channel = 3, },
> > > > > > > > > > > > + { .channel = 4, },
> > > > > > > > > > > > +};
> > > > > > > > > > >
> > > > > > > > > > > > +static const struct mfd_cell zl3073x_devs[] = {
> > > > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 0),
> > > > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 1),
> > > > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 2),
> > > > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 3),
> > > > > > > > > > > > + ZL3073X_CELL("zl3073x-dpll", 4),
> > > > > > > > > > > > +};
> > > > > > > > > > >
> > > > > > > > > > > > +#define ZL3073X_MAX_CHANNELS 5
> > > > > > > > > > >
> > > > > > > > > > > Btw, wouldn't be better to keep the above lists synchronised like
> > > > > > > > > > >
> > > > > > > > > > > 1. Make ZL3073X_CELL() to use indexed variant
> > > > > > > > > > >
> > > > > > > > > > > [idx] = ...
> > > > > > > > > > >
> > > > > > > > > > > 2. Define the channel numbers
> > > > > > > > > > >
> > > > > > > > > > > and use them in both data structures.
> > > > > > > > > > >
> > > > > > > > > > > ...
> > > > > > > > > >
> > > > > > > > > > WDYM?
> > > > > > > > > >
> > > > > > > > > > > OTOH, I'm not sure why we even need this. If this is going to be
> > > > > > > > > > > sequential, can't we make a core to decide which cell will be given
> > > > > > > > > > > which id?
> > > > > > > > > >
> > > > > > > > > > Just a note that after introduction of PHC sub-driver the array will look
> > > > > > > > > > like:
> > > > > > > > > > static const struct mfd_cell zl3073x_devs[] = {
> > > > > > > > > > ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
> > > > > > > > > > ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
> > > > > > > > > > ZL3073X_CELL("zl3073x-dpll", 1), // ...
> > > > > > > > > > ZL3073X_CELL("zl3073x-phc", 1),
> > > > > > > > > > ZL3073X_CELL("zl3073x-dpll", 2),
> > > > > > > > > > ZL3073X_CELL("zl3073x-phc", 2),
> > > > > > > > > > ZL3073X_CELL("zl3073x-dpll", 3),
> > > > > > > > > > ZL3073X_CELL("zl3073x-phc", 3),
> > > > > > > > > > ZL3073X_CELL("zl3073x-dpll", 4),
> > > > > > > > > > ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
> > > > > > > > > > };
> > > > > > > > >
> > > > > > > > > Ah, this is very important piece. Then I mean only this kind of change
> > > > > > > > >
> > > > > > > > > enum {
> > > > > > > > > // this or whatever meaningful names
> > > > > > > > > ..._CH_0 0
> > > > > > > > > ..._CH_1 1
> > > > > > > > > ...
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > static const struct zl3073x_pdata zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
> > > > > > > > > { .channel = ..._CH_0, },
> > > > > > > > > ...
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > static const struct mfd_cell zl3073x_devs[] = {
> > > > > > > > > ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
> > > > > > > > > ZL3073X_CELL("zl3073x-phc", ..._CH_0),
> > > > > > > > > ...
> > > > > > > > > };
> > > > > > > >
> > > > > > > > This is getting hectic. All for a sequential enumeration. Seeing as
> > > > > > > > there are no other differentiations, why not use IDA in the child
> > > > > > > > instead?
> > > > > > >
> > > > > > > For that, there have to be two IDAs, one for DPLLs and one for PHCs...
> > > > > >
> > > > > > Sorry, can you explain a bit more. Why is this a problem?
> > > > > >
> > > > > > The IDA API is very simple.
> > > > > >
> > > > > > Much better than building your own bespoke MACROs.
> > > > >
> > > > > I will try to explain this in more detail... This MFD driver handles
> > > > > chip family ZL3073x where the x == number of DPLL channels and can
> > > > > be from <1, 5>.
> > > > >
> > > > > The driver creates 'x' DPLL sub-devices during probe and has to pass
> > > > > channel number that should this sub-device use. Here can be used IDA
> > > > > in DPLL sub-driver:
> > > > > e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
> > > > >
> > > > > This way the DPLL sub-device get its own unique channel ID to use.
> > > > >
> > > > > The situation is getting more complicated with PHC sub-devices because
> > > > > the chip can provide UP TO 'x' PHC sub-devices depending on HW
> > > > > configuration. To handle this the MFD driver has to check this HW config
> > > > > for particular channel if it is capable to provide PHC functionality.
> > > > >
> > > > > E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
> > > > > create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
> > > > > PHC capable. Then the MFD driver should create 3 PHC sub-devices and
> > > > > pass 0, 2 resp. 4 for them.
> > > >
> > > > Where is the code that determines which channels are PHC capable?
> > >
> > > It is not included in this series and will be added once the PTP driver
> > > will be added. But the code looks like:
> > >
> > > for (i = 0; i < ZL3073X_MAX_CHANNELS; i++) {
> > > if (channel_is_in_nco_mode(..., i)) {
> > > struct mfd_cell phc_dev = ZL3073X_CELL("zl3073x-phc", i);
> > > rc = devm_mfd_add_devices(zldev->dev,
> > > PLATFORM_DEVID_AUTO, &phc_dev,
> > > 1, NULL, 0, NULL);
> > > ...
> > > }
> > > }
> >
> > It's the channel_is_in_nco_mode() code I wanted to see.
>
> The function is like this:
>
> static bool zl3073x_chan_in_nco_mode(struct zl3073x_dev *zldev, u8 ch)
> {
> u8 mode, mode_refsel;
> int rc;
>
> rc = zl3073x_read_u8(zlptp->mfd,
> ZL_REG_DPLL_MODE_REFSEL(ch), &mode_refsel);
> if (rc)
> return false;
>
> mode = FIELD_GET(ZL_DPLL_MODE_REFSEL_MODE, mode_refsel);
>
> return (mode == ZL_DPLL_MODE_REFSEL_MODE_NCO);
> }
>
> > What if you register all PHC devices, then bomb out if
> > !channel_is_in_nco_mode()? Presumably this can / should also live in
> > the PHC driver as well?
>
> Yes, we can register PHC sub-dev for all channels disregard to channel
> mode. The PHC driver checks for the mode and return -ENODEV when it is
> different from NCO. But in this case the user will see PHC platform
> devices under /sys/bus/platform/device and some of them won't have
> driver bound (they will look like some kind of phantom devices).
> I'm not sure if this is OK and not confusing.
There will be plenty of devices which do not successfully probe for one
reason or another. This is all that these 'phantom devices' will
indicate.
--
Lee Jones [李琼斯]
On 13. 05. 25 12:47 odp., Ivan Vecera wrote:
> On 13. 05. 25 11:41 dop., Lee Jones wrote:
>> On Mon, 12 May 2025, Ivan Vecera wrote:
>>
>>> On 07. 05. 25 5:26 odp., Lee Jones wrote:
>>>> On Wed, 07 May 2025, Andy Shevchenko wrote:
>>>>
>>>>> On Wed, May 07, 2025 at 03:56:37PM +0200, Ivan Vecera wrote:
>>>>>> On 07. 05. 25 3:41 odp., Andy Shevchenko wrote:
>>>>>>> On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com>
>>>>>>> wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>> +static const struct zl3073x_pdata
>>>>>>>> zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>>>>> + { .channel = 0, },
>>>>>>>> + { .channel = 1, },
>>>>>>>> + { .channel = 2, },
>>>>>>>> + { .channel = 3, },
>>>>>>>> + { .channel = 4, },
>>>>>>>> +};
>>>>>>>
>>>>>>>> +static const struct mfd_cell zl3073x_devs[] = {
>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 0),
>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 1),
>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>>>> + ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>>>> +};
>>>>>>>
>>>>>>>> +#define ZL3073X_MAX_CHANNELS 5
>>>>>>>
>>>>>>> Btw, wouldn't be better to keep the above lists synchronised like
>>>>>>>
>>>>>>> 1. Make ZL3073X_CELL() to use indexed variant
>>>>>>>
>>>>>>> [idx] = ...
>>>>>>>
>>>>>>> 2. Define the channel numbers
>>>>>>>
>>>>>>> and use them in both data structures.
>>>>>>>
>>>>>>> ...
>>>>>>
>>>>>> WDYM?
>>>>>>
>>>>>>> OTOH, I'm not sure why we even need this. If this is going to be
>>>>>>> sequential, can't we make a core to decide which cell will be given
>>>>>>> which id?
>>>>>>
>>>>>> Just a note that after introduction of PHC sub-driver the array
>>>>>> will look
>>>>>> like:
>>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>>> ZL3073X_CELL("zl3073x-dpll", 0), // DPLL sub-dev for chan 0
>>>>>> ZL3073X_CELL("zl3073x-phc", 0), // PHC sub-dev for chan 0
>>>>>> ZL3073X_CELL("zl3073x-dpll", 1), // ...
>>>>>> ZL3073X_CELL("zl3073x-phc", 1),
>>>>>> ZL3073X_CELL("zl3073x-dpll", 2),
>>>>>> ZL3073X_CELL("zl3073x-phc", 2),
>>>>>> ZL3073X_CELL("zl3073x-dpll", 3),
>>>>>> ZL3073X_CELL("zl3073x-phc", 3),
>>>>>> ZL3073X_CELL("zl3073x-dpll", 4),
>>>>>> ZL3073X_CELL("zl3073x-phc", 4), // PHC sub-dev for chan 4
>>>>>> };
>>>>>
>>>>> Ah, this is very important piece. Then I mean only this kind of change
>>>>>
>>>>> enum {
>>>>> // this or whatever meaningful names
>>>>> ..._CH_0 0
>>>>> ..._CH_1 1
>>>>> ...
>>>>> };
>>>>>
>>>>> static const struct zl3073x_pdata
>>>>> zl3073x_pdata[ZL3073X_MAX_CHANNELS] = {
>>>>> { .channel = ..._CH_0, },
>>>>> ...
>>>>> };
>>>>>
>>>>> static const struct mfd_cell zl3073x_devs[] = {
>>>>> ZL3073X_CELL("zl3073x-dpll", ..._CH_0),
>>>>> ZL3073X_CELL("zl3073x-phc", ..._CH_0),
>>>>> ...
>>>>> };
>>>>
>>>> This is getting hectic. All for a sequential enumeration. Seeing as
>>>> there are no other differentiations, why not use IDA in the child
>>>> instead?
>>>
>>> For that, there have to be two IDAs, one for DPLLs and one for PHCs...
>>
>> Sorry, can you explain a bit more. Why is this a problem?
>>
>> The IDA API is very simple.
>>
>> Much better than building your own bespoke MACROs.
>
> I will try to explain this in more detail... This MFD driver handles
> chip family ZL3073x where the x == number of DPLL channels and can
> be from <1, 5>.
>
> The driver creates 'x' DPLL sub-devices during probe and has to pass
> channel number that should this sub-device use. Here can be used IDA
> in DPLL sub-driver:
> e.g. ida_alloc_max(zldev->channels, zldev->max_channels, GFP_KERNEL);
>
> This way the DPLL sub-device get its own unique channel ID to use.
>
> The situation is getting more complicated with PHC sub-devices because
> the chip can provide UP TO 'x' PHC sub-devices depending on HW
> configuration. To handle this the MFD driver has to check this HW config
> for particular channel if it is capable to provide PHC functionality.
>
> E.g. ZL30735 chip has 5 channels, in this case the MFD driver should
> create 5 DPLL sub-devices. And then lets say channel 0, 2 and 4 are
> PHC capable. Then the MFD driver should create 3 PHC sub-devices and
> pass 0, 2 resp. 4 for them.
>
> In that case IDA cannot be simply used as the allocation is not
> sequential.
>
> So yes, for DPLL sub-devices IDA could be used but for the PHCs another
> approach (platform data) has to be used.
>
> There could be a hacky way to use IDA for PHCs: MFD would create PHC
> sub-devices for all channels and PHC sub-driver would check the channel
> config during probe and if the channel is not capable then returns
> -ENODEV. But I don't think this is good idea to create MFD cells this
> way.
>
> Thanks for advices.
>
> Ivan
Lee, any comment? What about my proposal in v8?
Thanks,
Ivan
On Wed, May 7, 2025 at 3:45 PM Ivan Vecera <ivecera@redhat.com> wrote:
>
> Register DPLL sub-devices to expose the functionality provided
> by ZL3073x chip family. Each sub-device represents one of
> the available DPLL channels.
...
> +/**
> + * struct zl3073x_pdata - zl3073x sub-device platform data
> + * @channel: channel to use
> + */
> +struct zl3073x_pdata {
> + u8 channel;
> +};
You can also use software nodes (via device properties).
But since the current solution doesn't require any additional files or
something like that, I don't care much.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.