Add .clock_id to zl3073x_dev structure that will be used by later
commits introducing DPLL feature. The clock ID is required for DPLL
device registration.
To generate this ID, use chip ID read during device initialization.
In case where multiple zl3073x based chips are present, the chip ID
is shifted and lower bits are filled by an unique value - using
the I2C device address for I2C connections and the chip-select value
for SPI connections.
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
drivers/dpll/zl3073x/core.c | 6 +++++-
drivers/dpll/zl3073x/core.h | 4 +++-
drivers/dpll/zl3073x/i2c.c | 4 +++-
drivers/dpll/zl3073x/spi.c | 4 +++-
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
index b99dd81077d56..94c78a36d9158 100644
--- a/drivers/dpll/zl3073x/core.c
+++ b/drivers/dpll/zl3073x/core.c
@@ -743,13 +743,14 @@ static void zl3073x_devlink_unregister(void *ptr)
* zl3073x_dev_probe - initialize zl3073x device
* @zldev: pointer to zl3073x device
* @chip_info: chip info based on compatible
+ * @dev_id: device ID to be used as part of clock ID
*
* Common initialization of zl3073x device structure.
*
* Returns: 0 on success, <0 on error
*/
int zl3073x_dev_probe(struct zl3073x_dev *zldev,
- const struct zl3073x_chip_info *chip_info)
+ const struct zl3073x_chip_info *chip_info, u8 dev_id)
{
u16 id, revision, fw_ver;
struct devlink *devlink;
@@ -793,6 +794,9 @@ int zl3073x_dev_probe(struct zl3073x_dev *zldev,
FIELD_GET(GENMASK(15, 8), cfg_ver),
FIELD_GET(GENMASK(7, 0), cfg_ver));
+ /* Use chip ID and given dev ID as clock ID */
+ zldev->clock_id = ((u64)id << 8) | dev_id;
+
/* Initialize mutex for operations where multiple reads, writes
* and/or polls are required to be done atomically.
*/
diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
index 2c831f1a4a5d1..1df2dc194980d 100644
--- a/drivers/dpll/zl3073x/core.h
+++ b/drivers/dpll/zl3073x/core.h
@@ -57,6 +57,7 @@ struct zl3073x_synth {
* @dev: pointer to device
* @regmap: regmap to access device registers
* @multiop_lock: to serialize multiple register operations
+ * @clock_id: clock id of the device
* @ref: array of input references' invariants
* @out: array of outs' invariants
* @synth: array of synths' invariants
@@ -65,6 +66,7 @@ struct zl3073x_dev {
struct device *dev;
struct regmap *regmap;
struct mutex multiop_lock;
+ u64 clock_id;
/* Invariants */
struct zl3073x_ref ref[ZL3073X_NUM_REFS];
@@ -87,7 +89,7 @@ extern const struct regmap_config zl3073x_regmap_config;
struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev);
int zl3073x_dev_probe(struct zl3073x_dev *zldev,
- const struct zl3073x_chip_info *chip_info);
+ const struct zl3073x_chip_info *chip_info, u8 dev_id);
/**********************
* Registers operations
diff --git a/drivers/dpll/zl3073x/i2c.c b/drivers/dpll/zl3073x/i2c.c
index 7bbfdd4ed8671..13942ee43b9e5 100644
--- a/drivers/dpll/zl3073x/i2c.c
+++ b/drivers/dpll/zl3073x/i2c.c
@@ -22,7 +22,9 @@ static int zl3073x_i2c_probe(struct i2c_client *client)
return dev_err_probe(dev, PTR_ERR(zldev->regmap),
"Failed to initialize regmap\n");
- return zl3073x_dev_probe(zldev, i2c_get_match_data(client));
+ /* Initialize device and use I2C address as dev ID */
+ return zl3073x_dev_probe(zldev, i2c_get_match_data(client),
+ client->addr);
}
static const struct i2c_device_id zl3073x_i2c_id[] = {
diff --git a/drivers/dpll/zl3073x/spi.c b/drivers/dpll/zl3073x/spi.c
index af901b4d6dda0..670b44a7b270f 100644
--- a/drivers/dpll/zl3073x/spi.c
+++ b/drivers/dpll/zl3073x/spi.c
@@ -22,7 +22,9 @@ static int zl3073x_spi_probe(struct spi_device *spi)
return dev_err_probe(dev, PTR_ERR(zldev->regmap),
"Failed to initialize regmap\n");
- return zl3073x_dev_probe(zldev, spi_get_device_match_data(spi));
+ /* Initialize device and use SPI chip select value as dev ID */
+ return zl3073x_dev_probe(zldev, spi_get_device_match_data(spi),
+ spi_get_chipselect(spi, 0));
}
static const struct spi_device_id zl3073x_spi_id[] = {
--
2.49.0Sun, Jun 29, 2025 at 09:10:42PM +0200, ivecera@redhat.com wrote:
>Add .clock_id to zl3073x_dev structure that will be used by later
>commits introducing DPLL feature. The clock ID is required for DPLL
>device registration.
>
>To generate this ID, use chip ID read during device initialization.
>In case where multiple zl3073x based chips are present, the chip ID
>is shifted and lower bits are filled by an unique value - using
>the I2C device address for I2C connections and the chip-select value
>for SPI connections.
You say that multiple chips may have the same chip ID? How is that
possible? Isn't it supposed to be unique?
I understand clock ID to be invariant regardless where you plug your
device. When you construct it from i2c address, sounds wrong.
>
>Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>---
> drivers/dpll/zl3073x/core.c | 6 +++++-
> drivers/dpll/zl3073x/core.h | 4 +++-
> drivers/dpll/zl3073x/i2c.c | 4 +++-
> drivers/dpll/zl3073x/spi.c | 4 +++-
> 4 files changed, 14 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
>index b99dd81077d56..94c78a36d9158 100644
>--- a/drivers/dpll/zl3073x/core.c
>+++ b/drivers/dpll/zl3073x/core.c
>@@ -743,13 +743,14 @@ static void zl3073x_devlink_unregister(void *ptr)
> * zl3073x_dev_probe - initialize zl3073x device
> * @zldev: pointer to zl3073x device
> * @chip_info: chip info based on compatible
>+ * @dev_id: device ID to be used as part of clock ID
> *
> * Common initialization of zl3073x device structure.
> *
> * Returns: 0 on success, <0 on error
> */
> int zl3073x_dev_probe(struct zl3073x_dev *zldev,
>- const struct zl3073x_chip_info *chip_info)
>+ const struct zl3073x_chip_info *chip_info, u8 dev_id)
> {
> u16 id, revision, fw_ver;
> struct devlink *devlink;
>@@ -793,6 +794,9 @@ int zl3073x_dev_probe(struct zl3073x_dev *zldev,
> FIELD_GET(GENMASK(15, 8), cfg_ver),
> FIELD_GET(GENMASK(7, 0), cfg_ver));
>
>+ /* Use chip ID and given dev ID as clock ID */
>+ zldev->clock_id = ((u64)id << 8) | dev_id;
>+
> /* Initialize mutex for operations where multiple reads, writes
> * and/or polls are required to be done atomically.
> */
>diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
>index 2c831f1a4a5d1..1df2dc194980d 100644
>--- a/drivers/dpll/zl3073x/core.h
>+++ b/drivers/dpll/zl3073x/core.h
>@@ -57,6 +57,7 @@ struct zl3073x_synth {
> * @dev: pointer to device
> * @regmap: regmap to access device registers
> * @multiop_lock: to serialize multiple register operations
>+ * @clock_id: clock id of the device
> * @ref: array of input references' invariants
> * @out: array of outs' invariants
> * @synth: array of synths' invariants
>@@ -65,6 +66,7 @@ struct zl3073x_dev {
> struct device *dev;
> struct regmap *regmap;
> struct mutex multiop_lock;
>+ u64 clock_id;
>
> /* Invariants */
> struct zl3073x_ref ref[ZL3073X_NUM_REFS];
>@@ -87,7 +89,7 @@ extern const struct regmap_config zl3073x_regmap_config;
>
> struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev);
> int zl3073x_dev_probe(struct zl3073x_dev *zldev,
>- const struct zl3073x_chip_info *chip_info);
>+ const struct zl3073x_chip_info *chip_info, u8 dev_id);
>
> /**********************
> * Registers operations
>diff --git a/drivers/dpll/zl3073x/i2c.c b/drivers/dpll/zl3073x/i2c.c
>index 7bbfdd4ed8671..13942ee43b9e5 100644
>--- a/drivers/dpll/zl3073x/i2c.c
>+++ b/drivers/dpll/zl3073x/i2c.c
>@@ -22,7 +22,9 @@ static int zl3073x_i2c_probe(struct i2c_client *client)
> return dev_err_probe(dev, PTR_ERR(zldev->regmap),
> "Failed to initialize regmap\n");
>
>- return zl3073x_dev_probe(zldev, i2c_get_match_data(client));
>+ /* Initialize device and use I2C address as dev ID */
>+ return zl3073x_dev_probe(zldev, i2c_get_match_data(client),
>+ client->addr);
> }
>
> static const struct i2c_device_id zl3073x_i2c_id[] = {
>diff --git a/drivers/dpll/zl3073x/spi.c b/drivers/dpll/zl3073x/spi.c
>index af901b4d6dda0..670b44a7b270f 100644
>--- a/drivers/dpll/zl3073x/spi.c
>+++ b/drivers/dpll/zl3073x/spi.c
>@@ -22,7 +22,9 @@ static int zl3073x_spi_probe(struct spi_device *spi)
> return dev_err_probe(dev, PTR_ERR(zldev->regmap),
> "Failed to initialize regmap\n");
>
>- return zl3073x_dev_probe(zldev, spi_get_device_match_data(spi));
>+ /* Initialize device and use SPI chip select value as dev ID */
>+ return zl3073x_dev_probe(zldev, spi_get_device_match_data(spi),
>+ spi_get_chipselect(spi, 0));
> }
>
> static const struct spi_device_id zl3073x_spi_id[] = {
>--
>2.49.0
>
On 02. 07. 25 12:31 odp., Jiri Pirko wrote: > Sun, Jun 29, 2025 at 09:10:42PM +0200, ivecera@redhat.com wrote: >> Add .clock_id to zl3073x_dev structure that will be used by later >> commits introducing DPLL feature. The clock ID is required for DPLL >> device registration. >> >> To generate this ID, use chip ID read during device initialization. >> In case where multiple zl3073x based chips are present, the chip ID >> is shifted and lower bits are filled by an unique value - using >> the I2C device address for I2C connections and the chip-select value >> for SPI connections. > > You say that multiple chips may have the same chip ID? How is that > possible? Isn't it supposed to be unique? > I understand clock ID to be invariant regardless where you plug your > device. When you construct it from i2c address, sounds wrong. The chip id is not like serial number but it is like device id under PCI. So if you will have multiple chips with this chip id you have to distinguish somehow between them, this is the reason why I2C address is added into the final value. Anyway this device does not have any attribute that corresponds to clock id (as per our previous discussion) and it will be better to NOT require clock id from DPLL core side. Ivan
Wed, Jul 02, 2025 at 01:43:38PM +0200, ivecera@redhat.com wrote: > >On 02. 07. 25 12:31 odp., Jiri Pirko wrote: >> Sun, Jun 29, 2025 at 09:10:42PM +0200, ivecera@redhat.com wrote: >> > Add .clock_id to zl3073x_dev structure that will be used by later >> > commits introducing DPLL feature. The clock ID is required for DPLL >> > device registration. >> > >> > To generate this ID, use chip ID read during device initialization. >> > In case where multiple zl3073x based chips are present, the chip ID >> > is shifted and lower bits are filled by an unique value - using >> > the I2C device address for I2C connections and the chip-select value >> > for SPI connections. >> >> You say that multiple chips may have the same chip ID? How is that >> possible? Isn't it supposed to be unique? >> I understand clock ID to be invariant regardless where you plug your >> device. When you construct it from i2c address, sounds wrong. > >The chip id is not like serial number but it is like device id under >PCI. So if you will have multiple chips with this chip id you have to >distinguish somehow between them, this is the reason why I2C address >is added into the final value. > >Anyway this device does not have any attribute that corresponds to >clock id (as per our previous discussion) and it will be better to NOT >require clock id from DPLL core side. Yes, better not to require it comparing to having it wrong. > >Ivan > >
On 02. 07. 25 2:01 odp., Jiri Pirko wrote: > Wed, Jul 02, 2025 at 01:43:38PM +0200, ivecera@redhat.com wrote: >> >> On 02. 07. 25 12:31 odp., Jiri Pirko wrote: >>> Sun, Jun 29, 2025 at 09:10:42PM +0200, ivecera@redhat.com wrote: >>>> Add .clock_id to zl3073x_dev structure that will be used by later >>>> commits introducing DPLL feature. The clock ID is required for DPLL >>>> device registration. >>>> >>>> To generate this ID, use chip ID read during device initialization. >>>> In case where multiple zl3073x based chips are present, the chip ID >>>> is shifted and lower bits are filled by an unique value - using >>>> the I2C device address for I2C connections and the chip-select value >>>> for SPI connections. >>> >>> You say that multiple chips may have the same chip ID? How is that >>> possible? Isn't it supposed to be unique? >>> I understand clock ID to be invariant regardless where you plug your >>> device. When you construct it from i2c address, sounds wrong. >> >> The chip id is not like serial number but it is like device id under >> PCI. So if you will have multiple chips with this chip id you have to >> distinguish somehow between them, this is the reason why I2C address >> is added into the final value. >> >> Anyway this device does not have any attribute that corresponds to >> clock id (as per our previous discussion) and it will be better to NOT >> require clock id from DPLL core side. > > Yes, better not to require it comparing to having it wrong. It looks that using clock_id==0 is safe from DPLL API point of view. The problem is if you will have multiple zl3073x based chips because the driver would call dpll_device_get(0 /* clock_id */, channel, module) For 1st chip (e.g. 2 channel) the driver will call: dpll_device_get(0, 0, module); dpll_device_get(0, 1, module); and for the second the same that is wrong. The clock_id would help to distinguish between them. Wouldn't it be better to use a random number for clock_id from the driver? Ivan
Wed, Jul 02, 2025 at 04:51:47PM +0200, ivecera@redhat.com wrote: >On 02. 07. 25 2:01 odp., Jiri Pirko wrote: >> Wed, Jul 02, 2025 at 01:43:38PM +0200, ivecera@redhat.com wrote: >> > >> > On 02. 07. 25 12:31 odp., Jiri Pirko wrote: >> > > Sun, Jun 29, 2025 at 09:10:42PM +0200, ivecera@redhat.com wrote: >> > > > Add .clock_id to zl3073x_dev structure that will be used by later >> > > > commits introducing DPLL feature. The clock ID is required for DPLL >> > > > device registration. >> > > > >> > > > To generate this ID, use chip ID read during device initialization. >> > > > In case where multiple zl3073x based chips are present, the chip ID >> > > > is shifted and lower bits are filled by an unique value - using >> > > > the I2C device address for I2C connections and the chip-select value >> > > > for SPI connections. >> > > >> > > You say that multiple chips may have the same chip ID? How is that >> > > possible? Isn't it supposed to be unique? >> > > I understand clock ID to be invariant regardless where you plug your >> > > device. When you construct it from i2c address, sounds wrong. >> > >> > The chip id is not like serial number but it is like device id under >> > PCI. So if you will have multiple chips with this chip id you have to >> > distinguish somehow between them, this is the reason why I2C address >> > is added into the final value. >> > >> > Anyway this device does not have any attribute that corresponds to >> > clock id (as per our previous discussion) and it will be better to NOT >> > require clock id from DPLL core side. >> >> Yes, better not to require it comparing to having it wrong. > >It looks that using clock_id==0 is safe from DPLL API point of view. >The problem is if you will have multiple zl3073x based chips because >the driver would call dpll_device_get(0 /* clock_id */, channel, module) > >For 1st chip (e.g. 2 channel) the driver will call: >dpll_device_get(0, 0, module); >dpll_device_get(0, 1, module); > >and for the second the same that is wrong. The clock_id would help to >distinguish between them. > >Wouldn't it be better to use a random number for clock_id from the >driver? I take my suggestion to not require it back, does not make sense. Clock id actually has a reason to exist from UAPI perspective. Checkout dpll_device_find_from_nlattr(). The user passes CLOCK_ID attr (among others) to obtain device by DPLL_CMD_DEVICE_ID_GET command. He expects to get a result back from kernel regardless where the device is plugged and across the reboots/rebinds. Clock id should be properly filled with static and device specific value. If your chip can't be queried for it, I'm sure the embedded world has a solution for such cases. It's similar to MAC of a NIC device.
On 03. 07. 25 12:09 odp., Jiri Pirko wrote: > Wed, Jul 02, 2025 at 04:51:47PM +0200, ivecera@redhat.com wrote: >> On 02. 07. 25 2:01 odp., Jiri Pirko wrote: >>> Wed, Jul 02, 2025 at 01:43:38PM +0200, ivecera@redhat.com wrote: >>>> >>>> On 02. 07. 25 12:31 odp., Jiri Pirko wrote: >>>>> Sun, Jun 29, 2025 at 09:10:42PM +0200, ivecera@redhat.com wrote: >>>>>> Add .clock_id to zl3073x_dev structure that will be used by later >>>>>> commits introducing DPLL feature. The clock ID is required for DPLL >>>>>> device registration. >>>>>> >>>>>> To generate this ID, use chip ID read during device initialization. >>>>>> In case where multiple zl3073x based chips are present, the chip ID >>>>>> is shifted and lower bits are filled by an unique value - using >>>>>> the I2C device address for I2C connections and the chip-select value >>>>>> for SPI connections. >>>>> >>>>> You say that multiple chips may have the same chip ID? How is that >>>>> possible? Isn't it supposed to be unique? >>>>> I understand clock ID to be invariant regardless where you plug your >>>>> device. When you construct it from i2c address, sounds wrong. >>>> >>>> The chip id is not like serial number but it is like device id under >>>> PCI. So if you will have multiple chips with this chip id you have to >>>> distinguish somehow between them, this is the reason why I2C address >>>> is added into the final value. >>>> >>>> Anyway this device does not have any attribute that corresponds to >>>> clock id (as per our previous discussion) and it will be better to NOT >>>> require clock id from DPLL core side. >>> >>> Yes, better not to require it comparing to having it wrong. >> >> It looks that using clock_id==0 is safe from DPLL API point of view. >> The problem is if you will have multiple zl3073x based chips because >> the driver would call dpll_device_get(0 /* clock_id */, channel, module) >> >> For 1st chip (e.g. 2 channel) the driver will call: >> dpll_device_get(0, 0, module); >> dpll_device_get(0, 1, module); >> >> and for the second the same that is wrong. The clock_id would help to >> distinguish between them. >> >> Wouldn't it be better to use a random number for clock_id from the >> driver? > > I take my suggestion to not require it back, does not make sense. > > Clock id actually has a reason to exist from UAPI perspective. Checkout > dpll_device_find_from_nlattr(). The user passes CLOCK_ID attr (among > others) to obtain device by DPLL_CMD_DEVICE_ID_GET command. He expects > to get a result back from kernel regardless where the device is plugged > and across the reboots/rebinds. > > Clock id should be properly filled with static and device specific > value. If your chip can't be queried for it, I'm sure the embedded world > has a solution for such cases. It's similar to MAC of a NIC device. Yes, there are such cases and for such devices 'mac-address' property can be specified in the device tree. For our case I could extend the dpll device schema to include 'clock-id' or 'dpll-clock-id' 64bit property to allow specify clock ID for the devices that are unable to query this information from the hardware. Krzysztof, WDYT about it? Thanks, Ivan
© 2016 - 2025 Red Hat, Inc.