diff mbox series

[2/4] input: misc: da9063_onkey: add mode change support

Message ID 20190916140358.30036-3-m.felsch@pengutronix.de (mailing list archive)
State Superseded
Headers show
Series DA9063 Onkey Improvements and Fixes | expand

Commit Message

Marco Felsch Sept. 16, 2019, 2:03 p.m. UTC
The pmic state machine behaviour upon a 'onkey press' event can be
configured using the ONKEY_PIN bit field. Most the time this is
configured correct by the OTP but sometimes we need to adjust the
behaviour so we need to add the support here.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/input/misc/da9063_onkey.c | 10 ++++++++++
 drivers/mfd/da9062-core.c         |  1 +
 2 files changed, 11 insertions(+)

Comments

Adam Thomson Sept. 16, 2019, 3:01 p.m. UTC | #1
On 16 September 2019 15:04, Marco Felsch wrote:

> The pmic state machine behaviour upon a 'onkey press' event can be
> configured using the ONKEY_PIN bit field. Most the time this is
> configured correct by the OTP but sometimes we need to adjust the
> behaviour so we need to add the support here.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/input/misc/da9063_onkey.c | 10 ++++++++++
>  drivers/mfd/da9062-core.c         |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/input/misc/da9063_onkey.c
> b/drivers/input/misc/da9063_onkey.c
> index fd355cf59397..bc982fcc87eb 100644
> --- a/drivers/input/misc/da9063_onkey.c
> +++ b/drivers/input/misc/da9063_onkey.c
> @@ -40,6 +40,7 @@ struct da9063_onkey {
>  	const struct da906x_chip_config *config;
>  	char phys[32];
>  	bool key_power;
> +	u8 key_opmode;
>  };
> 
>  static const struct da906x_chip_config da9063_regs = {
> @@ -193,6 +194,7 @@ static int da9063_onkey_probe(struct platform_device
> *pdev)
>  {
>  	struct da9063_onkey *onkey;
>  	const struct of_device_id *match;
> +	unsigned int val;
>  	int irq;
>  	int error;
> 
> @@ -220,6 +222,14 @@ static int da9063_onkey_probe(struct platform_device
> *pdev)
>  	onkey->key_power = !of_property_read_bool(pdev->dev.of_node,
>  						  "dlg,disable-key-power");
> 
> +	if (!of_property_read_u32(pdev->dev.of_node, "dlg,key-opmode",
> &val)) {
> +		error = regmap_update_bits(onkey->regmap,
> DA9062AA_CONFIG_I,
> +					   DA9062AA_NONKEY_PIN_MASK,
> +					   val << DA9062AA_NONKEY_PIN_SHIFT);
> +		if (error)
> +			return error;
> +	}
> +

This driver is used to cover DA9061, DA9062 and DA9063. This binding therefore
can apply to any of those as there's no checking of which device is being used.
For DA9063 usage, if this option is present then the probe will fail as your
regmap range update below only takes care of DA9061/2.

Ideally I think you should update the da906x_chip_config structure for this to
support all devices as I believe the same or similar options are available for
DA9063.

>  	onkey->input = devm_input_allocate_device(&pdev->dev);
>  	if (!onkey->input) {
>  		dev_err(&pdev->dev, "Failed to allocated input device.\n");
> diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> index e69626867c26..aaa1f1841bc3 100644
> --- a/drivers/mfd/da9062-core.c
> +++ b/drivers/mfd/da9062-core.c
> @@ -510,6 +510,7 @@ static const struct regmap_range
> da9062_aa_writeable_ranges[] = {
>  	regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
>  	regmap_reg_range(DA9062AA_BBAT_CONT, DA9062AA_BBAT_CONT),
>  	regmap_reg_range(DA9062AA_GP_ID_0, DA9062AA_GP_ID_19),
> +	regmap_reg_range(DA9062AA_CONFIG_I, DA9062AA_CONFIG_I),
>  };
> 
>  static const struct regmap_range da9062_aa_volatile_ranges[] = {
> --
> 2.20.1
Marco Felsch Sept. 17, 2019, 10:18 a.m. UTC | #2
Hi Adam,

On 19-09-16 15:01, Adam Thomson wrote:
> On 16 September 2019 15:04, Marco Felsch wrote:
> 
> > The pmic state machine behaviour upon a 'onkey press' event can be
> > configured using the ONKEY_PIN bit field. Most the time this is
> > configured correct by the OTP but sometimes we need to adjust the
> > behaviour so we need to add the support here.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/input/misc/da9063_onkey.c | 10 ++++++++++
> >  drivers/mfd/da9062-core.c         |  1 +
> >  2 files changed, 11 insertions(+)
> > 
> > diff --git a/drivers/input/misc/da9063_onkey.c
> > b/drivers/input/misc/da9063_onkey.c
> > index fd355cf59397..bc982fcc87eb 100644
> > --- a/drivers/input/misc/da9063_onkey.c
> > +++ b/drivers/input/misc/da9063_onkey.c
> > @@ -40,6 +40,7 @@ struct da9063_onkey {
> >  	const struct da906x_chip_config *config;
> >  	char phys[32];
> >  	bool key_power;
> > +	u8 key_opmode;
> >  };
> > 
> >  static const struct da906x_chip_config da9063_regs = {
> > @@ -193,6 +194,7 @@ static int da9063_onkey_probe(struct platform_device
> > *pdev)
> >  {
> >  	struct da9063_onkey *onkey;
> >  	const struct of_device_id *match;
> > +	unsigned int val;
> >  	int irq;
> >  	int error;
> > 
> > @@ -220,6 +222,14 @@ static int da9063_onkey_probe(struct platform_device
> > *pdev)
> >  	onkey->key_power = !of_property_read_bool(pdev->dev.of_node,
> >  						  "dlg,disable-key-power");
> > 
> > +	if (!of_property_read_u32(pdev->dev.of_node, "dlg,key-opmode",
> > &val)) {
> > +		error = regmap_update_bits(onkey->regmap,
> > DA9062AA_CONFIG_I,
> > +					   DA9062AA_NONKEY_PIN_MASK,
> > +					   val << DA9062AA_NONKEY_PIN_SHIFT);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> 
> This driver is used to cover DA9061, DA9062 and DA9063. This binding therefore
> can apply to any of those as there's no checking of which device is being used.
> For DA9063 usage, if this option is present then the probe will fail as your
> regmap range update below only takes care of DA9061/2.

That's right and I only updated the da9061/2 bindings.. 

> Ideally I think you should update the da906x_chip_config structure for this to
> support all devices as I believe the same or similar options are available for
> DA9063.

After checking the da9062/3 register.h this bitfield is the same for
da9062/3 devices. What about adding a comment here? The CONFIG_I
register is already writeable for the da9063 devices.

Regards,
  Marco

> 
> >  	onkey->input = devm_input_allocate_device(&pdev->dev);
> >  	if (!onkey->input) {
> >  		dev_err(&pdev->dev, "Failed to allocated input device.\n");
> > diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> > index e69626867c26..aaa1f1841bc3 100644
> > --- a/drivers/mfd/da9062-core.c
> > +++ b/drivers/mfd/da9062-core.c
> > @@ -510,6 +510,7 @@ static const struct regmap_range
> > da9062_aa_writeable_ranges[] = {
> >  	regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
> >  	regmap_reg_range(DA9062AA_BBAT_CONT, DA9062AA_BBAT_CONT),
> >  	regmap_reg_range(DA9062AA_GP_ID_0, DA9062AA_GP_ID_19),
> > +	regmap_reg_range(DA9062AA_CONFIG_I, DA9062AA_CONFIG_I),
> >  };
> > 
> >  static const struct regmap_range da9062_aa_volatile_ranges[] = {
> > --
> > 2.20.1
> 
>
Adam Thomson Sept. 17, 2019, 10:44 a.m. UTC | #3
On 17 September 2019 11:19, Marco Felsch wrote:

> Hi Adam,
> 
> On 19-09-16 15:01, Adam Thomson wrote:
> > On 16 September 2019 15:04, Marco Felsch wrote:
> >
> > > The pmic state machine behaviour upon a 'onkey press' event can be
> > > configured using the ONKEY_PIN bit field. Most the time this is
> > > configured correct by the OTP but sometimes we need to adjust the
> > > behaviour so we need to add the support here.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/input/misc/da9063_onkey.c | 10 ++++++++++
> > >  drivers/mfd/da9062-core.c         |  1 +
> > >  2 files changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/input/misc/da9063_onkey.c
> > > b/drivers/input/misc/da9063_onkey.c
> > > index fd355cf59397..bc982fcc87eb 100644
> > > --- a/drivers/input/misc/da9063_onkey.c
> > > +++ b/drivers/input/misc/da9063_onkey.c
> > > @@ -40,6 +40,7 @@ struct da9063_onkey {
> > >  	const struct da906x_chip_config *config;
> > >  	char phys[32];
> > >  	bool key_power;
> > > +	u8 key_opmode;
> > >  };
> > >
> > >  static const struct da906x_chip_config da9063_regs = {
> > > @@ -193,6 +194,7 @@ static int da9063_onkey_probe(struct platform_device
> > > *pdev)
> > >  {
> > >  	struct da9063_onkey *onkey;
> > >  	const struct of_device_id *match;
> > > +	unsigned int val;
> > >  	int irq;
> > >  	int error;
> > >
> > > @@ -220,6 +222,14 @@ static int da9063_onkey_probe(struct
> platform_device
> > > *pdev)
> > >  	onkey->key_power = !of_property_read_bool(pdev->dev.of_node,
> > >  						  "dlg,disable-key-power");
> > >
> > > +	if (!of_property_read_u32(pdev->dev.of_node, "dlg,key-opmode",
> > > &val)) {
> > > +		error = regmap_update_bits(onkey->regmap,
> > > DA9062AA_CONFIG_I,
> > > +					   DA9062AA_NONKEY_PIN_MASK,
> > > +					   val << DA9062AA_NONKEY_PIN_SHIFT);
> > > +		if (error)
> > > +			return error;
> > > +	}
> > > +
> >
> > This driver is used to cover DA9061, DA9062 and DA9063. This binding therefore
> > can apply to any of those as there's no checking of which device is being used.
> > For DA9063 usage, if this option is present then the probe will fail as your
> > regmap range update below only takes care of DA9061/2.
> 
> That's right and I only updated the da9061/2 bindings..

D'oh! That's what comes from taking a holiday the week before. :|

> 
> > Ideally I think you should update the da906x_chip_config structure for this to
> > support all devices as I believe the same or similar options are available for
> > DA9063.
> 
> After checking the da9062/3 register.h this bitfield is the same for
> da9062/3 devices. What about adding a comment here? The CONFIG_I
> register is already writeable for the da9063 devices.

Given the current implementation of this driver only uses tables to access the
correct registers and masks for each device, it would be neater to follow this
approach, although I am aware the register addresses and bit fields are the same.

> Regards,
>   Marco
> 
> >
> > >  	onkey->input = devm_input_allocate_device(&pdev->dev);
> > >  	if (!onkey->input) {
> > >  		dev_err(&pdev->dev, "Failed to allocated input device.\n");
> > > diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> > > index e69626867c26..aaa1f1841bc3 100644
> > > --- a/drivers/mfd/da9062-core.c
> > > +++ b/drivers/mfd/da9062-core.c
> > > @@ -510,6 +510,7 @@ static const struct regmap_range
> > > da9062_aa_writeable_ranges[] = {
> > >  	regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
> > >  	regmap_reg_range(DA9062AA_BBAT_CONT, DA9062AA_BBAT_CONT),
> > >  	regmap_reg_range(DA9062AA_GP_ID_0, DA9062AA_GP_ID_19),
> > > +	regmap_reg_range(DA9062AA_CONFIG_I, DA9062AA_CONFIG_I),
> > >  };
> > >
> > >  static const struct regmap_range da9062_aa_volatile_ranges[] = {
> > > --
> > > 2.20.1
> >
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Adam Thomson Sept. 17, 2019, 10:49 a.m. UTC | #4
On 17 September 2019 11:45, Adam Thomson wrote:

> > Hi Adam,
> >
> > On 19-09-16 15:01, Adam Thomson wrote:
> > > On 16 September 2019 15:04, Marco Felsch wrote:
> > >
> > > > The pmic state machine behaviour upon a 'onkey press' event can be
> > > > configured using the ONKEY_PIN bit field. Most the time this is
> > > > configured correct by the OTP but sometimes we need to adjust the
> > > > behaviour so we need to add the support here.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  drivers/input/misc/da9063_onkey.c | 10 ++++++++++
> > > >  drivers/mfd/da9062-core.c         |  1 +
> > > >  2 files changed, 11 insertions(+)
> > > >
> > > > diff --git a/drivers/input/misc/da9063_onkey.c
> > > > b/drivers/input/misc/da9063_onkey.c
> > > > index fd355cf59397..bc982fcc87eb 100644
> > > > --- a/drivers/input/misc/da9063_onkey.c
> > > > +++ b/drivers/input/misc/da9063_onkey.c
> > > > @@ -40,6 +40,7 @@ struct da9063_onkey {
> > > >  	const struct da906x_chip_config *config;
> > > >  	char phys[32];
> > > >  	bool key_power;
> > > > +	u8 key_opmode;
> > > >  };
> > > >
> > > >  static const struct da906x_chip_config da9063_regs = {
> > > > @@ -193,6 +194,7 @@ static int da9063_onkey_probe(struct
> platform_device
> > > > *pdev)
> > > >  {
> > > >  	struct da9063_onkey *onkey;
> > > >  	const struct of_device_id *match;
> > > > +	unsigned int val;
> > > >  	int irq;
> > > >  	int error;
> > > >
> > > > @@ -220,6 +222,14 @@ static int da9063_onkey_probe(struct
> > platform_device
> > > > *pdev)
> > > >  	onkey->key_power = !of_property_read_bool(pdev->dev.of_node,
> > > >  						  "dlg,disable-key-power");
> > > >
> > > > +	if (!of_property_read_u32(pdev->dev.of_node, "dlg,key-opmode",
> > > > &val)) {
> > > > +		error = regmap_update_bits(onkey->regmap,
> > > > DA9062AA_CONFIG_I,
> > > > +					   DA9062AA_NONKEY_PIN_MASK,
> > > > +					   val << DA9062AA_NONKEY_PIN_SHIFT);
> > > > +		if (error)
> > > > +			return error;
> > > > +	}
> > > > +
> > >
> > > This driver is used to cover DA9061, DA9062 and DA9063. This binding
> therefore
> > > can apply to any of those as there's no checking of which device is being used.
> > > For DA9063 usage, if this option is present then the probe will fail as your
> > > regmap range update below only takes care of DA9061/2.
> >
> > That's right and I only updated the da9061/2 bindings..
> 
> D'oh! That's what comes from taking a holiday the week before. :|

Actually I was right the first time. There's one binding covering this driver
for the 3 devices so my original point was valid although if that register is
in the valid regmap_range for DA9063 then it would succeed.

> 
> >
> > > Ideally I think you should update the da906x_chip_config structure for this to
> > > support all devices as I believe the same or similar options are available for
> > > DA9063.
> >
> > After checking the da9062/3 register.h this bitfield is the same for
> > da9062/3 devices. What about adding a comment here? The CONFIG_I
> > register is already writeable for the da9063 devices.
> 
> Given the current implementation of this driver only uses tables to access the
> correct registers and masks for each device, it would be neater to follow this
> approach, although I am aware the register addresses and bit fields are the same.
> 
> > Regards,
> >   Marco
> >
> > >
> > > >  	onkey->input = devm_input_allocate_device(&pdev->dev);
> > > >  	if (!onkey->input) {
> > > >  		dev_err(&pdev->dev, "Failed to allocated input device.\n");
> > > > diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> > > > index e69626867c26..aaa1f1841bc3 100644
> > > > --- a/drivers/mfd/da9062-core.c
> > > > +++ b/drivers/mfd/da9062-core.c
> > > > @@ -510,6 +510,7 @@ static const struct regmap_range
> > > > da9062_aa_writeable_ranges[] = {
> > > >  	regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
> > > >  	regmap_reg_range(DA9062AA_BBAT_CONT, DA9062AA_BBAT_CONT),
> > > >  	regmap_reg_range(DA9062AA_GP_ID_0, DA9062AA_GP_ID_19),
> > > > +	regmap_reg_range(DA9062AA_CONFIG_I, DA9062AA_CONFIG_I),
> > > >  };
> > > >
> > > >  static const struct regmap_range da9062_aa_volatile_ranges[] = {
> > > > --
> > > > 2.20.1
> > >
> > >
> >
> > --
> > Pengutronix e.K.                           |                             |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Marco Felsch Sept. 17, 2019, 10:56 a.m. UTC | #5
On 19-09-17 10:49, Adam Thomson wrote:
> On 17 September 2019 11:45, Adam Thomson wrote:
> 
> > > Hi Adam,
> > >
> > > On 19-09-16 15:01, Adam Thomson wrote:
> > > > On 16 September 2019 15:04, Marco Felsch wrote:
> > > >
> > > > > The pmic state machine behaviour upon a 'onkey press' event can be
> > > > > configured using the ONKEY_PIN bit field. Most the time this is
> > > > > configured correct by the OTP but sometimes we need to adjust the
> > > > > behaviour so we need to add the support here.
> > > > >
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > ---
> > > > >  drivers/input/misc/da9063_onkey.c | 10 ++++++++++
> > > > >  drivers/mfd/da9062-core.c         |  1 +
> > > > >  2 files changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/drivers/input/misc/da9063_onkey.c
> > > > > b/drivers/input/misc/da9063_onkey.c
> > > > > index fd355cf59397..bc982fcc87eb 100644
> > > > > --- a/drivers/input/misc/da9063_onkey.c
> > > > > +++ b/drivers/input/misc/da9063_onkey.c
> > > > > @@ -40,6 +40,7 @@ struct da9063_onkey {
> > > > >  	const struct da906x_chip_config *config;
> > > > >  	char phys[32];
> > > > >  	bool key_power;
> > > > > +	u8 key_opmode;
> > > > >  };
> > > > >
> > > > >  static const struct da906x_chip_config da9063_regs = {
> > > > > @@ -193,6 +194,7 @@ static int da9063_onkey_probe(struct
> > platform_device
> > > > > *pdev)
> > > > >  {
> > > > >  	struct da9063_onkey *onkey;
> > > > >  	const struct of_device_id *match;
> > > > > +	unsigned int val;
> > > > >  	int irq;
> > > > >  	int error;
> > > > >
> > > > > @@ -220,6 +222,14 @@ static int da9063_onkey_probe(struct
> > > platform_device
> > > > > *pdev)
> > > > >  	onkey->key_power = !of_property_read_bool(pdev->dev.of_node,
> > > > >  						  "dlg,disable-key-power");
> > > > >
> > > > > +	if (!of_property_read_u32(pdev->dev.of_node, "dlg,key-opmode",
> > > > > &val)) {
> > > > > +		error = regmap_update_bits(onkey->regmap,
> > > > > DA9062AA_CONFIG_I,
> > > > > +					   DA9062AA_NONKEY_PIN_MASK,
> > > > > +					   val << DA9062AA_NONKEY_PIN_SHIFT);
> > > > > +		if (error)
> > > > > +			return error;
> > > > > +	}
> > > > > +
> > > >
> > > > This driver is used to cover DA9061, DA9062 and DA9063. This binding
> > therefore
> > > > can apply to any of those as there's no checking of which device is being used.
> > > > For DA9063 usage, if this option is present then the probe will fail as your
> > > > regmap range update below only takes care of DA9061/2.
> > >
> > > That's right and I only updated the da9061/2 bindings..
> > 
> > D'oh! That's what comes from taking a holiday the week before. :|
> 
> Actually I was right the first time. There's one binding covering this driver
> for the 3 devices so my original point was valid although if that register is
> in the valid regmap_range for DA9063 then it would succeed.

You're right, there is a bit of mixing the naming.. The driver is called
da9063 and the binding is called da9062-onkey.txt.. Anyway, as you said
the regmap_range will be valid for both cases :)

> > 
> > >
> > > > Ideally I think you should update the da906x_chip_config structure for this to
> > > > support all devices as I believe the same or similar options are available for
> > > > DA9063.
> > >
> > > After checking the da9062/3 register.h this bitfield is the same for
> > > da9062/3 devices. What about adding a comment here? The CONFIG_I
> > > register is already writeable for the da9063 devices.
> > 
> > Given the current implementation of this driver only uses tables to access the
> > correct registers and masks for each device, it would be neater to follow this
> > approach, although I am aware the register addresses and bit fields are the same.

That's right because they are needed on other places during the value
evaluation. This register is only set once during probe and shouldn't be
changed afterwards.

Regards,
  Marco

> > 
> > > Regards,
> > >   Marco
> > >
> > > >
> > > > >  	onkey->input = devm_input_allocate_device(&pdev->dev);
> > > > >  	if (!onkey->input) {
> > > > >  		dev_err(&pdev->dev, "Failed to allocated input device.\n");
> > > > > diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> > > > > index e69626867c26..aaa1f1841bc3 100644
> > > > > --- a/drivers/mfd/da9062-core.c
> > > > > +++ b/drivers/mfd/da9062-core.c
> > > > > @@ -510,6 +510,7 @@ static const struct regmap_range
> > > > > da9062_aa_writeable_ranges[] = {
> > > > >  	regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
> > > > >  	regmap_reg_range(DA9062AA_BBAT_CONT, DA9062AA_BBAT_CONT),
> > > > >  	regmap_reg_range(DA9062AA_GP_ID_0, DA9062AA_GP_ID_19),
> > > > > +	regmap_reg_range(DA9062AA_CONFIG_I, DA9062AA_CONFIG_I),
> > > > >  };
> > > > >
> > > > >  static const struct regmap_range da9062_aa_volatile_ranges[] = {
> > > > > --
> > > > > 2.20.1
> > > >
> > > >
> > >
> > > --
> > > Pengutronix e.K.                           |                             |
> > > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
Adam Thomson Sept. 17, 2019, 3:58 p.m. UTC | #6
On 17 September 2019 11:56, Marco Felsch wrote:

> > > > > This driver is used to cover DA9061, DA9062 and DA9063. This binding
> > > therefore
> > > > > can apply to any of those as there's no checking of which device is being
> used.
> > > > > For DA9063 usage, if this option is present then the probe will fail as your
> > > > > regmap range update below only takes care of DA9061/2.
> > > >
> > > > That's right and I only updated the da9061/2 bindings..
> > >
> > > D'oh! That's what comes from taking a holiday the week before. :|
> >
> > Actually I was right the first time. There's one binding covering this driver
> > for the 3 devices so my original point was valid although if that register is
> > in the valid regmap_range for DA9063 then it would succeed.
> 
> You're right, there is a bit of mixing the naming.. The driver is called
> da9063 and the binding is called da9062-onkey.txt.. Anyway, as you said
> the regmap_range will be valid for both cases :)

Yes, that is a bit misleading. Really the binding document name should really
match the driver name.

> 
> > >
> > > >
> > > > > Ideally I think you should update the da906x_chip_config structure for this
> to
> > > > > support all devices as I believe the same or similar options are available for
> > > > > DA9063.
> > > >
> > > > After checking the da9062/3 register.h this bitfield is the same for
> > > > da9062/3 devices. What about adding a comment here? The CONFIG_I
> > > > register is already writeable for the da9063 devices.
> > >
> > > Given the current implementation of this driver only uses tables to access the
> > > correct registers and masks for each device, it would be neater to follow this
> > > approach, although I am aware the register addresses and bit fields are the
> same.
> 
> That's right because they are needed on other places during the value
> evaluation. This register is only set once during probe and shouldn't be
> changed afterwards.

I just think that given we already have a mechanism to differentiate between
the devices for register map access we should probably stick with it. To me it
feels a little messy using the direct DA9062 register definition for both
devices, but will leave the final word to Dimitry.
diff mbox series

Patch

diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c
index fd355cf59397..bc982fcc87eb 100644
--- a/drivers/input/misc/da9063_onkey.c
+++ b/drivers/input/misc/da9063_onkey.c
@@ -40,6 +40,7 @@  struct da9063_onkey {
 	const struct da906x_chip_config *config;
 	char phys[32];
 	bool key_power;
+	u8 key_opmode;
 };
 
 static const struct da906x_chip_config da9063_regs = {
@@ -193,6 +194,7 @@  static int da9063_onkey_probe(struct platform_device *pdev)
 {
 	struct da9063_onkey *onkey;
 	const struct of_device_id *match;
+	unsigned int val;
 	int irq;
 	int error;
 
@@ -220,6 +222,14 @@  static int da9063_onkey_probe(struct platform_device *pdev)
 	onkey->key_power = !of_property_read_bool(pdev->dev.of_node,
 						  "dlg,disable-key-power");
 
+	if (!of_property_read_u32(pdev->dev.of_node, "dlg,key-opmode", &val)) {
+		error = regmap_update_bits(onkey->regmap, DA9062AA_CONFIG_I,
+					   DA9062AA_NONKEY_PIN_MASK,
+					   val << DA9062AA_NONKEY_PIN_SHIFT);
+		if (error)
+			return error;
+	}
+
 	onkey->input = devm_input_allocate_device(&pdev->dev);
 	if (!onkey->input) {
 		dev_err(&pdev->dev, "Failed to allocated input device.\n");
diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
index e69626867c26..aaa1f1841bc3 100644
--- a/drivers/mfd/da9062-core.c
+++ b/drivers/mfd/da9062-core.c
@@ -510,6 +510,7 @@  static const struct regmap_range da9062_aa_writeable_ranges[] = {
 	regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
 	regmap_reg_range(DA9062AA_BBAT_CONT, DA9062AA_BBAT_CONT),
 	regmap_reg_range(DA9062AA_GP_ID_0, DA9062AA_GP_ID_19),
+	regmap_reg_range(DA9062AA_CONFIG_I, DA9062AA_CONFIG_I),
 };
 
 static const struct regmap_range da9062_aa_volatile_ranges[] = {