Message-ID: <97872e83-1490-4f1a-81ff-3f7692571dd1@microchip.com>
Date:   Wed, 25 Oct 2023 12:02:58 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <andrew@...n.ch>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <corbet@....net>, <Steen.Hegelund@...rochip.com>,
        <rdunlap@...radead.org>, <horms@...nel.org>,
        <casper.casan@...il.com>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <Horatiu.Vultur@...rochip.com>,
        <Woojung.Huh@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
        <UNGLinuxDriver@...rochip.com>, <Thorsten.Kummermehr@...rochip.com>
Subject: Re: [PATCH net-next v2 3/9] net: ethernet: oa_tc6: implement OA TC6
 configuration function

Hi Andrew,

On 24/10/23 4:28 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +     /* Read and configure the IMASK0 register for unmasking the interrupts */
>> +     ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, false, false);
>> +     if (ret)
>> +             return ret;
> 
> Can you use oa_tc6_read_register() here? I guess the question is, what
> does tc6->protect default to until it is set later in this function?
> So long as it defaults to false, i guess you can use the register
> read/write functions, which are a lot more readable than this generic
> oa_tc6_perform_ctrl().
Yes, I will do that. Also for next two calls as well.
> 
>> +
>> +     regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
>> +
>> +     ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, true, false);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Read STDCAP register to get the MAC-PHY standard capabilities */
>> +     ret = oa_tc6_perform_ctrl(tc6, STDCAP, &regval, 1, false, false);
>> +     if (ret)
>> +             return ret;
>> +
>> +     mincps = FIELD_GET(MINCPS, regval);
>> +     ctc = (regval & CTC) ? true : false;
>> +
>> +     regval = 0;
>> +     oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
>> +     if (oa_node) {
>> +             /* Read OA parameters from DT */
>> +             if (of_property_present(oa_node, "oa-cps")) {
>> +                     ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);
> 
> If of_property_read_u32() does not find the property, it is documented
> to not touch tc6->cps. So you can set tc6->cps to the default 64,
> before the big if, and skip the of_property_present(). You can then
> probably remove the else at the end as well.
Ah ok, will do that.
> 
>> +                     if (ret < 0)
>> +                             return ret;
>> +                     /* Return error if the configured cps is less than the
>> +                      * minimum cps supported by the MAC-PHY.
>> +                      */
>> +                     if (tc6->cps < mincps)
>> +                             return -ENODEV;
> 
> A dev_err() would be nice here to indicate why.
Ok sure.
> 
>> +             } else {
>> +                     tc6->cps = 64;
>> +             }
>> +             if (of_property_present(oa_node, "oa-txcte")) {
>> +                     /* Return error if the tx cut through mode is configured
>> +                      * but it is not supported by MAC-PHY.
>> +                      */
>> +                     if (ctc)
>> +                             regval |= TXCTE;
>> +                     else
>> +                             return -ENODEV;
> 
> and a dev_err() here as well.
Ok sure.
> 
>> +             }
>> +             if (of_property_present(oa_node, "oa-rxcte")) {
>> +                     /* Return error if the rx cut through mode is configured
>> +                      * but it is not supported by MAC-PHY.
>> +                      */
>> +                     if (ctc)
>> +                             regval |= RXCTE;
>> +                     else
>> +                             return -ENODEV;
>> +             }
> 
> and another dev_err(). Without these prints, you probably need to
> modify the code to figure out why the probe failed.
Yes I understand. Will do that in the next revision.
> 
>> +             if (of_property_present(oa_node, "oa-prote")) {
>> +                     regval |= PROTE;
>> +                     tc6->prote = true;
>> +             }
>> +     } else {
>> +             tc6->cps = 64;
>> +     }
>> +
>> +     regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
>> +
>> +     return oa_tc6_perform_ctrl(tc6, CONFIG0, &regval, 1, true, false);
>> +}
>> +
>>   static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
>>   {
>>        u32 regval;
>> @@ -310,7 +387,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
>>    * Returns pointer reference to the oa_tc6 structure if all the memory
>>    * allocation success otherwise NULL.
>>    */
>> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> 
> Was there a reason to have prote initially, and then remove it here?
The reason is, control communication uses "protect". But in the first 
patch there was no dt used. Later in this patch, dt used for all the 
configuration parameters and this also part of that. That's why removed 
and moved this to dt configuration.

What's your opinion? shall I keep as it is like this? or remove the 
protect in the first two patches and introduce in this patch?

Best Regards,
Parthiban V
> 
>      Andrew

Powered by blists - more mailing lists