linux-kernel - Re: [RESEND 10/14] iio: adc: mt6370: Add Mediatek MT6370 support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220603181917.3f737913@jic23-huawei>
Date:   Fri, 3 Jun 2022 18:19:17 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     ChiaEn Wu <peterwu.pub@...il.com>
Cc:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        lee.jones@...aro.org, daniel.thompson@...aro.org,
        jingoohan1@...il.com, pavel@....cz, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        sre@...nel.org, chunfeng.yun@...iatek.com,
        gregkh@...uxfoundation.org, lars@...afoo.de, lgirdwood@...il.com,
        broonie@...nel.org, linux@...ck-us.net,
        heikki.krogerus@...ux.intel.com, deller@....de,
        ChiYuan Huang <cy_huang@...htek.com>, alice_chen@...htek.com,
        chiaen_wu@...htek.com, dri-devel@...ts.freedesktop.org,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-iio@...r.kernel.org, linux-fbdev@...r.kernel.org
Subject: Re: [RESEND 10/14] iio: adc: mt6370: Add Mediatek MT6370 support

> >  
> > > +
> > > +#define MT6370_AICR_400MA            0x6
> > > +#define MT6370_ICHG_500MA            0x4
> > > +#define MT6370_ICHG_900MA            0x8
> > > +
> > > +#define ADC_CONV_TIME_US             35000
> > > +#define ADC_CONV_POLLING_TIME                1000
> > > +
> > > +struct mt6370_adc_data {
> > > +     struct device *dev;
> > > +     struct regmap *regmap;
> > > +     struct mutex lock;  
> >
> > All locks need documentation.  What is the scope of the lock?
> > Looks like it protects device state when doing setup, wait for read, read
> > cycles.  
> 
> This mutex lock is for preventing the different adc channel from being
> read at the same time.
> So, if I just change its name to adc_chan_lock or adc_lock and add the
> comment for this mutex lock, does this change meet your requirement

Yes

> 
> >  
> > > +};
> > > +
> > > +static int mt6370_adc_read_scale(struct mt6370_adc_data *priv,
> > > +                              int chan, int *val1, int *val2)
> > > +{
> > > +     unsigned int reg_val;
> > > +     int ret;
> > > +
> > > +     switch (chan) {
> > > +     case MT6370_CHAN_VBAT:
> > > +     case MT6370_CHAN_VSYS:
> > > +     case MT6370_CHAN_CHG_VDDP:
> > > +             *val1 = 5000;  
> >
> > This seems very large.  Voltages are in millivolts
> > as per Documentation/ABI/testing/sysfs-bus-iio
> > and this means each step is 5 volts.
> >
> > So value in mv is currently 5 * _raw
> >  
> 
> OK, I got it. Also, I will add the ABI file in the next version. Thanks!
Only add ABI documentation for anything non-standard.

The documentation scripts really don't like repeats!

> 
> > > +static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = {  
> >
> > Perhaps define an enum with which to index this and the chan spec
> > and hence ensure they end up matching.
> >  [vbusdiv5] = "vbusdiv5", etc
> >  
> 
> Do you mean that I can refine this const char array to the following array??
> 
> static const char * const mt6370_channel_labels[MT6370_CHAN_MAX] = {
>     [MT6370_CHAN_VBUSDIV5] =  "vbusdiv5",
>     [MT6370_CHAN_VBUSDIV2] =  "vbusdiv2",
>     ...
>     ...
>     [MT6370_CHAN_TEMP_JC] =  "temp_jc",
> };

Yes

thanks,

Jonathan