Hi Wolfram, Thank you for the review. On Mon, Jun 17, 2024 at 9:31 AM Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hi Prabhakar, > > > - Ive modelled the regulator now to control the PWEN aswell. > > Yay, this looks much better. Good work! > > > - I have still kept regulator bits in quirks I was wondering if I should > > move this to renesas_sdhi_of_data instead? > > I think so. An internal regulator is not a quirk. > Agreed. > > - I still need to add checks if the internal regulator used and > > only then call regulator_enable/regulator_set_voltage. ATM I am still > > unclear on differentiating if internal/external regulator is used. > > When it comes to re-enabling the regulator in sdhi_reset, I think this > can be a sdhi_flag like SDHI_FLAG_ENABLE_REGULATOR_IN_RESET or alike. > OK. > When it comes to the regulator, I wonder if it wouldn't be clearer to > replace renesas_sdhi_internal_dmac_register_regulator() with a proper > probe function and a dedicated compatible value for it. We could use > platform_driver_probe() to instantiate the new driver within the SDHI > probe function. This will ensure that the regulator driver will only be > started once the main driver got all needed resources (mapped > registers). > I did give it a try with platform_driver_probe() and failed. - Firstly I had to move the regulator node outside the SDHI node for platform_driver_probe() to succeed or else it failed with -ENODEV (at https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L953) - In Renesas SoCs we have multiple instances of SDHI, the problem being for each instance we are calling platform_driver_probe(). Which causes a problem as the regulator node will use the first device. Let me know if I have missed something obvious here. > My gut feeling is that it will pay off if the internal regulator will be > described in DT as any other regulator. Like, we could name the > regulator in DT as always etc... > > More opinions on this idea are welcome, though... > > > --- a/drivers/mmc/host/renesas_sdhi.h > > +++ b/drivers/mmc/host/renesas_sdhi.h > > @@ -11,6 +11,9 @@ > > > > #include <linux/dmaengine.h> > > #include <linux/platform_device.h> > > +#include <linux/regmap.h> > > Regmap can luckily go now. > Agreed. > > +#include <linux/regulator/driver.h> > > +#include <linux/regulator/machine.h> > > #include "tmio_mmc.h" > > > > struct renesas_sdhi_scc { > > @@ -49,6 +52,9 @@ struct renesas_sdhi_quirks { > > bool manual_tap_correction; > > bool old_info1_layout; > > u32 hs400_bad_taps; > > + bool internal_regulator; > > + struct regulator_desc *rdesc; > > + struct regulator_init_data *reg_init_data; > > const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX]; > > }; > > > > @@ -93,6 +99,8 @@ struct renesas_sdhi { > > unsigned int tap_set; > > > > struct reset_control *rstc; > > + > > + struct regulator_dev *sd_status; > > This is a strange name for the regulater. Especially given that you have > as well the more fitting 'u32 sd_status' in the code later. > I will update it. > ... > > > +static struct regulator_init_data r9a09g057_regulator_init_data = { > > + .constraints = { > > + .valid_ops_mask = REGULATOR_CHANGE_STATUS, > > Don't we need REGULATOR_CHANGE_VOLTAGE here as well? Or is this implicit > because of REGULATOR_VOLTAGE? Can't find this, though. > I will investigate it. Cheers, Prabhakar
