On 13/09/23 8:09 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +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?
Ok the spec says the below, that's why I implemented like this.
9.2.8.8 RESETC
Reset Complete. This bit is set when the MAC-PHY reset is complete and
ready for configuration. When it is set, it will generate a non-maskable
interrupt assertion on IRQn to alert the SPI host. Additionally, setting
of the RESETC bit shall also set EXST = 1 in the receive data footer
until this bit is cleared by action of the SPI host writing a ‘1’.
Yes, I agree that the reset is performed once in the beginning. So I
will poll for the completion and remove this block in the next revision.
>
>> + /* 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.
Ah, it is a mistake. I supposed to use,
if (ret)
I will correct it in the next version.
>
>> -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.
Yes, I agree. Will do that.
>
> 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.
Yes sure.
>
>> +
>> +/* 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.
Ok, I will follow this in the next version.
Best Regards,
Parthiban V
>
> Andrew
>
