Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY

[<Parthiban.Veerasooran@microchip.com>]

Hi Conor,

On 30/04/24 10:25 pm, Conor Dooley wrote:
> On Tue, Apr 30, 2024 at 01:30:22PM +0000,Parthiban.Veerasooran@microchip.com  wrote:
>> Hi Andrew,
>>
>> On 29/04/24 5:39 pm, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>>> Looks like, the below changes needed to work correctly,
>>>>
>>>> lan865x.c:
>>>> - compatible string to be changed like below as it is a fallback for all
>>>> variants,
>>>>         .compatible = "microchip,lan8650"
>>>> - DRV_NAME to be changed like below,
>>>>         #define DRV_NAME                        "lan8650"
>>>>
>>>> microchip,lan865x.example.dts for lan8650:
>>>> - compatible string to be changed like below,
>>>>         .compatible = "microchip,lan8650";
>>>>         OR
>>>> microchip,lan865x.example.dts for lan8651:
>>>> - compatible string to be changed like below,
>>>>         compatible = "microchip,lan8651", "microchip,lan8650";
>>>>
>>>> I tested with the above changes and there was no issues observed. Any
>>>> comments on this? Otherwise we can stick with these changes for the next
>>>> version.
>>> As Conor said, this is probably relying on the fallback
>>> mechanism. Please look at other SPI devices, e.g. hwmon, and see how
>>> they probe for multiple different devices.
>> I just referred the below drivers for the spi devices handling along
>> with the compatible,
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/davicom/dm9051.c#L1239
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/adi/adin1110.c#L1644
>>
>> lan8650 - MAC-PHY chip
>> lan8651 - ETH Click-Mikroe with MAC-PHY chip
>>
>> So, they are different in interface but not in functionality. There is
>> no difference in the configuration. So let's consider lan8650 is the
>> fallback option for lan8651.
>>
>> By referring the above links, I have restructured the code like below to
>> support with lan8651 fallback. Still there is no change in the existing
>> device tree binding. This is the only change in lan865x.c.
>>
>> static const struct spi_device_id spidev_spi_ids[] = {
>>           { .name = "lan8650" },
>>           {},
>> };
>>
>> static const struct of_device_id lan865x_dt_ids[] = {
>>           { .compatible = "microchip,lan8650" },
>>           { /* Sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>>
>> static struct spi_driver lan865x_driver = {
>>           .driver = {
>>                   .name = DRV_NAME,
>>                   .of_match_table = lan865x_dt_ids,
>>            },
>>           .probe = lan865x_probe,
>>           .remove = lan865x_remove,
>>           .id_table = spidev_spi_ids,
>> };
>>
>> I also referred the below two links for the device tree binding and
>> driver in case of fallback.
> Did you also verify that the warning from the spi core is no longer
> generated when using compatible = "microchip,lan8651", "microchip,lan8650"?
Do you mean changing in the lan865x.c file? if yes then I got the 
warning "SPI driver lan865x has no spi_device_id for microchip,lan8651"

But after updating the lan865x.c like below, the warning disappeared.

static const struct spi_device_id spidev_spi_ids[] = {
         { .name = "lan8650" },
         { .name = "lan8651" },
         {},
};

Note: In both the above two cases compatible in the dts is
compatible = "microchip,lan8651", "microchip,lan8650";

Best regards,
Parthiban V