[PATCH RFC 3/3] arm64: dts: ti: grove: Add Grove Sunlight Sensor overlay

[Ayush Singh]

On 7/4/24 22:25, Andrew Davis wrote:
> 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 at ti.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 at 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.

Sure. Will look forward to testing it.


>
>>> +
>>> +&GROVE_PIN1_I2C {
>>> +    status = "okay";
>>> +    #address-cells = <1>;
>>> +    #size-cells = <0>;
>>> +
>>> +    clock-frequency = <100000>;
>>> +
>>> +    si1145 at 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..).


Yes, the same problem will also occur for interrupt. What I am referring 
to here is the SPI chipselect. In a child of SPI controller, the reg 
property in child specifies the logical chipselect using a u32 number 
which is then mapped to the physical chipselect by the controller. So 
the Mikrobus CS pin might be SPI_CS0 on one system while it might be 
SPI_CS1 on another. This means we need a way to do the following:


&MIKROBUS_SCK_SPI {
     status = "okay";

     #address-cells = <1>;
     #size-cells = <0>;

     lsm6dsl_click: lsm6dsl-click at MIKROBUS_CS_SPI_REG {
         reg = <MIKROBUS_CS_SPI_REG>;
         compatible = "st,lsm6ds3";
         spi-max-frequency = <1000000>;
     };
};


Additionally, other pins can also be used as chipselect (Eg, SPI Extend 
Click uses RST and AN as chipselect). Some chipselect might be directly 
controlled by SPI controller while others might be normal GPIOs which 
were added using `cs-gpios` property in the controller.


of_spi_parse_dt(): 
https://github.com/torvalds/linux/blob/795c58e4c7fc6163d8fb9f2baa86cfe898fa4b19/drivers/spi/spi.c#L2468


>
> Andrew
>
>>
>> Ayush Singh
>>