Re: [PATCH net-next v12 07/14] dpll: zl3073x: Add clock_id field

[Jiri Pirko]

Wed, Jul 02, 2025 at 04:51:47PM +0200, ivecera@xxxxxxxxxx wrote:
>On 02. 07. 25 2:01 odp., Jiri Pirko wrote:
>> Wed, Jul 02, 2025 at 01:43:38PM +0200, ivecera@xxxxxxxxxx wrote:
>> > 
>> > On 02. 07. 25 12:31 odp., Jiri Pirko wrote:
>> > > Sun, Jun 29, 2025 at 09:10:42PM +0200, ivecera@xxxxxxxxxx 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.