Re: [PATCH 28/40] mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support

Grygorii Strashko
2021-09-02T09:58:00Z

On 02/09/2021 09:47, Miquel Raynal wrote:
> Hi Grygorii,
>
> Grygorii Strashko <grygorii.strashko@ti.com> wrote on Wed, 1 Sep 2021
> 22:26:25 +0300:
>
>> On 25/08/2021 18:25, Miquel Raynal wrote:
>>> Introduce a new compatible that has another set of driver data,
>>> targeting am437x SoCs with a magnetic reader instead of the
>>> touchscreen and a more featureful set of registers.
>>>
>>> Co-developed-by: Jason Reeder <jreeder@ti.com>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Signed-off-by: Jason Reeder <jreeder@ti.com>
>>> ---
>>> drivers/mfd/ti_am335x_tscadc.c | 43 ++++++++++++++++++++++------
>>> include/linux/mfd/ti_am335x_tscadc.h | 9 +++++-
>>> 2 files changed, 43 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>>> index 1a30610dc65f..f4f6b9db4d2a 100644
>>> --- a/drivers/mfd/ti_am335x_tscadc.c
>>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>>> @@ -122,9 +122,9 @@ static int ti_tscadc_probe(struct platform_device *pdev)
>>> const __be32 *cur;
>>> struct clk *clk;
>>> u32 val;
>>> - bool use_tsc = false;
>>> + bool use_tsc = false, use_mag = false;
>>> int tscmag_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0;
>>> - int total_channels, err;
>>> + int mag_tracks = 0, total_channels, err;
>>> > /* Allocate memory for device */
>>> tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
>>> @@ -146,6 +146,12 @@ static int ti_tscadc_probe(struct platform_device *pdev)
>>> of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
>>> if (tscmag_wires)
>>> use_tsc = true;
>>> + } else {
>>> + node = of_get_child_by_name(pdev->dev.of_node, "mag");
>>> + of_property_read_u32(node, "ti,tracks", &mag_tracks);
>>
>> "ti,tracks" seems undocumented?
>
> Well that's true and almost on purpose, I am not focusing on the
> magnetic reader feature, it is not supported, I don't have one, I don't
> plan to add support for it. But in the driver I need to know how many
> "tracks" are unavailable for the ADC in order to implement the entire
> logic (this block comes from TI and the naming from Jason Reeder).
>
> I am not comfortable writing a binding file for a device that I won't
> use, it's the best way to miss something and have stable broken
> bindings in the future. So I assumed it was not a big deal to have this
> property in the code, which may be updated/removed/enhanced later if
> needed without having to mess with the code too much. What do you think?

Sry, it's not ok.
You need to (a) add binding or (b) w/a it in some way -
like drop it and use const value instead, for example.

--
Best regards,
grygorii