2017-12-12 17:54:11

by Egil Hjelmeland

Subject: [PATCH net-next] net: dsa: lan9303: Introduce lan9303_read_wait

Simplify lan9303_indirect_phy_wait_for_completion()
and lan9303_switch_wait_for_completion() by using a new function
lan9303_read_wait()

Signed-off-by: Egil Hjelmeland <[email protected]>
---
drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++-----------------------
1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c1b004fa64d9..96ccce0939d3 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned int offset, u32 *reg)
return -EIO;
}

+/* Wait a while until mask & reg == value. Otherwise return timeout. */
+static int lan9303_read_wait(struct lan9303 *chip, int offset, int mask,
+ char value)
+{
+ int i;
+
+ for (i = 0; i < 25; i++) {
+ u32 reg;
+ int ret;
+
+ ret = lan9303_read(chip->regmap, offset, &reg);
+ if (ret) {
+ dev_err(chip->dev, "%s failed to read offset %d: %d\n",
+ __func__, offset, ret);
+ return ret;
+ }
+ if ((reg & mask) == value)
+ return 0;
+ usleep_range(1000, 2000);
+ }
+ return -ETIMEDOUT;
+}
+
static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
{
int ret;
@@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)

static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
{
- int ret, i;
- u32 reg;
-
- for (i = 0; i < 25; i++) {
- ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
- if (ret) {
- dev_err(chip->dev,
- "Failed to read pmi access status: %d\n", ret);
- return ret;
- }
- if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
- return 0;
- usleep_range(1000, 2000);
- }
-
- return -EIO;
+ return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
+ LAN9303_PMI_ACCESS_MII_BUSY, 0);
}

static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)
@@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);

static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
{
- int ret, i;
- u32 reg;
-
- for (i = 0; i < 25; i++) {
- ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
- if (ret) {
- dev_err(chip->dev,
- "Failed to read csr command status: %d\n", ret);
- return ret;
- }
- if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
- return 0;
- usleep_range(1000, 2000);
- }
-
- return -EIO;
+ return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
+ LAN9303_SWITCH_CSR_CMD_BUSY, 0);
}

static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)
--
2.14.1

2017-12-12 18:08:30

by Vivien Didelot

Subject: Re: [PATCH net-next] net: dsa: lan9303: Introduce lan9303_read_wait

Hi Egil,

Egil Hjelmeland <[email protected]> writes:

> Simplify lan9303_indirect_phy_wait_for_completion()
> and lan9303_switch_wait_for_completion() by using a new function
> lan9303_read_wait()
>
> Signed-off-by: Egil Hjelmeland <[email protected]>
> ---
> drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++-----------------------
> 1 file changed, 27 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index c1b004fa64d9..96ccce0939d3 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned int offset, u32 *reg)
> return -EIO;
> }
>
> +/* Wait a while until mask & reg == value. Otherwise return timeout. */
> +static int lan9303_read_wait(struct lan9303 *chip, int offset, int mask,
> + char value)
> +{
> + int i;
> +
> + for (i = 0; i < 25; i++) {
> + u32 reg;
> + int ret;
> +
> + ret = lan9303_read(chip->regmap, offset, &reg);
> + if (ret) {
> + dev_err(chip->dev, "%s failed to read offset %d: %d\n",
> + __func__, offset, ret);
> + return ret;
> + }
> + if ((reg & mask) == value)
> + return 0;

That is weird to mix int, u32 and char for mask checking. I suggest you
to use the u32 type as well for both mask and value.

Looking at how lan9303_read_wait is called, the value argument doesn't
seem necessary. You can directly return 0 if (!(reg & mask)).

> + usleep_range(1000, 2000);
> + }
> + return -ETIMEDOUT;

A newline before the return statment would be appreciated.

> +}
> +
> static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
> {
> int ret;
> @@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
>
> static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
> {
> - int ret, i;
> - u32 reg;
> -
> - for (i = 0; i < 25; i++) {
> - ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
> - if (ret) {
> - dev_err(chip->dev,
> - "Failed to read pmi access status: %d\n", ret);
> - return ret;
> - }
> - if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
> - return 0;
> - usleep_range(1000, 2000);
> - }
> -
> - return -EIO;
> + return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
> + LAN9303_PMI_ACCESS_MII_BUSY, 0);
> }
>
> static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)
> @@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);
>
> static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
> {
> - int ret, i;
> - u32 reg;
> -
> - for (i = 0; i < 25; i++) {
> - ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
> - if (ret) {
> - dev_err(chip->dev,
> - "Failed to read csr command status: %d\n", ret);
> - return ret;
> - }
> - if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
> - return 0;
> - usleep_range(1000, 2000);
> - }
> -
> - return -EIO;
> + return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
> + LAN9303_SWITCH_CSR_CMD_BUSY, 0);
> }
>
> static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)


