Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

drivers: ethernet: ksz8081: streamline codebase #81816

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+22 −29
Open

drivers: ethernet: ksz8081: streamline codebase #81816

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2d2001
drivers: ethernet: ksz8081: simplify error handling in `get_link`
ndrs-pst Nov 23, 2024
97ac694
drivers: ethernet: ksz8081: simplify gpio reset logic in `reset`
ndrs-pst Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 22 additions & 29 deletions drivers/ethernet/phy/phy_microchip_ksz8081.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,35 +161,28 @@ static int phy_mc_ksz8081_get_link(const struct device *dev,
ret = phy_mc_ksz8081_read(dev, MII_BMSR, &bmsr);
if (ret) {
LOG_ERR("Error reading phy (%d) basic status register", config->addr);
k_mutex_unlock(&data->mutex);
return ret;
goto done;
}
state->is_up = bmsr & MII_BMSR_LINK_STATUS;

if (!state->is_up) {
k_mutex_unlock(&data->mutex);
goto result;
}

/* Read currently configured advertising options */
ret = phy_mc_ksz8081_read(dev, MII_ANAR, &anar);
if (ret) {
LOG_ERR("Error reading phy (%d) advertising register", config->addr);
k_mutex_unlock(&data->mutex);
return ret;
goto done;
}

/* Read link partner capability */
ret = phy_mc_ksz8081_read(dev, MII_ANLPAR, &anlpar);
if (ret) {
LOG_ERR("Error reading phy (%d) link partner register", config->addr);
k_mutex_unlock(&data->mutex);
return ret;
goto done;
}

/* Unlock mutex */
k_mutex_unlock(&data->mutex);

uint32_t mutual_capabilities = anar & anlpar;

if (mutual_capabilities & MII_ADVERTISE_100_FULL) {
Expand All @@ -214,6 +207,9 @@ static int phy_mc_ksz8081_get_link(const struct device *dev,
}
}

done:
k_mutex_unlock(&data->mutex);

return ret;
}

Expand Down Expand Up @@ -269,9 +265,6 @@ static int phy_mc_ksz8081_static_cfg(const struct device *dev)

static int phy_mc_ksz8081_reset(const struct device *dev)
{
#if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios)
const struct mc_ksz8081_config *config = dev->config;
#endif /* DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios) */
struct mc_ksz8081_data *data = dev->data;
int ret;

Expand All @@ -283,27 +276,27 @@ static int phy_mc_ksz8081_reset(const struct device *dev)
}

#if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios)
if (!config->reset_gpio.port) {
goto skip_reset_gpio;
Copy link
Member

@decsny decsny Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rest of the PR is good but I preferred the negative check here to avoid the extra indentation (ie keeping indentation strictly max of 2 indents), maybe would be better to make the gpio reset into a helper function, like:

#if DT_ANY_INST_HAS_PROP_STATUS_OKAY
static int phy_ksz8081_reset_gpio(config *)
{
  int ret;

  if (!config->reset_gpio.port) {
    return 0;
  }

  /* rest of reset code */
  return ret;
}
#else 
#define phy_ksz8081_reset_gpio(...)
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing helper functions might cause ambiguity between a missing GPIO port (returning 0) and a successful GPIO operation (also returning 0).
Adding extra return values to differentiate them would complicate the code.

Given the constraint of maintaining a max of 2 indents, I suggest reverting to the previous code structure but improving it by moving const struct mc_ksz8081_config *config = dev->config; inside the #if block.

WDYT? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already silence if there's a missing gpios port, maybe what you want to do is add some kind of log message

}
const struct mc_ksz8081_config *config = dev->config;

/* Start reset */
ret = gpio_pin_set_dt(&config->reset_gpio, 0);
if (ret) {
goto done;
}
if (config->reset_gpio.port) {
/* Start reset */
ret = gpio_pin_set_dt(&config->reset_gpio, 0);
if (ret) {
goto done;
}

/* Wait for at least 500 us as specified by datasheet */
k_busy_wait(1000);
/* Wait for at least 500 us as specified by datasheet */
k_busy_wait(1000);

/* Reset over */
ret = gpio_pin_set_dt(&config->reset_gpio, 1);
/* Reset over */
ret = gpio_pin_set_dt(&config->reset_gpio, 1);

/* After deasserting reset, must wait at least 100 us to use programming interface */
k_busy_wait(200);
/* After deasserting reset, must wait at least 100 us to use programming interface
*/
k_busy_wait(200);

goto done;
skip_reset_gpio:
goto done;
}
#endif /* DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios) */
ret = phy_mc_ksz8081_write(dev, MII_BMCR, MII_BMCR_RESET);
if (ret) {
Expand Down
Loading