diff mbox series

[net-next,3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust

Message ID 20230905232610.1403647-4-arkadiusz.kubalewski@intel.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series dpll: add phase offset and phase adjust | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next

Commit Message

Arkadiusz Kubalewski Sept. 5, 2023, 11:26 p.m. UTC
Add callback ops for pin-dpll phase measurment.
Add callback for pin signal phase adjustment.
Add min and max phase adjustment values to pin proprties.
Invoke callbacks in dpll_netlink.c when filling the pin details to
provide user with phase related attribute values.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/dpll/dpll_netlink.c | 99 ++++++++++++++++++++++++++++++++++++-
 include/linux/dpll.h        | 18 +++++++
 2 files changed, 116 insertions(+), 1 deletion(-)

Comments

Wojciech Drewek Sept. 6, 2023, 8:02 a.m. UTC | #1
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Arkadiusz Kubalewski
> Sent: Wednesday, September 6, 2023 1:26 AM
> To: kuba@kernel.org; jiri@resnulli.us; jonathan.lemon@gmail.com;
> pabeni@redhat.com; vadim.fedorenko@linux.dev
> Cc: bvanassche@acm.org; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org; linux-clk@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support
> for pin-dpll signal phase offset/adjust
> 
> Add callback ops for pin-dpll phase measurment.
> Add callback for pin signal phase adjustment.
> Add min and max phase adjustment values to pin proprties.
> Invoke callbacks in dpll_netlink.c when filling the pin details to
> provide user with phase related attribute values.
> 
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> ---
>  drivers/dpll/dpll_netlink.c | 99
> ++++++++++++++++++++++++++++++++++++-
>  include/linux/dpll.h        | 18 +++++++
>  2 files changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> index 764437a0661b..548517d9ca4c 100644
> --- a/drivers/dpll/dpll_netlink.c
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -212,6 +212,53 @@ dpll_msg_add_pin_direction(struct sk_buff *msg,
> struct dpll_pin *pin,
>  	return 0;
>  }
> 
> +static int
> +dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin *pin,
> +			      struct dpll_pin_ref *ref,
> +			      struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> +	struct dpll_device *dpll = ref->dpll;
> +	s32 phase_adjust;
> +	int ret;
> +
> +	if (!ops->phase_adjust_get)
> +		return 0;

Why 0 is returned here? If it's intended, I would put a comment stating why.
Same thing in dpll_msg_add_phase_offset.

> +	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
> +				    dpll, dpll_priv(dpll),
> +				    &phase_adjust, extack);
> +	if (ret)
> +		return ret;
> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}
> +
> +static int
> +dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
> +			  struct dpll_pin_ref *ref,
> +			  struct netlink_ext_ack *extack)
> +{
> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> +	struct dpll_device *dpll = ref->dpll;
> +	s64 phase_offset;
> +	int ret;
> +
> +	if (!ops->phase_offset_get)
> +		return 0;
> +	ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
> +				    dpll, dpll_priv(dpll), &phase_offset,
> +				    extack);
> +	if (ret)
> +		return ret;
> +	if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET,
> sizeof(phase_offset),
> +			  &phase_offset, DPLL_A_PIN_PAD))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}
> +
>  static int
>  dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
>  		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
> @@ -330,6 +377,9 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg, struct
> dpll_pin *pin,
>  		if (ret)
>  			goto nest_cancel;
>  		ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
> +		if (ret)
> +			goto nest_cancel;
> +		ret = dpll_msg_add_phase_offset(msg, pin, ref, extack);
>  		if (ret)
>  			goto nest_cancel;
>  		nla_nest_end(msg, attr);
> @@ -377,6 +427,15 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct
> dpll_pin *pin,
>  	if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities))
>  		return -EMSGSIZE;
>  	ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
> +	if (ret)
> +		return ret;
> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
> +			prop->phase_range.min))
> +		return -EMSGSIZE;
> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX,
> +			prop->phase_range.max))
> +		return -EMSGSIZE;
> +	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
>  	if (ret)
>  		return ret;
>  	if (xa_empty(&pin->parent_refs))
> @@ -416,7 +475,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct
> sk_buff *msg,
>  	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>  		return -EMSGSIZE;
> 
> -	return ret;
> +	return 0;
>  }
> 
>  static int
> @@ -705,6 +764,39 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct
> dpll_device *dpll,
>  	return 0;
>  }
> 
> +static int
> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
> +		       struct netlink_ext_ack *extack)
> +{
> +	struct dpll_pin_ref *ref;
> +	unsigned long i;
> +	s32 phase_adj;
> +	int ret;
> +
> +	phase_adj = nla_get_s32(phase_adj_attr);
> +	if (phase_adj > pin->prop->phase_range.max ||
> +	    phase_adj < pin->prop->phase_range.min) {
> +		NL_SET_ERR_MSG(extack, "phase adjust value not
> supported");
> +		return -EINVAL;
> +	}
> +	xa_for_each(&pin->dpll_refs, i, ref) {
> +		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> +		struct dpll_device *dpll = ref->dpll;
> +
> +		if (!ops->phase_adjust_set)
> +			return -EOPNOTSUPP;
> +		ret = ops->phase_adjust_set(pin,
> +					    dpll_pin_on_dpll_priv(dpll, pin),
> +					    dpll, dpll_priv(dpll), phase_adj,
> +					    extack);
> +		if (ret)
> +			return ret;
> +	}
> +	__dpll_pin_change_ntf(pin);
> +
> +	return 0;
> +}
> +
>  static int
>  dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr *parent_nest,
>  			   struct netlink_ext_ack *extack)
> @@ -793,6 +885,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct
> genl_info *info)
>  			if (ret)
>  				return ret;
>  			break;
> +		case DPLL_A_PIN_PHASE_ADJUST:
> +			ret = dpll_pin_phase_adj_set(pin, a, info->extack);
> +			if (ret)
> +				return ret;
> +			break;
>  		case DPLL_A_PIN_PARENT_DEVICE:
>  			ret = dpll_pin_parent_device_set(pin, a, info->extack);
>  			if (ret)
> diff --git a/include/linux/dpll.h b/include/linux/dpll.h
> index bbc480cd2932..578fc5fa3750 100644
> --- a/include/linux/dpll.h
> +++ b/include/linux/dpll.h
> @@ -68,6 +68,18 @@ struct dpll_pin_ops {
>  	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
>  			const struct dpll_device *dpll, void *dpll_priv,
>  			const u32 prio, struct netlink_ext_ack *extack);
> +	int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv,
> +				const struct dpll_device *dpll, void *dpll_priv,
> +				s64 *phase_offset,
> +				struct netlink_ext_ack *extack);
> +	int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv,
> +				const struct dpll_device *dpll, void *dpll_priv,
> +				s32 *phase_adjust,
> +				struct netlink_ext_ack *extack);
> +	int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv,
> +				const struct dpll_device *dpll, void *dpll_priv,
> +				const s32 phase_adjust,
> +				struct netlink_ext_ack *extack);
>  };
> 
>  struct dpll_pin_frequency {
> @@ -91,6 +103,11 @@ struct dpll_pin_frequency {
>  #define DPLL_PIN_FREQUENCY_DCF77 \
>  	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
> 
> +struct dpll_pin_phase_adjust_range {
> +	s32 min;
> +	s32 max;
> +};
> +
>  struct dpll_pin_properties {
>  	const char *board_label;
>  	const char *panel_label;
> @@ -99,6 +116,7 @@ struct dpll_pin_properties {
>  	unsigned long capabilities;
>  	u32 freq_supported_num;
>  	struct dpll_pin_frequency *freq_supported;
> +	struct dpll_pin_phase_adjust_range phase_range;
>  };
> 
>  #if IS_ENABLED(CONFIG_DPLL)
> --
> 2.38.1
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Arkadiusz Kubalewski Sept. 6, 2023, 10:26 a.m. UTC | #2
>From: Drewek, Wojciech <wojciech.drewek@intel.com>
>Sent: Wednesday, September 6, 2023 10:02 AM
>
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>> Arkadiusz Kubalewski
>> Sent: Wednesday, September 6, 2023 1:26 AM
>> To: kuba@kernel.org; jiri@resnulli.us; jonathan.lemon@gmail.com;
>> pabeni@redhat.com; vadim.fedorenko@linux.dev
>> Cc: bvanassche@acm.org; netdev@vger.kernel.org; intel-wired-
>> lan@lists.osuosl.org; linux-clk@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org
>> Subject: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add
>> support
>> for pin-dpll signal phase offset/adjust
>>
>> Add callback ops for pin-dpll phase measurment.
>> Add callback for pin signal phase adjustment.
>> Add min and max phase adjustment values to pin proprties.
>> Invoke callbacks in dpll_netlink.c when filling the pin details to
>> provide user with phase related attribute values.
>>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> ---
>>  drivers/dpll/dpll_netlink.c | 99
>> ++++++++++++++++++++++++++++++++++++-
>>  include/linux/dpll.h        | 18 +++++++
>>  2 files changed, 116 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>> index 764437a0661b..548517d9ca4c 100644
>> --- a/drivers/dpll/dpll_netlink.c
>> +++ b/drivers/dpll/dpll_netlink.c
>> @@ -212,6 +212,53 @@ dpll_msg_add_pin_direction(struct sk_buff *msg,
>> struct dpll_pin *pin,
>>  	return 0;
>>  }
>>
>> +static int
>> +dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin *pin,
>> +			      struct dpll_pin_ref *ref,
>> +			      struct netlink_ext_ack *extack)
>> +{
>> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>> +	struct dpll_device *dpll = ref->dpll;
>> +	s32 phase_adjust;
>> +	int ret;
>> +
>> +	if (!ops->phase_adjust_get)
>> +		return 0;
>
>Why 0 is returned here? If it's intended, I would put a comment stating
>why.
>Same thing in dpll_msg_add_phase_offset.

The callback is optional, any driver implementing dpll interface doesn't
have to implement this callback and it must not be seen as an error.
Callback that are required are pointed out in documentation:
Documentation/driver-api/dpll.rst

All the optional callbacks are returning this way, I don't see a point
in adding extra comment here.

Thank you!
Arkadiusz

>
>> +	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>> +				    dpll, dpll_priv(dpll),
>> +				    &phase_adjust, extack);
>> +	if (ret)
>> +		return ret;
>> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust))
>> +		return -EMSGSIZE;
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
>> +			  struct dpll_pin_ref *ref,
>> +			  struct netlink_ext_ack *extack)
>> +{
>> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>> +	struct dpll_device *dpll = ref->dpll;
>> +	s64 phase_offset;
>> +	int ret;
>> +
>> +	if (!ops->phase_offset_get)
>> +		return 0;
>> +	ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>> +				    dpll, dpll_priv(dpll), &phase_offset,
>> +				    extack);
>> +	if (ret)
>> +		return ret;
>> +	if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET,
>> sizeof(phase_offset),
>> +			  &phase_offset, DPLL_A_PIN_PAD))
>> +		return -EMSGSIZE;
>> +
>> +	return 0;
>> +}
>> +
>>  static int
>>  dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
>>  		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
>> @@ -330,6 +377,9 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg, struct
>> dpll_pin *pin,
>>  		if (ret)
>>  			goto nest_cancel;
>>  		ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
>> +		if (ret)
>> +			goto nest_cancel;
>> +		ret = dpll_msg_add_phase_offset(msg, pin, ref, extack);
>>  		if (ret)
>>  			goto nest_cancel;
>>  		nla_nest_end(msg, attr);
>> @@ -377,6 +427,15 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct
>> dpll_pin *pin,
>>  	if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities))
>>  		return -EMSGSIZE;
>>  	ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
>> +	if (ret)
>> +		return ret;
>> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
>> +			prop->phase_range.min))
>> +		return -EMSGSIZE;
>> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX,
>> +			prop->phase_range.max))
>> +		return -EMSGSIZE;
>> +	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
>>  	if (ret)
>>  		return ret;
>>  	if (xa_empty(&pin->parent_refs))
>> @@ -416,7 +475,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>> sk_buff *msg,
>>  	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>>  		return -EMSGSIZE;
>>
>> -	return ret;
>> +	return 0;
>>  }
>>
>>  static int
>> @@ -705,6 +764,39 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct
>> dpll_device *dpll,
>>  	return 0;
>>  }
>>
>> +static int
>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
>> *phase_adj_attr,
>> +		       struct netlink_ext_ack *extack)
>> +{
>> +	struct dpll_pin_ref *ref;
>> +	unsigned long i;
>> +	s32 phase_adj;
>> +	int ret;
>> +
>> +	phase_adj = nla_get_s32(phase_adj_attr);
>> +	if (phase_adj > pin->prop->phase_range.max ||
>> +	    phase_adj < pin->prop->phase_range.min) {
>> +		NL_SET_ERR_MSG(extack, "phase adjust value not
>> supported");
>> +		return -EINVAL;
>> +	}
>> +	xa_for_each(&pin->dpll_refs, i, ref) {
>> +		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>> +		struct dpll_device *dpll = ref->dpll;
>> +
>> +		if (!ops->phase_adjust_set)
>> +			return -EOPNOTSUPP;
>> +		ret = ops->phase_adjust_set(pin,
>> +					    dpll_pin_on_dpll_priv(dpll, pin),
>> +					    dpll, dpll_priv(dpll), phase_adj,
>> +					    extack);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	__dpll_pin_change_ntf(pin);
>> +
>> +	return 0;
>> +}
>> +
>>  static int
>>  dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr
>> *parent_nest,
>>  			   struct netlink_ext_ack *extack)
>> @@ -793,6 +885,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin,
>> struct
>> genl_info *info)
>>  			if (ret)
>>  				return ret;
>>  			break;
>> +		case DPLL_A_PIN_PHASE_ADJUST:
>> +			ret = dpll_pin_phase_adj_set(pin, a, info->extack);
>> +			if (ret)
>> +				return ret;
>> +			break;
>>  		case DPLL_A_PIN_PARENT_DEVICE:
>>  			ret = dpll_pin_parent_device_set(pin, a, info->extack);
>>  			if (ret)
>> diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>> index bbc480cd2932..578fc5fa3750 100644
>> --- a/include/linux/dpll.h
>> +++ b/include/linux/dpll.h
>> @@ -68,6 +68,18 @@ struct dpll_pin_ops {
>>  	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
>>  			const struct dpll_device *dpll, void *dpll_priv,
>>  			const u32 prio, struct netlink_ext_ack *extack);
>> +	int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv,
>> +				const struct dpll_device *dpll, void *dpll_priv,
>> +				s64 *phase_offset,
>> +				struct netlink_ext_ack *extack);
>> +	int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv,
>> +				const struct dpll_device *dpll, void *dpll_priv,
>> +				s32 *phase_adjust,
>> +				struct netlink_ext_ack *extack);
>> +	int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv,
>> +				const struct dpll_device *dpll, void *dpll_priv,
>> +				const s32 phase_adjust,
>> +				struct netlink_ext_ack *extack);
>>  };
>>
>>  struct dpll_pin_frequency {
>> @@ -91,6 +103,11 @@ struct dpll_pin_frequency {
>>  #define DPLL_PIN_FREQUENCY_DCF77 \
>>  	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
>>
>> +struct dpll_pin_phase_adjust_range {
>> +	s32 min;
>> +	s32 max;
>> +};
>> +
>>  struct dpll_pin_properties {
>>  	const char *board_label;
>>  	const char *panel_label;
>> @@ -99,6 +116,7 @@ struct dpll_pin_properties {
>>  	unsigned long capabilities;
>>  	u32 freq_supported_num;
>>  	struct dpll_pin_frequency *freq_supported;
>> +	struct dpll_pin_phase_adjust_range phase_range;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DPLL)
>> --
>> 2.38.1
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Wojciech Drewek Sept. 6, 2023, 11:45 a.m. UTC | #3
> -----Original Message-----
> From: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>
> Sent: Wednesday, September 6, 2023 12:26 PM
> To: Drewek, Wojciech <wojciech.drewek@intel.com>; kuba@kernel.org;
> jiri@resnulli.us; jonathan.lemon@gmail.com; pabeni@redhat.com;
> vadim.fedorenko@linux.dev
> Cc: bvanassche@acm.org; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org; linux-clk@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: RE: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add
> support for pin-dpll signal phase offset/adjust
> 
> >From: Drewek, Wojciech <wojciech.drewek@intel.com>
> >Sent: Wednesday, September 6, 2023 10:02 AM
> >
> >> -----Original Message-----
> >> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> >> Arkadiusz Kubalewski
> >> Sent: Wednesday, September 6, 2023 1:26 AM
> >> To: kuba@kernel.org; jiri@resnulli.us; jonathan.lemon@gmail.com;
> >> pabeni@redhat.com; vadim.fedorenko@linux.dev
> >> Cc: bvanassche@acm.org; netdev@vger.kernel.org; intel-wired-
> >> lan@lists.osuosl.org; linux-clk@vger.kernel.org; linux-arm-
> >> kernel@lists.infradead.org
> >> Subject: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add
> >> support
> >> for pin-dpll signal phase offset/adjust
> >>
> >> Add callback ops for pin-dpll phase measurment.
> >> Add callback for pin signal phase adjustment.
> >> Add min and max phase adjustment values to pin proprties.
> >> Invoke callbacks in dpll_netlink.c when filling the pin details to
> >> provide user with phase related attribute values.
> >>
> >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> >> ---
> >>  drivers/dpll/dpll_netlink.c | 99
> >> ++++++++++++++++++++++++++++++++++++-
> >>  include/linux/dpll.h        | 18 +++++++
> >>  2 files changed, 116 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> >> index 764437a0661b..548517d9ca4c 100644
> >> --- a/drivers/dpll/dpll_netlink.c
> >> +++ b/drivers/dpll/dpll_netlink.c
> >> @@ -212,6 +212,53 @@ dpll_msg_add_pin_direction(struct sk_buff *msg,
> >> struct dpll_pin *pin,
> >>  	return 0;
> >>  }
> >>
> >> +static int
> >> +dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin
> *pin,
> >> +			      struct dpll_pin_ref *ref,
> >> +			      struct netlink_ext_ack *extack)
> >> +{
> >> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> >> +	struct dpll_device *dpll = ref->dpll;
> >> +	s32 phase_adjust;
> >> +	int ret;
> >> +
> >> +	if (!ops->phase_adjust_get)
> >> +		return 0;
> >
> >Why 0 is returned here? If it's intended, I would put a comment stating
> >why.
> >Same thing in dpll_msg_add_phase_offset.
> 
> The callback is optional, any driver implementing dpll interface doesn't
> have to implement this callback and it must not be seen as an error.
> Callback that are required are pointed out in documentation:
> Documentation/driver-api/dpll.rst
> 
> All the optional callbacks are returning this way, I don't see a point
> in adding extra comment here.
> 
> Thank you!
> Arkadiusz

I see,
Thanks

> 
> >
> >> +	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
> >> +				    dpll, dpll_priv(dpll),
> >> +				    &phase_adjust, extack);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust))
> >> +		return -EMSGSIZE;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int
> >> +dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
> >> +			  struct dpll_pin_ref *ref,
> >> +			  struct netlink_ext_ack *extack)
> >> +{
> >> +	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> >> +	struct dpll_device *dpll = ref->dpll;
> >> +	s64 phase_offset;
> >> +	int ret;
> >> +
> >> +	if (!ops->phase_offset_get)
> >> +		return 0;
> >> +	ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
> >> +				    dpll, dpll_priv(dpll), &phase_offset,
> >> +				    extack);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET,
> >> sizeof(phase_offset),
> >> +			  &phase_offset, DPLL_A_PIN_PAD))
> >> +		return -EMSGSIZE;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int
> >>  dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
> >>  		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
> >> @@ -330,6 +377,9 @@ dpll_msg_add_pin_dplls(struct sk_buff *msg,
> struct
> >> dpll_pin *pin,
> >>  		if (ret)
> >>  			goto nest_cancel;
> >>  		ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
> >> +		if (ret)
> >> +			goto nest_cancel;
> >> +		ret = dpll_msg_add_phase_offset(msg, pin, ref, extack);
> >>  		if (ret)
> >>  			goto nest_cancel;
> >>  		nla_nest_end(msg, attr);
> >> @@ -377,6 +427,15 @@ dpll_cmd_pin_get_one(struct sk_buff *msg,
> struct
> >> dpll_pin *pin,
> >>  	if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities))
> >>  		return -EMSGSIZE;
> >>  	ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
> >> +			prop->phase_range.min))
> >> +		return -EMSGSIZE;
> >> +	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX,
> >> +			prop->phase_range.max))
> >> +		return -EMSGSIZE;
> >> +	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
> >>  	if (ret)
> >>  		return ret;
> >>  	if (xa_empty(&pin->parent_refs))
> >> @@ -416,7 +475,7 @@ dpll_device_get_one(struct dpll_device *dpll,
> struct
> >> sk_buff *msg,
> >>  	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
> >>  		return -EMSGSIZE;
> >>
> >> -	return ret;
> >> +	return 0;
> >>  }
> >>
> >>  static int
> >> @@ -705,6 +764,39 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct
> >> dpll_device *dpll,
> >>  	return 0;
> >>  }
> >>
> >> +static int
> >> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
> >> *phase_adj_attr,
> >> +		       struct netlink_ext_ack *extack)
> >> +{
> >> +	struct dpll_pin_ref *ref;
> >> +	unsigned long i;
> >> +	s32 phase_adj;
> >> +	int ret;
> >> +
> >> +	phase_adj = nla_get_s32(phase_adj_attr);
> >> +	if (phase_adj > pin->prop->phase_range.max ||
> >> +	    phase_adj < pin->prop->phase_range.min) {
> >> +		NL_SET_ERR_MSG(extack, "phase adjust value not
> >> supported");
> >> +		return -EINVAL;
> >> +	}
> >> +	xa_for_each(&pin->dpll_refs, i, ref) {
> >> +		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> >> +		struct dpll_device *dpll = ref->dpll;
> >> +
> >> +		if (!ops->phase_adjust_set)
> >> +			return -EOPNOTSUPP;
> >> +		ret = ops->phase_adjust_set(pin,
> >> +					    dpll_pin_on_dpll_priv(dpll, pin),
> >> +					    dpll, dpll_priv(dpll), phase_adj,
> >> +					    extack);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +	__dpll_pin_change_ntf(pin);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int
> >>  dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr
> >> *parent_nest,
> >>  			   struct netlink_ext_ack *extack)
> >> @@ -793,6 +885,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin,
> >> struct
> >> genl_info *info)
> >>  			if (ret)
> >>  				return ret;
> >>  			break;
> >> +		case DPLL_A_PIN_PHASE_ADJUST:
> >> +			ret = dpll_pin_phase_adj_set(pin, a, info->extack);
> >> +			if (ret)
> >> +				return ret;
> >> +			break;
> >>  		case DPLL_A_PIN_PARENT_DEVICE:
> >>  			ret = dpll_pin_parent_device_set(pin, a, info->extack);
> >>  			if (ret)
> >> diff --git a/include/linux/dpll.h b/include/linux/dpll.h
> >> index bbc480cd2932..578fc5fa3750 100644
> >> --- a/include/linux/dpll.h
> >> +++ b/include/linux/dpll.h
> >> @@ -68,6 +68,18 @@ struct dpll_pin_ops {
> >>  	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
> >>  			const struct dpll_device *dpll, void *dpll_priv,
> >>  			const u32 prio, struct netlink_ext_ack *extack);
> >> +	int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv,
> >> +				const struct dpll_device *dpll, void *dpll_priv,
> >> +				s64 *phase_offset,
> >> +				struct netlink_ext_ack *extack);
> >> +	int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv,
> >> +				const struct dpll_device *dpll, void *dpll_priv,
> >> +				s32 *phase_adjust,
> >> +				struct netlink_ext_ack *extack);
> >> +	int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv,
> >> +				const struct dpll_device *dpll, void *dpll_priv,
> >> +				const s32 phase_adjust,
> >> +				struct netlink_ext_ack *extack);
> >>  };
> >>
> >>  struct dpll_pin_frequency {
> >> @@ -91,6 +103,11 @@ struct dpll_pin_frequency {
> >>  #define DPLL_PIN_FREQUENCY_DCF77 \
> >>  	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
> >>
> >> +struct dpll_pin_phase_adjust_range {
> >> +	s32 min;
> >> +	s32 max;
> >> +};
> >> +
> >>  struct dpll_pin_properties {
> >>  	const char *board_label;
> >>  	const char *panel_label;
> >> @@ -99,6 +116,7 @@ struct dpll_pin_properties {
> >>  	unsigned long capabilities;
> >>  	u32 freq_supported_num;
> >>  	struct dpll_pin_frequency *freq_supported;
> >> +	struct dpll_pin_phase_adjust_range phase_range;
> >>  };
> >>
> >>  #if IS_ENABLED(CONFIG_DPLL)
> >> --
> >> 2.38.1
> >>
> >> _______________________________________________
> >> Intel-wired-lan mailing list
> >> Intel-wired-lan@osuosl.org
> >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
diff mbox series

Patch

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 764437a0661b..548517d9ca4c 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -212,6 +212,53 @@  dpll_msg_add_pin_direction(struct sk_buff *msg, struct dpll_pin *pin,
 	return 0;
 }
 
+static int
+dpll_msg_add_pin_phase_adjust(struct sk_buff *msg, struct dpll_pin *pin,
+			      struct dpll_pin_ref *ref,
+			      struct netlink_ext_ack *extack)
+{
+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+	struct dpll_device *dpll = ref->dpll;
+	s32 phase_adjust;
+	int ret;
+
+	if (!ops->phase_adjust_get)
+		return 0;
+	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				    dpll, dpll_priv(dpll),
+				    &phase_adjust, extack);
+	if (ret)
+		return ret;
+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST, phase_adjust))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int
+dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
+			  struct dpll_pin_ref *ref,
+			  struct netlink_ext_ack *extack)
+{
+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+	struct dpll_device *dpll = ref->dpll;
+	s64 phase_offset;
+	int ret;
+
+	if (!ops->phase_offset_get)
+		return 0;
+	ret = ops->phase_offset_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				    dpll, dpll_priv(dpll), &phase_offset,
+				    extack);
+	if (ret)
+		return ret;
+	if (nla_put_64bit(msg, DPLL_A_PIN_PHASE_OFFSET, sizeof(phase_offset),
+			  &phase_offset, DPLL_A_PIN_PAD))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
 static int
 dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
 		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
