linux-kernel - Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230723134721.1404d533@jic23-huawei>
Date:   Sun, 23 Jul 2023 13:47:21 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Andrea Collamati <andrea.collamati@...il.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

On Sun, 23 Jul 2023 14:15:10 +0200
Andrea Collamati <andrea.collamati@...il.com> wrote:

> On 7/23/23 13:41, Jonathan Cameron wrote:
> >>>> +
> >>>> +	if (mode == MCP4728_VREF_EXTERNAL_VDD &&
> >>>> +	    data->channel_data[chan->channel].g_mode == MCP4728_GAIN_X2) {
> >>>> +		dev_warn(&data->client->dev,
> >>>> +			 "CH%d: Gain x2 not effective when vref is vdd, force to x1",
> >>>> +			 chan->channel);    
> >>> Even better if you don't present the option at all and wrap it up in the
> >>> standard ABI of _scale
> >>>    
> >> I think that the solution could be:
> >>
> >> - Removing custom ABI (vref/gain)
> >>
> >> - Initialize them at device tree level using two 4-elements arrays.  
> > If doing with device tree, they should reflect something that is a characteristic
> > of how the chips is wired up.  So you would need to explain why that is the case here.
> >
> > However, I'm still not understanding why _SCALE is not appropriate here.  We have
> > a small set of options with well defined scales.  
> 
> SCALE is appropriate. I didn't know that scale_available was a standard ABI.
> 
> I will follow  the implementation done n https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/ad5592r-base.c#L487

That's long out of date wrt to best practice.  Instead follow.
drivers/iio/dac/ad7293.c or any of the other drivers that set
.info_mask_shared_by_*_available = BIT(IIO_CHAN_INFO_SCALE)
and provide the read_avail() callback.

The advantage that brings is that consumer drivers can then
access this information much more simply.

One day we will hopefully finish converting old drivers over to
newer interfaces, but it's slow progress!

Jonathan