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

Parthiban.Veerasooran
2024-05-02T05:56:57Z

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