Message-ID: <TY2PR01MB3692AE9149111BDD1A36F712D8CA0@TY2PR01MB3692.jpnprd01.prod.outlook.com>
Date:   Fri, 11 Dec 2020 00:49:10 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
CC:     "marek.vasut+renesas@...il.com" <marek.vasut+renesas@...il.com>,
        "geert+renesas@...der.be" <geert+renesas@...der.be>,
        Khiem Nguyen <khiem.nguyen.xt@...esas.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

Hi Matti-san,

> From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 8:56 PM
> 
> Hi Yoshihiro san,
> 
> On Thu, 2020-12-10 at 10:58 +0000, Yoshihiro Shimoda wrote:
> > Hi Matti,
> >
> > > From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 5:28 PM
> > >
> > > On Tue, 2020-12-08 at 17:04 +0900, Yoshihiro Shimoda wrote:
<snip>
> > > and populating the platform_driver id_table for sub driver(s) using
> > > something like:
> > >
> > > static const struct platform_device_id bd957x_gpio_id[] = {
> > >         { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> > >         { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
> > >         { },
> > > };
> > >
> > > Then you can get the IC type using
> > > platform_get_device_id(pdev)->driver_data.
> >
> > I got it. So, perhaps, I should add these types into
> > include/linux/mfd/rohm-generic.h.
> 
> That would make sense to me. I like the idea of collecting the IDs in
> same place - but on the other hand, the define in id-table is not
> really visible outside the sub-driver - so you can probably also define
> the type in sub-device driver if you wish. I don't have strong opinion
> on that.

I got it. ROHM_CHIP_TYPE_BD957[14] are used from both gpio and regulator
drivers so that adding IDs into rohm-generic.h is reasonable.

> > > Next, I think the parent data from MFD is only used to get the
> > > regmap
> > > and dev in sub-devices, right? Maybe you could simplify this and
> > > get
> > > rid of the whole MFD parent data structure? I think you can use
> > >
> > > pdev->dev.parent to get the parent device and
> > > dev_get_regmap(pdev->dev.parent, NULL);
> > >
> > > to get the regmap?
> >
> > IIUC, these comments are related to gpio-bd9571mwv.c.
> > # Also, bd9571mwv-regulator.c?
> > If so, I didn't try this yet, but perhaps, we can modify such things.
> 
> Correct. My suggestion was related to how regmap and dev pointers are
> obtained in sub-devices. It is related to MFD because I think you could
> remove the MFD driver data usage.

I understood it.

> > > (After this I wonder if you need the
> > > struct bd9571mwv at all?)
> >
> > I'm sorry, but I could not understand this.
> 
> I believe the struct bd9571mwv is defined only to collect all the MFD
> driver data in one struct so that it can be passed to sub-drivers using
> the MFD device private data. But as far as I can tell, the sub-devices
> only use regmap and dev pointers from this data. If this is the case,
> then I think there is no need to define this struct or populate the MFD
> driver data (unless I am missing something).
> 
> (And as a further clen-up, one could probably switch from:
> regmap_add_irq_chip
> to
> devm_regmap_add_irq_chip
> and get rid of the remove function)

Thank you for the detail. I understood it.

> but as I said - these are only 'nit' comments and I am not insisting on
> changing these. Especially since some of the comments are more related
> to changing the original bd9571mwv than adding support for this new IC.
> I just think one might be able to make this a little bit simpler :)

I got it :) For now, I would like to focus adding BD9574MWF support at first.
After that, I'll try to clean-up these drivers later.

> > <snip>
> > > >  /**
> > > >   * struct bd957x_data - internal data for the bd957x driver
> > > >   *
> > >
> > > Overall a good looking driver! Thanks a lot!
> >
> > Thank you very much for your review!
> >
> > By the way, I realized Linux kernel supports bd9576-regulator.c
> > and it has "bd957x", but it doesn't seem to be compatible with
> > BD9571.
> > So, I wonder if I should not use "bd957x" in the bd9571mwv driver to
> > avoid
> > confusion. But, what do you think?
> 
> Valid point. I think BD9573 and BD9573 are close to each-others but
> largely different from BD9571 and BD9574... Good example why wildcards
> are so hard. I have previously attempted to use the wildcards in ROHM
> PMIC software naming - and usually failed. The numbering does not
> really seem to reflect the functionality... So maybe i am not the best
> person to comment on this XD
> 
> On the other hand, sometimes we want to highlight that some of the
> functions/defines are used by all IC versions a driver supports - while
> others are IC specific. For example, inside the driver which supports
> BD71837 and BD71847 I used BD718XX as a prefix for defines that were
> common for both ICs. IC specific defines I named with BD71837 and
> BD71847. I think that was quite clear inside the driver (until BD71850
> was made - which uses same defines as BD71847... :| )
> 
> So... My suggestion - you can probably use wildcards inside a driver
> (and comment things when wildcards do not match anymore). But I would
> not add wildcards to any globally visible defines (in definitions
> included from headers, file names, ...)

Thank you for the suggestion! I think it's a good idea. So, I'll move
struct bd957x_data from the header to MFD driver.

> As Rob put it a while ago - "Naming is hard". :)

I think so :)

Best regards,
Yoshihiro Shimoda

Powered by blists - more mailing lists