[PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Andreas Kemnade]

Add the version of the EC in the Tolino Shine 2 HD
to the supported versions. It seems not to have an RTC
and does not ack data written to it.
The vendor kernel happily ignores write errors, using
I2C via userspace i2c-set also shows the error.
So add a quirk to ignore that error.

PWM can be successfully configured despite of that error.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v2:
- more comments about stacking regmap construction
- fix accidential line removal
- better naming for subdevices

 drivers/mfd/ntxec.c       | 61 +++++++++++++++++++++++++++++++++++++--
 include/linux/mfd/ntxec.h |  1 +
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
index 957de2b03529..0bbd2e34e89c 100644
--- a/drivers/mfd/ntxec.c
+++ b/drivers/mfd/ntxec.c
@@ -96,6 +96,38 @@ static struct notifier_block ntxec_restart_handler = {
 	.priority = 128,
 };
 
+static int regmap_ignore_write(void *context,
+			       unsigned int reg, unsigned int val)
+
+{
+	struct regmap *regmap = context;
+
+	regmap_write(regmap, reg, val);
+
+	return 0;
+}
+
+static int regmap_wrap_read(void *context, unsigned int reg,
+			    unsigned int *val)
+{
+	struct regmap *regmap = context;
+
+	return regmap_read(regmap, reg, val);
+}
+
+/*
+ * Some firmware versions do not ack written data, add a wrapper. It
+ * is used to stack another regmap on top.
+ */
+static const struct regmap_config regmap_config_noack = {
+	.name = "ntxec_noack",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.cache_type = REGCACHE_NONE,
+	.reg_write = regmap_ignore_write,
+	.reg_read = regmap_wrap_read
+};
+
 static const struct regmap_config regmap_config = {
 	.name = "ntxec",
 	.reg_bits = 8,
@@ -104,15 +136,20 @@ static const struct regmap_config regmap_config = {
 	.val_format_endian = REGMAP_ENDIAN_BIG,
 };
 
-static const struct mfd_cell ntxec_subdevices[] = {
+static const struct mfd_cell ntxec_subdev[] = {
 	{ .name = "ntxec-rtc" },
 	{ .name = "ntxec-pwm" },
 };
 
+static const struct mfd_cell ntxec_subdev_pwm[] = {
+	{ .name = "ntxec-pwm" },
+};
+
 static int ntxec_probe(struct i2c_client *client)
 {
 	struct ntxec *ec;
 	unsigned int version;
+	bool has_rtc;
 	int res;
 
 	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
@@ -137,6 +174,16 @@ static int ntxec_probe(struct i2c_client *client)
 	/* Bail out if we encounter an unknown firmware version */
 	switch (version) {
 	case NTXEC_VERSION_KOBO_AURA:
+		has_rtc = true;
+		break;
+	case NTXEC_VERSION_TOLINO_SHINE2:
+		has_rtc = false;
+		/* Another regmap stacked on top of the other */
+		ec->regmap = devm_regmap_init(ec->dev, NULL,
+					      ec->regmap,
+					      &regmap_config_noack);
+		if (IS_ERR(ec->regmap))
+			return PTR_ERR(ec->regmap);
 		break;
 	default:
 		dev_err(ec->dev,
@@ -181,8 +228,16 @@ static int ntxec_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, ec);
 
-	res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices,
-				   ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL);
+	if (has_rtc)
+		res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE,
+					   ntxec_subdev,
+					   ARRAY_SIZE(ntxec_subdev),
+					   NULL, 0, NULL);
+	else
+		res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE,
+					   ntxec_subdev_pwm,
+					   ARRAY_SIZE(ntxec_subdev_pwm),
+					   NULL, 0, NULL);
 	if (res)
 		dev_err(ec->dev, "Failed to add subdevices: %d\n", res);
 
diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
index 361204d125f1..26ab3b8eb612 100644
--- a/include/linux/mfd/ntxec.h
+++ b/include/linux/mfd/ntxec.h
@@ -33,5 +33,6 @@ static inline __be16 ntxec_reg8(u8 value)
 
 /* Known firmware versions */
 #define NTXEC_VERSION_KOBO_AURA	0xd726	/* found in Kobo Aura */
+#define NTXEC_VERSION_TOLINO_SHINE2 0xf110 /* found in Tolino Shine 2 HD */
 
 #endif
-- 
2.29.2

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Mon, Mar 08, 2021 at 10:29:52PM +0100, Andreas Kemnade wrote:
> Add the version of the EC in the Tolino Shine 2 HD
> to the supported versions. It seems not to have an RTC
> and does not ack data written to it.
> The vendor kernel happily ignores write errors, using
> I2C via userspace i2c-set also shows the error.
> So add a quirk to ignore that error.
> 
> PWM can be successfully configured despite of that error.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
[...]
>  	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> @@ -137,6 +174,16 @@ static int ntxec_probe(struct i2c_client *client)
>  	/* Bail out if we encounter an unknown firmware version */
>  	switch (version) {
>  	case NTXEC_VERSION_KOBO_AURA:
> +		has_rtc = true;
> +		break;
> +	case NTXEC_VERSION_TOLINO_SHINE2:
> +		has_rtc = false;
> +		/* Another regmap stacked on top of the other */

"[...] on top of the first", perhaps

> +		ec->regmap = devm_regmap_init(ec->dev, NULL,
> +					      ec->regmap,
> +					      &regmap_config_noack);
> +		if (IS_ERR(ec->regmap))
> +			return PTR_ERR(ec->regmap);

In any case,

Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Lee Jones]

Mark,

Could you take a look at this for me please:

On Mon, 08 Mar 2021, Andreas Kemnade wrote:
> Add the version of the EC in the Tolino Shine 2 HD
> to the supported versions. It seems not to have an RTC
> and does not ack data written to it.
> The vendor kernel happily ignores write errors, using
> I2C via userspace i2c-set also shows the error.
> So add a quirk to ignore that error.
> 
> PWM can be successfully configured despite of that error.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
> - more comments about stacking regmap construction
> - fix accidential line removal
> - better naming for subdevices
> 
>  drivers/mfd/ntxec.c       | 61 +++++++++++++++++++++++++++++++++++++--
>  include/linux/mfd/ntxec.h |  1 +
>  2 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
> index 957de2b03529..0bbd2e34e89c 100644
> --- a/drivers/mfd/ntxec.c
> +++ b/drivers/mfd/ntxec.c
> @@ -96,6 +96,38 @@ static struct notifier_block ntxec_restart_handler = {
>  	.priority = 128,
>  };
>  
> +static int regmap_ignore_write(void *context,
> +			       unsigned int reg, unsigned int val)
> +
> +{
> +	struct regmap *regmap = context;
> +
> +	regmap_write(regmap, reg, val);
> +
> +	return 0;
> +}
> +
> +static int regmap_wrap_read(void *context, unsigned int reg,
> +			    unsigned int *val)
> +{
> +	struct regmap *regmap = context;
> +
> +	return regmap_read(regmap, reg, val);
> +}
> +
> +/*
> + * Some firmware versions do not ack written data, add a wrapper. It
> + * is used to stack another regmap on top.
> + */
> +static const struct regmap_config regmap_config_noack = {
> +	.name = "ntxec_noack",
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.cache_type = REGCACHE_NONE,
> +	.reg_write = regmap_ignore_write,
> +	.reg_read = regmap_wrap_read
> +};
> +
>  static const struct regmap_config regmap_config = {
>  	.name = "ntxec",
>  	.reg_bits = 8,
> @@ -104,15 +136,20 @@ static const struct regmap_config regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_BIG,
>  };
>  
> -static const struct mfd_cell ntxec_subdevices[] = {
> +static const struct mfd_cell ntxec_subdev[] = {
>  	{ .name = "ntxec-rtc" },
>  	{ .name = "ntxec-pwm" },
>  };
>  
> +static const struct mfd_cell ntxec_subdev_pwm[] = {
> +	{ .name = "ntxec-pwm" },
> +};
> +
>  static int ntxec_probe(struct i2c_client *client)
>  {
>  	struct ntxec *ec;
>  	unsigned int version;
> +	bool has_rtc;
>  	int res;
>  
>  	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> @@ -137,6 +174,16 @@ static int ntxec_probe(struct i2c_client *client)
>  	/* Bail out if we encounter an unknown firmware version */
>  	switch (version) {
>  	case NTXEC_VERSION_KOBO_AURA:
> +		has_rtc = true;
> +		break;
> +	case NTXEC_VERSION_TOLINO_SHINE2:
> +		has_rtc = false;
> +		/* Another regmap stacked on top of the other */
> +		ec->regmap = devm_regmap_init(ec->dev, NULL,
> +					      ec->regmap,
> +					      &regmap_config_noack);
> +		if (IS_ERR(ec->regmap))
> +			return PTR_ERR(ec->regmap);
>  		break;
>  	default:
>  		dev_err(ec->dev,
> @@ -181,8 +228,16 @@ static int ntxec_probe(struct i2c_client *client)
>  
>  	i2c_set_clientdata(client, ec);
>  
> -	res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices,
> -				   ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL);
> +	if (has_rtc)
> +		res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE,
> +					   ntxec_subdev,
> +					   ARRAY_SIZE(ntxec_subdev),
> +					   NULL, 0, NULL);
> +	else
> +		res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE,
> +					   ntxec_subdev_pwm,
> +					   ARRAY_SIZE(ntxec_subdev_pwm),
> +					   NULL, 0, NULL);
>  	if (res)
>  		dev_err(ec->dev, "Failed to add subdevices: %d\n", res);
>  
> diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
> index 361204d125f1..26ab3b8eb612 100644
> --- a/include/linux/mfd/ntxec.h
> +++ b/include/linux/mfd/ntxec.h
> @@ -33,5 +33,6 @@ static inline __be16 ntxec_reg8(u8 value)
>  
>  /* Known firmware versions */
>  #define NTXEC_VERSION_KOBO_AURA	0xd726	/* found in Kobo Aura */
> +#define NTXEC_VERSION_TOLINO_SHINE2 0xf110 /* found in Tolino Shine 2 HD */
>  
>  #endif

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Wed, Mar 10, 2021 at 09:48:21AM +0000, Lee Jones wrote:

> Could you take a look at this for me please:

> > +static int regmap_ignore_write(void *context,
> > +			       unsigned int reg, unsigned int val)
> > +
> > +{
> > +	struct regmap *regmap = context;
> > +
> > +	regmap_write(regmap, reg, val);
> > +
> > +	return 0;
> > +}

If there were more users it'd be better to have this in the core so that
problems we can detect like cache stuff if that's used but if it's just
one broken device it's probably not worth it, this seems like something
you'd have to try to end up with and which is going to cause timeout
problems with a lot of I2C controllers which would tank performance
enough that people would notice.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Lee Jones]

On Thu, 11 Mar 2021, Mark Brown wrote:

> On Wed, Mar 10, 2021 at 09:48:21AM +0000, Lee Jones wrote:
> 
> > Could you take a look at this for me please:
> 
> > > +static int regmap_ignore_write(void *context,
> > > +			       unsigned int reg, unsigned int val)
> > > +
> > > +{
> > > +	struct regmap *regmap = context;
> > > +
> > > +	regmap_write(regmap, reg, val);
> > > +
> > > +	return 0;
> > > +}
> 
> If there were more users it'd be better to have this in the core so that
> problems we can detect like cache stuff if that's used but if it's just
> one broken device it's probably not worth it, this seems like something
> you'd have to try to end up with and which is going to cause timeout
> problems with a lot of I2C controllers which would tank performance
> enough that people would notice.

So Yoda, is this to go into the core, or stay where it is?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Mon, Mar 22, 2021 at 02:59:25PM +0000, Lee Jones wrote:
> On Thu, 11 Mar 2021, Mark Brown wrote:

> > If there were more users it'd be better to have this in the core so that
> > problems we can detect like cache stuff if that's used but if it's just
> > one broken device it's probably not worth it, this seems like something
> > you'd have to try to end up with and which is going to cause timeout
> > problems with a lot of I2C controllers which would tank performance
> > enough that people would notice.

> So Yoda, is this to go into the core, or stay where it is?

Well, nobody's sent me any patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Lee Jones]

On Tue, 23 Mar 2021, Mark Brown wrote:

> On Mon, Mar 22, 2021 at 02:59:25PM +0000, Lee Jones wrote:
> > On Thu, 11 Mar 2021, Mark Brown wrote:
> 
> > > If there were more users it'd be better to have this in the core so that
> > > problems we can detect like cache stuff if that's used but if it's just
> > > one broken device it's probably not worth it, this seems like something
> > > you'd have to try to end up with and which is going to cause timeout
> > > problems with a lot of I2C controllers which would tank performance
> > > enough that people would notice.
> 
> > So Yoda, is this to go into the core, or stay where it is?
> 
> Well, nobody's sent me any patches.

Code is still in the driver in v4.

My question is; should these functions really live in the SS?

This is the latest submission:

  https://lore.kernel.org/lkml/20210315191832.16842-1-andreas@kemnade.info/


-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Tue, Mar 23, 2021 at 05:20:02PM +0000, Lee Jones wrote:
> On Tue, 23 Mar 2021, Mark Brown wrote:
> 
> > On Mon, Mar 22, 2021 at 02:59:25PM +0000, Lee Jones wrote:
> > > On Thu, 11 Mar 2021, Mark Brown wrote:

> > > > If there were more users it'd be better to have this in the core so that
> > > > problems we can detect like cache stuff if that's used but if it's just
> > > > one broken device it's probably not worth it, this seems like something
> > > > you'd have to try to end up with and which is going to cause timeout
> > > > problems with a lot of I2C controllers which would tank performance
> > > > enough that people would notice.

> > > So Yoda, is this to go into the core, or stay where it is?

> > Well, nobody's sent me any patches.

> Code is still in the driver in v4.

> My question is; should these functions really live in the SS?

Perhaps we could avoid using that particular abbreviation.

Like I say it depends on how common this is - are we seeing other
devices with the same problem?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Lee Jones]

