| Message ID | 20171108034911.16382-4-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: sfp: Fixes and debugging improvements | expand |
On Tue, Nov 07, 2017 at 07:49:10PM -0800, Florian Fainelli wrote: > Create separate enumerations for the SFP physical state (computed from GPIOs), > device state, module state, and actual state machine. This will make it easier > to make sure the correct states are used, and also pretty print those to help > debugging. The compiler does no type checking of these, so I don't see how it makes it any "easier to make sure the correct states are used".
On 11/08/2017 12:58 AM, Russell King - ARM Linux wrote: > On Tue, Nov 07, 2017 at 07:49:10PM -0800, Florian Fainelli wrote: >> Create separate enumerations for the SFP physical state (computed from GPIOs), >> device state, module state, and actual state machine. This will make it easier >> to make sure the correct states are used, and also pretty print those to help >> debugging. > > The compiler does no type checking of these, so I don't see how it > makes it any "easier to make sure the correct states are used". The types currently used (unsigned char, unsigned short) do not make it easy to spot what the enumeration is about and what values could be valid. Overall it seems to me like this improves code readability if nothing else.
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index dfb28b269687..e4662bc58c01 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -23,13 +23,17 @@ enum { GPIO_TX_DISABLE, GPIO_RATE_SELECT, GPIO_MAX, +}; +enum sfp_f_state { SFP_F_PRESENT = BIT(GPIO_MODDEF0), SFP_F_LOS = BIT(GPIO_LOS), SFP_F_TX_FAULT = BIT(GPIO_TX_FAULT), SFP_F_TX_DISABLE = BIT(GPIO_TX_DISABLE), SFP_F_RATE_SELECT = BIT(GPIO_RATE_SELECT), +}; +enum sfp_event_state { SFP_E_INSERT = 0, SFP_E_REMOVE, SFP_E_DEV_DOWN, @@ -39,15 +43,21 @@ enum { SFP_E_LOS_HIGH, SFP_E_LOS_LOW, SFP_E_TIMEOUT, +}; +enum sfp_mod_state { SFP_MOD_EMPTY = 0, SFP_MOD_PROBE, SFP_MOD_PRESENT, SFP_MOD_ERROR, +}; +enum sfp_dev_state { SFP_DEV_DOWN = 0, SFP_DEV_UP, +}; +enum sfp_sm_state { SFP_S_DOWN = 0, SFP_S_INIT, SFP_S_WAIT_LOS, @@ -115,9 +125,9 @@ struct sfp { struct delayed_work poll; struct delayed_work timeout; struct mutex sm_mutex; - unsigned char sm_mod_state; - unsigned char sm_dev_state; - unsigned short sm_state; + enum sfp_mod_state sm_mod_state; + enum sfp_dev_state sm_dev_state; + enum sfp_sm_state sm_state; unsigned int sm_retries; struct sfp_eeprom_id id;
Create separate enumerations for the SFP physical state (computed from GPIOs), device state, module state, and actual state machine. This will make it easier to make sure the correct states are used, and also pretty print those to help debugging. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/sfp.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)