Re: [PATCH net-next v2 3/3] net: phy: microchip_rds_ptp : Add PEROUT feature library for RDS PTP supported phys

Jakub Kicinski
2025-01-14T03:20:04Z

On Thu, 9 Jan 2025 15:55:33 +0530 Divya Koppera wrote:
> + switch (ts_on_nsec) {
> + case 200000000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_200MS_;
> + break;
> + case 100000000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_100MS_;
> + break;
> + case 50000000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_50MS_;
> + break;
> + case 10000000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_10MS_;
> + break;
> + case 5000000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_5MS_;
> + break;
> + case 1000000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_1MS_;
> + break;
> + case 500000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_500US_;
> + break;
> + case 100000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_100US_;
> + break;
> + case 50000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_50US_;
> + break;
> + case 10000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_10US_;
> + break;
> + case 5000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_5US_;
> + break;
> + case 1000:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_1US_;
> + break;
> + case 500:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_500NS_;
> + break;
> + case 100:
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_100NS_;
> + break;
> + default:
> + phydev_warn(phydev, "Using default pulse width of 200ms\n");
> + *pulse_width = MCHP_RDS_PTP_GEN_CFG_LTC_EVT_200MS_;
> + break;

+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_200MS_ 13
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_100MS_ 12
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_50MS_ 11
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_10MS_ 10
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_5MS_ 9
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_1MS_ 8
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_500US_ 7
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_100US_ 6
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_50US_ 5
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_10US_ 4
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_5US_ 3
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_1US_ 2
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_500NS_ 1
+#define MCHP_RDS_PTP_GEN_CFG_LTC_EVT_100NS_ 0

1) seems a bit weird to me that you go to 200ms whenever user asks for
a value that couldn't be provided exactly. Why not go to the next
good value?
2) this is not very well coded up, given that the values you're
translating to are a just natural numbers you can use a table

static const sup_on_necs[] = {
100, /* 100ns */
500, /* 500ns */
1000, /* 1us */
5000, /* 5us */
...
};

for (i = 0; i < ARRAY_SIZE(sup_on_necs); i++) {
if (ts_on_nsec <= sup_on_necs[i]) {
*pulse_width = i;
break;
}
}
--
pw-bot: cr