@@ -330,6 +377,9 @@  dpll_msg_add_pin_dplls(struct sk_buff *msg, struct dpll_pin *pin,
 		if (ret)
 			goto nest_cancel;
 		ret = dpll_msg_add_pin_direction(msg, pin, ref, extack);
+		if (ret)
+			goto nest_cancel;
+		ret = dpll_msg_add_phase_offset(msg, pin, ref, extack);
 		if (ret)
 			goto nest_cancel;
 		nla_nest_end(msg, attr);
@@ -377,6 +427,15 @@  dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
 	if (nla_put_u32(msg, DPLL_A_PIN_CAPABILITIES, prop->capabilities))
 		return -EMSGSIZE;
 	ret = dpll_msg_add_pin_freq(msg, pin, ref, extack);
+	if (ret)
+		return ret;
+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MIN,
+			prop->phase_range.min))
+		return -EMSGSIZE;
+	if (nla_put_s32(msg, DPLL_A_PIN_PHASE_ADJUST_MAX,
+			prop->phase_range.max))
+		return -EMSGSIZE;
+	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
 	if (ret)
 		return ret;
 	if (xa_empty(&pin->parent_refs))
@@ -416,7 +475,7 @@  dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
 	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
 		return -EMSGSIZE;
 
-	return ret;
+	return 0;
 }
 
 static int
