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.
> - 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.
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).
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.
> +#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.
...
> +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.
So much for now. Thanks!
Wolfram
Attachment:
signature.asc
Description: PGP signature
