Re: [PATCH 2/2] dt-bindings: iio: dac: Add DAC8163

[David Lechner]

On 6/24/26 1:25 AM, Lukas Metz wrote:
> Thanks a lot for the review. This is my first time submitting a
> patch so im grateful for the detailed comments and suggestions.
> 
> On Tue, Jun 23, 2026 at 02:17:04PM -0500, David Lechner wrote:
>> It is more logical to put the dt-bindings patch first in the series
>> before the driver that makes use of it.
> 
> I will reorder the commits in v2.

For future reference, you don't need to respond to comments that you
agree with (save us time reading).

> 
>> There are a couple of more SPI properties needed since this is not a "normal"
>> SPI device. We can only write and not read because there is no D_OUT pin. So
>>
>> spi-rx-bus-width:
>>   items:
>>     - const: 0
>>
>> will describe this. 
> 
> I will the add the suggested changes to v2. Are there any other poperties 
> i have missed? Same for the other comments regarding vendor-prefix, 
> spi-max-frequency, avdd-supply and vref supply name.
> 
>> We also want the binding to be complete even if the driver doesn't all of it, so
>> `clear-gpios` and `sync-gpios` probably make sense too.
> 
> SYNC pin is the chip select pin of the device as described below. In
> that case i dont need to add it here right?

Ah, OK. It is probably worth mentioning that in the top-level description
here in the bindings.

> 
>> Usually, we don't bother with a property like this since it is redundant.
>> If an external reference supply is given, then it gets used, otherwise
>> the internal reference is used.
> 
> That sounds logical. I will remove the property completly.
> 
>> These chips don't appear to have a chip select pin, so this comment
>> doesn't make sense to me. More logical would be to just use dac@0
>> and reg = <0>; since it should just be ignored.
> 
> The SYNC pin on the device acts like a chip select pin.
> According to the datasheet: when the pin goes low it enables the input shift
> register. At least that was my understanding. On my board i have tested the 
> driver with the chip select signal connected to the SYNC pin. The
> example comes straight from my own device tree where i have two devices
> on the bus. Thats why i used reg<1> here but i can change it to 0 and
> remove the comment.>  
>> The pin is marked active low in the datasheet, so I would expect
>> this to be GPIO_ACTIVE_LOW.
> 
> I wasnt sure about that. The pin needs to be held low continuously. I
> thought when the pin is marked active low and i initialize the pin with
> GPIOD_OUT_LOW the result will be that the pin is held high. To match the
> datasheet description seems logical though.
> 

The GPIOD_OUT_LOW and GPIOD_OUT_HIGH names in the kernel are not ideal.
Think of them as "deasserted" and "asserted". If the devicetree has
GPIO_ACTIVE_LOW, then GPIOD_OUT_HIGH will "assert" the pin by pulling
the voltage low (because it is active low).