@@ -705,6 +764,39 @@  dpll_pin_direction_set(struct dpll_pin *pin, struct dpll_device *dpll,
 	return 0;
 }
 
+static int
+dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
+		       struct netlink_ext_ack *extack)
+{
+	struct dpll_pin_ref *ref;
+	unsigned long i;
+	s32 phase_adj;
+	int ret;
+
+	phase_adj = nla_get_s32(phase_adj_attr);
+	if (phase_adj > pin->prop->phase_range.max ||
+	    phase_adj < pin->prop->phase_range.min) {
+		NL_SET_ERR_MSG(extack, "phase adjust value not supported");
+		return -EINVAL;
+	}
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+		struct dpll_device *dpll = ref->dpll;
+
+		if (!ops->phase_adjust_set)
+			return -EOPNOTSUPP;
+		ret = ops->phase_adjust_set(pin,
+					    dpll_pin_on_dpll_priv(dpll, pin),
+					    dpll, dpll_priv(dpll), phase_adj,
+					    extack);
+		if (ret)
+			return ret;
+	}
+	__dpll_pin_change_ntf(pin);
+
+	return 0;
+}
+
 static int
 dpll_pin_parent_device_set(struct dpll_pin *pin, struct nlattr *parent_nest,
 			   struct netlink_ext_ack *extack)
