Hello Jonathan,
On Sun, May 07, 2023 at 02:46:08PM +0100, Jonathan Cameron wrote:
> On Sun, 7 May 2023 11:10:25 +0200
> Philippe De Muyter <Philippe.DeMuyter@xxxxxxx> wrote:
>
> > From: Philippe De Muyter <phdm@xxxxxxxxx>
> >
> > The Texas Instruments DAC121C* chips are the I2C counterparts of
> > the DAC121S* SPI chips which are already supported by this ad5446 driver.
> >
> > Add them to the compatible list.
>
> Hi Philippe,
>
> DT binding should be updated and include the fallback to adi,ad5622.
> Does this driver actually have a bindings doc? If not please add one
> as a precursor patch then add binding for this new part on top.
No, there's no ad5446 to find in Documentation/ :(
I will try to make one.
>
> >
> > Signed-off-by: Philippe De Muyter <phdm@xxxxxxxxx>
> > Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Cc: Michael Hennerich <Michael.Hennerich@xxxxxxxxxx>
> > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> > ---
> > drivers/iio/dac/ad5446.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/iio/dac/ad5446.c b/drivers/iio/dac/ad5446.c
> > index aa3130b33456..b95c0ccbb796 100644
> > --- a/drivers/iio/dac/ad5446.c
> > +++ b/drivers/iio/dac/ad5446.c
> > @@ -587,6 +587,7 @@ static const struct i2c_device_id ad5446_i2c_ids[] = {
> > {"ad5602", ID_AD5602},
> > {"ad5612", ID_AD5612},
> > {"ad5622", ID_AD5622},
> > + {"dac121", ID_AD5622}, /* 'ti,' is dropped by 'of_modalias_node' */
>
> True, but why is the comment needed?
I will remove it
> Also, for consistency with the spi equivalent it should be dac121c101 or similar
> I think.
Actually the chip I use is a dac121c085, and there exists also dac121c081.
They share the same datasheet. The difference is only in the number of
i2c addresses the hardware designer may choose from and the pin used for the
external Vref.
Do you prefer I add only dac121c085, both dac121c085 and dac121c081, or
stick to the common part of the name, with or without the 'c' that stands
for i2C, like the 's' in dac121s101 stands for Spi ?
The documentation also mentions lower-resolution chips in the same family :
dac101c081 and dac101c085 (10 bit resolution), and dac081c081 and dac081c085
(8 bit resolution).
>
> I think this use of the driver with multiple vendor prefixes,
> also indicates we should really add the of_device_id table for this
> driver. To do that nicely will require more changes as we'd want to do
> the same for the SPI side which has a single entry table (which is odd)
> then deal with the data fields which should probably all be pointers
> rather than enum values.
>
> Still I'm fine with proper explicit DT support being left for a follow up patch.
>
> I do want the missing binding doc fixed though (which is independent of the
> question of how the driver binds based on the compatible values).
>
> Thanks,
>
> Jonathan
>
>
> > {}
> > };
> > MODULE_DEVICE_TABLE(i2c, ad5446_i2c_ids);
Best regards
Philippe
