Re: [PATCH v2 04/25] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs

[Jonathan Cameron]

On 28/01/17 15:12, Quentin Schulz wrote:
> Hi Jonathan
> 
> On 28/01/2017 15:49, Jonathan Cameron wrote:
>> On 27/01/17 08:54, Quentin Schulz wrote:
>>> The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose the
>>> battery voltage, battery charge and discharge currents, AC-in and VBUS
>>> voltages and currents, 2 GPIOs muxable in ADC mode and PMIC temperature.
>>>
>>> This adds support for most of AXP20X and AXP22X ADCs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx>
>> Pretty good, but not everything seems to be cleaned up on error paths
>> in probe.
>>
>> A few other suggestions / questions inline.
>>
>> Jonathan
>>> ---
> [...]
>>> +static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>> +			  struct iio_chan_spec const *chan, int *val)
>>> +{
>>> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>>> +	int size = 12;
>>> +
>>> +	switch (chan->type) {
>>> +	case IIO_CURRENT:
>>> +		/*
>>> +		 * Unlike the Chinese datasheets tell, the charging current is
>>> +		 * stored on 12 bits, not 13 bits.
>>> +		 */
>>> +		if (chan->channel == AXP20X_BATT_DISCHRG_I)
>>> +			size = 13;
>> Given I don't think you can get here without it being current, voltage or temp;
>> couldn't this be done more cleanly with
>> if ((chan->type == IIO_CURRENT) && (chan->channel == AXP20X_BAT_DISCHRG_I))
>>    size = 13;
>>
>> and have the rest in the normal code flow?
>>
> 
> Indeed.
> 
>>> +	case IIO_VOLTAGE:
>>> +	case IIO_TEMP:
>>> +		*val = axp20x_read_variable_width(info->regmap, chan->address,
>>> +						  size);
>>> +		if (*val < 0)
>>> +			return *val;
>>> +
>>> +		return IIO_VAL_INT;
>>> +
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
> [...]
>>> +static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val,
>>> +			    int *val2)
>>> +{
>>> +	switch (chan->type) {
>>> +	case IIO_VOLTAGE:
>>> +		if (chan->channel != AXP22X_BATT_V)
>>> +			return -EINVAL;
>>> +
>>> +		*val = 1;
>>> +		*val2 = 100000;
>>> +		return IIO_VAL_INT_PLUS_MICRO;
>> A fixed scale of 1.1x? (just checking)
> 
> Yes, there is only one voltage exposed for AXP22X PMICs: the battery
> voltage which has a scale of 1.1 (for mV).
Fair enough - just seemed an oddly specific number!
> 
> [...]
>>> +static int axp20x_probe(struct platform_device *pdev)
>>> +{
>>> +	struct axp20x_adc_iio *info;
>>> +	struct iio_dev *indio_dev;
>>> +	struct axp20x_dev *axp20x_dev;
>>> +	int ret;
>>> +
>>> +	axp20x_dev = dev_get_drvdata(pdev->dev.parent);
>>> +
>>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>>> +	if (!indio_dev)
>>> +		return -ENOMEM;
>>> +
>>> +	info = iio_priv(indio_dev);
>>> +	platform_set_drvdata(pdev, indio_dev);
>>> +
>>> +	info->regmap = axp20x_dev->regmap;
>>> +	indio_dev->name = dev_name(&pdev->dev);
>> Not sure on this name - what does end up as?  Expected to be
>> a description of the part so in this case something like axp209-adc.
>> I've been lax at picking up on this in the past and it's led to some
>> crazy naming that is no use at all to userspace.  Basically this
>> name just provides a convenient user readable name for userspace apps to
>> use.
>>
> 
> ACK. Should we have a different name for AXP20X and AXP22X PMICs?
ideally yes.
> 
>>> +	indio_dev->dev.parent = &pdev->dev;
>>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> +	info->data = (struct axp_data *)of_device_get_match_data(&pdev->dev);
>>> +
>>> +	indio_dev->info = info->data->iio_info;
>>> +	indio_dev->num_channels = info->data->num_channels;
>>> +	indio_dev->channels = info->data->channels;
>>> +
>>> +	/* Enable the ADCs on IP */
>>> +	regmap_write(info->regmap, AXP20X_ADC_EN1, info->data->adc_en1_mask);
>>> +
>>> +	if (info->data->adc_en2)
>>> +		/* Enable GPIO0/1 and internal temperature ADCs */
>>> +		regmap_update_bits(info->regmap, AXP20X_ADC_EN2,
>>> +				   AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
>>> +
>>> +	/* Configure ADCs rate */
>>> +	regmap_update_bits(info->regmap, AXP20X_ADC_RATE, AXP20X_ADC_RATE_MASK,
>>> +			   info->data->adc_rate(100));
>>> +
>>> +	ret = iio_map_array_register(indio_dev, info->data->maps);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "failed to register IIO maps: %d\n", ret);
>> This should be disabling channels.
> 
> You mean disabling ADCs as it is done in the remove? If so, indeed.
yes.
> 
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = iio_device_register(indio_dev);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "could not register the device\n");
>>> +		regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>>> +
>>> +		if (info->data->adc_en2)
>>> +			regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>> I'd expect to see a complete unwind of what has been done earlier in probe
>> including iio_map_array_unregister.
>>
>> The traditional goto error* approach is probably worth having here to
>> make sure the unwind makes sense.
> 
> Indeed.
> 
>>> +
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int axp20x_remove(struct platform_device *pdev)
>>> +{
>>> +	struct axp20x_adc_iio *info;
>>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> +	info = iio_priv(indio_dev);
>>> +
>>> +	iio_device_unregister(indio_dev);
>> iio_map_array_unregister?
> 
> Yes. I see iio_device_unregister already disable all buffers, why not
> unregistering the map array as well in this function?
Mostly about symmetry - it's easier to be sure things are right
if there are explicit calls to set stuff up and then to unwind it.

Jonathan
> 
> [...]
> Thanks,
> Quentin
>