Re: [PATCH] drivers: mfd: ti_am335x_tscadc: increase ADC ref clock to 24MHz

[John Syne]

> On Oct 31, 2016, at 4:39 AM, Vignesh R <vigneshr@xxxxxx> wrote:
> 
> 
> 
> On Friday 28 October 2016 02:47 AM, John Syne wrote:
> 
>>>>>>>>>>> 
>>>>>>>>>>> ---
>>>>>>>>>>> include/linux/mfd/ti_am335x_tscadc.h | 4 ++--
>>>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>>> 
>>>>>>>>>>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
>>>>>>>>>>> index b9a53e0..96c4207 100644
>>>>>>>>>>> --- a/include/linux/mfd/ti_am335x_tscadc.h
>>>>>>>>>>> +++ b/include/linux/mfd/ti_am335x_tscadc.h
>>>>>>>>>>> @@ -90,7 +90,7 @@
>>>>>>>>>>> /* Delay register */
>>>>>>>>>>> #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
>>>>>>>>>>> #define STEPDELAY_OPEN(val)	((val) << 0)
>>>>>>>>>>> -#define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
>>>>>>>>> Wouldnât this be better to add this to the devicetree?
>>>>>>>>> 
>>>>>>>>> 	ti,chan-step-avg = <0x16 0x16 0x16 0x16 0x16 0x16 0x16>;
>>>>>>>>> 	ti,chan-step-opendelay = <0x500 0x500 0x500 0x500 0x500 0x500 0x500>;
>>>>>>>>> 	ti,chan-step-sampledelay = <0x0 0x0 0x0 0x0 0x0 0x0 0x0>;
>>>>>>>> 
>>>>>>>> For a touch screen, there is not need to change in these parameter
>>>>>>>> settings, so my opinion is to keep it as is. Or am I missing something?
>>>>>>> I was thinking that if you are using this driver as an ADC, you may want the flexibility to make these changes in the DT. Iâm doing this by connecting sensors to the ADC inputs. Iâm not using this driver for a touchscreen. 
>>>>>> 
>>>>>> Here is a DT overlay were this gets using on the BeagleBoneBlack.  
>>>>>> 
>>>>>> https://github.com/RobertCNelson/bb.org-overlays/blob/master/src/arm/BB-ADC-00A0.dts
>>>>>> 
>>>>>> Besides, these DT features are already implemented in the driver so it is just a matter of adding these entries to the am33xx.dtsi & am4372.dtsi, which you modified in this patch series.
>>>>> 
>>>>> This looks like configuration, no?
>>>>> 
>>>>> DT should be used to describe the hardware.
>>>> You may be right, but how is this different to setting the baud rate on a serial channel or sampling rate on a audio channel? Looking through the DT, there are many configuration settings, so Iâm not sure what is the correct way to handle this. Surely it is better to handle this in DT vs hard coding these settings?
>>> 
>>> I think setting the UART baud rate is also an invalid DT entry.
>>> 
>>> It's okay to list all of the options in DT, but to actually select
>>> one, that should be done either in userspace or as a kernel option.
>>> Perhaps as a Kconfig selection.
>> Yeah, this has been inconsistent for a long time. My only point was that these DT parameters had already been implemented in the ti_am335x_adc KM and I thought that this was better than hard coding these settings. Implementing this in Kconfig means rebuilding the KM, which isnât desirable. 
> 
>> Perhaps this should be done via sysfs attributes so as you say, a userspace app can configure this driver. 
> 
> This was discussed when DT properties were added.  Patches are welcome
> to add sysfs entries. There is nothing wrong with specifying an initial
> value in the DT.
> 
>> I guess the DT code in ti_am335x_adc.c should be removed. 
>> 
> 
> Removing DT properties is not an option as it will break DT backward
> compatibility.
Hi Vignesh,

OK, then back to my original question. Given that these DT properties are supported in the driver, shouldnât the following be added to am33xx.dtsi and am4372.dtsi?

ti,chan-step-avg = <0x16 0x16 0x16 0x16 0x16 0x16 0x16>;
ti,chan-step-opendelay = <0x500 0x500 0x500 0x500 0x500 0x500 0x500>;
ti,chan-step-sampledelay = <0x0 0x0 0x0 0x0 0x0 0x0 0x0>;

Regards,
John
> 
> -- 
> Regards
> Vignesh