| 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
| ||
|
Message-ID: <f40d7523-9eac-619c-1621-42b35c6cefbc@sholland.org>
Date: Sun, 5 Jan 2020 11:58:57 -0600
From: Samuel Holland <samuel@...lland.org>
To: Chen-Yu Tsai <wens@...e.org>
Cc: Sebastian Reichel <sre@...nel.org>,
Lee Jones <lee.jones@...aro.org>,
Hans de Goede <hdegoede@...hat.com>,
Oskari Lemmela <oskari@...mela.net>,
"open list:THERMAL" <linux-pm@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-sunxi <linux-sunxi@...glegroups.com>
Subject: Re: [linux-sunxi] [PATCH v2 6/9] power: supply: axp20x_usb_power: Use
a match structure
On 1/5/20 4:34 AM, Chen-Yu Tsai wrote:
> On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@...lland.org> wrote:
>>
>> Instead of ad-hoc variant ID checks throughout the code, let's start
>> moving the variant-specific details to a match structure. This allows
>> for future flexibility, and it better matches the other axp20x power
>> supply drivers.
>
> You should probably mention that there are still parts of the code
> where ID matching is done.
Will do for v3.
>> Signed-off-by: Samuel Holland <samuel@...lland.org>
>> ---
>> drivers/power/supply/axp20x_usb_power.c | 91 ++++++++++++++++---------
>> 1 file changed, 60 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
>> index dd3f3f12e41d..2d7272e19a87 100644
>> --- a/drivers/power/supply/axp20x_usb_power.c
>> +++ b/drivers/power/supply/axp20x_usb_power.c
>> @@ -405,6 +405,50 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
>> .set_property = axp20x_usb_power_set_property,
>> };
>>
>> +static const char * const axp20x_irq_names[] = {
>> + "VBUS_PLUGIN",
>> + "VBUS_REMOVAL",
>> + "VBUS_VALID",
>> + "VBUS_NOT_VALID",
>> + NULL
>> +};
>> +
>> +static const char * const axp22x_irq_names[] = {
>> + "VBUS_PLUGIN",
>> + "VBUS_REMOVAL",
>> + NULL
>> +};
>> +
>> +struct axp_data {
>> + const struct power_supply_desc *power_desc;
>> + const char * const *irq_names;
>> + enum axp20x_variants axp20x_id;
>> +};
>> +
>> +static const struct axp_data axp202_data = {
>> + .power_desc = &axp20x_usb_power_desc,
>> + .irq_names = axp20x_irq_names,
>> + .axp20x_id = AXP202_ID,
>> +};
>> +
>> +static const struct axp_data axp221_data = {
>> + .power_desc = &axp22x_usb_power_desc,
>> + .irq_names = axp22x_irq_names,
>> + .axp20x_id = AXP221_ID,
>> +};
>> +
>> +static const struct axp_data axp223_data = {
>> + .power_desc = &axp22x_usb_power_desc,
>> + .irq_names = axp22x_irq_names,
>> + .axp20x_id = AXP223_ID,
>> +};
>> +
>> +static const struct axp_data axp813_data = {
>> + .power_desc = &axp22x_usb_power_desc,
>> + .irq_names = axp22x_irq_names,
>> + .axp20x_id = AXP813_ID,
>> +};
>> +
>> static int configure_iio_channels(struct platform_device *pdev,
>> struct axp20x_usb_power *power)
>> {
>> @@ -440,12 +484,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>> struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> struct power_supply_config psy_cfg = {};
>> struct axp20x_usb_power *power;
>> - static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
>> - "VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
>> - static const char * const axp22x_irq_names[] = {
>> - "VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
>> - const char * const *irq_names;
>> - const struct power_supply_desc *usb_power_desc;
>> + const struct axp_data *axp_data;
>> int i, irq, ret;
>>
>> if (!of_device_is_available(pdev->dev.of_node))
>> @@ -456,15 +495,16 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>> return -EINVAL;
>> }
>>
>> + axp_data = of_device_get_match_data(&pdev->dev);
>> +
>> power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>> if (!power)
>> return -ENOMEM;
>>
>> - platform_set_drvdata(pdev, power);
>> - power->axp20x_id = (enum axp20x_variants)of_device_get_match_data(
>> - &pdev->dev);
>> -
>> power->regmap = axp20x->regmap;
>> + power->axp20x_id = axp_data->axp20x_id;
>> +
>> + platform_set_drvdata(pdev, power);
>
> Not sure why this needs to be reordered.
It doesn't necessarily; moving the call to platform_set_drvdata() matches the
order in axp20x_ac_power, which makes it easier to compare the two probe
functions. I can drop it for v3 if you prefer.
>> if (power->axp20x_id == AXP202_ID) {
>> /* Enable vbus valid checking */
>> @@ -481,18 +521,6 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>>
>> if (ret)
>> return ret;
>> -
>> - usb_power_desc = &axp20x_usb_power_desc;
>> - irq_names = axp20x_irq_names;
>> - } else if (power->axp20x_id == AXP221_ID ||
>> - power->axp20x_id == AXP223_ID ||
>> - power->axp20x_id == AXP813_ID) {
>> - usb_power_desc = &axp22x_usb_power_desc;
>> - irq_names = axp22x_irq_names;
>> - } else {
>> - dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
>> - axp20x->variant);
>> - return -EINVAL;
>> }
>>
>> if (power->axp20x_id == AXP813_ID) {
>> @@ -504,17 +532,18 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>> psy_cfg.of_node = pdev->dev.of_node;
>> psy_cfg.drv_data = power;
>>
>> - power->supply = devm_power_supply_register(&pdev->dev, usb_power_desc,
>> + power->supply = devm_power_supply_register(&pdev->dev,
>> + axp_data->power_desc,
>> &psy_cfg);
>> if (IS_ERR(power->supply))
>> return PTR_ERR(power->supply);
>>
>> /* Request irqs after registering, as irqs may trigger immediately */
>> - for (i = 0; irq_names[i]; i++) {
>> - irq = platform_get_irq_byname(pdev, irq_names[i]);
>> + for (i = 0; axp_data->irq_names[i]; i++) {
>> + irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
>> if (irq < 0) {
>> dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
>> - irq_names[i], irq);
>> + axp_data->irq_names[i], irq);
>> continue;
>> }
>> irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
>> @@ -522,7 +551,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>> axp20x_usb_power_irq, 0, DRVNAME, power);
>> if (ret < 0)
>> dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
>> - irq_names[i], ret);
>> + axp_data->irq_names[i], ret);
>> }
>>
>> INIT_DELAYED_WORK(&power->vbus_detect, axp20x_usb_power_poll_vbus);
>> @@ -544,16 +573,16 @@ static int axp20x_usb_power_remove(struct platform_device *pdev)
>> static const struct of_device_id axp20x_usb_power_match[] = {
>> {
>> .compatible = "x-powers,axp202-usb-power-supply",
>> - .data = (void *)AXP202_ID,
>> + .data = &axp202_data,
>> }, {
>> .compatible = "x-powers,axp221-usb-power-supply",
>> - .data = (void *)AXP221_ID,
>> + .data = &axp221_data,
>> }, {
>> .compatible = "x-powers,axp223-usb-power-supply",
>> - .data = (void *)AXP223_ID,
>> + .data = &axp223_data,
>> }, {
>> .compatible = "x-powers,axp813-usb-power-supply",
>> - .data = (void *)AXP813_ID,
>> + .data = &axp813_data,
>> }, { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
>> --
>> 2.23.0
>
> Otherwise,
>
> Reviewed-by: Chen-Yu Tsai <wens@...e.org>
>
Powered by blists - more mailing lists