Received: by 2002:a05:7412:b995:b0:f9:9502:5bb8 with SMTP id it21csp6416074rdb; Tue, 2 Jan 2024 00:31:41 -0800 (PST) X-Google-Smtp-Source: AGHT+IFPHCoHuvWksynUKeZJIgmxVloohVUGIGsN5ys8AFdtwrJs5WqQKz4lDLN/xOWPYelBn3J3 X-Received: by 2002:a05:6e02:1bce:b0:35f:ff8c:7a31 with SMTP id x14-20020a056e021bce00b0035fff8c7a31mr19056557ilv.65.1704184301037; Tue, 02 Jan 2024 00:31:41 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704184301; cv=none; d=google.com; s=arc-20160816; b=Sg8D/2x0FjVPAseB+r8gfjoox0AbeUxXvDbHWyT9ZsS5jWXfNypmXnLgwnthz3mY8j sNuTKwPOLJ0683ZKOAn0tZGLQi8S2iV/4DyBO/LIYXMLsBQ1SteupUV7FG08ruDt5kow Jt7IrWBGmOSgLJ3UvQUo0c+M46gUCLWnBIKrpL+P8ngANu5qs+QYGBojkDX2/W/bx0iP TgeNM4neBOYLt/jRF1BeSt8YaiVhHy+YEAyBlBkwjUhgvcucbjtrNGiOKRRbHAjxjmuP Hg04QTQiK5ZAwPj5h5dDVHFHuRqElNkYxZzRpeCh+GaSpY+04wbHMx87G7MbYfWDtIAY 8lzQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version :list-unsubscribe:list-subscribe:list-id:precedence:references :message-id:subject:cc:to:from:date; bh=RlFhDgGPtyeE9gCctRKkwMYJ+SImhCZp5hrSRLzMNaE=; fh=1J9kQzyKRxKGtZalq1izbnONX5ZVm30jAmgzo+vP1ek=; b=l2yzhmnWlzYW/7oyk5+CgZpZhc3n9bK+N1e9oGPNeI+kOztl94UMfufdti5BstZn3W 9o1UO7uoZLRsxo5YmyH55RYArGdcJahfwc4mLOt1X64cRsHjQpauDmDIr3zBAK+MdzbA 3C5hJXU7kJ8Gy+0Hai4jne43XkDTW4I2lRhadhOiUGpIu4veBgLV7ncXK2glww9MlsZ0 TTIV1Y9YvfHIUqZGivQWnWxTd8fmaNccfO7MqnvoW1L/rU2meoHSTPGNH8nLL+z/ECdV Oubxc9aLU+0aY/hb7H2LTJeVTlFirEWU7r8A4IDewuc23dMKA3QiZqx+AyLuolD8g+u6 7wkw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel+bounces-14147-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-14147-linux.lists.archive=gmail.com@vger.kernel.org" Return-Path: Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id 71-20020a63014a000000b005cdfb96ea62si17150839pgb.243.2024.01.02.00.31.40 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jan 2024 00:31:41 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-14147-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel+bounces-14147-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-14147-linux.lists.archive=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id A682F282A4B for ; Tue, 2 Jan 2024 08:31:40 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id EEF5553A9; Tue, 2 Jan 2024 08:31:31 +0000 (UTC) X-Original-To: linux-kernel@vger.kernel.org Received: from mg.richtek.com (mg.richtek.com [220.130.44.152]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 5E1624693; Tue, 2 Jan 2024 08:31:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=richtek.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=richtek.com X-MailGates: (SIP:2,PASS,NONE)(compute_score:DELIVER,40,3) Received: from 192.168.10.46 by mg.richtek.com with MailGates ESMTPS Server V6.0(636823:0:AUTH_RELAY) (envelope-from ) (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256/256); Tue, 02 Jan 2024 16:30:43 +0800 (CST) Received: from ex3.rt.l (192.168.10.46) by ex3.rt.l (192.168.10.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1258.27; Tue, 2 Jan 2024 16:30:42 +0800 Received: from linuxcarl2.richtek.com (192.168.10.154) by ex3.rt.l (192.168.10.45) with Microsoft SMTP Server id 15.2.1258.27 via Frontend Transport; Tue, 2 Jan 2024 16:30:42 +0800 Date: Tue, 2 Jan 2024 16:30:42 +0800 From: ChiYuan Huang To: Jonathan Cameron CC: Krzysztof Kozlowski , Conor Dooley , Lars-Peter Clausen , Rob Herring , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , , , Subject: Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family Message-ID: <20240102083042.GA13611@linuxcarl2.richtek.com> References: <74db15583a9a68701dbff5a1a967c0d987d6dfb6.1703762557.git.cy_huang@richtek.com> <20231230120347.0816bd09@jic23-huawei> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20231230120347.0816bd09@jic23-huawei> User-Agent: Mutt/1.5.21 (2010-09-15) Hi, Johathan: Most comments are good and will be fixed in next revision. Still one comment I cannot make sure. Please see the comment that's below yours. On Sat, Dec 30, 2023 at 12:03:47PM +0000, Jonathan Cameron wrote: > On Thu, 28 Dec 2023 19:29:35 +0800 > wrote: > > > From: ChiYuan Huang > > > > RTQ6053 and RTQ6059 are the same series of RTQ6056. > > > > The respective differences with RTQ6056 are listed below > > RTQ6053 > > - chip package type > > > > RTQ6059 > > - Reduce the pinout for vbus sensing pin > > - Some internal ADC scaling change > > > > Signed-off-by: ChiYuan Huang > Hi ChiYuan, > > Some comments inline, most focus on not mixing device specific features > across code and data. You always want them to be fully specified by > the the device specific const struct directly not by code using an > ID from there. It ends up more readable and more flexible to have it > all done via data or callbacks where things are a too complex for data. > > Thanks, > > Jonathan > > > --- > > v2 > > - Remove rtq6053 in DT match table and make rtq6053 fallback compatible > > with rtq6056 > > --- > > drivers/iio/adc/rtq6056.c | 264 ++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 250 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c > > index ad4cea6839b2..5587178cea83 100644 > > --- a/drivers/iio/adc/rtq6056.c > > +++ b/drivers/iio/adc/rtq6056.c > > @@ -39,6 +39,16 @@ > > #define RTQ6056_DEFAULT_CONFIG 0x4127 > > #define RTQ6056_CONT_ALLON 7 > > > > +#define RTQ6059_DEFAULT_CONFIG 0x3C47 > > +#define RTQ6059_VBUS_LSB_OFFSET 3 > > +#define RTQ6059_AVG_BASE 8 > > + > > +enum { > > + RICHTEK_DEV_RTQ6056 = 0, > > + RICHTEK_DEV_RTQ6059, > > + RICHTEK_DEV_MAX > > +}; > > + > > enum { > > RTQ6056_CH_VSHUNT = 0, > > RTQ6056_CH_VBUS, > > @@ -50,16 +60,29 @@ enum { > > enum { > > F_OPMODE = 0, > > F_VSHUNTCT, > > + F_SADC = F_VSHUNTCT, > > If the devices have different register fields, better to have different enums > for them as well as that should result in less confusing code. > Actually, this is all the same register, just the control naming difference. If not to define the new eum, I can remain to use the same field to handle rtq6059 part. > > > F_VBUSCT, > > + F_BADC = F_VBUSCT, > > F_AVG, > > + F_PGA = F_AVG, > > F_RESET, > > F_MAX_FIELDS > > }; > > > > +struct richtek_dev_data { > > + int dev_id; > > It almost always turns out to be a bad idea to use a mixture of > 'data' in a structure like this and a device id plus special casing int he > code. Better to add more data to this structure or callbacks specific > to the individual devices types. So I shouldn't see a dev_id field in > here at all. > > > + int default_conv_time; > > + unsigned int default_config; > > + unsigned int calib_coefficient; > > + const struct reg_field *reg_fields; > > + const struct iio_chan_spec *channels; > > +}; > > ... > > > static int rtq6056_adc_read_channel(struct rtq6056_priv *priv, > > struct iio_chan_spec const *ch, > > int *val) > > { > > + const struct richtek_dev_data *devdata = priv->devdata; > > struct device *dev = priv->dev; > > unsigned int addr = ch->address; > > unsigned int regval; > > @@ -168,10 +282,18 @@ static int rtq6056_adc_read_channel(struct rtq6056_priv *priv, > > return ret; > > > > /* Power and VBUS is unsigned 16-bit, others are signed 16-bit */ > > - if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER) > > + switch (addr) { > > + case RTQ6056_REG_BUSVOLT: > > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059) > > + regval >>= RTQ6059_VBUS_LSB_OFFSET; > > Store the offset as for other case below and apply it unconditionally. If it is > zero then this is a noop. > > > + fallthrough; > > + case RTQ6056_REG_POWER: > > *val = regval; > > - else > > + break; > > + default: > > *val = sign_extend32(regval, 16); > > Fallthrough stuff is harder to read so only use it when there is significant saving > in code. here just repeat the sign_extend32() in both cases. > > > + break; > > + } > > > > return IIO_VAL_INT; > > } > > @@ -199,6 +321,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val, > > } > > } > > > > > static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv, > > struct iio_chan_spec const *ch, int *val) > > { > > @@ -292,11 +464,15 @@ static int rtq6056_adc_read_raw(struct iio_dev *indio_dev, > > int *val2, long mask) > > { > > struct rtq6056_priv *priv = iio_priv(indio_dev); > > + const struct richtek_dev_data *devdata = priv->devdata; > > > > switch (mask) { > > case IIO_CHAN_INFO_RAW: > > return rtq6056_adc_read_channel(priv, chan, val); > > case IIO_CHAN_INFO_SCALE: > > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059) > > + return rtq6059_adc_read_scale(chan, val, val2); > > Provide a callback for this as for other examples below. > > > + > > return rtq6056_adc_read_scale(chan, val, val2); > > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > > *val = priv->avg_sample; > > @@ -313,16 +489,28 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev, > > const int **vals, int *type, int *length, > > long mask) > > { > > + struct rtq6056_priv *priv = iio_priv(indio_dev); > > + const struct richtek_dev_data *devdata = priv->devdata; > > + > > switch (mask) { > > case IIO_CHAN_INFO_SAMP_FREQ: > > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059) > > + return -EINVAL; > > Shouldn't need this protection as the channels won't have relevant > bitmap bit set and this will never be called. > > > + > > *vals = rtq6056_samp_freq_list; > > *type = IIO_VAL_INT; > > *length = ARRAY_SIZE(rtq6056_samp_freq_list); > > return IIO_AVAIL_LIST; > > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > > - *vals = rtq6056_avg_sample_list; > > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059) { > > + *vals = rtq6059_avg_sample_list; > > + *length = ARRAY_SIZE(rtq6059_avg_sample_list); > Store all this in devdata. > > *vals = devdata->avg_sample_list; > *length = devdata->avg_sample_list_length; > > > + } else { > > + *vals = rtq6056_avg_sample_list; > > + *length = ARRAY_SIZE(rtq6056_avg_sample_list); > > + } > > + > > *type = IIO_VAL_INT; > > - *length = ARRAY_SIZE(rtq6056_avg_sample_list); > > return IIO_AVAIL_LIST; > > default: > > return -EINVAL; > > @@ -334,6 +522,7 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev, > > int val2, long mask) > > { > > struct rtq6056_priv *priv = iio_priv(indio_dev); > > + const struct richtek_dev_data *devdata = priv->devdata; > > int ret; > > > > ret = iio_device_claim_direct_mode(indio_dev); > > @@ -342,10 +531,16 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev, > > > > switch (mask) { > > case IIO_CHAN_INFO_SAMP_FREQ: > > - ret = rtq6056_adc_set_samp_freq(priv, chan, val); > > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059) > > + ret = -EINVAL; > > + else > > + ret = rtq6056_adc_set_samp_freq(priv, chan, val); > > break; > > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > > - ret = rtq6056_adc_set_average(priv, val); > > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059) > > + ret = rtq6059_adc_set_average(priv, val); > > + else > > + ret = rtq6056_adc_set_average(priv, val); > > Provide a callback in devdata so this becomes something like > ret = devdata->set_averate(priv, val); > > > break; > > default: > > > +static const struct iio_info rtq6059_info = { > > + .attrs = &rtq6056_attribute_group, > > This is odd. you don't provide an access functions so you won't be able to read > channels etc. It isn't used so I guess you should just get rid of it. > > > +}; > > + > > static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p) > > { > > struct iio_poll_func *pf = p; > > struct iio_dev *indio_dev = pf->indio_dev; > > struct rtq6056_priv *priv = iio_priv(indio_dev); > > + const struct richtek_dev_data *devdata = priv->devdata; > > struct device *dev = priv->dev; > > struct { > > u16 vals[RTQ6056_MAX_CHANNEL]; > > @@ -469,6 +670,10 @@ static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p) > > if (ret) > > goto out; > > > > + if (devdata->dev_id == RICHTEK_DEV_RTQ6059 && > > + addr == RTQ6056_REG_BUSVOLT) > > Store an offset in the devdata->dev_id and this becomes something like. > if (addr == RTQ6056_REG_BUS_VOLT) > raw >>= devdata->vbus_offset; > > > + raw >>= RTQ6059_VBUS_LSB_OFFSET; > > + > > data.vals[i++] = raw; > > } > > > > @@ -528,20 +733,26 @@ static int rtq6056_probe(struct i2c_client *i2c) > > struct rtq6056_priv *priv; > > struct device *dev = &i2c->dev; > > struct regmap *regmap; > > + const struct richtek_dev_data *devdata; > > unsigned int vendor_id, shunt_resistor_uohm; > > int ret; > > > > if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA)) > > return -EOPNOTSUPP; > > > > + devdata = device_get_match_data(dev); > > + if (!devdata) > > + return dev_err_probe(dev, -EINVAL, "Invalid dev data\n"); > > + > > indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); > > if (!indio_dev) > > return -ENOMEM; > > > > priv = iio_priv(indio_dev); > > priv->dev = dev; > > - priv->vshuntct_us = priv->vbusct_us = 1037; > > + priv->vshuntct_us = priv->vbusct_us = devdata->default_conv_time; > > I'd keep a _us postfix for default_conv_time to make the units of that > self documenting as well. > > > priv->avg_sample = 1; > > + priv->devdata = devdata; > > i2c_set_clientdata(i2c, priv); > > > > regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config); > > @@ -556,20 +767,26 @@ static int rtq6056_probe(struct i2c_client *i2c) > > return dev_err_probe(dev, ret, > > "Failed to get manufacturer info\n"); > > > > + /* For RTQ6059, this vendor id value is meaningless */ > > If that is the case, why are you checking it? > I'd read that comment as meaning this will fail for RTQ6059 > > > if (vendor_id != RTQ6056_VENDOR_ID) > > return dev_err_probe(dev, -ENODEV, > > "Invalid vendor id 0x%04x\n", vendor_id); > > > > ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields, > > - rtq6056_reg_fields, F_MAX_FIELDS); > > + devdata->reg_fields, F_MAX_FIELDS); > > if (ret) > > return dev_err_probe(dev, ret, "Failed to init regmap field\n"); > > > > /* > > + * RTQ6053 & RTQ6056: > > * By default, configure average sample as 1, bus and shunt conversion > > * time as 1037 microsecond, and operating mode to all on. > > + * > > + * RTQ6059: > > + * By default, configure average sample as 1, bus and shunt conversion > > + * time as 532 microsecond, and operating mode to all on. > Move this documentation to where devdata->default_config is set. > It's device specific information, so put it in the device specific place not > the main code flow. > > > */ > > - ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG); > > + ret = regmap_write(regmap, RTQ6056_REG_CONFIG, devdata->default_config); > > if (ret) > > return dev_err_probe(dev, ret, > > "Failed to enable continuous sensing\n"); > > @@ -598,8 +815,8 @@ static int rtq6056_probe(struct i2c_client *i2c) > > > > indio_dev->name = "rtq6056"; > > indio_dev->modes = INDIO_DIRECT_MODE; > > - indio_dev->channels = rtq6056_channels; > > - indio_dev->num_channels = ARRAY_SIZE(rtq6056_channels); > > + indio_dev->channels = devdata->channels; > > + indio_dev->num_channels = RTQ6056_MAX_CHANNEL + 1; > > I'd embed the number of channels in devdata as well then the ARRAY_SIZE() code > that is obviously correct can still be used (just with different things being > counted depending on which channels are being used). The gain in readability > is worth the tiny bit of extra code and data. > > > indio_dev->info = &rtq6056_info; > > > > ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > > @@ -640,8 +857,27 @@ static int rtq6056_runtime_resume(struct device *dev) > > static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend, > > rtq6056_runtime_resume, NULL); > > > > +static const struct richtek_dev_data rtq6056_devdata = { > > + .dev_id = RICHTEK_DEV_RTQ6056, > > + .default_conv_time = 1037, > > + .calib_coefficient = 5120000, > > + .default_config = RTQ6056_DEFAULT_CONFIG, > > + .reg_fields = rtq6056_reg_fields, > > + .channels = rtq6056_channels, > > +}; > > + > > +static const struct richtek_dev_data rtq6059_devdata = { > > + .dev_id = RICHTEK_DEV_RTQ6059, > As mentioned above, this mix of data and a dev_id is not a good design pattern. > It tends to end up as insufficiently flexible as support for more devices is > added to a driver - plus it scatters the device type specific stuff all through > the driver rather than having it all in one place. > > > + .default_conv_time = 532, > > + .calib_coefficient = 40960000, > > + .default_config = RTQ6059_DEFAULT_CONFIG, > > + .reg_fields = rtq6059_reg_fields, > > + .channels = rtq6059_channels, > > +}; > > + > > static const struct of_device_id rtq6056_device_match[] = { > > - { .compatible = "richtek,rtq6056" }, > > + { .compatible = "richtek,rtq6056", .data = &rtq6056_devdata }, > > + { .compatible = "richtek,rtq6059", .data = &rtq6059_devdata }, > > {} > > }; > > MODULE_DEVICE_TABLE(of, rtq6056_device_match); >