From: ChiYuan Huang <u0084500@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Lars-Peter Clausen <lars@metafoo.de>,
cy_huang <cy_huang@richtek.com>,
linux-iio <linux-iio@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>
Subject: Re: [PATCH 2/2] iio: adc: Add rtq6056 support
Date: Sat, 18 Jun 2022 23:16:44 +0800 [thread overview]
Message-ID: <CADiBU3-wO0-0c9y-GpE15ra2AqkAG-ftqTh+hCvYEY-TojZ-pg@mail.gmail.com> (raw)
In-Reply-To: <CAHp75VfEQ6tu4-NQKwA+63Ae5busUwV+ZRqD6BtbUA+EkCZnWw@mail.gmail.com>
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月18日 週六 凌晨1:08寫道:
>
> On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@gmail.com> wrote:
> >
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add Richtek RTQ6056 supporting.
> >
> > It can be used for the system to monitor load current and power with 16bit
>
> 16-bit
>
Ack in next.
> > resolution.
>
> Overall looks good, needs some cosmetic work.
>
> ...
>
> > +KernelVersion: 5.18.2
>
> Wrong version, this won't be part of a stable kernel.
>
From kernel.org, currently the stable kernel version is 5.18.5.
Change to 5.18.5?
> ...
>
> > +#include <linux/of.h>
>
> Any users of this?
>
function 'of_property_read_u32'.
But from Jonathan's reply, I may change it to 'device_property_read_u32'.
And also property.h will be included.
> But for sure you missed
>
> mod_devicetable.h
> types.h
>
Ack in next. But for types.h, i2c.h already include device.h.
And device.h already include types.h.
Is it still needed to declare explicitly for types.h??
> ...
>
> > +#define RTQ6056_DEFAULT_RSHUNT 2000
>
> _mOHMs ?
>
From Jonathan's reply, I may remove this value defined.
Since it's already a straight value. To define it, seems to decrease
the readability.
> ...
>
> > +enum {
> > + F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
> > + F_MAX_FIELDS
>
> Hard to read this way. Split to be one emum entry per line.
>
Ack in next.
> > +};
>
> ...
>
> > +struct rtq6056_priv {
> > + struct device *dev;
> > + struct regmap *regmap;
>
> Swapping these two might give less code in the generated binary. Have
> you run bloat-o-meter?
>
I never know about this tool.
I'll check it before I submit the next revision.
Thanks for the reminding.
But from Jonathan's reply, I may remove 'struct regmap *regmap'.
If all function need the 'regmap', a local variable 'regmap' need to
be declared.
To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.
> > + struct regmap_field *rm_fields[F_MAX_FIELDS];
> > + u32 shunt_resistor_uohm;
> > + int vshuntct; /* vshunt conversion time in uS */
> > + int vbusct; /* vbus conversion time in uS */
> > + int avg_sample;
> > +};
>
> ...
>
> > + IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)
>
> Keep a comma.
>
Ack in next
> ...
>
>
> > + /* Only power and vbus channel is unsigned */
> > + if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
> > + *val = regval;
> > + else
>
> > + *val = (s16)regval;
>
> Why casting? At very minimum this requires a comment.
The value is already the 16-bit 2's complement value. That's why the
casting is here.
From Jonathan's reply, will replace it by sign_extend32.
>
> ...
>
> > + if (val > 8205 || val < 139)
> > + return -EINVAL;
>
> This strange range requires a good comment with possible references to
> the datasheet.
>
Ack in next.
> ...
>
> > +static const int rtq6056_avg_sample_list[] = {
> > + 1, 4, 16, 64, 128, 256, 512, 1024
>
> Keep a comma at the end.
>
> > +};
>
> ...
>
> > +static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + char *label)
> > +{
> > + return snprintf(label, PAGE_SIZE, "%s\n",
> > + rtq6056_channel_labels[chan->channel]);
>
> sysfs_emit()
>
> > +}
>
> ...
>
> > +static IIO_DEVICE_ATTR(shunt_resistor, 0644,
> > + rtq6056_shunt_resistor_show,
> > + rtq6056_shunt_resistor_store, 0);
>
> IIO_DEVICE_ATTR_RW()
>
> ...
>
> > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > + indio_dev->masklength) {
>
> On one line it's better.
>
Ack in next
> > +
>
> Redundant blank line.
>
Ack in next.
> > + ret = regmap_read(priv->regmap, RTQ6056_REG_SHUNTVOLT + bit,
> > + &raw);
> > + if (ret)
> > + goto out;
> > +
> > + data.vals[i++] = raw;
> > + }
>
> > + ret = of_property_read_u32(i2c->dev.of_node,
> > + "richtek,shunt-resistor-uohm",
> > + &shunt_resistor_uohm);
>
> device_property_read()
>
From you and other's reply, I may refine this part about the resistor parsing.
> > + if (ret)
> > + shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;
>
> Can be done without branch
>
OK
> ... = DEFAULT;
> device_property_read_u32(...); // no error checking.
>
> ...
>
> > +static int rtq6056_remove(struct i2c_client *i2c)
> > +{
> > + struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> > +
> > + /* Config opmode to 'shutdown' mode to minimize quiescient current */
>
> quiescent
>
Sorry for the typo
> > + return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> > +}
> > +
> > +static void rtq6056_shutdown(struct i2c_client *i2c)
> > +{
> > + struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> > +
> > + /* Config opmode to 'shutdown' mode to minimize quiescient current */
>
> quiescent
>
Sorry for the typo
> > + regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
next prev parent reply other threads:[~2022-06-18 15:16 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-17 9:32 [PATCH 0/2] Add Richtek RTQ6056 support cy_huang
2022-06-17 9:32 ` [PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support cy_huang
2022-06-18 1:45 ` Krzysztof Kozlowski
2022-06-18 15:50 ` ChiYuan Huang
2022-06-18 17:00 ` Krzysztof Kozlowski
2022-06-18 17:34 ` Jonathan Cameron
2022-06-18 17:35 ` Jonathan Cameron
2022-06-19 10:26 ` Krzysztof Kozlowski
2022-06-17 9:32 ` [PATCH 2/2] iio: adc: Add rtq6056 support cy_huang
2022-06-17 17:07 ` Andy Shevchenko
2022-06-18 15:16 ` ChiYuan Huang [this message]
2022-06-19 10:38 ` Andy Shevchenko
2022-06-24 3:39 ` ChiYuan Huang
2022-06-28 11:47 ` Andy Shevchenko
2022-06-17 17:10 ` Jonathan Cameron
2022-06-18 15:49 ` ChiYuan Huang
2022-06-19 11:32 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADiBU3-wO0-0c9y-GpE15ra2AqkAG-ftqTh+hCvYEY-TojZ-pg@mail.gmail.com \
--to=u0084500@gmail.com \
--cc=andy.shevchenko@gmail.com \
--cc=cy_huang@richtek.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).