Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1206151 > unrolled thread

Re: [PATCH] regmap: Add function check before called format_val

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] regmap: Add function check before called format_val Daniel Kurtz <djkurtz@chromium.org> - 2015-08-12 16:30 +0200
    Re: [PATCH] regmap: Add function check before called format_val Henry Chen <HenryC.Chen@mediatek.com> - 2015-08-13 17:30 +0200
      Re: [PATCH] regmap: Add function check before called format_val Mark Brown <broonie@kernel.org> - 2015-08-15 04:20 +0200

#1206151 — Re: [PATCH] regmap: Add function check before called format_val

Hi Henry & Mark,

On Tue, Jul 21, 2015 at 2:07 PM, Henry Chen <HenryC.Chen@mediatek.com> wrote:
> On Mon, 2015-07-20 at 16:02 +0100, Mark Brown wrote:
>> On Mon, Jul 20, 2015 at 08:41:50PM +0800, Henry Chen wrote:
>> > The regmap_format will not be initialize since regmap_bus is not assgined
>> > on regmap_init(). It should has a function check before using
>> > format_val() to avoid null function called on regmap_bulk_read().
>>
>> > -                   map->format.format_val(val + (i * val_bytes), ival, 0);
>> > +                   if (map->format.format_val)
>> > +                           map->format.format_val(val + (i * val_bytes), ival, 0);
>> > +                   else
>> > +                           memcpy(val + (i * val_bytes), &ival, val_bytes);
>>
>> Your changelog doesn't explan why we are in this code path in the first
>> place without a format_val() and why a memcpy() is an appropriate
>> replacement.  It should, it's not clear to me that this is a good fix
>> but I don't feel I fully understand the problem.
>
> Sorry for being unclear for issue, the call flow as following,
>
> First, in drivers/mfd/mtk_pmic_wrap.c which registered regmap without
> rebmap_bus.
> devm_regmap_init(wrp->dev, NULL, wrp, &pwrap_regmap_config);
>
> It call to regmap_init() and go to "skip_format_initialization" because
> regmap_bus didn't assign by driver.
>
> if (!bus) {
>         map->reg_read  = config->reg_read;
>         map->reg_write = config->reg_write;
>
>         map->defer_caching = false;
>         goto skip_format_initialization;"
>
> Then in driver rtc-mt6397.c, it used regmap_bulk_read() to get the time
> of PMIC, and hit the null function of format_val(), because the
> regmap_bus was null.
>
> It skipped the initialization of format_val() because bus == null, but
> called the format_val() at regmap_bulk_read() if bus == null.
>
> Maybe it was not the good fix for this, but should be a problem need to
> be reported, or should I need to give the regmap_bus on mtk_pmic_wrap.c?

I ran into this bug when testing Matthias' v4.2-next/for-next branch
on mt8173.  It now crashes on boot.

Since I didn't see it elsewhere in this discussion, I'll point out
that the "regression" here was introduced by commit [0], which added
the call to map->format.format_val from regmap_bulk_read() when
map->bus == NULL.

[0] commit 15b8d2c41fe5839582029f65c5f7004db451cc2b
  Author: Arun Chandran <achandran@mvista.com>
  regmap: Fix regmap_bulk_read in BE mode

Perhaps the easiest work around to unbreak v4.2 is, as Henry mentions,
for mtk_pmic_wrap to define its own regmap_bus, with .read() &
.write() handlers.  This way they will inherit the default built-in
format_val() from the regmap core.

Making mtk_pmic-wrap into a regmap_bus makes a bit of sense
architecturally, too, since it is essentially just a bus for accessing
the registers of an off-chip PMIC.  The CPU sees a platform bus, but
the registers of the remote PMIC are accessed over a dedicated SPI
bus.

WDYT?

Henry, can you try to implement this?

Thanks,
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1206915

On Wed, 2015-08-12 at 22:20 +0800, Daniel Kurtz wrote:

> 
> Since I didn't see it elsewhere in this discussion, I'll point out
> that the "regression" here was introduced by commit [0], which added
> the call to map->format.format_val from regmap_bulk_read() when
> map->bus == NULL.
> 
> [0] commit 15b8d2c41fe5839582029f65c5f7004db451cc2b
>   Author: Arun Chandran <achandran@mvista.com>
>   regmap: Fix regmap_bulk_read in BE mode
> 
> Perhaps the easiest work around to unbreak v4.2 is, as Henry mentions,
> for mtk_pmic_wrap to define its own regmap_bus, with .read() &
> .write() handlers.  This way they will inherit the default built-in
> format_val() from the regmap core.
> 
> Making mtk_pmic-wrap into a regmap_bus makes a bit of sense
> architecturally, too, since it is essentially just a bus for accessing
> the registers of an off-chip PMIC.  The CPU sees a platform bus, but
> the registers of the remote PMIC are accessed over a dedicated SPI
> bus.
> 
> WDYT?
> 
> Henry, can you try to implement this?

Hi Daniel,
I can try to create a regmap_bus for pmic wrap. But I'm not sure if it
was the good solution for this problem.

Hi Mark,
Sorry, I'm afraid that I cannot do this right on init as you said last
time. What do you think about regmap_bus, can you accept that way?

Thanks,
Henry
> 
> Thanks,
> -Dan


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207982

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 13, 2015 at 11:25:05PM +0800, Henry Chen wrote:
> On Wed, 2015-08-12 at 22:20 +0800, Daniel Kurtz wrote:

> > Making mtk_pmic-wrap into a regmap_bus makes a bit of sense
> > architecturally, too, since it is essentially just a bus for accessing
> > the registers of an off-chip PMIC.  The CPU sees a platform bus, but
> > the registers of the remote PMIC are accessed over a dedicated SPI
> > bus.

> Sorry, I'm afraid that I cannot do this right on init as you said last
> time. What do you think about regmap_bus, can you accept that way?

I don't have enough context from the above to understand what's going
on, sorry.  Implementing a bus for something that isn't actually a Linux
bus doesn't seem like the best idea though and in general what I'm
seeing of the discussion sounds like you're hacking around in driver
code to bodge around problems that are being seen rather than really
addressing things, the code I can see certainly looks like it just wants
to be implementing read and write operations for a single MFD.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web