Messages in this thread | | | From | Yoshihiro Shimoda <> | Subject | RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF | Date | Fri, 11 Dec 2020 00:49:10 +0000 |
| |
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
| |