Thanks,

Vivien

2017-12-12 20:59:49

by Egil Hjelmeland

Subject: Re: [PATCH net-next] net: dsa: lan9303: Introduce lan9303_read_wait

Hi Vivien.

Den 12. des. 2017 19:08, skrev Vivien Didelot:
> Hi Egil,
>
> Egil Hjelmeland <[email protected]> writes:
>
>> Simplify lan9303_indirect_phy_wait_for_completion()
>> and lan9303_switch_wait_for_completion() by using a new function
>> lan9303_read_wait()
>>
>> Signed-off-by: Egil Hjelmeland <[email protected]>
>> ---
>> drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++-----------------------
>> 1 file changed, 27 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index c1b004fa64d9..96ccce0939d3 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned int offset, u32 *reg)
>> return -EIO;
>> }
>>
>> +/* Wait a while until mask & reg == value. Otherwise return timeout. */
>> +static int lan9303_read_wait(struct lan9303 *chip, int offset, int mask,
>> + char value)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < 25; i++) {
>> + u32 reg;
>> + int ret;
>> +
>> + ret = lan9303_read(chip->regmap, offset, &reg);
>> + if (ret) {
>> + dev_err(chip->dev, "%s failed to read offset %d: %d\n",
>> + __func__, offset, ret);
>> + return ret;
>> + }
>> + if ((reg & mask) == value)
>> + return 0;
>
> That is weird to mix int, u32 and char for mask checking. I suggest you
> to use the u32 type as well for both mask and value.
>

Good catch. Will fix that. Same with lan9303_csr_reg_wait() then.


> Looking at how lan9303_read_wait is called, the value argument doesn't
> seem necessary. You can directly return 0 if (!(reg & mask)).
>

The idea was to make in more general usable, in case one need to wait
for a bit to be set. But I don't have any example from the datasheet
that needs it, so I could take "value" away.

>> + usleep_range(1000, 2000);
>> + }
>> + return -ETIMEDOUT;
>
> A newline before the return statment would be appreciated.
>
Ok.

>> +}
>> +
>> static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
>> {
>> int ret;
>> @@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
>>
>> static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
>> {
>> - int ret, i;
>> - u32 reg;
>> -
>> - for (i = 0; i < 25; i++) {
>> - ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
>> - if (ret) {
>> - dev_err(chip->dev,
>> - "Failed to read pmi access status: %d\n", ret);
>> - return ret;
>> - }
>> - if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
>> - return 0;
>> - usleep_range(1000, 2000);
>> - }
>> -
>> - return -EIO;
>> + return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
>> + LAN9303_PMI_ACCESS_MII_BUSY, 0);
>> }
>>
>> static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)
>> @@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);
>>
>> static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
>> {
>> - int ret, i;
>> - u32 reg;
>> -
>> - for (i = 0; i < 25; i++) {
>> - ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
>> - if (ret) {
>> - dev_err(chip->dev,
>> - "Failed to read csr command status: %d\n", ret);
>> - return ret;
>> - }
>> - if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
>> - return 0;
>> - usleep_range(1000, 2000);
>> - }
>> -
>> - return -EIO;
>> + return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
>> + LAN9303_SWITCH_CSR_CMD_BUSY, 0);
>> }
>>
>> static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)
>
>
> Thanks,
>
> Vivien
>

Thanks, Egil