On Tue, 23 Mar 2021, Mark Brown wrote:

> On Tue, Mar 23, 2021 at 05:20:02PM +0000, Lee Jones wrote:
> > On Tue, 23 Mar 2021, Mark Brown wrote:
> > 
> > > On Mon, Mar 22, 2021 at 02:59:25PM +0000, Lee Jones wrote:
> > > > On Thu, 11 Mar 2021, Mark Brown wrote:
> 
> > > > > If there were more users it'd be better to have this in the core so that
> > > > > problems we can detect like cache stuff if that's used but if it's just
> > > > > one broken device it's probably not worth it, this seems like something
> > > > > you'd have to try to end up with and which is going to cause timeout
> > > > > problems with a lot of I2C controllers which would tank performance
> > > > > enough that people would notice.
> 
> > > > So Yoda, is this to go into the core, or stay where it is?
> 
> > > Well, nobody's sent me any patches.
> 
> > Code is still in the driver in v4.
> 
> > My question is; should these functions really live in the SS?
> 
> Perhaps we could avoid using that particular abbreviation.

Too soon?

> Like I say it depends on how common this is - are we seeing other
> devices with the same problem?

I'm not witness to any.  If this is your first encounter too then
maybe just leave it where it is.  At least for the moment.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On Tue, Mar 23, 2021 at 06:52:54PM +0000, Lee Jones wrote:
> On Tue, 23 Mar 2021, Mark Brown wrote:
> > On Tue, Mar 23, 2021 at 05:20:02PM +0000, Lee Jones wrote:

