On Mon, Jan 6, 2020 at 1:59 AM Samuel Holland <[email protected]> wrote:
>
> On 1/5/20 4:34 AM, Chen-Yu Tsai wrote:
> > On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >
>
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web, visit
https://groups.google.com/d/msgid/linux-sunxi/CAGb2v65GCs04ZwvvLGYNTH4x1Z4AKKac0-GtOaNGFgwaFrOHeg%40mail.gmail.com.
