linux-kernel - Re: [PATCH v3 04/18] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs

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]

Date:   Sat, 25 Feb 2017 16:53:40 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>,
        knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
        robh+dt@...nel.org, mark.rutland@....com, wens@...e.org,
        sre@...nel.org, linux@...linux.org.uk,
        maxime.ripard@...e-electrons.com, lee.jones@...aro.org
Cc:     linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        thomas.petazzoni@...e-electrons.com, icenowy@...c.xyz,
        bonbons@...ux-vserver.org
Subject: Re: [PATCH v3 04/18] iio: adc: add support for X-Powers AXP20X and
 AXP22X PMICs ADCs

On 21/02/17 18:03, Quentin Schulz wrote:
> Hi Jonathan,
> 
> On 19/02/2017 13:40, Jonathan Cameron wrote:
>> On 14/02/17 09:40, 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@...e-electrons.com>
>>> Acked-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
>> Hi Quentin,
>>
>> A few bits and bobs inline.  The bigest one is that I don't think
>> you need to carry your struct axp_data pointer around in the iio_priv
>> structure as it only seems to be used in probe.
>>
>> Anyhow, tidy these little bits up (quite a few are optional in the
>> sense that they are a matter or personal taste and I don't feel
>> strongly about them) and you can add
>>
>> Reviewed-by: Jonathan Cameron <jic23@...nel.org>
>>
>> A nice driver.
>>
>> Jonathan
> [...]
>>> +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->dev.parent = &pdev->dev;
>>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> +	info->data = (struct axp_data *)platform_get_device_id(pdev)->driver_data;
>> I think this is only actually used in probe, so could be handled more
>> neatly with a local variable rather than sticking it in info.
>>
> [...]
>>> +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);
>> Reorder the two lines above this one and then you could just do this
>> inline. (really trivial so feel free to not bother ;)
>>> +
>>> +	iio_device_unregister(indio_dev);
>>> +	iio_map_array_unregister(indio_dev);
>>> +
>>> +	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>>> +
>>> +	if (info->data->adc_en2)
> 
> data is used here to know if I should disable misc ADCs (if supported by
> the PMIC). So kinda need it only for that purpose, maybe I can use a
> slightly simpler structure to only store regmap and this adc_en2 boolean.
> 
> Ack for everything else.
cool. Not sure how my searching missed that!

Up to you on whether you simplify that structure or not, I'm happy either way.
> 
> Thanks,
> Quentin
>