> > > My question is; should these functions really live in the SS?

> > Perhaps we could avoid using that particular abbreviation.

> Too soon?

Sadly there's still people who keep it relevant around.

> > Like I say it depends on how common this is - are we seeing other
> > devices with the same problem?

> I'm not witness to any.  If this is your first encounter too then
> maybe just leave it where it is.  At least for the moment.

Sounds reasonable, if more turn up the code can always be pulled into
the core.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Lee Jones]

On Mon, 08 Mar 2021, Andreas Kemnade wrote:

> Add the version of the EC in the Tolino Shine 2 HD
> to the supported versions. It seems not to have an RTC
> and does not ack data written to it.
> The vendor kernel happily ignores write errors, using
> I2C via userspace i2c-set also shows the error.
> So add a quirk to ignore that error.
> 
> PWM can be successfully configured despite of that error.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
> - more comments about stacking regmap construction
> - fix accidential line removal
> - better naming for subdevices
> 
>  drivers/mfd/ntxec.c       | 61 +++++++++++++++++++++++++++++++++++++--
>  include/linux/mfd/ntxec.h |  1 +
>  2 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
> index 957de2b03529..0bbd2e34e89c 100644
> --- a/drivers/mfd/ntxec.c
> +++ b/drivers/mfd/ntxec.c
> @@ -96,6 +96,38 @@ static struct notifier_block ntxec_restart_handler = {
>  	.priority = 128,
>  };
>  
> +static int regmap_ignore_write(void *context,
> +			       unsigned int reg, unsigned int val)
> +
> +{
> +	struct regmap *regmap = context;
> +
> +	regmap_write(regmap, reg, val);
> +
> +	return 0;
> +}
> +
> +static int regmap_wrap_read(void *context, unsigned int reg,
> +			    unsigned int *val)
> +{
> +	struct regmap *regmap = context;
> +
> +	return regmap_read(regmap, reg, val);
> +}
> +
> +/*
> + * Some firmware versions do not ack written data, add a wrapper. It
> + * is used to stack another regmap on top.
> + */
> +static const struct regmap_config regmap_config_noack = {
> +	.name = "ntxec_noack",
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.cache_type = REGCACHE_NONE,
> +	.reg_write = regmap_ignore_write,
> +	.reg_read = regmap_wrap_read
> +};
> +
>  static const struct regmap_config regmap_config = {
>  	.name = "ntxec",
>  	.reg_bits = 8,
> @@ -104,15 +136,20 @@ static const struct regmap_config regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_BIG,
>  };
>  
> -static const struct mfd_cell ntxec_subdevices[] = {
> +static const struct mfd_cell ntxec_subdev[] = {
>  	{ .name = "ntxec-rtc" },
>  	{ .name = "ntxec-pwm" },
>  };
>  
> +static const struct mfd_cell ntxec_subdev_pwm[] = {
> +	{ .name = "ntxec-pwm" },
> +};

To put across what you're trying to achieve, maybe call this:

  ntxec_subdev_no_rtc[]

Then if other devices are added, the semantics/intent stays the same
and it won't have to be renamed.

>  static int ntxec_probe(struct i2c_client *client)
>  {
>  	struct ntxec *ec;
>  	unsigned int version;
> +	bool has_rtc;
>  	int res;
>  
>  	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> @@ -137,6 +174,16 @@ static int ntxec_probe(struct i2c_client *client)
>  	/* Bail out if we encounter an unknown firmware version */
>  	switch (version) {
>  	case NTXEC_VERSION_KOBO_AURA:
> +		has_rtc = true;
> +		break;
> +	case NTXEC_VERSION_TOLINO_SHINE2:
> +		has_rtc = false;
> +		/* Another regmap stacked on top of the other */
> +		ec->regmap = devm_regmap_init(ec->dev, NULL,
> +					      ec->regmap,
> +					      &regmap_config_noack);
> +		if (IS_ERR(ec->regmap))
> +			return PTR_ERR(ec->regmap);
>  		break;
>  	default:
>  		dev_err(ec->dev,
> @@ -181,8 +228,16 @@ static int ntxec_probe(struct i2c_client *client)
>  
>  	i2c_set_clientdata(client, ec);
>  
> -	res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices,
> -				   ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL);
> +	if (has_rtc)
> +		res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE,
> +					   ntxec_subdev,
> +					   ARRAY_SIZE(ntxec_subdev),
> +					   NULL, 0, NULL);
> +	else
> +		res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE,
> +					   ntxec_subdev_pwm,
> +					   ARRAY_SIZE(ntxec_subdev_pwm),
> +					   NULL, 0, NULL);

You're over-complicating things.

Simply have a local mfd_cells variable that you assign either
'ntxec_subdev' or 'ntxec_subdev_pwm' to, then simply call
devm_mfd_add_devices() once.

It means you can also drop 'has_rtc'.

>  	if (res)
>  		dev_err(ec->dev, "Failed to add subdevices: %d\n", res);
>  
> diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
> index 361204d125f1..26ab3b8eb612 100644
> --- a/include/linux/mfd/ntxec.h
> +++ b/include/linux/mfd/ntxec.h
> @@ -33,5 +33,6 @@ static inline __be16 ntxec_reg8(u8 value)
>  
>  /* Known firmware versions */
>  #define NTXEC_VERSION_KOBO_AURA	0xd726	/* found in Kobo Aura */
> +#define NTXEC_VERSION_TOLINO_SHINE2 0xf110 /* found in Tolino Shine 2 HD */
>  
>  #endif

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

On Wed, Mar 10, 2021 at 09:55:45AM +0000, Lee Jones wrote:
> On Mon, 08 Mar 2021, Andreas Kemnade wrote:
[...]
> > -static const struct mfd_cell ntxec_subdevices[] = {
> > +static const struct mfd_cell ntxec_subdev[] = {
> >  	{ .name = "ntxec-rtc" },
> >  	{ .name = "ntxec-pwm" },
> >  };
> >  
> > +static const struct mfd_cell ntxec_subdev_pwm[] = {
> > +	{ .name = "ntxec-pwm" },
> > +};
> 
> To put across what you're trying to achieve, maybe call this:
> 
>   ntxec_subdev_no_rtc[]
> 
> Then if other devices are added, the semantics/intent stays the same
> and it won't have to be renamed.

Andreas, what is the full amount of functionality this version of the EC
can ever provide?

If it's only PWM, I think ntxec_subdev_pwm fits well.

[ The next subdevice that I can foresee is battery monitoring. ]


Sorry for the delay,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mfd: ntxec: Support for EC in Tolino Shine 2 HD

[Andreas Kemnade]

On Sat, 13 Mar 2021 12:17:34 +0100
Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:

> On Wed, Mar 10, 2021 at 09:55:45AM +0000, Lee Jones wrote:
> > On Mon, 08 Mar 2021, Andreas Kemnade wrote:  
> [...]
> > > -static const struct mfd_cell ntxec_subdevices[] = {
> > > +static const struct mfd_cell ntxec_subdev[] = {
> > >  	{ .name = "ntxec-rtc" },
> > >  	{ .name = "ntxec-pwm" },
> > >  };
> > >  
> > > +static const struct mfd_cell ntxec_subdev_pwm[] = {
> > > +	{ .name = "ntxec-pwm" },
> > > +};  
> > 
> > To put across what you're trying to achieve, maybe call this:
> > 
> >   ntxec_subdev_no_rtc[]
> > 
> > Then if other devices are added, the semantics/intent stays the same
> > and it won't have to be renamed.  
> 
> Andreas, what is the full amount of functionality this version of the EC
> can ever provide?
> 
> If it's only PWM, I think ntxec_subdev_pwm fits well.
> 
I think it is only PWM. At least I could not see any other I2C access.

> [ The next subdevice that I can foresee is battery monitoring. ]
> 
That is done via the RC5T619 on that device.

Regards,
Andreas