Re: [linux-sunxi] [PATCH v2 6/9] power: supply: axp20x_usb_power: Use a match structure

[Chen-Yu Tsai <wens@csie.org>]

On Mon, Jan 6, 2020 at 1:59 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 1/5/20 4:34 AM, Chen-Yu Tsai wrote:
> > On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.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@sholland.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.

I was referring to the whole hunk. Seems of_device_get_match_data() needs to
be before devm_kzalloc() due to changes in the next patch. I would keep the
ordering the same in this patch, and do the shuffling when needed, i.e. in
the next patch. Please also mention any not required but desired reordering
in the commit log, such as moving platform_set_drvdata() to match the other
drivers.

Thanks
ChenYu

> >>         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@csie.org>
> >
>