Re: [Linuxptp-devel] [PATCH 01/11] synce4l: add config knobs for SyncE

[Miroslav L. <mli...@re...>]

Thanks for your work on this feature. I don't have hardware to test
it. Just some comments on the code.

On Mon, May 02, 2022 at 11:05:55AM +0200, Arkadiusz Kubalewski wrote:
> Allow config interface to parse SyncE related config files.
> 

> +	if (type == PTP) {
> +		ci_tab = &config_tab_ptp[0];
> +		n_items = N_CONFIG_ITEMS_PTP;
> +	} else {
> +		ci_tab = &config_tab_synce[0];
> +		n_items = N_CONFIG_ITEMS_SYNCE;
> +	}
> +	n_items = (type == PTP ? N_CONFIG_ITEMS_PTP : N_CONFIG_ITEMS_SYNCE);

This looks duplicated. The last line shouldn't be there?

> +# Shell command to be executed in order to obtain current DPLL status of a
> +# device.
> +#
> +dpll_get_state_cmd		cat /sys/class/net/enp1s0f0/device/cgu_state

If I understand it correctly, this shell command is executed 50 times
per second. Wouldn't it be sufficient to specify it as a file to be
read directly instead instead of calling popen()?

I assume this is a temporary solution until the kernel provides a
standard ethtool/netlink API for monitoring and configuring SyncE.

-- 
Miroslav Lichvar