| Message ID | 1376219071-29946-3-git-send-email-josh.wu@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Josh, On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: > For at91 boards, there are different IPs for adc. Different IPs has > different STARTUP & PRESCAL mask in ADC_MR. > > This patch introduce the multiple compatible string for those > different IPs. > > Signed-off-by: Josh Wu <josh.wu@atmel.com> Overall it looks like the right ways, but I think we can take it a step further. I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, atmel,adc-status-register, atmel,adc-trigger-register properties (and probably the triggers as well description as well). Maxime
Hi, Maxime On 8/16/2013 3:20 AM, Maxime Ripard wrote: > Hi Josh, > > On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: >> For at91 boards, there are different IPs for adc. Different IPs has >> different STARTUP & PRESCAL mask in ADC_MR. >> >> This patch introduce the multiple compatible string for those >> different IPs. >> >> Signed-off-by: Josh Wu <josh.wu@atmel.com> > Overall it looks like the right ways, but I think we can take it a step > further. > > I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, > atmel,adc-status-register, atmel,adc-trigger-register properties (and > probably the triggers as well description as well). yeah, right. Currently I want to drop following: atmel,adc-drdy-mask, atmel,adc-status-register, atmel,adc-trigger-register, atmel,adc-channel-base For the adc-num-channels, I'd like to leave it in dt parameters. It is a description for an adc capablity. For the triggers, I am not decided. An obvious benifit to remove trigger in dt will save many lines of code. > > Maxime > Best Regards, Josh Wu
Add Ludovic in the loop. On 8/22/2013 5:51 PM, Josh Wu wrote: > Hi, Maxime > > On 8/16/2013 3:20 AM, Maxime Ripard wrote: >> Hi Josh, >> >> On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: >>> For at91 boards, there are different IPs for adc. Different IPs has >>> different STARTUP & PRESCAL mask in ADC_MR. >>> >>> This patch introduce the multiple compatible string for those >>> different IPs. >>> >>> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> Overall it looks like the right ways, but I think we can take it a step >> further. >> >> I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, >> atmel,adc-status-register, atmel,adc-trigger-register properties (and >> probably the triggers as well description as well). > > yeah, right. Currently I want to drop following: > > atmel,adc-drdy-mask, atmel,adc-status-register, > atmel,adc-trigger-register, atmel,adc-channel-base > > For the adc-num-channels, I'd like to leave it in dt parameters. It is > a description for an adc capablity. > > For the triggers, I am not decided. An obvious benifit to remove > trigger in dt will save many lines of code. > >> >> Maxime >> > > Best Regards, > Josh Wu
Hi, On Thu, Aug 22, 2013 at 05:53:00PM +0800, Josh Wu wrote: > Add Ludovic in the loop. > > On 8/22/2013 5:51 PM, Josh Wu wrote: > >Hi, Maxime > > > >On 8/16/2013 3:20 AM, Maxime Ripard wrote: > >>Hi Josh, > >> > >>On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: > >>>For at91 boards, there are different IPs for adc. Different IPs has > >>>different STARTUP & PRESCAL mask in ADC_MR. > >>> > >>>This patch introduce the multiple compatible string for those > >>>different IPs. > >>> > >>>Signed-off-by: Josh Wu <josh.wu@atmel.com> > >>Overall it looks like the right ways, but I think we can take it a step > >>further. > >> > >>I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, > >>atmel,adc-status-register, atmel,adc-trigger-register properties (and > >>probably the triggers as well description as well). > > > >yeah, right. Currently I want to drop following: > > > >atmel,adc-drdy-mask, atmel,adc-status-register, > >atmel,adc-trigger-register, atmel,adc-channel-base > > > >For the adc-num-channels, I'd like to leave it in dt parameters. > >It is a description for an adc capablity. About this parameter, I'll remove it too from the dt bindings. To set it you need to have a look to the datasheet and to copy a constant value into the dt. From my point of view, it means than this parameter should be managed by the driver and by the dt. On the other side since there are some dynamic allocation depending on this parameter maybe it makes sense to keep it in the dt. If the user wants to use only 2 channels why doing allocation for max channel number. By the way, this case is only valid if he uses the two first channels. > > > >For the triggers, I am not decided. An obvious benifit to remove > >trigger in dt will save many lines of code. > > > >> > >>Maxime > >> > > > >Best Regards, > >Josh Wu > Since we are talking about reworking this binding I was thinking about resolution stuff. Filling atmel,adc-res is also copying constant value from the device datasheet, so if I was consistent I would say it has to be removed too. But I am not consistent! I mean by doing this the only thing the user will have to fill is the resolution value. He can't set the value he wants, there are only two choices. By keeping it into the dt then he will immediately see the choices he has. Regards Ludovic
Hi Ludovic, Josh, On Fri, Aug 23, 2013 at 05:46:03PM +0200, Desroches, Ludovic wrote: > On Thu, Aug 22, 2013 at 05:53:00PM +0800, Josh Wu wrote: > > On 8/22/2013 5:51 PM, Josh Wu wrote: > > >Hi, Maxime > > > > > >On 8/16/2013 3:20 AM, Maxime Ripard wrote: > > >>Hi Josh, > > >> > > >>On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: > > >>>For at91 boards, there are different IPs for adc. Different IPs has > > >>>different STARTUP & PRESCAL mask in ADC_MR. > > >>> > > >>>This patch introduce the multiple compatible string for those > > >>>different IPs. > > >>> > > >>>Signed-off-by: Josh Wu <josh.wu@atmel.com> > > >>Overall it looks like the right ways, but I think we can take it a step > > >>further. > > >> > > >>I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, > > >>atmel,adc-status-register, atmel,adc-trigger-register properties (and > > >>probably the triggers as well description as well). > > > > > >yeah, right. Currently I want to drop following: > > > > > >atmel,adc-drdy-mask, atmel,adc-status-register, > > >atmel,adc-trigger-register, atmel,adc-channel-base > > > > > >For the adc-num-channels, I'd like to leave it in dt parameters. > > >It is a description for an adc capablity. > > About this parameter, I'll remove it too from the dt bindings. To set it you > need to have a look to the datasheet and to copy a constant value into the > dt. From my point of view, it means than this parameter should be managed by > the driver and by the dt. > > On the other side since there are some dynamic allocation depending on this > parameter maybe it makes sense to keep it in the dt. If the user wants to use > only 2 channels why doing allocation for max channel number. By the way, this > case is only valid if he uses the two first channels. I don't recall it very well, is there any reason to not have it in the DT? Can the ADC channels be used for something else? Or is it just some IP-specific number of channels? > > > > > >For the triggers, I am not decided. An obvious benifit to remove > > >trigger in dt will save many lines of code. > > > > > >> > > >>Maxime > > >> > > > > > >Best Regards, > > >Josh Wu > > > > Since we are talking about reworking this binding I was thinking about > resolution stuff. Filling atmel,adc-res is also copying constant value from > the device datasheet, so if I was consistent I would say it has to be removed > too. But I am not consistent! I mean by doing this the only thing the user > will have to fill is the resolution value. He can't set the value he wants, > there are only two choices. By keeping it into the dt then he will immediately > see the choices he has. But the resolution should probably be somehow user-defined, probably not really related to the DT has well. I think some other IIO ADC drivers are using sysfs files for this purpose, maybe that would be a better fit? Maxime
On Fri, Aug 23, 2013 at 06:59:36PM +0200, Maxime Ripard wrote: > Hi Ludovic, Josh, > > On Fri, Aug 23, 2013 at 05:46:03PM +0200, Desroches, Ludovic wrote: > > On Thu, Aug 22, 2013 at 05:53:00PM +0800, Josh Wu wrote: > > > On 8/22/2013 5:51 PM, Josh Wu wrote: > > > >Hi, Maxime > > > > > > > >On 8/16/2013 3:20 AM, Maxime Ripard wrote: > > > >>Hi Josh, > > > >> > > > >>On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: > > > >>>For at91 boards, there are different IPs for adc. Different IPs has > > > >>>different STARTUP & PRESCAL mask in ADC_MR. > > > >>> > > > >>>This patch introduce the multiple compatible string for those > > > >>>different IPs. > > > >>> > > > >>>Signed-off-by: Josh Wu <josh.wu@atmel.com> > > > >>Overall it looks like the right ways, but I think we can take it a step > > > >>further. > > > >> > > > >>I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, > > > >>atmel,adc-status-register, atmel,adc-trigger-register properties (and > > > >>probably the triggers as well description as well). > > > > > > > >yeah, right. Currently I want to drop following: > > > > > > > >atmel,adc-drdy-mask, atmel,adc-status-register, > > > >atmel,adc-trigger-register, atmel,adc-channel-base > > > > > > > >For the adc-num-channels, I'd like to leave it in dt parameters. > > > >It is a description for an adc capablity. > > > > About this parameter, I'll remove it too from the dt bindings. To set it you > > need to have a look to the datasheet and to copy a constant value into the > > dt. From my point of view, it means than this parameter should be managed by > > the driver and by the dt. > > > > On the other side since there are some dynamic allocation depending on this > > parameter maybe it makes sense to keep it in the dt. If the user wants to use > > only 2 channels why doing allocation for max channel number. By the way, this > > case is only valid if he uses the two first channels. > > I don't recall it very well, is there any reason to not have it in the > DT? Can the ADC channels be used for something else? Or is it just some > IP-specific number of channels? > It is IP-specific. I don't see what benefit we could have to keep it in the DT? But Josh seems to have arguments to keep it. > > > > > > > >For the triggers, I am not decided. An obvious benifit to remove > > > >trigger in dt will save many lines of code. > > > > > > > >> > > > >>Maxime > > > >> > > > > > > > >Best Regards, > > > >Josh Wu > > > > > > > Since we are talking about reworking this binding I was thinking about > > resolution stuff. Filling atmel,adc-res is also copying constant value from > > the device datasheet, so if I was consistent I would say it has to be removed > > too. But I am not consistent! I mean by doing this the only thing the user > > will have to fill is the resolution value. He can't set the value he wants, > > there are only two choices. By keeping it into the dt then he will immediately > > see the choices he has. > > But the resolution should probably be somehow user-defined, probably not > really related to the DT has well. I think some other IIO ADC drivers > are using sysfs files for this purpose, maybe that would be a better > fit? It sounds to be a good solution. Ludovic
Hi, Ludovic and Maxime On 8/26/2013 4:32 PM, Ludovic Desroches wrote: > On Fri, Aug 23, 2013 at 06:59:36PM +0200, Maxime Ripard wrote: >> Hi Ludovic, Josh, >> >> On Fri, Aug 23, 2013 at 05:46:03PM +0200, Desroches, Ludovic wrote: >>> On Thu, Aug 22, 2013 at 05:53:00PM +0800, Josh Wu wrote: >>>> On 8/22/2013 5:51 PM, Josh Wu wrote: >>>>> Hi, Maxime >>>>> >>>>> On 8/16/2013 3:20 AM, Maxime Ripard wrote: >>>>>> Hi Josh, >>>>>> >>>>>> On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: >>>>>>> For at91 boards, there are different IPs for adc. Different IPs has >>>>>>> different STARTUP & PRESCAL mask in ADC_MR. >>>>>>> >>>>>>> This patch introduce the multiple compatible string for those >>>>>>> different IPs. >>>>>>> >>>>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com> >>>>>> Overall it looks like the right ways, but I think we can take it a step >>>>>> further. >>>>>> >>>>>> I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, >>>>>> atmel,adc-status-register, atmel,adc-trigger-register properties (and >>>>>> probably the triggers as well description as well). >>>>> yeah, right. Currently I want to drop following: >>>>> >>>>> atmel,adc-drdy-mask, atmel,adc-status-register, >>>>> atmel,adc-trigger-register, atmel,adc-channel-base >>>>> >>>>> For the adc-num-channels, I'd like to leave it in dt parameters. >>>>> It is a description for an adc capablity. >>> About this parameter, I'll remove it too from the dt bindings. To set it you >>> need to have a look to the datasheet and to copy a constant value into the >>> dt. From my point of view, it means than this parameter should be managed by >>> the driver and by the dt. >>> >>> On the other side since there are some dynamic allocation depending on this >>> parameter maybe it makes sense to keep it in the dt. If the user wants to use >>> only 2 channels why doing allocation for max channel number. By the way, this >>> case is only valid if he uses the two first channels. >> I don't recall it very well, is there any reason to not have it in the >> DT? Can the ADC channels be used for something else? Or is it just some >> IP-specific number of channels? >> > It is IP-specific. I don't see what benefit we could have to keep it in the DT? > But Josh seems to have arguments to keep it. I'm ok to remove the channel number. What I worried is there also has a channel-used mask in DT. This mask should be removed too if channel number is removed. So maybe we can also use the sysfs to set the mask. > >>>>> For the triggers, I am not decided. An obvious benifit to remove >>>>> trigger in dt will save many lines of code. >>>>> >>>>>> Maxime >>>>>> >>>>> Best Regards, >>>>> Josh Wu >>> Since we are talking about reworking this binding I was thinking about >>> resolution stuff. Filling atmel,adc-res is also copying constant value from >>> the device datasheet, so if I was consistent I would say it has to be removed >>> too. But I am not consistent! I mean by doing this the only thing the user >>> will have to fill is the resolution value. He can't set the value he wants, >>> there are only two choices. By keeping it into the dt then he will immediately >>> see the choices he has. >> But the resolution should probably be somehow user-defined, probably not >> really related to the DT has well. I think some other IIO ADC drivers >> are using sysfs files for this purpose, maybe that would be a better >> fit? > It sounds to be a good solution. ok, I will check the other IIO ADC driver about this point. Maybe this sysfs replacement need a bit more time. I prefer to send out the patches first without the sysfs implement in v3. And the sysfs replacement patch will be send out serperately. What do you think? Maxime. > > > Ludovic Best Regards, Josh Wu
On Mon, Aug 26, 2013 at 06:03:31PM +0800, Josh Wu wrote: > Hi, Ludovic and Maxime > > On 8/26/2013 4:32 PM, Ludovic Desroches wrote: > >On Fri, Aug 23, 2013 at 06:59:36PM +0200, Maxime Ripard wrote: > >>Hi Ludovic, Josh, > >> > >>On Fri, Aug 23, 2013 at 05:46:03PM +0200, Desroches, Ludovic wrote: > >>>On Thu, Aug 22, 2013 at 05:53:00PM +0800, Josh Wu wrote: > >>>>On 8/22/2013 5:51 PM, Josh Wu wrote: > >>>>>Hi, Maxime > >>>>> > >>>>>On 8/16/2013 3:20 AM, Maxime Ripard wrote: > >>>>>>Hi Josh, > >>>>>> > >>>>>>On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: > >>>>>>>For at91 boards, there are different IPs for adc. Different IPs has > >>>>>>>different STARTUP & PRESCAL mask in ADC_MR. > >>>>>>> > >>>>>>>This patch introduce the multiple compatible string for those > >>>>>>>different IPs. > >>>>>>> > >>>>>>>Signed-off-by: Josh Wu <josh.wu@atmel.com> > >>>>>>Overall it looks like the right ways, but I think we can take it a step > >>>>>>further. > >>>>>> > >>>>>>I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, > >>>>>>atmel,adc-status-register, atmel,adc-trigger-register properties (and > >>>>>>probably the triggers as well description as well). > >>>>>yeah, right. Currently I want to drop following: > >>>>> > >>>>>atmel,adc-drdy-mask, atmel,adc-status-register, > >>>>>atmel,adc-trigger-register, atmel,adc-channel-base > >>>>> > >>>>>For the adc-num-channels, I'd like to leave it in dt parameters. > >>>>>It is a description for an adc capablity. > >>>About this parameter, I'll remove it too from the dt bindings. To set it you > >>>need to have a look to the datasheet and to copy a constant value into the > >>>dt. From my point of view, it means than this parameter should be managed by > >>>the driver and by the dt. > >>> > >>>On the other side since there are some dynamic allocation depending on this > >>>parameter maybe it makes sense to keep it in the dt. If the user wants to use > >>>only 2 channels why doing allocation for max channel number. By the way, this > >>>case is only valid if he uses the two first channels. > >>I don't recall it very well, is there any reason to not have it in the > >>DT? Can the ADC channels be used for something else? Or is it just some > >>IP-specific number of channels? > >> > >It is IP-specific. I don't see what benefit we could have to keep it in the DT? > >But Josh seems to have arguments to keep it. > > I'm ok to remove the channel number. What I worried is there also > has a channel-used mask in DT. > This mask should be removed too if channel number is removed. > So maybe we can also use the sysfs to set the mask. Indeed, that would make adc-channel-used irrelevant. But I'm not sure the mask is useful at all. Just enable all the channels and that's it? > >>>>>For the triggers, I am not decided. An obvious benifit to remove > >>>>>trigger in dt will save many lines of code. > >>>>> > >>>>>>Maxime > >>>>>> > >>>>>Best Regards, > >>>>>Josh Wu > >>>Since we are talking about reworking this binding I was thinking about > >>>resolution stuff. Filling atmel,adc-res is also copying constant value from > >>>the device datasheet, so if I was consistent I would say it has to be removed > >>>too. But I am not consistent! I mean by doing this the only thing the user > >>>will have to fill is the resolution value. He can't set the value he wants, > >>>there are only two choices. By keeping it into the dt then he will immediately > >>>see the choices he has. > >>But the resolution should probably be somehow user-defined, probably not > >>really related to the DT has well. I think some other IIO ADC drivers > >>are using sysfs files for this purpose, maybe that would be a better > >>fit? > >It sounds to be a good solution. > > ok, I will check the other IIO ADC driver about this point. > Maybe this sysfs replacement need a bit more time. I prefer to send > out the patches first without the sysfs implement in v3. > And the sysfs replacement patch will be send out serperately. What > do you think? Maxime. Yes, of course. The resolution rework can definitely be done later. Maxime
On 27/08/2013 10:15, Maxime Ripard : > On Mon, Aug 26, 2013 at 06:03:31PM +0800, Josh Wu wrote: >> Hi, Ludovic and Maxime >> >> On 8/26/2013 4:32 PM, Ludovic Desroches wrote: >>> On Fri, Aug 23, 2013 at 06:59:36PM +0200, Maxime Ripard wrote: >>>> Hi Ludovic, Josh, >>>> >>>> On Fri, Aug 23, 2013 at 05:46:03PM +0200, Desroches, Ludovic wrote: >>>>> On Thu, Aug 22, 2013 at 05:53:00PM +0800, Josh Wu wrote: >>>>>> On 8/22/2013 5:51 PM, Josh Wu wrote: >>>>>>> Hi, Maxime >>>>>>> >>>>>>> On 8/16/2013 3:20 AM, Maxime Ripard wrote: >>>>>>>> Hi Josh, >>>>>>>> >>>>>>>> On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: >>>>>>>>> For at91 boards, there are different IPs for adc. Different IPs has >>>>>>>>> different STARTUP & PRESCAL mask in ADC_MR. >>>>>>>>> >>>>>>>>> This patch introduce the multiple compatible string for those >>>>>>>>> different IPs. >>>>>>>>> >>>>>>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com> >>>>>>>> Overall it looks like the right ways, but I think we can take it a step >>>>>>>> further. >>>>>>>> >>>>>>>> I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, >>>>>>>> atmel,adc-status-register, atmel,adc-trigger-register properties (and >>>>>>>> probably the triggers as well description as well). >>>>>>> yeah, right. Currently I want to drop following: >>>>>>> >>>>>>> atmel,adc-drdy-mask, atmel,adc-status-register, >>>>>>> atmel,adc-trigger-register, atmel,adc-channel-base >>>>>>> >>>>>>> For the adc-num-channels, I'd like to leave it in dt parameters. >>>>>>> It is a description for an adc capablity. >>>>> About this parameter, I'll remove it too from the dt bindings. To set it you >>>>> need to have a look to the datasheet and to copy a constant value into the >>>>> dt. From my point of view, it means than this parameter should be managed by >>>>> the driver and by the dt. >>>>> >>>>> On the other side since there are some dynamic allocation depending on this >>>>> parameter maybe it makes sense to keep it in the dt. If the user wants to use >>>>> only 2 channels why doing allocation for max channel number. By the way, this >>>>> case is only valid if he uses the two first channels. >>>> I don't recall it very well, is there any reason to not have it in the >>>> DT? Can the ADC channels be used for something else? Or is it just some >>>> IP-specific number of channels? >>>> >>> It is IP-specific. I don't see what benefit we could have to keep it in the DT? >>> But Josh seems to have arguments to keep it. >> >> I'm ok to remove the channel number. What I worried is there also >> has a channel-used mask in DT. >> This mask should be removed too if channel number is removed. >> So maybe we can also use the sysfs to set the mask. > > Indeed, that would make adc-channel-used irrelevant. But I'm not sure > the mask is useful at all. Just enable all the channels and that's it? On the top of my head: keeping all channels enabled, won't it have an impact on: 1/ power consumption? 2/ minimum period of sampling for a particular channel in case of continuous ADC trigger? And do not forget that sysfs is an interface that is: - needing a userspace tool to be configured properly (even just an echo xx) - hard to design - a strict ABI that can't be changed once deployed But yes, it is true that if the user has to configure ADC at runtime, it is certainly an interface worth considering... >>>>>>> For the triggers, I am not decided. An obvious benifit to remove >>>>>>> trigger in dt will save many lines of code. >>>>>>> >>>>>>>> Maxime >>>>>>>> >>>>>>> Best Regards, >>>>>>> Josh Wu >>>>> Since we are talking about reworking this binding I was thinking about >>>>> resolution stuff. Filling atmel,adc-res is also copying constant value from >>>>> the device datasheet, so if I was consistent I would say it has to be removed >>>>> too. But I am not consistent! I mean by doing this the only thing the user >>>>> will have to fill is the resolution value. He can't set the value he wants, >>>>> there are only two choices. By keeping it into the dt then he will immediately >>>>> see the choices he has. >>>> But the resolution should probably be somehow user-defined, probably not >>>> really related to the DT has well. I think some other IIO ADC drivers >>>> are using sysfs files for this purpose, maybe that would be a better >>>> fit? >>> It sounds to be a good solution. >> >> ok, I will check the other IIO ADC driver about this point. >> Maybe this sysfs replacement need a bit more time. I prefer to send >> out the patches first without the sysfs implement in v3. >> And the sysfs replacement patch will be send out serperately. What >> do you think? Maxime. > > Yes, of course. The resolution rework can definitely be done later. > > Maxime >
Hi Nicolas, On Tue, Aug 27, 2013 at 11:05:22AM +0200, Nicolas Ferre wrote: > On 27/08/2013 10:15, Maxime Ripard : > >On Mon, Aug 26, 2013 at 06:03:31PM +0800, Josh Wu wrote: > >>Hi, Ludovic and Maxime > >> > >>On 8/26/2013 4:32 PM, Ludovic Desroches wrote: > >>>On Fri, Aug 23, 2013 at 06:59:36PM +0200, Maxime Ripard wrote: > >>>>Hi Ludovic, Josh, > >>>> > >>>>On Fri, Aug 23, 2013 at 05:46:03PM +0200, Desroches, Ludovic wrote: > >>>>>On Thu, Aug 22, 2013 at 05:53:00PM +0800, Josh Wu wrote: > >>>>>>On 8/22/2013 5:51 PM, Josh Wu wrote: > >>>>>>>Hi, Maxime > >>>>>>> > >>>>>>>On 8/16/2013 3:20 AM, Maxime Ripard wrote: > >>>>>>>>Hi Josh, > >>>>>>>> > >>>>>>>>On Sun, Aug 11, 2013 at 07:04:29PM +0800, Josh Wu wrote: > >>>>>>>>>For at91 boards, there are different IPs for adc. Different IPs has > >>>>>>>>>different STARTUP & PRESCAL mask in ADC_MR. > >>>>>>>>> > >>>>>>>>>This patch introduce the multiple compatible string for those > >>>>>>>>>different IPs. > >>>>>>>>> > >>>>>>>>>Signed-off-by: Josh Wu <josh.wu@atmel.com> > >>>>>>>>Overall it looks like the right ways, but I think we can take it a step > >>>>>>>>further. > >>>>>>>> > >>>>>>>>I'd drop at least the atmel,adc-drdy-mask, atmel,adc-num-channels, > >>>>>>>>atmel,adc-status-register, atmel,adc-trigger-register properties (and > >>>>>>>>probably the triggers as well description as well). > >>>>>>>yeah, right. Currently I want to drop following: > >>>>>>> > >>>>>>>atmel,adc-drdy-mask, atmel,adc-status-register, > >>>>>>>atmel,adc-trigger-register, atmel,adc-channel-base > >>>>>>> > >>>>>>>For the adc-num-channels, I'd like to leave it in dt parameters. > >>>>>>>It is a description for an adc capablity. > >>>>>About this parameter, I'll remove it too from the dt bindings. To set it you > >>>>>need to have a look to the datasheet and to copy a constant value into the > >>>>>dt. From my point of view, it means than this parameter should be managed by > >>>>>the driver and by the dt. > >>>>> > >>>>>On the other side since there are some dynamic allocation depending on this > >>>>>parameter maybe it makes sense to keep it in the dt. If the user wants to use > >>>>>only 2 channels why doing allocation for max channel number. By the way, this > >>>>>case is only valid if he uses the two first channels. > >>>>I don't recall it very well, is there any reason to not have it in the > >>>>DT? Can the ADC channels be used for something else? Or is it just some > >>>>IP-specific number of channels? > >>>> > >>>It is IP-specific. I don't see what benefit we could have to keep it in the DT? > >>>But Josh seems to have arguments to keep it. > >> > >>I'm ok to remove the channel number. What I worried is there also > >>has a channel-used mask in DT. > >>This mask should be removed too if channel number is removed. > >>So maybe we can also use the sysfs to set the mask. > > > >Indeed, that would make adc-channel-used irrelevant. But I'm not sure > >the mask is useful at all. Just enable all the channels and that's it? > > On the top of my head: keeping all channels enabled, won't it have > an impact on: > 1/ power consumption? > 2/ minimum period of sampling for a particular channel in case of > continuous ADC trigger? I don't know, you tell me ;) This is exactly why I was asking. > And do not forget that sysfs is an interface that is: > - needing a userspace tool to be configured properly (even just an > echo xx) > - hard to design > - a strict ABI that can't be changed once deployed > > But yes, it is true that if the user has to configure ADC at > runtime, it is certainly an interface worth considering... Yes, of course. But the resolution is something I'd expect to be user-configurable, probably at runtime, and the DT doesn't make it very convenient to achieve.
diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt index 16769d9..6e60781 100644 --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt @@ -1,7 +1,8 @@ * AT91's Analog to Digital Converter (ADC) Required properties: - - compatible: Should be "atmel,at91sam9260-adc" + - compatible: Should be "atmel,<chip>-adc" + <chip> can be "at91sam9260", "at91sam9g45" or "at91sam9x5" - reg: Should contain ADC registers location and length - interrupts: Should contain the IRQ line for the ADC - atmel,adc-channel-base: Offset of the first channel data register diff --git a/arch/arm/mach-at91/include/mach/at91_adc.h b/arch/arm/mach-at91/include/mach/at91_adc.h index 8e7ed5c..f00926a 100644 --- a/arch/arm/mach-at91/include/mach/at91_adc.h +++ b/arch/arm/mach-at91/include/mach/at91_adc.h @@ -28,9 +28,12 @@ #define AT91_ADC_TRGSEL_EXTERNAL (6 << 1) #define AT91_ADC_LOWRES (1 << 4) /* Low Resolution */ #define AT91_ADC_SLEEP (1 << 5) /* Sleep Mode */ -#define AT91_ADC_PRESCAL (0x3f << 8) /* Prescalar Rate Selection */ +#define AT91_ADC_PRESCAL_9260 (0x3f << 8) /* Prescalar Rate Selection */ +#define AT91_ADC_PRESCAL_9G45 (0xff << 8) #define AT91_ADC_PRESCAL_(x) ((x) << 8) -#define AT91_ADC_STARTUP (0x1f << 16) /* Startup Up Time */ +#define AT91_ADC_STARTUP_9260 (0x1f << 16) /* Startup Up Time */ +#define AT91_ADC_STARTUP_9G45 (0x7f << 16) +#define AT91_ADC_STARTUP_9X5 (0xf << 16) #define AT91_ADC_STARTUP_(x) ((x) << 16) #define AT91_ADC_SHTIM (0xf << 24) /* Sample & Hold Time */ #define AT91_ADC_SHTIM_(x) ((x) << 24) diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c index 9cf8ab5..b41eb60 100644 --- a/drivers/iio/adc/at91_adc.c +++ b/drivers/iio/adc/at91_adc.c @@ -39,6 +39,11 @@ #define at91_adc_writel(st, reg, val) \ (writel_relaxed(val, st->reg_base + reg)) +struct at91_adc_caps { + u32 mr_prescal_mask; + u32 mr_startup_mask; +}; + struct at91_adc_state { struct clk *adc_clk; u16 *buffer; @@ -62,6 +67,7 @@ struct at91_adc_state { u32 res; /* resolution used for convertions */ bool low_res; /* the resolution corresponds to the lowest one */ wait_queue_head_t wq_data_avail; + struct at91_adc_caps *caps; }; static irqreturn_t at91_adc_trigger_handler(int irq, void *p) @@ -429,6 +435,8 @@ ret: return ret; } +static const struct of_device_id at91_adc_dt_ids[]; + static int at91_adc_probe_dt(struct at91_adc_state *st, struct platform_device *pdev) { @@ -441,6 +449,9 @@ static int at91_adc_probe_dt(struct at91_adc_state *st, if (!node) return -EINVAL; + st->caps = (struct at91_adc_caps *) + of_match_device(at91_adc_dt_ids, &pdev->dev)->data; + st->use_external = of_property_read_bool(node, "atmel,adc-use-external-triggers"); if (of_property_read_u32(node, "atmel,adc-channels-used", &prop)) { @@ -704,8 +715,8 @@ static int at91_adc_probe(struct platform_device *pdev) shtim = round_up((st->sample_hold_time * adc_clk_khz / 1000) - 1, 1); - reg = AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL; - reg |= AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP; + reg = AT91_ADC_PRESCAL_(prsc) & st->caps->mr_prescal_mask; + reg |= AT91_ADC_STARTUP_(ticks) & st->caps->mr_startup_mask; if (st->low_res) reg |= AT91_ADC_LOWRES; if (st->sleep_mode) @@ -776,8 +787,25 @@ static int at91_adc_remove(struct platform_device *pdev) } #ifdef CONFIG_OF +static struct at91_adc_caps at91sam9260_caps = { + .mr_prescal_mask = AT91_ADC_PRESCAL_9260, + .mr_startup_mask = AT91_ADC_STARTUP_9260, +}; + +static struct at91_adc_caps at91sam9g45_caps = { + .mr_prescal_mask = AT91_ADC_PRESCAL_9G45, + .mr_startup_mask = AT91_ADC_STARTUP_9G45, +}; + +static struct at91_adc_caps at91sam9x5_caps = { + .mr_prescal_mask = AT91_ADC_PRESCAL_9G45, /* same as 9G45 */ + .mr_startup_mask = AT91_ADC_STARTUP_9X5, +}; + static const struct of_device_id at91_adc_dt_ids[] = { - { .compatible = "atmel,at91sam9260-adc" }, + { .compatible = "atmel,at91sam9260-adc", .data = &at91sam9260_caps }, + { .compatible = "atmel,at91sam9g45-adc", .data = &at91sam9g45_caps }, + { .compatible = "atmel,at91sam9x5-adc", .data = &at91sam9x5_caps }, {}, }; MODULE_DEVICE_TABLE(of, at91_adc_dt_ids);
For at91 boards, there are different IPs for adc. Different IPs has different STARTUP & PRESCAL mask in ADC_MR. This patch introduce the multiple compatible string for those different IPs. Signed-off-by: Josh Wu <josh.wu@atmel.com> --- v1 --> v2: 1. use multiple compatible string for different IPs instead of use run-time IP check of hardware register. 2. rebase on top of linus' master tree. .../devicetree/bindings/arm/atmel-adc.txt | 3 +- arch/arm/mach-at91/include/mach/at91_adc.h | 7 ++-- drivers/iio/adc/at91_adc.c | 34 ++++++++++++++++++-- 3 files changed, 38 insertions(+), 6 deletions(-)