On 18.12.2019 18:58, Alexandre Belloni wrote:
> On 18/12/2019 16:52:21+0000, [email protected] wrote:
>>
>>
>> On 18.12.2019 18:43, Alexandre Belloni wrote:
>>
>>> Hi,
>>>
>>> On 18/12/2019 16:24:00+0000, [email protected] wrote:
>>>> From: Eugen Hristev <[email protected]>
>>>>
>>>> This allows the RTC node to have child nodes in DT.
>>>> This allows subnodes to be probed.
>>>>
>>>> Signed-off-by: Eugen Hristev <[email protected]>
>>>> ---
>>>> 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,
>>
>> 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.
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
On 19/12/2019 09:15:02+0000, [email protected] wrote:
>
>
> On 18.12.2019 18:58, Alexandre Belloni wrote:
> > On 18/12/2019 16:52:21+0000, [email protected] wrote:
> >>
> >>
> >> On 18.12.2019 18:43, Alexandre Belloni wrote:
> >>
> >>> Hi,
> >>>
> >>> On 18/12/2019 16:24:00+0000, [email protected] wrote:
> >>>> From: Eugen Hristev <[email protected]>
> >>>>
> >>>> This allows the RTC node to have child nodes in DT.
> >>>> This allows subnodes to be probed.
> >>>>
> >>>> Signed-off-by: Eugen Hristev <[email protected]>
> >>>> ---
> >>>> 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,
> >>
> >> 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.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, 19 Dec 2019 11:23:21 +0100
Alexandre Belloni <[email protected]> wrote:
> On 19/12/2019 09:15:02+0000, [email protected] wrote:
> >
> >
> > On 18.12.2019 18:58, Alexandre Belloni wrote:
> > > On 18/12/2019 16:52:21+0000, [email protected] wrote:
> > >>
> > >>
> > >> On 18.12.2019 18:43, Alexandre Belloni wrote:
> > >>
> > >>> Hi,
> > >>>
> > >>> On 18/12/2019 16:24:00+0000, [email protected] wrote:
> > >>>> From: Eugen Hristev <[email protected]>
> > >>>>
> > >>>> This allows the RTC node to have child nodes in DT.
> > >>>> This allows subnodes to be probed.
> > >>>>
> > >>>> Signed-off-by: Eugen Hristev <[email protected]>
> > >>>> ---
> > >>>> 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,
> > >>
> > >> 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
On 23.12.2019 13:16, Jonathan Cameron wrote:
> On Thu, 19 Dec 2019 11:23:21 +0100
> Alexandre Belloni <[email protected]> wrote:
>
>> On 19/12/2019 09:15:02+0000, [email protected] wrote:
>>>
>>>
>>> On 18.12.2019 18:58, Alexandre Belloni wrote:
>>>> On 18/12/2019 16:52:21+0000, [email protected] wrote:
>>>>>
>>>>>
>>>>> On 18.12.2019 18:43, Alexandre Belloni wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 18/12/2019 16:24:00+0000, [email protected] wrote:
>>>>>>> From: Eugen Hristev <[email protected]>
>>>>>>>
>>>>>>> This allows the RTC node to have child nodes in DT.
>>>>>>> This allows subnodes to be probed.
>>>>>>>
>>>>>>> Signed-off-by: Eugen Hristev <[email protected]>
>>>>>>> ---
>>>>>>> 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
>
>
On 09/01/2020 11:19:45+0000, [email protected] wrote:
> 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.
>
As suggested earlier in the thread, you can use platform_add_device
which fits all your requirements.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com