@@ -793,6 +885,11 @@  dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
 			if (ret)
 				return ret;
 			break;
+		case DPLL_A_PIN_PHASE_ADJUST:
+			ret = dpll_pin_phase_adj_set(pin, a, info->extack);
+			if (ret)
+				return ret;
+			break;
 		case DPLL_A_PIN_PARENT_DEVICE:
 			ret = dpll_pin_parent_device_set(pin, a, info->extack);
 			if (ret)
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index bbc480cd2932..578fc5fa3750 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -68,6 +68,18 @@  struct dpll_pin_ops {
 	int (*prio_set)(const struct dpll_pin *pin, void *pin_priv,
 			const struct dpll_device *dpll, void *dpll_priv,
 			const u32 prio, struct netlink_ext_ack *extack);
+	int (*phase_offset_get)(const struct dpll_pin *pin, void *pin_priv,
+				const struct dpll_device *dpll, void *dpll_priv,
+				s64 *phase_offset,
+				struct netlink_ext_ack *extack);
+	int (*phase_adjust_get)(const struct dpll_pin *pin, void *pin_priv,
+				const struct dpll_device *dpll, void *dpll_priv,
+				s32 *phase_adjust,
+				struct netlink_ext_ack *extack);
+	int (*phase_adjust_set)(const struct dpll_pin *pin, void *pin_priv,
+				const struct dpll_device *dpll, void *dpll_priv,
+				const s32 phase_adjust,
+				struct netlink_ext_ack *extack);
 };
 
 struct dpll_pin_frequency {
@@ -91,6 +103,11 @@  struct dpll_pin_frequency {
 #define DPLL_PIN_FREQUENCY_DCF77 \
 	DPLL_PIN_FREQUENCY(DPLL_PIN_FREQUENCY_77_5_KHZ)
 
+struct dpll_pin_phase_adjust_range {
+	s32 min;
+	s32 max;
+};
+
 struct dpll_pin_properties {
 	const char *board_label;
 	const char *panel_label;
@@ -99,6 +116,7 @@  struct dpll_pin_properties {
 	unsigned long capabilities;
 	u32 freq_supported_num;
 	struct dpll_pin_frequency *freq_supported;
+	struct dpll_pin_phase_adjust_range phase_range;
 };
 
 #if IS_ENABLED(CONFIG_DPLL)