> +static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
> +{
> + long timeleft;
> + u32 regval;
> + int ret;
> +
> + /* Perform software reset with both protected and unprotected control
> + * commands because the driver doesn't know the current status of the
> + * MAC-PHY.
> + */
> + regval = SW_RESET;
> + reinit_completion(&tc6->rst_complete);
> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, false);
> + if (ret) {
> + dev_err(&tc6->spi->dev, "RESET register write failed\n");
> + return ret;
> + }
> +
> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, true);
> + if (ret) {
> + dev_err(&tc6->spi->dev, "RESET register write failed\n");
> + return ret;
> + }
> + timeleft = wait_for_completion_interruptible_timeout(&tc6->rst_complete,
> + msecs_to_jiffies(1));
> + if (timeleft <= 0) {
> + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n");
> + return -ENODEV;
> + }
This seems a bit messy and complex. I assume reset is performed once
during probe, and never again? So i wonder if it would be cleaner to
actually just poll for the reset to complete? You can then remove all
this completion code, and the interrupt handler gets simpler?
> + /* Register MAC-PHY interrupt service routine */
> + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int",
> + tc6);
> + if ((ret != -ENOTCONN) && ret < 0) {
> + dev_err(&spi->dev, "Error attaching macphy irq %d\n", ret);
> + goto err_macphy_irq;
> + }
Why is -ENOTCONN special? A comment would be good here.
> -void oa_tc6_deinit(struct oa_tc6 *tc6)
> +int oa_tc6_deinit(struct oa_tc6 *tc6)
> {
> - kfree(tc6);
> + int ret;
> +
> + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6);
> + ret = kthread_stop(tc6->tc6_task);
> + if (!ret)
> + kfree(tc6);
> + return ret;
> }
What is the MAC driver supposed to do if this fails?
But this problem probably goes away once you use a threaded interrupt
handler.
w> +/* Open Alliance TC6 Standard Control and Status Registers */
> +#define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */
> +#define OA_TC6_STS0 0x0008 /* Status Register #0 */
Please use the same name as the standard. It use STATUS0, so
OA_TC6_STATUS0. Please make sure all your defines follow the standard.
> +
> +/* RESET register field */
> +#define SW_RESET BIT(0) /* Software Reset */
It is pretty normal to put #defines for a register members after the
#define for the register itself:
#define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */
#define OA_TC6_RESET_SWRESET BIT(0)
#define OA_TC6_STATUS0 0x0008 /* Status Register #0 */
#define OA_TC6_STATUS0_RESETC BIT(6) /* Reset Complete */
The naming like this also helps avoid mixups.
Andrew
