Re: [PATCH 04/10] rtc: at91rm9200: use of_platform_populate as return value

[<Eugen.Hristev@microchip.com>]

On 23.12.2019 13:16, Jonathan Cameron wrote:

> On Thu, 19 Dec 2019 11:23:21 +0100
> Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> 
>> On 19/12/2019 09:15:02+0000, Eugen.Hristev@microchip.com wrote:
>>>
>>>
>>> On 18.12.2019 18:58, Alexandre Belloni wrote:
>>>> On 18/12/2019 16:52:21+0000, Eugen.Hristev@microchip.com wrote:
>>>>>
>>>>>
>>>>> On 18.12.2019 18:43, Alexandre Belloni wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 18/12/2019 16:24:00+0000, Eugen.Hristev@microchip.com wrote:
>>>>>>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>>>
>>>>>>> This allows the RTC node to have child nodes in DT.
>>>>>>> This allows subnodes to be probed.
>>>>>>>
>>>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>>> ---
>>>>>>>     drivers/rtc/rtc-at91rm9200.c | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
>>>>>>> index 3b833e0..f1b5b3d 100644
>>>>>>> --- a/drivers/rtc/rtc-at91rm9200.c
>>>>>>> +++ b/drivers/rtc/rtc-at91rm9200.c
>>>>>>> @@ -421,7 +421,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
>>>>>>>          at91_rtc_write_ier(AT91_RTC_SECEV);
>>>>>>>
>>>>>>>          dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
>>>>>>> -     return 0;
>>>>>>> +     return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>>>>>>>
>>>>>>
>>>>>> You can avoid the DT binding change and DT parsing by using
>>>>>> platform_add_device here. I don't think there is any point describing
>>>>>> the trigger as a child node (a watchdog functionality wouldn't be
>>>>>> described for example).

Hi Alexandre,

I started to work on this, I am trying to add and probe the 
rtc_adc_trigger with platform_device_add.

However, some issues arise: this means that the rtc_adc_trigger will not 
be OF-compatible, so, how can I identify the driver to probe ?
Second, by adding a new platform device from the RTC driver, would mean 
that I would have to supply it's probe/remove functions, which I cannot 
have here. Those are in the rtc_adc_trigger iio driver.

In fact, the question is, which is the mechanism you suggested, to be 
able to probe the rtc_adc_trigger, from inside the rtc driver, without 
using a child node in DT, as you requested ?
The rtc_adc_trigger needs a MEM resource, and a parent, and it must 
reside inside the IIO subsystem.

Thanks,
Eugen


>>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> It's needed because the ADC needs a link to the trigger device. This is
>>>>> a hardware link inside the SoC, so I thought the best way is to describe
>>>>> this hardware is in the Device Tree.
>>>>> Otherwise the ADC node is unaware of the RTC triggering possibility.
>>>>> If we just assign the RTC trigger device to the ADC through the sysfs,
>>>>> the ADC cannot distinguish between the RTC trigger and other various
>>>>> triggers which can be attached.
>>>>>
>>>>
>>>> I'm not sure this links is required but I will let Jonathan review. Even
>>>> if it is needed, you can still use the rtc node to describe that link.
>>>
>>> Actually, the RTC node could potentially have two different ADC
>>> triggers. There is another OUT1 field that can do a second trigger for
>>> the ADC only for the last channel. Future development might add this
>>> trigger, so, with that in mind, I think it's best to link the exact
>>> trigger and not the RTC node.
>>
>> Nothing prevents you from using an index with the phandle (and I would
>> add a type in that case then). Having subnodes in the DT is not really a
>> good idea. The IP is the RTC, it just happens to have some outputs.
>> See what has been done for the PMC.
>>
>>
> 
> If it can be done either way, let's avoid adding to the rtc dt binding.
> 
> Jonathan
> 
>