Message-ID: <edfac25c-d708-4a55-a9bb-97d329877444@ti.com>
Date: Thu, 4 Jul 2024 11:55:45 -0500
From: Andrew Davis <afd@...com>
To: Ayush Singh <ayush@...gleboard.org>, Nishanth Menon <nm@...com>,
        Vignesh
 Raghavendra <vigneshr@...com>,
        Tero Kristo <kristo@...nel.org>, Rob Herring
	<robh+dt@...nel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vaishnav M A <vaishnav@...gleboard.org>,
        Derek
 Kiernan <derek.kiernan@....com>,
        Dragan Cvetic <dragan.cvetic@....com>, Arnd
 Bergmann <arnd@...db.de>,
        Michael Walle <mwalle@...nel.org>,
        Jason Kridner
	<jkridner@...gleboard.org>,
        Robert Nelson <robertcnelson@...gleboard.org>,
        Robert Nelson <robertcnelson@...il.com>,
        Ayush Singh
	<ayushdevel1325@...il.com>
CC: <linux-arm-kernel@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 3/3] arm64: dts: ti: grove: Add Grove Sunlight Sensor
 overlay

On 7/3/24 9:15 AM, Ayush Singh wrote:
> On 7/2/24 22:14, Andrew Davis wrote:
> 
>> Add DT overlay for the Grove Sunlight Sensor[0].
>>
>> [0] https://wiki.seeedstudio.com/Grove-Sunlight_Sensor/
>>
>> Signed-off-by: Andrew Davis <afd@...com>
>> ---
>>   arch/arm64/boot/dts/ti/Makefile               |  3 ++
>>   .../boot/dts/ti/grove-sunlight-sensor.dtso    | 31 +++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/ti/grove-sunlight-sensor.dtso
>>
>> diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
>> index a859629a6072c..7d1ce7a5d97bc 100644
>> --- a/arch/arm64/boot/dts/ti/Makefile
>> +++ b/arch/arm64/boot/dts/ti/Makefile
>> @@ -8,6 +8,9 @@
>>   # Entries are grouped as per SoC present on the board. Groups are sorted
>>   # alphabetically.
>> +# This needs a better directory location
>> +dtb-$(CONFIG_OF_OVERLAY) += grove-sunlight-sensor.dtbo
>> +
>>   # Boards with AM62x SoC
>>   dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
>>   dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-ov5640.dtbo
>> diff --git a/arch/arm64/boot/dts/ti/grove-sunlight-sensor.dtso b/arch/arm64/boot/dts/ti/grove-sunlight-sensor.dtso
>> new file mode 100644
>> index 0000000000000..ab2f102e1f8ab
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/ti/grove-sunlight-sensor.dtso
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
>> +/**
>> + * Grove - Sunlight Sensor v1.0
>> + *
>> + * https://wiki.seeedstudio.com/Grove-Sunlight_Sensor/
>> + *
>> + * Copyright (C) 2024 Texas Instruments Incorporated - http://www.ti.com/
>> + */
>> +
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +&GROVE_CONNECTOR {
>> +    status = "okay";
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&GROVE_PIN1_MUX_I2C_SCL>,
>> +                <&GROVE_PIN2_MUX_I2C_SDA>;
>> +};
> 
> On setting pinctrl in the mikrobus connector, I seem to encounter problem with the SPI driver trying to use the device before the pins are ready. So I think, the pinctrl should probably be defined in the respective i2c, spi, etc nodes instead of connector.
> 

Maybe, I originally did that but the issue there is it can overwrite
any existing pinmux for that IP node. For instance if you add the
pinmux to a GPIO node, any other users of that GPIO lose their mux.

But you are right, they belong in the IP node. Maybe even in the
specific consumer device node (si1145@60 in this case).

The general idea with all of this is that if we have a board in a
static state (with add-ons already attached) we could write a DTS
that fully describes that steady state. Our challenge is to create
an overlay that transforms the base board into what we would have
written in the static case. In the static case we would have added
the pinmux to the IP node, so that is where it belongs.

The issue then is the overlay mechanism is not complete. We
can add properties to nodes, and add nodes to nodes, and append
properties to nodes, but cannot append values to existing properties,
only replace them completely. This gap in the overlay system will
prevent a general solution. So I've started to work on adding
that property appending ability to the overlay system. I should
have some patches posted against the upstream dtc/libfdt here
in the next week or so.

>> +
>> +&GROVE_PIN1_I2C {
>> +    status = "okay";
>> +    #address-cells = <1>;
>> +    #size-cells = <0>;
>> +
>> +    clock-frequency = <100000>;
>> +
>> +    si1145@60 {
>> +        compatible = "si,si1145";
>> +        reg = <0x60>;
>> +    };
>> +};
> 
> 
> I also have question regarding how to define reg property in SPI (chipselect). Ideally, we want to define it relative to the connector pins, but since the SPI device(s) is a child of SPI controller, I am not sure how I can do remapping.
> 

Could you give me an example? Sounds like the interrupt issue, where
we want say the interrupt belonging to "pin 5", but need to specify
it relative to the base interrupt controller, which we cannot know
anything about in the general case. Instead we need a map, from
pin number to both interrupt controller and IRQ number (or SPI
controller and chipselect number, etc..). I think you are on to
something with the GPIO names, select the GPIO by generic name,
not by board specific number. That might be extendable to IRQs
and other numbered items (but yes, that is still an open item
and I'm waiting on some add-on boards to arrive before I can
start testing ideas on this..).

Andrew

> 
> Ayush Singh
> 

Powered by blists - more mailing lists