Re: [PATCH 4/4] power: supply: add AXP803/AXP813 AC and battery power supply support
Hi Quentin,
On 05.10.2018 11:29, Quentin Schulz wrote:
> Hi Oskari,
>
> On Thu, Oct 04, 2018 at 10:34:10PM +0300, Oskari Lemmela wrote:
>> AXP803 PMIC is register compatible with AXP813.
>>
>> Added support for AXP803/AXP813 AC power supply.
>> AXP8x3 is capable to limit input current and minimum input voltage.
>> Both of these register values are writeable.
>>
>> Signed-off-by: Oskari Lemmela
>> ---
>> drivers/iio/adc/axp20x_adc.c | 1 +
>> drivers/mfd/axp20x.c | 22 ++-
>> drivers/pinctrl/pinctrl-axp209.c | 1 +
>> drivers/power/supply/axp20x_ac_power.c | 194 +
>> drivers/power/supply/axp20x_battery.c | 3 +
>> include/linux/mfd/axp20x.h | 1 +
>> 6 files changed, 221 insertions(+), 1 deletion(-)
>>
> We usually want one commit per logical change. e.g. here, we would want
> *at least* one commit for the ADC, one for the MFD, one for the pinctrl,
> one for the battery, one for the AC.
This was my very first kernel patch. I was thinking based on git log that
I should split commits even more. Now I know and do this in next versions.
>> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
>> index 5be789269353..f919a16a8533 100644
>> --- a/drivers/iio/adc/axp20x_adc.c
>> +++ b/drivers/iio/adc/axp20x_adc.c
>> @@ -641,6 +641,7 @@ static const struct axp_data axp813_data = {
>> static const struct of_device_id axp20x_adc_of_match[] = {
>> { .compatible = "x-powers,axp209-adc", .data = (void *)_data, },
>> { .compatible = "x-powers,axp221-adc", .data = (void *)_data, },
>> +{ .compatible = "x-powers,axp803-adc", .data = (void *)_data, },
>> { .compatible = "x-powers,axp813-adc", .data = (void *)_data, },
>> { /* sentinel */ }
>> };
> Then it means AXP813 and AXP803 ADCs are identical. Use the compatible
> of the AXP813, no need to add a compatible for each and every PMIC's ADC
> that'll be compatible with one that already exists.
Understood. I suppose if some feature doesn't work for both models then
we have to do new compatible?
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 0be511dd93d0..c540f17549d5 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -731,8 +731,23 @@ static const struct mfd_cell axp803_cells[] = {
>> .name = "axp221-pek",
>> .num_resources = ARRAY_SIZE(axp803_pek_resources),
>> .resources = axp803_pek_resources,
>> +}, {
>> +.name = "axp20x-regulator"
>> +}, {
>> +.name = "axp20x-gpio",
>> +.of_compatible = "x-powers,axp803-gpio",
>> +}, {
>> +.name = "axp813-adc",
>> +.of_compatible = "x-powers,axp803-adc",
>> +}, {
>> +.name = "axp20x-battery-power-supply",
>> +.of_compatible = "x-powers,axp803-battery-power-supply",
>> +}, {
>> +.name = "axp20x-ac-power-supply",
>> +.of_compatible = "x-powers,axp803-ac-power-supply",
>> +.num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>> +.resources = axp20x_ac_power_supply_resources,
>> },
>> -{ .name = "axp20x-regulator" },
>> };
>>
>> static const struct mfd_cell axp806_self_working_cells[] = {
>> @@ -778,6 +793,11 @@ static const struct mfd_cell axp813_cells[] = {
>> }, {
>> .name = "axp20x-battery-power-supply",
>> .of_compatible = "x-powers,axp813-battery-power-supply",
>> +}, {
>> +.name = "axp20x-ac-power-supply",
>> +.of_compatible = "x-powers,axp813-ac-power-supply",
>> +.num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>> +.resources = axp20x_ac_power_supply_resources,
>> },
>> };
>>
>> diff --git a/drivers/pinctrl/pinctrl-axp209.c
>> b/drivers/pinctrl/pinctrl-axp209.c
>> index afd0b533c40a..21859579e0a2 100644
>> --- a/drivers/pinctrl/pinctrl-axp209.c
>> +++ b/drivers/pinctrl/pinctrl-axp209.c
>> @@ -387,6 +387,7 @@ static int axp20x_build_funcs_groups(struct
>> platform_device *pdev)
>>
>> static const struct of_device_id axp20x_pctl_match[] = {
>> { .compatible = "x-powers,axp209-gpio", .data = _data, },
>> +{ .compatible = "x-powers,axp803-gpio", .data = _data, },
>> { .compatible = "x-powers,axp813-gpio", .data = _data, },
>> { }
>> };
> Same here. No need for a new compatible.
>
>> diff --git a/drivers/power/supply/axp20x_ac_power.c
>> b/drivers/power/supply/axp20x_ac_power.c
>> index 0771f951b11f..3247efb81cd1 100644
>> --- a/drivers/power/supply/axp20x_ac_power.c
>> +++ b/drivers/power/supply/axp20x_ac_power.c
>> @@ -27,6 +27,23 @@
>> #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7)
>> #define AXP20X_PWR_STATUS_ACIN_AVAIL
Re: [PATCH 4/4] power: supply: add AXP803/AXP813 AC and battery power supply support
Hi Quentin,
On 05.10.2018 11:29, Quentin Schulz wrote:
> Hi Oskari,
>
> On Thu, Oct 04, 2018 at 10:34:10PM +0300, Oskari Lemmela wrote:
>> AXP803 PMIC is register compatible with AXP813.
>>
>> Added support for AXP803/AXP813 AC power supply.
>> AXP8x3 is capable to limit input current and minimum input voltage.
>> Both of these register values are writeable.
>>
>> Signed-off-by: Oskari Lemmela
>> ---
>> drivers/iio/adc/axp20x_adc.c | 1 +
>> drivers/mfd/axp20x.c | 22 ++-
>> drivers/pinctrl/pinctrl-axp209.c | 1 +
>> drivers/power/supply/axp20x_ac_power.c | 194 +
>> drivers/power/supply/axp20x_battery.c | 3 +
>> include/linux/mfd/axp20x.h | 1 +
>> 6 files changed, 221 insertions(+), 1 deletion(-)
>>
> We usually want one commit per logical change. e.g. here, we would want
> *at least* one commit for the ADC, one for the MFD, one for the pinctrl,
> one for the battery, one for the AC.
This was my very first kernel patch. I was thinking based on git log that
I should split commits even more. Now I know and do this in next versions.
>> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
>> index 5be789269353..f919a16a8533 100644
>> --- a/drivers/iio/adc/axp20x_adc.c
>> +++ b/drivers/iio/adc/axp20x_adc.c
>> @@ -641,6 +641,7 @@ static const struct axp_data axp813_data = {
>> static const struct of_device_id axp20x_adc_of_match[] = {
>> { .compatible = "x-powers,axp209-adc", .data = (void *)_data, },
>> { .compatible = "x-powers,axp221-adc", .data = (void *)_data, },
>> +{ .compatible = "x-powers,axp803-adc", .data = (void *)_data, },
>> { .compatible = "x-powers,axp813-adc", .data = (void *)_data, },
>> { /* sentinel */ }
>> };
> Then it means AXP813 and AXP803 ADCs are identical. Use the compatible
> of the AXP813, no need to add a compatible for each and every PMIC's ADC
> that'll be compatible with one that already exists.
Understood. I suppose if some feature doesn't work for both models then
we have to do new compatible?
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 0be511dd93d0..c540f17549d5 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -731,8 +731,23 @@ static const struct mfd_cell axp803_cells[] = {
>> .name = "axp221-pek",
>> .num_resources = ARRAY_SIZE(axp803_pek_resources),
>> .resources = axp803_pek_resources,
>> +}, {
>> +.name = "axp20x-regulator"
>> +}, {
>> +.name = "axp20x-gpio",
>> +.of_compatible = "x-powers,axp803-gpio",
>> +}, {
>> +.name = "axp813-adc",
>> +.of_compatible = "x-powers,axp803-adc",
>> +}, {
>> +.name = "axp20x-battery-power-supply",
>> +.of_compatible = "x-powers,axp803-battery-power-supply",
>> +}, {
>> +.name = "axp20x-ac-power-supply",
>> +.of_compatible = "x-powers,axp803-ac-power-supply",
>> +.num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>> +.resources = axp20x_ac_power_supply_resources,
>> },
>> -{ .name = "axp20x-regulator" },
>> };
>>
>> static const struct mfd_cell axp806_self_working_cells[] = {
>> @@ -778,6 +793,11 @@ static const struct mfd_cell axp813_cells[] = {
>> }, {
>> .name = "axp20x-battery-power-supply",
>> .of_compatible = "x-powers,axp813-battery-power-supply",
>> +}, {
>> +.name = "axp20x-ac-power-supply",
>> +.of_compatible = "x-powers,axp813-ac-power-supply",
>> +.num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>> +.resources = axp20x_ac_power_supply_resources,
>> },
>> };
>>
>> diff --git a/drivers/pinctrl/pinctrl-axp209.c
>> b/drivers/pinctrl/pinctrl-axp209.c
>> index afd0b533c40a..21859579e0a2 100644
>> --- a/drivers/pinctrl/pinctrl-axp209.c
>> +++ b/drivers/pinctrl/pinctrl-axp209.c
>> @@ -387,6 +387,7 @@ static int axp20x_build_funcs_groups(struct
>> platform_device *pdev)
>>
>> static const struct of_device_id axp20x_pctl_match[] = {
>> { .compatible = "x-powers,axp209-gpio", .data = _data, },
>> +{ .compatible = "x-powers,axp803-gpio", .data = _data, },
>> { .compatible = "x-powers,axp813-gpio", .data = _data, },
>> { }
>> };
> Same here. No need for a new compatible.
>
>> diff --git a/drivers/power/supply/axp20x_ac_power.c
>> b/drivers/power/supply/axp20x_ac_power.c
>> index 0771f951b11f..3247efb81cd1 100644
>> --- a/drivers/power/supply/axp20x_ac_power.c
>> +++ b/drivers/power/supply/axp20x_ac_power.c
>> @@ -27,6 +27,23 @@
>> #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7)
>> #define AXP20X_PWR_STATUS_ACIN_AVAIL
Re: [PATCH 4/4] power: supply: add AXP803/AXP813 AC and battery power supply support
Hi Oskari,
On Thu, Oct 04, 2018 at 10:34:10PM +0300, Oskari Lemmela wrote:
> AXP803 PMIC is register compatible with AXP813.
>
> Added support for AXP803/AXP813 AC power supply.
> AXP8x3 is capable to limit input current and minimum input voltage.
> Both of these register values are writeable.
>
> Signed-off-by: Oskari Lemmela
> ---
> drivers/iio/adc/axp20x_adc.c | 1 +
> drivers/mfd/axp20x.c | 22 ++-
> drivers/pinctrl/pinctrl-axp209.c | 1 +
> drivers/power/supply/axp20x_ac_power.c | 194 +
> drivers/power/supply/axp20x_battery.c | 3 +
> include/linux/mfd/axp20x.h | 1 +
> 6 files changed, 221 insertions(+), 1 deletion(-)
>
We usually want one commit per logical change. e.g. here, we would want
*at least* one commit for the ADC, one for the MFD, one for the pinctrl,
one for the battery, one for the AC.
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 5be789269353..f919a16a8533 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -641,6 +641,7 @@ static const struct axp_data axp813_data = {
> static const struct of_device_id axp20x_adc_of_match[] = {
> { .compatible = "x-powers,axp209-adc", .data = (void *)_data, },
> { .compatible = "x-powers,axp221-adc", .data = (void *)_data, },
> + { .compatible = "x-powers,axp803-adc", .data = (void *)_data, },
> { .compatible = "x-powers,axp813-adc", .data = (void *)_data, },
> { /* sentinel */ }
> };
Then it means AXP813 and AXP803 ADCs are identical. Use the compatible
of the AXP813, no need to add a compatible for each and every PMIC's ADC
that'll be compatible with one that already exists.
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 0be511dd93d0..c540f17549d5 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -731,8 +731,23 @@ static const struct mfd_cell axp803_cells[] = {
> .name = "axp221-pek",
> .num_resources = ARRAY_SIZE(axp803_pek_resources),
> .resources = axp803_pek_resources,
> + }, {
> + .name = "axp20x-regulator"
> + }, {
> + .name = "axp20x-gpio",
> + .of_compatible = "x-powers,axp803-gpio",
> + }, {
> + .name = "axp813-adc",
> + .of_compatible = "x-powers,axp803-adc",
> + }, {
> + .name = "axp20x-battery-power-supply",
> + .of_compatible = "x-powers,axp803-battery-power-supply",
> + }, {
> + .name = "axp20x-ac-power-supply",
> + .of_compatible = "x-powers,axp803-ac-power-supply",
> + .num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
> + .resources = axp20x_ac_power_supply_resources,
> },
> - { .name = "axp20x-regulator" },
> };
>
> static const struct mfd_cell axp806_self_working_cells[] = {
> @@ -778,6 +793,11 @@ static const struct mfd_cell axp813_cells[] = {
> }, {
> .name = "axp20x-battery-power-supply",
> .of_compatible = "x-powers,axp813-battery-power-supply",
> + }, {
> + .name = "axp20x-ac-power-supply",
> + .of_compatible = "x-powers,axp813-ac-power-supply",
> + .num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
> + .resources = axp20x_ac_power_supply_resources,
> },
> };
>
> diff --git a/drivers/pinctrl/pinctrl-axp209.c
> b/drivers/pinctrl/pinctrl-axp209.c
> index afd0b533c40a..21859579e0a2 100644
> --- a/drivers/pinctrl/pinctrl-axp209.c
> +++ b/drivers/pinctrl/pinctrl-axp209.c
> @@ -387,6 +387,7 @@ static int axp20x_build_funcs_groups(struct
> platform_device *pdev)
>
> static const struct of_device_id axp20x_pctl_match[] = {
> { .compatible = "x-powers,axp209-gpio", .data = _data, },
> + { .compatible = "x-powers,axp803-gpio", .data = _data, },
> { .compatible = "x-powers,axp813-gpio", .data = _data, },
> { }
> };
Same here. No need for a new compatible.
> diff --git a/drivers/power/supply/axp20x_ac_power.c
> b/drivers/power/supply/axp20x_ac_power.c
> index 0771f951b11f..3247efb81cd1 100644
> --- a/drivers/power/supply/axp20x_ac_power.c
> +++ b/drivers/power/supply/axp20x_ac_power.c
> @@ -27,6 +27,23 @@
> #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7)
> #define AXP20X_PWR_STATUS_ACIN_AVAIL BIT(6)
>
> +#define AXP8X3_PWR_ACIN_VHOLDGENMASK(5, 3)
I would add _MASK at the end to be extra explicit.
> +#define AXP8X3_PWR_ACIN_VHOLD_4_0V (0 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_1V (1 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_2V (2 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_3V (3 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_4V (4 << 3)
>
Re: [PATCH 4/4] power: supply: add AXP803/AXP813 AC and battery power supply support
Hi Oskari,
On Thu, Oct 04, 2018 at 10:34:10PM +0300, Oskari Lemmela wrote:
> AXP803 PMIC is register compatible with AXP813.
>
> Added support for AXP803/AXP813 AC power supply.
> AXP8x3 is capable to limit input current and minimum input voltage.
> Both of these register values are writeable.
>
> Signed-off-by: Oskari Lemmela
> ---
> drivers/iio/adc/axp20x_adc.c | 1 +
> drivers/mfd/axp20x.c | 22 ++-
> drivers/pinctrl/pinctrl-axp209.c | 1 +
> drivers/power/supply/axp20x_ac_power.c | 194 +
> drivers/power/supply/axp20x_battery.c | 3 +
> include/linux/mfd/axp20x.h | 1 +
> 6 files changed, 221 insertions(+), 1 deletion(-)
>
We usually want one commit per logical change. e.g. here, we would want
*at least* one commit for the ADC, one for the MFD, one for the pinctrl,
one for the battery, one for the AC.
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 5be789269353..f919a16a8533 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -641,6 +641,7 @@ static const struct axp_data axp813_data = {
> static const struct of_device_id axp20x_adc_of_match[] = {
> { .compatible = "x-powers,axp209-adc", .data = (void *)_data, },
> { .compatible = "x-powers,axp221-adc", .data = (void *)_data, },
> + { .compatible = "x-powers,axp803-adc", .data = (void *)_data, },
> { .compatible = "x-powers,axp813-adc", .data = (void *)_data, },
> { /* sentinel */ }
> };
Then it means AXP813 and AXP803 ADCs are identical. Use the compatible
of the AXP813, no need to add a compatible for each and every PMIC's ADC
that'll be compatible with one that already exists.
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 0be511dd93d0..c540f17549d5 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -731,8 +731,23 @@ static const struct mfd_cell axp803_cells[] = {
> .name = "axp221-pek",
> .num_resources = ARRAY_SIZE(axp803_pek_resources),
> .resources = axp803_pek_resources,
> + }, {
> + .name = "axp20x-regulator"
> + }, {
> + .name = "axp20x-gpio",
> + .of_compatible = "x-powers,axp803-gpio",
> + }, {
> + .name = "axp813-adc",
> + .of_compatible = "x-powers,axp803-adc",
> + }, {
> + .name = "axp20x-battery-power-supply",
> + .of_compatible = "x-powers,axp803-battery-power-supply",
> + }, {
> + .name = "axp20x-ac-power-supply",
> + .of_compatible = "x-powers,axp803-ac-power-supply",
> + .num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
> + .resources = axp20x_ac_power_supply_resources,
> },
> - { .name = "axp20x-regulator" },
> };
>
> static const struct mfd_cell axp806_self_working_cells[] = {
> @@ -778,6 +793,11 @@ static const struct mfd_cell axp813_cells[] = {
> }, {
> .name = "axp20x-battery-power-supply",
> .of_compatible = "x-powers,axp813-battery-power-supply",
> + }, {
> + .name = "axp20x-ac-power-supply",
> + .of_compatible = "x-powers,axp813-ac-power-supply",
> + .num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
> + .resources = axp20x_ac_power_supply_resources,
> },
> };
>
> diff --git a/drivers/pinctrl/pinctrl-axp209.c
> b/drivers/pinctrl/pinctrl-axp209.c
> index afd0b533c40a..21859579e0a2 100644
> --- a/drivers/pinctrl/pinctrl-axp209.c
> +++ b/drivers/pinctrl/pinctrl-axp209.c
> @@ -387,6 +387,7 @@ static int axp20x_build_funcs_groups(struct
> platform_device *pdev)
>
> static const struct of_device_id axp20x_pctl_match[] = {
> { .compatible = "x-powers,axp209-gpio", .data = _data, },
> + { .compatible = "x-powers,axp803-gpio", .data = _data, },
> { .compatible = "x-powers,axp813-gpio", .data = _data, },
> { }
> };
Same here. No need for a new compatible.
> diff --git a/drivers/power/supply/axp20x_ac_power.c
> b/drivers/power/supply/axp20x_ac_power.c
> index 0771f951b11f..3247efb81cd1 100644
> --- a/drivers/power/supply/axp20x_ac_power.c
> +++ b/drivers/power/supply/axp20x_ac_power.c
> @@ -27,6 +27,23 @@
> #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7)
> #define AXP20X_PWR_STATUS_ACIN_AVAIL BIT(6)
>
> +#define AXP8X3_PWR_ACIN_VHOLDGENMASK(5, 3)
I would add _MASK at the end to be extra explicit.
> +#define AXP8X3_PWR_ACIN_VHOLD_4_0V (0 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_1V (1 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_2V (2 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_3V (3 << 3)
> +#define AXP8X3_PWR_ACIN_VHOLD_4_4V (4 << 3)
>
